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

Actually terminate (clean up) the Microsoft UIA Abstraction remote operations library on NVDA exit. #16222

Merged
merged 3 commits into from Feb 25, 2024

Conversation

michaelDCurran
Copy link
Member

@michaelDCurran michaelDCurran commented Feb 24, 2024

Link to issue number:

Fixes #16072

Summary of the issue:

As seen in a dump file provided by @irrah68 when debugging #16072 , NVDA can hang on exit while trying to destruct an instance of the CUIAutomation8 COMClass object:

Call stack
win32u.dll!_NtUserMsgWaitForMultipleObjectsEx@20()	Unknown
user32.dll!MsgWaitForMultipleObjectsEx()	Unknown
combase.dll!CCliModalLoop::BlockFn(void * * ahEvent, unsigned long cEvents, unsigned long * lpdwSignaled) Line 2116	C++
combase.dll!ClassicSTAThreadWaitForHandles(unsigned long dwFlags, unsigned long dwTimeout, unsigned long cHandles, void * * pHandles, unsigned long * pdwIndex) Line 54	C++
combase.dll!CoWaitForMultipleHandles(unsigned long dwFlags, unsigned long dwTimeout, unsigned long cHandles, void * * pHandles, unsigned long * lpdwindex) Line 126	C++
combase.dll!CGIPTable::_RevokeInterfaceFromGlobal(unsigned long dwCookie, bool bPostedRevoke, bool waitIfBusy) Line 1010	C++
combase.dll!CGIPTable::RevokeInterfaceFromGlobal(unsigned long dwCookie) Line 935	C++
uiautomationcore.dll!ViewManagerEventTracker::Uninitialize(void)	Unknown
uiautomationcore.dll!ClientEventManager::~ClientEventManager(void)	Unknown
uiautomationcore.dll!CUIAutomation::~CUIAutomation(void)	Unknown
uiautomationcore.dll!ATL::CComObject::`scalar deleting destructor'(unsigned int)	Unknown
uiautomationcore.dll!ATL::CComObject::Release(void)	Unknown
[Inline Frame] UIARemote.dll!wil::com_ptr_t::{dtor}() Line 260	C++
[Inline Frame] UIARemote.dll!wil::object_without_destructor_on_shutdown>::{dtor}() Line 2209	C++
UIARemote.dll!UiaOperationAbstraction::`anonymous namespace'::`dynamic atexit destructor for 'g_automation''()	C++
ucrtbase.dll!(void)()	Unknown
ucrtbase.dll!__crt_seh_guarded_call::operator()<,(void) &,(void)>()	Unknown
ucrtbase.dll!__execute_onexit_table()	Unknown
UIARemote.dll!__scrt_dllmain_uninitialize_c() Line 398	C++
UIARemote.dll!dllmain_crt_process_detach(const bool is_terminating) Line 182	C++
In short, it looks as if the destruction of the CUIAutomation8 COMClass object held by the UIA abstraction Remote Operations library locks up as it is happening from a different thread from where it was created. It was originally created in NvDA's UIA MTA thread, but it is being automatically cleaned up by the CRT of NVDA's UIARemote.dll in NVDA's main thread. I'm not entirely sure why this is only a problem on @irrah68's machine, and why it is more likely to happen with the UIA Rate Limited event handler in #14888. Either way though, NVDA is not cleaning it up correctly.

Description of user facing changes

NVDA is less likely to hang on NvDA exit.

Description of development approach

  • Add a cleanup function to UIARemote.dll which calls the UIA abstraction remote operations library's cleanup function. This function frees all remote contexts, but most importantly, releases the CUIAutomation8 COMClass object which was passed in on initialization.
  • Add a terminate function to UIAHandler/remote.py which calls UIARemote.dll's cleanup function.
  • at the end of UIA's MTA thread in Python, after removing event handlers, finally call UIAHandler.remtoe.terminate, ensuring that the UIA abstraction remote ops library is correctly cleaned up in the UIA MTA thread, where it was originally initialized.

Testing strategy:

  • Restarted NVDA multiple times. Ensured the NvDA's log showed the appropriate log messages in UIARemote.dll's cleanup method.
  • @irrah68 has tested a try build of this pr based on master and has confirmed the issue has been fixed for them.

Known issues with pull request:

None known.

Code Review Checklist:

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

@michaelDCurran michaelDCurran requested a review from a team as a code owner February 24, 2024 12:36
@michaelDCurran michaelDCurran requested review from seanbudd and removed request for a team February 24, 2024 12:36
@CyrilleB79
Copy link
Collaborator

@michaelDCurran please modify the issue number being fixed (twice) in the initial description. #16072 and not #16074.

@michaelDCurran
Copy link
Member Author

@CyrilleB79 oops, thanks. I've fixed this up now.

@michaelDCurran michaelDCurran added this to the 2023.3.4 milestone Feb 25, 2024
@seanbudd seanbudd changed the title Actually terminate (clean up) the Microsoft UIA Abstraction remote operations librry on NVDA exit. Actually terminate (clean up) the Microsoft UIA Abstraction remote operations library on NVDA exit. Feb 25, 2024
seanbudd
seanbudd previously approved these changes Feb 25, 2024
nvdaHelper/UIARemote/UIARemote.cpp Outdated Show resolved Hide resolved
nvdaHelper/UIARemote/UIARemote.cpp Outdated Show resolved Hide resolved
@seanbudd seanbudd merged commit 74460f5 into rc Feb 25, 2024
1 of 2 checks passed
@seanbudd seanbudd deleted the terminateUIARemote branch February 25, 2024 22:56
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.

None yet

3 participants