Skip to content

Fix rare recursion error in the winVersion module#12696

Merged
seanbudd merged 2 commits into
nvaccess:betafrom
lukaszgo1:I12666
Jul 29, 2021
Merged

Fix rare recursion error in the winVersion module#12696
seanbudd merged 2 commits into
nvaccess:betafrom
lukaszgo1:I12666

Conversation

@lukaszgo1

@lukaszgo1 lukaszgo1 commented Jul 28, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor

Opened against beta since this PR fixes regression which should be fixed before 2021.2

Link to issue number:

Fixes #12666
Fix-up of #12544

Summary of the issue:

In some rare cases winVersion.getWinVer can cause recursion errors since it calls winVersion._getRunningVersionNameFromWinReg which in turn executes winVersion.getWinVer to check if the currently running version of Windows contains release info in the registry.
A sample of the log posted in #12666 is as follows:

File "threading.pyc", line 890, in _bootstrap
  File "threading.pyc", line 926, in _bootstrap_inner
  File "threading.pyc", line 870, in run
  File "winInputHook.pyc", line 79, in hookThreadFunc
  File "winInputHook.pyc", line 54, in keyboardHook
  File "keyboardHandler.pyc", line 227, in internal_keyDownEvent
  File "keyboardHandler.pyc", line 116, in shouldUseToUnicodeEx
  File "winVersion.pyc", line 176, in getWinVer
  File "winVersion.pyc", line 47, in _getRunningVersionNameFromWinReg
  File "winVersion.pyc", line 176, in getWinVer

File "winVersion.pyc", line 176, in getWinVer
  File "winVersion.pyc", line 47, in _getRunningVersionNameFromWinReg
  File "winVersion.pyc", line 176, in getWinVer
  File "winVersion.pyc", line 47, in _getRunningVersionNameFromWinReg
  File "winVersion.pyc", line 164, in getWinVer
  File "garbageHandler.pyc", line 48, in _collectionCallback
Traceback (most recent call last):
  File "garbageHandler.pyc", line 46, in _collectionCallback
  File "garbageHandler.pyc", line 53, in _collectionCallback_helper
RecursionError: maximum recursion depth exceeded in comparison

Description of how this pull request fixes the issue:

Rather than checking if the current version of Windows contains release info in the registry by comparing it to the lowest version for which this is supported I'm just checking if the necessary info exists in the registry and if not appropriate exceptions are raised.
To avoid pointless registry accesses result of getWinVer is cached - version of Windows on which NVDA is running is not going to change during its lifetime.

Testing strategy:

Started NVDA ensured it works in general.

Known issues with pull request:

None known

Change log entries:

None necessary - unreleased regression.

Code Review Checklist:

  • Pull Request description is up to date.
  • Unit tests.
  • System (end to end) tests.
  • Manual testing.
  • User Documentation.
  • Change log entry.
  • Context sensitive help for GUI changes.
  • UX of all users considered:
    • Speech
    • Braille
    • Low Vision
    • Different web browsers

@lukaszgo1

lukaszgo1 commented Jul 28, 2021

Copy link
Copy Markdown
Contributor Author

cc @josephsl Would you be able to review this?

@lukaszgo1 lukaszgo1 mentioned this pull request Jul 28, 2021
Closed
@lukaszgo1 lukaszgo1 marked this pull request as ready for review July 28, 2021 11:20
@lukaszgo1 lukaszgo1 requested a review from a team as a code owner July 28, 2021 11:20
@lukaszgo1 lukaszgo1 requested review from seanbudd and removed request for a team July 28, 2021 11:20
josephsl
josephsl reviewed Jul 28, 2021
View reviewed changes
Comment thread source/winVersion.py
josephsl
josephsl previously approved these changes Jul 28, 2021
View reviewed changes

@josephsl josephsl left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@feerrenrut, any thoughts since you asked for checking for Windows 10 Version 1507 and earlier in the original PR? I can see the possibility of an infinite recursion (my mistake). Apart from the "from None" statement, I think this should be part of 2021.2.

One way to optimize this (without opening Windows Registry) is checking for Windows releases from getWinVer function before calling the private function, which actually removes the ability of this function to handle corner cases like the one discussed in this PR. Another option is checking for build ranges by calling sys.getwindowsversion from the private function or passing the current release information (major.minor.build) to the private function as an optional argument, but then the function cannot natively handle release info being None. Therefore what this PR proposes is what I think is the best solution at the moment unless the actual root cause of the recursion error is found.

Thanks.

@josephsl

josephsl commented Jul 28, 2021 via email

Copy link
Copy Markdown
Contributor

@lukaszgo1
Avoid pointless registry accesses by caching results of getWinVer a…
a550dac 
…s the version of Windows on which we're running cannot be changed.
josephsl
josephsl approved these changes Jul 28, 2021
View reviewed changes

@josephsl josephsl left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent - thanks for validating an optimization idea I had for the original PR.

@seanbudd seanbudd merged commit 4409193 into nvaccess:beta Jul 29, 2021
@nvaccessAuto nvaccessAuto added this to the 2021.3 milestone Jul 29, 2021
@seanbudd seanbudd linked an issue Jul 29, 2021 that may be closed by this pull request
NVDA Alpha crashes while selecting files in windows 7 explorer #12666
Closed
@seanbudd seanbudd modified the milestones: 2021.3, 2021.2 Jul 29, 2021
feerrenrut
feerrenrut reviewed Jul 29, 2021
View reviewed changes

@feerrenrut feerrenrut left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep looks good to me

@lukaszgo1 lukaszgo1 deleted the I12666 branch July 29, 2021 09:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@seanbudd seanbudd seanbudd approved these changes

@michaelDCurran michaelDCurran michaelDCurran approved these changes

+2 more reviewers

@feerrenrut feerrenrut feerrenrut left review comments

@josephsl josephsl josephsl approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

2021.2

Development

Successfully merging this pull request may close these issues.

NVDA Alpha crashes while selecting files in windows 7 explorer

6 participants

@lukaszgo1 @josephsl @feerrenrut @seanbudd @michaelDCurran @nvaccessAuto