Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor COM Registration Fixing Tool part 1: make the tool effective on 64-bit Windows and fix more problems #12560

Merged
merged 3 commits into from Jun 28, 2021

Conversation

XLTechie
Copy link
Contributor

@XLTechie XLTechie commented Jun 17, 2021

Link to issue number:

Fixes #9039

Summary of the issue:

The COM Registration Fixing Tool seems to have never worked fully on 64bit Windows, as a result of path confusion caused by the use of SysWOW64, and the executable names used.

In addition, modern browsers seem to check a typelib registration that could be damaged by certain uninstallations (e.g. older versions of MathPlayer). We didn't previously do anything about this.

Further background:

In 64 bit Windows, %WINDIR%\System32 contains 64-bit executables. Meanwhile, %WINDIR%\SysWOW64 contains 32-bit executables. That seems inverted from what it should be, but there is a good explanation here for why it's like that.

When a 32-bit application calls something in System32, SysWOW64 assumes that it wants the 32-bit version, and redirects the call to the version in the SysWOW64 directory.
If the 32-bit application really wants the 64-bit version of whatever it's calling, it has to use the virtual directory %WINDIR%\Sysnative, which SysWOW64 provides.

We were calling regedit.exe and regsvr32.exe in contexts where we assumed 32-bit, from either %WINDIR% or %WINDIR%\System32. Because NVDA is 32-bit, that would result in the 32-bit application being called. That's okay.
However when the test was made for 64-bit, executables in %WINDIR%\SysWOW64 were being used, which are also 32-bit.

At least one of those should be 64-bit for COM reregistrations, but neither are.

In no case should we ever call anything in %WINDIR%\SysWOW64 ourselves, which we currently do.

Description of how this pull request fixes the issue:

Taken from the commit that rewrites COMRegistrationFixes/__init__.py:
Repaired the COM Registration Fixing Tool so it is more effective on 64bit Windows (Partial fix of #9039).

  • Problems:
    • The registerServer function called the 32-bit version of regsvr32.exe, even in 64-bit contexts.
    • The applyRegistryPatch function called the 32-bit version of regedit, even in 64-bit contexts.
    • The Win7 32-bit run did not take into account 32-bit only systems (no Program Files (x86) folder).
  • Remediation:
    • Replaced applyRegistryPatch function with two new functions: apply32bitRegistryPatch and apply64bitRegistryPatch.
    • Replaced registerServer function with register32bitServer and register64bitServer, to make clear what they do.
    • The new functions don't check 32/64 bitness; they leave that to the caller and log errors on failure.
    • Updated to more descriptive error logging.
    • Adjusted the Windows 7 code to use server registration with proper bitness for each DLL.
  • Path remediations:
    • Moved the OLEACC_REG_FILE_PATH constant to the top of the file with the rest.
    • Added Sysnative path to the list of path constants at the top of the file.
    • Now use Sysnative in the path for intentional 64bit calls.
    • Now use System32 in the path for 32-bit calls on either 32-bit or 64-bit systems.
    • Now use reg.exe's import option to load .reg files instead of regedit.exe.
    • Now check whether to use "Program Files" or "Program Files (x86)" on Win7.
    • Removed now unused sysWow64 path constant.
  • Misc:
    • Added docstring note about 32 and 64 bit functions needing attention if NVDA goes 64-bit in the future.
    • Converted path constants to uppercase-with-underscore style, and corrected case on some Windows paths.
    • Moved comments with discussion links into module docstring, and rearranged.
    • Used subprocess.STARTUPINFO to prevent console windows from showing during executions.
    • In gui/__init__.py update the message shown on tool completion, to recommend a restart.

Separately, @michaelDCurran found a registry key that we should delete to restore browser functionality under certain circumstances. That is included as a separate joint commit.

Testing strategy:

Followed the STR in #9039 (comment).

  • With current Alphas, could not restore the registry, as described in that STR.
  • With these modifications, the registry key was restored, indicating that the reg patch successfully applied.
  • Installed and uninstalled MathPlayer to break browsers, and used the final version of the tool to fix it.

Known issues with pull request:

Windows 7 tests would be valuable, but I can not run them.

My original refactor of this tool is still in progress, but that will now be limited to UX enhancements, since this covers the guts of the tool. These are important updates to the functionality, and imo should be accepted apart from the rest of the refactor.

Change log entries:

Bug fixes
The COM Registration Fixing Tool has been rewritten to solve more problems, and work better on 64bit Windows systems.

Changes for developers:

- In the `COMRegistrationFixes` module, the `applyRegistryPatch` function has been replaced by `apply32bitRegistryPatch` and `apply64bitRegistryPatch` functions. The `registerServer` function has been replaced by `register32bitServer` and `register64bitServer` functions.

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

@AppVeyorBot
Copy link

@AppVeyorBot
Copy link

See test results for failed build of commit 6c7463fc66

@XLTechie XLTechie marked this pull request as ready for review June 18, 2021 08:03
@XLTechie XLTechie requested a review from a team as a code owner June 18, 2021 08:03
@XLTechie XLTechie requested review from michaelDCurran and removed request for a team June 18, 2021 08:03
@XLTechie XLTechie marked this pull request as draft June 18, 2021 09:26
@AppVeyorBot
Copy link

See test results for failed build of commit 57d30a95ae

@AppVeyorBot
Copy link

See test results for failed build of commit 87680deece

Copy link
Member

@michaelDCurran michaelDCurran left a comment

Choose a reason for hiding this comment

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

Code looks all great to me. However, due to careful testing required and NVDA 2021.1 rc just about to come out, I would like this pr rebased on master for NVDA 2021.2 please.

@cary-rowen
Copy link
Contributor

Important fixes, I hope it can be incorporated into the upcoming version.

@XLTechie
Copy link
Contributor Author

XLTechie commented Jun 21, 2021 via email

if is64bit:
registerServer(os.path.join(sysWow64,'oleaut32.dll'),wow64=True)
registerServer(os.path.join(sysWow64,'actxprxy.dll'),wow64=True)
register64bitServer(os.path.join(sysnative, "oleaut32.dll"))
Copy link
Member

Choose a reason for hiding this comment

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

As we are calling the 64 bit version of regsvr32, the dll path we give it must already assume to be 64 bit. Thus, system32 would be the appropriate directory from regsvr32's point of view. In fact, I don't think c:\windows\sysnative is a valid path unless running as a wow64 32 bit process. Same goes for the next line down as well.

@XLTechie XLTechie changed the base branch from beta to master June 22, 2021 04:10
@XLTechie
Copy link
Contributor Author

@michaelDCurran Great catch! I fell prey to the same problem as the original tool had, just in the opposite direction.
Fixed and rebased.
Also noticed that the Windows 7 checks were trying to pull a DLL from Program Files (x86), but on 32-bit systems there is no such thing as I recall. So in the case of non-64-bit, we need to load from the right place. Fixed in latest.
Needs re-review.

@michaelDCurran
Copy link
Member

@XLTechie Are you still requiring more testing on this? I'm happy to merge this when you are happy.

@XLTechie
Copy link
Contributor Author

@lukaszgo1 I believe you have access to Windows 7: can you do any Windows 7 and/or 32-bit testing of this? Particularly interested in whether the original STR can be fixed by the tool with this try-build, and whether any errors appear in the log about anything (but especially registration failures).

@XLTechie
Copy link
Contributor Author

@michaelDCurran I have a few tests outstanding that I have requested from people with known COM registration problems, or for whom earlier builds of this fix bugged out. I'd like to hear from at least one or two of them before subjecting this to the wider alpha community.
Also, I didn't find anyone who could test this in either Windows 7 or a 32-bit version of 7 or 10. If you think it doesn't need to wait for that, then we can go ahead and hope they try it in alpha.

@lukaszgo1
Copy link
Contributor

@lukaszgo1 I believe you have access to Windows 7: can you do any Windows 7 and/or 32-bit testing of this? Particularly interested in whether the original STR can be fixed by the tool with this try-build, and whether any errors appear in the log about anything (but especially registration failures).

Since my only remaining Win 7 machine is my main working one I'm reluctant to do any testing there I'll set up a Vm and try the build from this PR during the weekend. As to 32-bit testing no I don't have any 32-bit Windows install lying around. For what its worth setting up a 32-bit VM should not take long - have you considered testing yourself?

@XLTechie
Copy link
Contributor Author

@michaelDCurran I am declaring this good in as far as it goes. You are aware of the work still in progress; another PR will be required when that is done, but it should be a small update given the foundation here.
Also the commit message is already the same as the above except for spacing and markdown, and it is singly rebased on current master.

@XLTechie XLTechie marked this pull request as ready for review June 24, 2021 08:36
@XLTechie XLTechie changed the title Repaired the COM Registration Fixing Tool so it is effective on 64bit Windows First half of COM Registration Fixing Tool refactor: make the tool effective on 64-bit windows, and fix more problems Jun 25, 2021
@XLTechie
Copy link
Contributor Author

@michaelDCurran this has been rebased, and now includes your registry key deletion.
Additionally, it adds a strong recommendation to restart the machine, to the tool's completion dialog.

I have no information suggesting that the other typelib issue described in #9039 is not fixed by this, so I think this PR can close that issue now.

@lukaszgo1
Copy link
Contributor

@XLTechie I gave the last build from this PR a try on A Windows 7 X64 VM and everything works as expected. In particular:

  • No errors in the log
  • Registry patch is successfully applied to the registry

source/COMRegistrationFixes/__init__.py Outdated Show resolved Hide resolved
source/gui/__init__.py Outdated Show resolved Hide resolved
XLTechie and others added 2 commits June 25, 2021 13:29
…64bit Windows (Partial fix of nvaccess#9039).

Problems:
- The registerServer function called the 32-bit version of regsvr32.exe, even in 64-bit contexts.
- The applyRegistryPatch function called the 32-bit version of regedit, even in 64-bit contexts.
- The Win7 32-bit run did not take into account 32-bit only systems (no Program Files (x86) folder).
Remediation:
- Replaced applyRegistryPatch function with two new functions: apply32bitRegistryPatch and apply64bitRegistryPatch.
- Replaced registerServer function with register32bitServer and register64bitServer, to make clear what they do.
- The new functions don't check 32/64 bitness; they leave that to the caller and log errors on failure.
- Updated to more descriptive error logging.
- Adjusted the Windows 7 code to use server registration with proper bitness for each DLL.
Path remediations:
- Moved the OLEACC_REG_FILE_PATH constant to the top of the file with the rest.
- Added sysnative path to the list of path constants at the top of the file.
- Now use Sysnative in the path for intentional 64bit calls.
- Now use System32 in the path for 32-bit calls on either 32-bit or 64-bit systems.
- Now use reg.exe's import option to load .reg files instead of regedit.exe.
- Now check whether to use "Program Files" or "Program Files (x86)" on Win7.
- Removed now unused sysWow64 path constant.
Misc:
- Added docstring note about 32 and 64 bit functions needing attention if NVDA goes 64-bit in the future.
- Converted path constants to uppercase-with-underscore style, and corrected case on some Windows paths.
- Moved comments with discussion links into module docstring, and rearranged.
- Used subprocess.STARTUPINFO to prevent console windows from being shown.
- In gui/__init__.py: added a recommendation that the user restart the computer, to the completion message.
…ccessible interface key., solving browser problems.

Co-authored-by: Michael Curran <michaelDCurran@users.noreply.github.com>
Copy link
Member

@michaelDCurran michaelDCurran left a comment

Choose a reason for hiding this comment

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

Thanks for working on this :)

@michaelDCurran michaelDCurran merged commit 3f02af2 into nvaccess:master Jun 28, 2021
@nvaccessAuto nvaccessAuto added this to the 2021.2 milestone Jun 28, 2021
@XLTechie
Copy link
Contributor Author

XLTechie commented Jun 28, 2021 via email

@XLTechie XLTechie changed the title First half of COM Registration Fixing Tool refactor: make the tool effective on 64-bit windows, and fix more problems Refactor COM Registration Fixing Tool part 1: make the tool effective on 64-bit Windows and fix more problems Mar 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

COM Registration Fixing tool doesn't work
6 participants