Skip to content

Avoid UIA event flooding in NvDA by moving UIA event handler COM objects into c++#14888

Merged
michaelDCurran merged 32 commits into
masterfrom
UIAEventLimiting
Nov 27, 2023
Merged

Avoid UIA event flooding in NvDA by moving UIA event handler COM objects into c++#14888
michaelDCurran merged 32 commits into
masterfrom
UIAEventLimiting

Conversation

@michaelDCurran

@michaelDCurran michaelDCurran commented Apr 30, 2023

Copy link
Copy Markdown
Member

Link to issue number:

Summary of the issue:

If NVDA is flooded with UI Automation events, such as textChange events from Windows consoles, NVDA may seem to freeze, and or UIA focus tracking completely stops.

Description of user facing changes

NVDA should remain responsive when being flooded with many UI Automation events, E.g. large amounts of text being written to a Windows console.

Description of development approach

Implement a new UI Automation event handler COM object in c++ which avoids calling into Python, and instead stores the events, and requests NvDA's main thread to later wake and read off the events when it can. This COM object also coalesces duplicate automation and propertyChange events for the same element, so that NVDA is no longer flooded with duplicate events, E.g. textChange events in Windows consoles.
The path for a UI Automation event is now as follows:

  • In an MTA thread, the new c++ UIA event handler COM object receives the event via its handle*Event method from UI Automation core.
  • The event is stored in a list of events.
  • If the event type supports coalescing (automation and propertyChange events), An existing duplicate event is looked up in a map, and if one exists, the existing event is removed from the list, and the map entry is then pointed to the newest event.
  • If there was no existing event, a new entry is added to the map for this event.
  • In other words: the events are stored in the list in order they were received, but any duplicate events are removed, leaving only the latest ones. Ecentually an ordered dictionary.
  • If this event type does not support coalescing (E.g. focusChange event), NVDA is requested to wake and flush the event queue on its main thread as soon as it can.
  • If this event does support coalescing, and this is the first event since the last flush, then NVDA is requested to wake and flush the event queue in around 30 ms, giving time for more events to possibly be received and coalesced.
  • When NvDA wakes on its main thread, It requests the rate limited UIA event handler to flush its event list, emitting all the stored events out to our original Python UIA event handler COM object for normal handling.
    These changes mean that UIA core in the MTA is never blocked waiting for the Python GIL, and from its perspective, event handling is instantanious. Thus UIA core should never feel the need to kill off events, including focus change events.

This pr also provides a couple of extra optimizations for UIA handling in Python:

  • UIAHandler events: avoid needlessly creating an NVDAObject when the event is for the focus object.
  • UIA event handlers: pass windowHandle into NvDAObject constructor if we already have it.

Testing strategy:

  • Ensure that NVDA is set to use UIA for Windows consoles, but diffing (not notification events).
  • Open a cmd Windows console.
  • In the NVDA repo, run the command: git -P log -1000. to majorly flood the console with text.
  • Keep silencing speech with control until all text is written, probably about 15 seconds or so. And note that NVDA remains responsive, and focus change events are no longer missing.

Known issues with pull request:

UIA notification events are currently not coalesced in any way. Though I feel that Windows Terminal provides more 'responsible' rate of notification events, as compared to textchange events. If this is a problem, then perhaps we could coalesce on activityID, though I'm not sure if that is the same value for all console text events. Do you know / have an opinion @codeofdusk?

Change log entries:

New features
Changes
Bug fixes

  • NVDA should remain responsive when being flooded with many UI Automation events, E.g. large amounts
    For Developers

Code Review Checklist:

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

…voids calling into Python, and instead records the events, and requests NvDA's main thread to later wake and read off the events when it can. This COM object also coalesces duplicate automation and propertyChange events for the same element, so that NVDA is no longer flooded with duplicate events, E.g. textChange events in Windows consoles.
@michaelDCurran michaelDCurran requested review from jcsteh and seanbudd May 1, 2023 00:10
@codeofdusk

Copy link
Copy Markdown
Contributor

This is fantastic to see!

Yes, I believe the activity ID is the same for all notification events, but also CCing @carlos-zamora to be sure.

Another pain point with event flooding is Visual Studio (devenv), especially when navigating project directory structures and using the debugger. Hoping these optimizations will help there as well!

@codeofdusk

Copy link
Copy Markdown
Contributor

Also, just to clarify: the diffing/notifications setting only affects Windows Terminal (wt.exe), not Windows Console (conhost.exe), though in SV2+ wt is now the default terminal for most users.

@AppVeyorBot

Copy link
Copy Markdown
  • PASS: Translation comments check.
  • PASS: Unit tests.
  • PASS: Lint check.
  • Build execution time has reached the maximum allowed time for your plan (60 minutes).

See test results for failed build of commit d39b6e5e81

@zstanecic

Copy link
Copy Markdown
Contributor

failed

@jcsteh jcsteh 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.

Very nice overall!

HRESULT RateLimitedEventHandler::queueEvent(EventRecordArgTypes&&... args) {
LOG_DEBUG(L"RateLimitedUIAEventHandler::queueEvent called");
bool needsFlush = false;
const unsigned int flushTimeMS = (EventRecordClass::isCoalesceable)?30:0;

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.

The flush time is tricky. On one hand, we want rate limiting to be useful. On the other hand, this could impact perceived performance. In particular, property change events on the focus can be relevant to immediate user interaction; e.g. value changes on a focused volume slider. The response is likely to be slower than 30 ms because of timer resolution, additional time to queue the UIA event to NVDA core, etc.

I think the gains outweigh the losses here, but this is something we should at least consider for future optimisation. Perhaps we don't coalesce certain property changes on the focus or something like that?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I think it would be worth then only doing coalescing for automation events, and not propertyChange events in this case. Not hard to change. Noting that the main motivation for this PR was textchange automation events. I think I should also add an extra variable to track if a flush message has been posted due to a non-coalesced event, and not post any more until the next flush. Currently if there are a whole bunch of non-coalesced events in a row, we'll keep flooding NVDA with pointless flush messages.

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.

Doing this only for automation events seems reasonable for this first PR if you prefer. However, I could see situations where property change event flooding could be a real issue. Task Manager is a good example. In that case, I think it does make sense to coalesce property change events except for the focus.

Yeah, the pointless flush messages thing is definitely worth fixing. I hadn't considered that.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

A couple of points:

  1. PropertyChange events are only coalesced if their runtimeIDs match, and they are the same property (property ID). I.e. two name property changes on the same element are coalesced. I'm not too sure that this pr will really help task manager as I'm thinking its many properties across many list items.
  2. Selective UIA registration is used in NVDA by default on Windows 11 now. thus there are hardly any property changes on elements other than the focus or its ancestors. I'd love to have this on by default in Windows 10 also, but apparently there are some bugs in Task list / emoji picker which stop us from doing that. @josephsl @codeofdusk ?
  3. I've now added a commit that stops propertyChange events from being coalesced. and also a commit which stops the pointless flush messages.

Comment thread nvdaHelper/local/UIAEventLimiter/rateLimitedEventHandler.cpp Outdated
Comment thread nvdaHelper/local/UIAEventLimiter/rateLimitedEventHandler.cpp Outdated
Comment thread nvdaHelper/local/UIAEventLimiter/utils.h
Comment thread source/UIAHandler/__init__.py Outdated
@michaelDCurran

Copy link
Copy Markdown
Member Author

@jcsteh re your comments around documentation, I'll go through and document much of it shortly.

@beqabeqa473

beqabeqa473 commented May 2, 2023 via email

Copy link
Copy Markdown
Contributor

@beqabeqa473

Copy link
Copy Markdown
Contributor

@michaelDCurran Overal performance of NVDA dialogs are very bad, this is shown in voice, braille panels, also in the category list

@codeofdusk

Copy link
Copy Markdown
Contributor

@beqabeqa473 Cannot repro this in a self-signed build

@beqabeqa473

beqabeqa473 commented May 2, 2023 via email

Copy link
Copy Markdown
Contributor

@beqabeqa473

Copy link
Copy Markdown
Contributor

So, yes, #de5332f119ac0b6673df0deb08a0e90ff55fd6d7 breaks NVDA settings dialog on my machine

@michaelDCurran

michaelDCurran commented May 2, 2023 via email

Copy link
Copy Markdown
Member Author

@AppVeyorBot

Copy link
Copy Markdown
  • PASS: Translation comments check.
  • PASS: Unit tests.
  • PASS: Lint check.
  • Build execution time has reached the maximum allowed time for your plan (60 minutes).

See test results for failed build of commit 64574df5d2

… better that these are quick for responce, and it is really autoamtion events like textchange which truly need coalescing.
…ges after a quick flush is requested and before the flush has actually occured.
…thread rather than NVDA's main thread. This works around some weird freezing when accessing NVDA's own UI.

Note that UIA core always calls UIA event handlers on worker threads, except for when processing events within a UI Automation event registration call. Thus, our UIA Handler MTA thread sits there doing nothing 99% of the time. So moving the flush calls into this thread does not disrrupt true UI Automation event handler threads.
If this design seems okay, then we may consider registering a custom window on the UIAHandler thread in order to receive posted messages directly, rather than them being queued to UIAHandler's queue from NVDA's main thread.
@AppVeyorBot

Copy link
Copy Markdown

See test results for failed build of commit 39f0f91c27

* rather than the limiter sending a window message to NvDA to request a flush, instead the limiter class has its own dedicated thread which it wakes for flushing.
* Now all UI Automation events are coalesced. However, there is no delay before flushing. It will happen as soon as the flush thread can wake and flush.
These changes mean that UI Automation events again enter NVDA on a dedicated MTA thread, with no deliberate delay or having to go through NvDA's main thread first.
Therefore the primary reason for the event limiter in c++ is to ensure that UI Automation core is never blocked on an event handler - they should queue and return very fast and not hit Python at all.
Secondarily, any duplicate UI Automation events that exist before NVDA receives them are removed.
This all still has the major advantage that doing `git -P log -1000` in a Windows console will not take out UIA focus events, nor slow down NvDA while the console isn't in focus.
There could be further work in UIAHandler to stop adding duplicate textchange and caret UIA events to NVDA's event queue if the last one has not yet been processed.
@AppVeyorBot

Copy link
Copy Markdown

See test results for failed build of commit 2dc39508a4

@codeofdusk

Copy link
Copy Markdown
Contributor

@michaelDCurran commit b94f4a1 gives me a linker error:

nvdaHelperLocal.def : error LNK2001: unresolved external symbol rateLimitedUIAEventHandler_flush                        
build\x86\local\nvdaHelperLocal.lib : fatal error LNK1120: 1 unresolved externals

@michaelDCurran michaelDCurran requested a review from a team as a code owner November 26, 2023 22:58
@michaelDCurran michaelDCurran merged commit dd8a44c into master Nov 27, 2023
@michaelDCurran michaelDCurran deleted the UIAEventLimiting branch November 27, 2023 06:31
@nvaccessAuto nvaccessAuto added this to the 2024.1 milestone Nov 27, 2023
josephsl added a commit to josephsl/nvda that referenced this pull request Nov 27, 2023
…s.UIA.XamlEditableText when removin editable field support from emoji panel. Re nvaccess#15836.

Regression introduced with nvaccess#14888: Windows 11 emoji panel items are not annonced when pressing arrow keys as XAML editable text overlay class is used to mark emoji search field. Therefore, remove the newly introduced XAML editable text class from overlay class chooser.
@Adriani90

Copy link
Copy Markdown
Collaborator

Now that this is merged, is #11214 for selectively register for UIA events still needed at all in NVDA?

seanbudd pushed a commit that referenced this pull request Nov 27, 2023
…s.UIA.XamlEditableText when removin editable field support from emoji panel. Re #15836. (#15837)

Regression introduced with #14888: Windows 11 emoji panel items are not announced when pressing arrow keys as XAML editable text overlay class is used to mark emoji search field. Therefore, remove the newly introduced XAML editable text class from overlay class chooser.

Closes #15836

Summary of the issue:
Windows 11 emoji panel items are not annonced when using arrow keys to navigate the emoji panel.

Description of user facing changes
Emoji panel items are once again announced when using the arrow keys to navigate Windows 11 emoji panel.

Description of development approach
In modern keyboard/emoji panel app modue, replace regular editable text with NVDAObjects.UIA.XamlEditableText when detecting and removing edit field commands form emoji search field.
seanbudd pushed a commit that referenced this pull request Nov 28, 2023
follow up of #14888

Summary of the issue:
During development of #14888, it was observed that XAML edit fields tend to fire text change events before the caret position is changed, resulting in wrong text reporting. After using Visual Studio, I discovered that the same issue applies to the text controls in Visual Studio, notably WPF.

Description of user facing changes
In Visual Studio, the caret position is no longer sometimes reported inaccurately.

Description of development approach
use a common base class for XAML and WPF editable text.
seanbudd pushed a commit that referenced this pull request Dec 3, 2023
…p voice messages (#15840)

This pull request adds mention about the fact that the whatsapp voice messages no longer freeze nvda as per #15169, and #14888.
This pull request also fixes some mistakes and typos

Description of user facing changes
Fixes for the documentation

Description of development approach
Edited the english changes.t2t
michaelDCurran added a commit that referenced this pull request Feb 14, 2024
seanbudd pushed a commit that referenced this pull request Feb 25, 2024
…erations library on NVDA exit. (#16222)

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
Details
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.
seanbudd pushed a commit that referenced this pull request Jul 11, 2024
Partially reverts #14888, #15838

Summary of the issue:
In #14888, XamlEditableText was added to ensure that UIA events wouldn't be used to determine caret changes, as the hypothesis was that they were fired too early. In #15838, I expanded this workaround for WPF (Visual Studio) text controls.
It turns out @jcsteh found the actual cause of these issues and fixed them in #16711, allowing us to rely on events again.

Description of user facing changes
None, though caret movement would possibly be detected a little bit faster in XAML and WPF controls.

Description of development approach
Mostly reverts.
seanbudd pushed a commit that referenced this pull request May 26, 2026
Fixes #6291. Partially addresses #15786, #15850, #11002, and #14189. All of these describe the same general class of "NVDA stops responding during console flood" problem but also include symptoms (UIA event flooding, NVDA crashes destroying the console window, etc.) that are out of scope for this PR and were partly addressed in #14888 and #14067 anyway. Related to #2977 and #875.

Most of these are already closed, but the underlying root cause (which is the main thread blocking during line announcement) was never actually addressed.
Summary of the issue:

When a live-text region (such as a terminal) produces a large burst of new text in a short window, NVDA's main thread blocks announcing every line sequentially, becoming unresponsive to keyboard input until the burst finishes. In some cases this lasts tens of seconds. In other (more drastic) cases, it can last so long that the user has to sign out, restart, or force-kill NVDA.
Description of user facing changes:

NVDA no longer freezes when large bursts of text are reported in a live region.
Description of developer facing changes:

LiveText._reportNewLines is no longer guaranteed to report lines synchronously. Internally, it now registers a generator with queueHandler which yields between batches of lines so the main thread can service input during long announcements.

A new class attribute LiveText.MAX_LINES caps the number of lines announced per burst. We drop everything before that on the floor.

When a new burst arrives while a previous one is still being announced, the previous generator is cancelled and the new burst supersedes it.
Description of development approach:

Live-text regions announce new text via LiveText._reportNewLines, which was invoked synchronously from the main thread's event queue. For workloads producing substantial amounts of new lines in a short window, every line traverses the full speech pipeline sequentially without yielding, which can leave the main thread unresponsive to keyboard input for tens of seconds or longer.

This PR addresses the responsiveness problem at the announcement layer rather than at the diff or event-source layer, which were previously addressed by #14888 and #14067. Specifically:

(1) Bursts larger than MAX_LINES are truncated to the most recent N lines. Skipping the oldest lines was chosen over skipping the newest because most flooding scenarios carry their actionable content at the end.

(2) Reporting now happens via a generator registered with queueHandler.registerGeneratorObject, yielding every 5 lines. This is the same primitive previously used by speech.speakSpelling and the original sayAll. Between yields, the main thread services the event queue, which keeps NVDA responsive.

(3) A handler is registered with pre_speechCanceled so that pressing control (or any other speech-cancel trigger) also cancels the in-flight generator. Perceptibly this may not cancel immediately and up to 5 lines may still be announced past the cancel, but internally the generator won't proceed past that iteration.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

conceptApproved Similar 'triaged' for issues, PR accepted in theory, implementation needs review. merge-early Merge Early in a developer cycle

Projects

None yet

Development

Successfully merging this pull request may close these issues.