Simplify lock state tracking #14358
Closed
Simplify lock state tracking #14358
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
If NVDA freezes, session tracking notifications may be dropped. Instead, query and cache the lock state on each core cycle with AutoPropertyObject
michaelDCurran
approved these changes
Nov 10, 2022
There was a problem hiding this comment.
The approach looks okay to me. But also requesting review from @feerrenrut as there is a massive amount of code removal which I find hard to review efficiently.
|
Not that I have any evidence to suggest there is a performance issue, but if we do now or at some point become concerned there is, we could rather than use an AutoPropertyObject caching for a core cycle (which is rather short) we could instead cache it for ever as an instance variable on |
feerrenrut
suggested changes
Nov 14, 2022
| # https://docs.microsoft.com/en-us/windows/win32/api/wtsapi32/nf-wtsapi32-wtsregistersessionnotification#remarks | ||
| "Global\\TermSrvReadyEvent" # LPCWSTR lpName - The name of the event object. | ||
| def register(handle: int) -> bool: | ||
| # TODO: improve deprecation practice on beta/master merges |
There was a problem hiding this comment.
I'm not sure what this means. Happy for the plan to be documented on the PR description.
| """ | ||
| global _currentSessionState | ||
| log.debug("initializing session tracking") | ||
| _currentSessionState = _CurrentSessionState() | ||
|
|
||
|
|
||
| def isWindowsLocked() -> bool: |
There was a problem hiding this comment.
Let's deprecate this and fix the name on master:
Suggested change
| def isWindowsLocked() -> bool: | |
| def isWindowsLocked() -> bool: | |
| return isWindowsLockScreenActive() | |
| def isWindowsLockScreenActive() -> bool: | |
| ... |
15 tasks
seanbudd
added a commit
that referenced
this pull request
Dec 16, 2022
Link to issue number: Supercedes #14358 Fixes: #14379 Fixes: #14368 Summary of the issue: Issue 1: Session tracking notification failures (#14358) If NVDA freezes, session tracking notifications may be dropped. If this happens when locking, NVDA will be insecure while on the Windows lock screen. If this happens when unlocking, NVDA will not behave correctly and be inaccessible while Windows is unlocked. This is fixed by querying the session directly, and caching this every core cycle. If a query fails, NVDA should fall back accessible behaviour, rather than secure. Issue 2: Forgot my PIN workflow is inaccessible (#14368) NVDA cannot read content on the forgot my PIN workflow screen. This is a similar situation to the lock screen, except an Edge Spartan window is used for the workflow. This runs on a temporary user profile. This is fixed by detecting the z-order of windows, and making an window above the lock screen window accessible. Issue 2a: Object navigation does not work on the PIN workflow screen (#14416) This is because TextInfo.obj can be a TreeInterceptor, where it was previously documented as just NVDAObject. This assumption caused the _isSecureObjectWhileLockScreenActivated function to fail, making object navigation fail. In those cases, the TreeInterceptor.rootNVDAObject should be checked instead. Issue 3: NVDA fails to install in some environments (#14379) Sometimes an NVDA session query returns an unexpected value. In this case, default to the "session unknown" behaviour. If a session query fails, NVDA should roll back to accessible behaviour rather than failing to run. Description of user facing changes PIN workflow screen should become accessible. NVDA has better session tracking management (i.e. is aware of the lock state more accuractely). NVDA should handle session query failures without preventing installation, blocking usage, etc. Description of development approach There are 4 security modes of NVDA: normal, authenticated user secure mode: secure desktop mode (when serviceDebug param not set), or --secure param provided. Refer to existing docs on this. secure desktop mode: enabled secure mode (when serviceDebug param not set). Also prevents access to some controls, that should not be accessible from the sign-in/UAC dialog. lock screen mode: prevents access to user data. Used on the lock screen, which runs on a user desktop. Also include the reset PIN workflow and out of box experience. (Only Win 10+) Lock state session tracking is handled by NVDA now, by querying the session state every core pump cycle. When on lock screen mode, we need to check the z-order of windows to confirm if an NVDA object should be accessible. The window associated with the NVDAObject should be above the lowest lock screen window. Lock screen windows can be identified by known class names. This is risky as class names may change, but the lockapp appModule isn't detectable on the forgot PIN workflow. We can confirm the windows order by starting at the lowest lock screen, then navigating our way up. If we can confirm that the NVDA Object is not below the lock screen window, we can make the object accessible. This method is risky, as z-ordering is dynamic. There are unit tests to cover this, code aims to make NVDAObjects accessible where the order is unknown.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Link to issue number:
None
Summary of the issue:
If NVDA freezes, session tracking notifications may be dropped.
If this happens when locking, NVDA will be insecure while on the Windows lock screen.
If this happens when unlocking, NVDA will not behave correctly and be inaccessible while Windows is unlocked.
Description of user facing changes
Should avoid the described issue, otherwise no changes.
Description of development approach
The previous approach is querying an initial state, and tracking state changes via session tracking notification.
Instead, query and cache the lock state on each core cycle with
AutoPropertyObject.Testing strategy:
As it is difficult to time and simulate a freeze, general smoke testing on the new session tracking method should be performed instead.
With a try-build, use speech, enable error sounds, and monitor the logs for errors/warnings.
Known issues with pull request:
Change log entries:
Bug fixes
Deprecation warnings: TODO
Code Review Checklist: