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

Don't block an app's main thread when sending typed characters to NVDA #11425

Merged
merged 10 commits into from Jul 28, 2020

Conversation

michaelDCurran
Copy link
Member

@michaelDCurran michaelDCurran commented Jul 27, 2020

Link to issue number:

Re #11369 , #11398

Summary of the issue:

Currently, the main way that NVDA reports characters being typed is by detecting them in-process by watching for WM_CHAR window messages, and then sending the characters to NVDA via an RPC call.
However, the rpc call to NvDA is made directly from the app's main thread where the message was detected, and therefore blocks that thread until the rpc call is fully executed.
This can cause a deadlock between multiple threads in NVDA, bit more importantly a deadlock between NVDA and the app in question, if a cross-process COM object for the app happens to be released by Python's garbage collector within the incoming RPC call to NVDA, as this COM object's Release method will try and execute via the Windows message loop on the app's main thread, which is currently blocked by the rpc call.

Description of how this pull request fixes the issue:

Rather than making the RPC call directly, queue it to NVDA's in-process manager thread using APC (Asynchronous Procedure Calls) , and execute it there, no longer blocking the app's main thread.

Testing performed:

  • Ran NVDA with speak typed characters turned on. Ensured that typed characters could be heard in various situations such as in Microsoft Word, Notepad, and the Run dialog.
  • Placed temporarily logging in the in-process code, to verify that the rpc call was in deed being made from the manager thread.

Known issues with pull request:

Not an issue, but just a note for future: In Windows 10 1607 and higher, we are able to detect typed characters within NVDA itself, rather than having to rely on in-process code at all. We should consider disabling the typed character support in-process completely on these more recent versions of Windows 10. However, right now it is more important to address the issue at its source, where it affects all Operating System versions.

Change log entry:

Bug fixes:

  • NVDA should no longer freeze in Microsoft word when rapidly typing characters.

@AppVeyorBot
Copy link

AppVeyorBot commented Jul 27, 2020

See test results for failed build of commit 1ecbe0bb6b

@leonardder
Copy link
Collaborator

leonardder commented Jul 27, 2020

Just out of curiosity, have you ever considered moving to async RPC to avoid issues like this?

nvdaHelper/remote/typedCharacter.cpp Outdated Show resolved Hide resolved
@michaelDCurran
Copy link
Member Author

michaelDCurran commented Jul 27, 2020

Co-authored-by: Leonard de Ruijter <leonardder@users.noreply.github.com>
Copy link
Member

@feerrenrut feerrenrut left a comment

Some small changes requested, but overall looks good!

nvdaHelper/remote/injection.cpp Outdated Show resolved Hide resolved
nvdaHelper/remote/injection.cpp Outdated Show resolved Hide resolved
nvdaHelper/remote/injection.cpp Outdated Show resolved Hide resolved
Copy link
Member

@feerrenrut feerrenrut left a comment

@michaelDCurran michaelDCurran merged commit 5608333 into master Jul 28, 2020
1 check passed
@michaelDCurran michaelDCurran deleted the asyncTypedCharacterNotification branch Jul 28, 2020
@nvaccessAuto nvaccessAuto added this to the 2020.3 milestone Jul 28, 2020
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

5 participants