Register IA2 COM proxy on all Ui threads instead of only the main thread#13542
Conversation
|
Looking good, thanks. Is that in the next release? |
|
Nope. Note that it is still a prototype. Definitely need feedback from @michaelDCurran on it. |
|
Any test build to try for this PR? |
|
Thanks, downloading it
|
|
@LeonarddeR @michaelDCurran is this abandoned or still work in progress? |
|
Let's mark it ready for review to make it gain some attention. It seems there's demand for it. |
See test results for failed build of commit 0d9d3e3783 |
seanbudd
left a comment
There was a problem hiding this comment.
Hi @LeonarddeR, this is a preliminary review to pick up trivial things.
@michaelDCurran will review the broader approach
Thanks for the review. Mind if I wait with review actions until @michaelDCurran's broader approach review is there? Otherwise it's likely that I'll touch code that needs changing anyway. |
|
I'll try and look at this before the weekend.
|
|
Any news @michaelDCurran? |
michaelDCurran
left a comment
There was a problem hiding this comment.
The technical approach looks good to me.
I don't see any concern about not unregistering the winEvent hook once ia2 is installed on a thread. those register / unregister winEventHook functions from nvdaHelper just affect a map and don't handle dll lifetime or anything like that.
The code suggestions from @seanbudd can now be considered / applied at will.
Co-authored-by: Sean Budd <seanbudd123@gmail.com>
See test results for failed build of commit 2061177b15 |
See test results for failed build of commit 2a20df3386 |
|
Is this ready to merge? |
|
@yolpsoftware Thanks for the reminder. I guess @seanbudd will have to go over this once again and then it can be merged. |
…rom NVDA itself
…rom NVDA itself
… itself (#15517) Fixup of #13542 Summary of the issue: In #13542, we introduced support for multi thread IA2 initialization. As part of that, the signature of the install- and uninstallIA2Support functions were changed. Somehow, it was an oversight from my end that these functions were also called from NVDAHelper.py. This causes: NVDA to always log a debug warning on exit, saying that it was unable to unregister IA2 support. NVDA to call installIA2Initialize without a thread Id. It is actually a miracle that the segfault we're now seeing in preliminary PY3.11 work didn't hurt us earlyer. That said, I think I've seen cases were NVDA played the startup sound and then silently crashed. I think this should be the case. As per point 2, given the risk of NVDA calling a code path that results in undefined behavior and possible segfaults, I strongly recommend to merge this into beta. Description of user facing changes No crashes during NVDA startup. Description of development approach Removed the calls from NVDAHelper.py and no longer export the functions from NVDAHelper. The calls in NVDA core were actually obsolete, as IA2 support is registered by injection_initialize and injection_terminate. Note that the install and uninstall IA2 functions aren't called in de 64 bit loader of NVDAHelper, yet IA2 works as expected in X64 applications. Also, installIA2Support is no longer C friendly anyway. Note that an add-on author should never call these functions, therefore I don't consider this as an API breaking change.
Link to issue number:
Fixes #8553
Summary of the issue:
When an app offers IAccessible2 as its accessibility interface, NVDA needs to install a COM proxy in the UI thread to utilize IA2. However, some apps, particular those that embed Chromium Embedded Controls, have a separate UI thread for the browser control. IAccessible2 and therefore browse mode doesn't work in these controls.
Description of how this pull request fixes the issue:
Register IA COM Proxy in all threads that ever send focus or foreground win events
Testing strategy:
Tested with the Chromium Embedded Framework test case from #13493 (comment)
Known issues with pull request:
This might have performance/stability issues. Therefore an early merge is advised.
Change log entries:
Bug fixes
Code Review Checklist: