Set foreground object rather than desktop object when initializing object cache #14301

seanbudd · 2022-10-26T03:58:39Z

Link to issue number:

None

Summary of the issue:

This regression was introduced in 2022.3.1

If NVDA starts on the lock screen (such as when starting your device), NVDA fails to cache the desktop object as the focus object, mouse object, etc. as it is from below the lock screen.
This causes NVDA to not function correctly after sign-in, such as the inability to use nvda+q.

Description of user facing changes

Fixes severe regression with using NVDA after sign-in, if NVDA starts on the lock screen.

Description of development approach

Cache the foreground window when NVDA starts up, rather than the desktop object.
This window should be accessible from the lock screen when NVDA starts on the lock screen.
The setDesktopObject should continue to cached the desktop object as usual, as security code has not been implemented to prevent this from being set to insecure objects.

Creates a generic function blockUntilConditionMet, to wait for the foreground window to be fetched successfully.
Unit testing discovered a bug with the system test implementation of blockUntilConditionMet. This has now been fixed.

Testing strategy:

Using a try build tested the following steps:

Restart device
Wait for NVDA to start automatically on the lock screen
Sign-in to Windows
Continue to use NVDA normally after sign-in (e.g nvda+q opens the quit dialog)

Unit tests have been added for blockUntilConditionMet.

Known issues with pull request:

None

Change log entries:

- Fixes a regression from 2022.3.1 where certain functionality was disabled after sign-in, if NVDA started on the lock screen. (#14301)

Code Review Checklist:

Pull Request description:
- description is up to date
- change log entries
Testing:
- Unit tests
- System (end to end) tests
- Manual testing
API is compatible with existing add-ons.
Documentation:
- User Documentation
- Developer / Technical Documentation
- Context sensitive help for GUI changes
UX of all users considered:
- Speech
- Braille
- Low Vision
- Different web browsers
- Localization in other languages / culture than English
Security precautions taken.

source/core.py

source/winUser.py

feerrenrut

Overall, happy with the approach.

source/core.py

michaelDCurran

GetForegroundWindow will have to continuously polled to ensure it is not NULL. This polling should be allowed to take as long as it needs... E.g. 10, 20 seconds... the setting of these objects can not fail.
Though this situation is extremely rare, and most likely will correct itself within a second or two.

source/core.py

AppVeyorBot · 2022-10-27T00:20:10Z

PASS: Translation comments check.
PASS: Unit tests.
PASS: Lint check.
FAIL: System tests. See test results for more information.
Build (for testing PR): https://ci.appveyor.com/api/buildjobs/wfskw70cb9ghcmbh/artifacts/output/nvda_snapshot_pr14301-27004,dbca3e54.exe

See test results for failed build of commit dbca3e5488

source/core.py

feerrenrut · 2022-10-27T04:33:10Z

source/core.py

+	foregroundHWND = winUser.getForegroundWindow()
+	_hasWarned = False
+	WARN_AFTER_SECS = 5
+	MAX_WAIT_TIME_SECS = 20
+	POLL_INTERVAL_SECS = 0.1
+	waitForForegroundHWND_startTime = time.time()
+
+	while not foregroundHWND:
+		# The foreground window can be NULL in certain circumstances,
+		# such as when a window is losing activation.
+		# This should not remain the case for an extended period of time.
+
+		_timeElapsed = time.time() - waitForForegroundHWND_startTime
+		if (
+			not _hasWarned
+			and _timeElapsed > WARN_AFTER_SECS
+		):
+			_hasWarned = True
+			# Allow for braille / speech to be understood before exiting.
+			# Unfortunately we cannot block with a dialog as NVDA cannot read dialogs yet.
+			ui.message(_(
+				# Translators: Message when NVDA is having an issue starting up
+				"NVDA is failing to fetch the foreground window. "
+				"If this continues, NVDA will quit starting in %d seconds." % (MAX_WAIT_TIME_SECS - WARN_AFTER_SECS)
+			))
+
+		if _timeElapsed > MAX_WAIT_TIME_SECS:
+			log.critical("NVDA could not fetch the foreground window. Exiting NVDA.")
+			# Raising exception here causes core.main to exit and NVDA to fail to start
+			raise NVDANotInitializedError("Could not fetch foreground window")
+
+		log.debugWarning("Foreground window not found, fetching again")
+		time.sleep(POLL_INTERVAL_SECS)
+		foregroundHWND = winUser.getForegroundWindow()
+
+	return foregroundHWND


Looks like I didn't hit submit on this comment.

I want to make sure all the paths here are tested. Doing so manually will be difficult. I think this could be easier to read and unit testable if the polling algorithm was decoupled from the condition and outcomes.

Suggestion:

# The foreground window can be NULL in certain circumstances, # such as when a window is losing activation. # This should not remain the case for an extended period of time. WARN_AFTER_SECS = 5 MAX_WAIT_TIME_SECS = 20 POLL_INTERVAL_SECS = 0.1 success, foregroundHWND = pollForCondition( condition=lambda: bool(winUser.getForegroundWindow()), maxWait=WARN_AFTER_SECS, intervalSeconds=POLL_INTERVAL_SECS ) if not success: ui.message(_( # Translators: Message when NVDA is having an issue starting up "NVDA is failing to fetch the foreground window. " "If this continues, NVDA will quit starting in %d seconds." % (MAX_WAIT_TIME_SECS - WARN_AFTER_SECS) )) success, foregroundHWND = pollForCondition( condition=lambda: bool(winUser.getForegroundWindow()), maxWait=MAX_WAIT_TIME_SECS - WARN_AFTER_SECS, intervalSeconds=POLL_INTERVAL_SECS ) if not success: log.critical("NVDA could not fetch the foreground window. Exiting NVDA.") # Raising exception here causes core.main to exit and NVDA to fail to start raise NVDANotInitializedError("Could not fetch foreground window") return foregroundHWND

I have implemented something similar

AppVeyorBot · 2022-10-28T06:04:36Z

PASS: Translation comments check.
FAIL: Unit tests. See test results for more information.
PASS: Lint check.
FAIL: System tests. See test results for more information.
Build (for testing PR): https://ci.appveyor.com/api/buildjobs/sm258vmtw6eloeea/artifacts/output/nvda_snapshot_pr14301-27019,c7522f8c.exe

See test results for failed build of commit c7522f8c27

tests/system/libraries/SystemTestSpy/blockUntilConditionMet.py

tests/unit/test_util/test_blockUntilConditionMet.py

seanbudd

I've rewritten the testing suite

feerrenrut

Looks good @seanbudd

Summary of the issue: #14301 moved an api import from core.main as it was otherwise unused #14050 introduced a usage of api to core.main to beta This means that api was used without being imported on beta and master. Description of user facing changes None Description of development approach Fix import error

Summary of the issue: During NVDA initialization, the api module is used to set initial object caches. These api functions check if an object is behind the lock screen when Windows is locked before setting the cache. Until #14301, the api module initialised the object caches to the desktop object. When NVDA started on the lock screen, it failed to set the desktop object due to the added security checks. Now the object caches are initialised to the foreground window, to avoid leaking insecure information. Unfortunately, setting those object caches causes side-effects, which may rely on uninitialized dependencies. Description of user facing changes None Description of development approach Revert to setting initial object caches to the desktop object. Ensure security failures are not raised when initialising caches by ignoring the windows lock state.

Summary of the issue: Due to intermittent system test failures, #14284 increased the polling interval used in blockUntilConditionMet. When writing unit tests for blockUntilConditionMet in #14301, a bug was picked up. This bug caused blockUntilConditionMet to spin for longer than expected and potentially caused system tests to fail. This bug was fixed with a new implementation of blockUntilConditionMet. Description of user facing changes For devs, system tests should be faster (todo: estimate from build?) Description of development approach Lower default polling interval

seanbudd changed the title ~~Fix startup regression~~ Fix NVDA not functioning correctly when starting on the lock screen Oct 26, 2022

Set foreground object rather than desktop object when initializing

22eff08

seanbudd force-pushed the fix-startup-regression branch from d072829 to 22eff08 Compare October 26, 2022 04:23

seanbudd changed the title ~~Fix NVDA not functioning correctly when starting on the lock screen~~ Set foreground object rather than desktop object when initializing object cache Oct 26, 2022

seanbudd marked this pull request as ready for review October 26, 2022 04:24

seanbudd requested a review from a team as a code owner October 26, 2022 04:24

seanbudd requested review from feerrenrut and removed request for a team October 26, 2022 04:24

seanbudd added this to the 2022.3.2 milestone Oct 26, 2022

seanbudd self-assigned this Oct 26, 2022

seanbudd requested a review from michaelDCurran October 26, 2022 05:11

michaelDCurran reviewed Oct 26, 2022

View reviewed changes

source/core.py Outdated Show resolved Hide resolved

feerrenrut reviewed Oct 26, 2022

View reviewed changes

source/winUser.py Show resolved Hide resolved

feerrenrut previously approved these changes Oct 26, 2022

View reviewed changes

source/core.py Show resolved Hide resolved

address review comments

ba8c418

michaelDCurran requested changes Oct 26, 2022

View reviewed changes

seanbudd dismissed feerrenrut’s stale review via ba8c418 October 26, 2022 05:53

michaelDCurran previously approved these changes Oct 26, 2022

View reviewed changes

seanbudd dismissed michaelDCurran’s stale review via 23effc4 October 26, 2022 05:58

seanbudd force-pushed the fix-startup-regression branch from 23effc4 to ba8c418 Compare October 26, 2022 05:59

seanbudd marked this pull request as draft October 26, 2022 06:02

seanbudd commented Oct 26, 2022

View reviewed changes

source/core.py Outdated Show resolved Hide resolved

seanbudd added 2 commits October 26, 2022 19:41

add UX if foreground window fails to be fetched (unlikely to ever occur)

5829783

Merge branch 'rc' into fix-startup-regression

debb88f

seanbudd requested review from michaelDCurran and feerrenrut October 26, 2022 08:42

seanbudd marked this pull request as ready for review October 26, 2022 08:42

seanbudd added 3 commits October 26, 2022 19:43

Add new line

50e084c

remove debug line

12497d1

move comment

bd44d65

update changes

90c8503

seanbudd dismissed michaelDCurran’s stale review via 90c8503 October 26, 2022 22:54

feerrenrut suggested changes Oct 27, 2022

View reviewed changes

source/core.py Show resolved Hide resolved

feerrenrut reviewed Oct 27, 2022

View reviewed changes

seanbudd marked this pull request as draft October 27, 2022 04:43

add wait condition abstraction

9f21e6d

seanbudd marked this pull request as ready for review October 28, 2022 01:07

seanbudd requested a review from feerrenrut October 28, 2022 01:07

seanbudd added 2 commits October 28, 2022 12:11

fix lint

186a12b

attempt (not working) of patching time functions

40947b5

seanbudd added 2 commits October 28, 2022 19:06

fix up tests

f4cdebc

simplify code

48530ba

feerrenrut reviewed Oct 28, 2022

View reviewed changes

tests/system/libraries/SystemTestSpy/blockUntilConditionMet.py Outdated Show resolved Hide resolved

feerrenrut suggested changes Oct 28, 2022

View reviewed changes

tests/unit/test_util/test_blockUntilConditionMet.py Outdated Show resolved Hide resolved

tests/unit/test_util/test_blockUntilConditionMet.py Show resolved Hide resolved

tests/unit/test_util/test_blockUntilConditionMet.py Outdated Show resolved Hide resolved

seanbudd added 2 commits October 28, 2022 19:46

fix up interval tracking

100d4d9

improve test suite

fe75d3a

seanbudd commented Oct 31, 2022

View reviewed changes

seanbudd requested a review from feerrenrut October 31, 2022 04:23

fix typing

2a22ad1

feerrenrut approved these changes Oct 31, 2022

View reviewed changes

seanbudd merged commit 49b66db into rc Oct 31, 2022

seanbudd deleted the fix-startup-regression branch October 31, 2022 06:21

This was referenced Nov 2, 2022

Lower poll interval for blockUntilConditionMet #14324

Merged

restore API import to core.main on beta #14330

Merged

seanbudd mentioned this pull request Nov 4, 2022

Initialize object caches in a safer manner #14333

Merged

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Set foreground object rather than desktop object when initializing object cache #14301

Set foreground object rather than desktop object when initializing object cache #14301

seanbudd commented Oct 26, 2022 •

edited

feerrenrut left a comment

michaelDCurran left a comment

AppVeyorBot commented Oct 27, 2022

feerrenrut Oct 27, 2022

seanbudd Oct 28, 2022

AppVeyorBot commented Oct 28, 2022

seanbudd left a comment

feerrenrut left a comment

Set foreground object rather than desktop object when initializing object cache #14301

Set foreground object rather than desktop object when initializing object cache #14301

Conversation

seanbudd commented Oct 26, 2022 • edited

Link to issue number:

Summary of the issue:

Description of user facing changes

Description of development approach

Testing strategy:

Known issues with pull request:

Change log entries:

Code Review Checklist:

feerrenrut left a comment

Choose a reason for hiding this comment

michaelDCurran left a comment

Choose a reason for hiding this comment

AppVeyorBot commented Oct 27, 2022

feerrenrut Oct 27, 2022

Choose a reason for hiding this comment

seanbudd Oct 28, 2022

Choose a reason for hiding this comment

AppVeyorBot commented Oct 28, 2022

seanbudd left a comment

Choose a reason for hiding this comment

feerrenrut left a comment

Choose a reason for hiding this comment

seanbudd commented Oct 26, 2022 •

edited