Don't filter out MSAA events for the currently focused object, even if the winEvent limit has been exceeded for that thread by michaelDCurran · Pull Request #11520 · nvaccess/nvda

michaelDCurran · 2020-08-25T01:45:21Z

Link to issue number:

None.

Summary of the issue:

If an app floods NVDA with a lot of MSAA events and therefore NVDA starts ignoring events due to the winEvent limit for that thread being exceeded, important MSAA events for the currently focused object may be ignored.
An example of this is on https://drive.google.com in Google Chrome:
Press gn to go to the navigation treeview.
When pressing right and left arrow to expand / collapse particular directories in the tree, NVDA may fail to announce the expanded / collapsed state, if the directory contains a lot of subdirectories.
Google Chrome seems to fire a great deal of locationChange, stateChange, show and hide events for the subdirectories. And because of this, the important stateChange event on the focused item is dropped by NVDA.

Description of how this pull request fixes the issue:

OrderedWinEventLimitor.flushEvents now takes a list of winEvent params, for which it should always allow events for, even if the winEvent limit for that thread has been exceeded.
IAccessibleHandler.pumpAll now calls flushEvents giving it the winEvent params of the currently focused object (if it is an MSAA object), to ensure that all winEvents for the currently focused object are received.
The hard limit of 500 winEvents per core pump in IAccessibleHandler has now been removed as the OrderedWinEventLimitor already limits to 10 events per thread, and even when allowing all events for the focused object, this will not be many more as we don't produce duplicates.

Testing performed:

Expanded and collapsed a large directory in the drive.google.com navigation tree, ensuring that NVDA announced the expand / collapsed state, where as before this change it did not.

Known issues with pull request:

None.

Change log entry:

Bug fixes:

The expanded / collapsed state of directories in the navigation treeview on drive.google.com is always reported by NVDA.

…ntly focused object, even if the winEvent limit has been exceeded for that thread.

AppVeyorBot · 2020-08-25T01:54:29Z

Unit test failure
Build (for testing PR)

See test results for failed build of commit e941b52f9f

…bjects is now optional.

LeonarddeR

How about applications that fire lots of erroneous events on the focus object itself? They would no longer be ignored, right?
Rather than allowing all events for the focus object, have you considered applying a multiplier to MAX_WINEVENTS_PER_THREAD for allowed objects, or a separate counter for every allowed object?

LeonarddeR · 2020-08-25T05:44:29Z

-				continue
+			if not alwaysAllowedObjects or k[1:-1] not in alwaysAllowedObjects:
+				threadCount = threadCounters.get(k[-1], 0)
+				threadCounters[k[-1]] = threadCount + 1


A suggestion: May be an allowed object, while events for it should be allowed, yet should increase the counter. This ensures that focus event floading will block other events.

michaelDCurran · 2020-08-25T07:09:46Z

When events are added to the limitor, they are stored in a dictionary, therefore there will only ever be one event of a particular eventID and winEvent params. Thus, For a specific object, it would be very hard for an app to flood us. They would essentially need to fire one of every registered event type, which is highly unlikely. We could perhaps give events for the focus object their own max of 10 I guess...

…d still go up if the event is for the focus, even though we won't drop the event if we are over the limit.

LeonarddeR

Looks good to me now.

lukaszgo1 · 2020-08-25T08:52:20Z

It's possible this PR fixes #10484.

b-werner · 2020-08-25T13:14:02Z

Google has been trying to fix this on their end as well. See chromium/chromium@a2e28ea#diff-2b5622f887b43b804657e10e077be787

feerrenrut

Thanks for taking this on @michaelDCurran. Looks like like it is pretty close.

feerrenrut · 2020-08-25T13:42:38Z

 		return True

-	def flushEvents(self):
+	def flushEvents(self, alwaysAllowedObjects=None):


Could you please add tests for this to tests.unit.test_orderedWinEventLimiter.TestOrderedWinEventLimiter

Should this check for None in tuple elements? A test to show what is expected in this case would be good.

Please also add typing information for alwaysAllowedObjects. It seems to be:

Optional[List[Tuple[ int, # windowHandle int, # objectID int, # childID ]]]

You could define a reusable type (that can be used in source/IAccessibleHandler/__init__.py also:

Suggested change

def flushEvents(self, alwaysAllowedObjects=None):

AllowedObjectType = Tuple[

int, # windowHandle

int, # objectID

int, # childID

]

def flushEvents(self, alwaysAllowedObjects: Optional[List[AllowedObjectType]] = None):

Should this check for None in tuple elements? A test to show what is expected in this case would be good.

I don't understand what you mean here. Do you mean if one of the elements in one of the tuples is None? E.g. windowHandle is None? In that case, specific events would simply not match on that (poorly defined) object...

Do you mean if one of the elements in one of the tuples is None?

Yes, one or more.

In that case, specific events would simply not match on that (poorly defined) object...

That's fine. A test helps to document that it is an expected arg value, and the behavior in this case is intentional.

I got thinking about this because of this line:

nvda/source/IAccessibleHandler/__init__.py

Line 868 in 1d5e5ce

if isinstance(focus, NVDAObjects.IAccessible.IAccessible) and focus.event_objectID is not None:

AppVeyorBot · 2020-08-26T05:28:31Z

PR introduces Flake8 errors
Build (for testing PR)

See test results for failed build of commit 90d327b48b

AppVeyorBot · 2020-08-31T07:20:18Z

Unit test failure
Build (for testing PR)

See test results for failed build of commit 07d6218bd8

…flushEvents and improve logic readability around skipping events due to exceeded count per thread.

feerrenrut · 2020-08-31T08:02:01Z

It looks like tests were added and then removed. I assume this is being worked on right now, so I'll come back to review it later. Please re-request a review when it's ready. 😄

feerrenrut · 2020-09-03T14:33:35Z

I have added several tests, but there was some surprising behavior. @michaelDCurran can you take a look at these please?

… emitted now, not 11.

… error for events per thread, and no longer expect certain failures

michaelDCurran · 2020-09-04T04:50:34Z

+
+		for n in range(500):  # exceed the limit for a single thread
+			eventId = nonSpecialCaseEvents[n % len(nonSpecialCaseEvents)]
+			# same thread, different object. Use a second object to aid tracking.


I don't get this comment. Perhaps it was incorrectly copied from another method? source is hardcoded to (2,2,2) in this case. I.e. always the same object.

AppVeyorBot · 2020-09-04T05:54:36Z

PR introduces Flake8 errors
Build (for testing PR)

See test results for failed build of commit cfd3f6e676

michaelDCurran · 2020-09-04T06:12:00Z

@feerrenrut There was an off-by-one error for threadCount in OrderedWinEventLimiter. I have fixed this, so now 10 events (not 11) are now emitted. I was then able to remove the expectedFailure decorators off the tests. However, I did need to fix a couple of the other tests, that seemed to have hardcoded the assumption about 11.
I'm happy with the new tests. If you take a look at my fixes, and if happy then approve again and I can merge this.

feerrenrut · 2020-09-04T10:19:46Z

+		#  equal to MAX_WINEVENTS_PER_THREAD=10
+		# Plus 4 focus events,
+		# Plus the last menu event.
+		#  All totalling 15.
+		self.assertEqual(len(windowIds), 15)


Great! 🥳

feerrenrut · 2020-09-04T10:28:46Z

+			# Two Foreground events, because they are added to multiple queues.
+			# Added to both _focusEventCache and _genericEventCache
+			# See also test_limitEventsPerThread
+			(winUser.EVENT_SYSTEM_FOREGROUND, *allowedSource),
+			(winUser.EVENT_SYSTEM_FOREGROUND, *allowedSource),


I think it would be good to create a new "good first issue" to address this. The tests mean it is fairly well defined, the intent is easy to understand, and is unlikely to require a big change.

IAccessibleHandler: Still allow processing of winEvents for the curre…

310d89b

…ntly focused object, even if the winEvent limit has been exceeded for that thread.

michaelDCurran requested a review from feerrenrut August 25, 2020 01:45

michaelDCurran added this to the 2020.3 milestone Aug 25, 2020

IAccessibleHandler.OrderedWinEventLimitor.flushEvents: alwaysAllowedO…

39e7c71

…bjects is now optional.

LeonarddeR reviewed Aug 25, 2020

View reviewed changes

OrderedWinEventLimitor.flushEvents: counts of events per thread shoul…

68375d2

…d still go up if the event is for the focus, even though we won't drop the event if we are over the limit.

LeonarddeR previously approved these changes Aug 25, 2020

View reviewed changes

feerrenrut suggested changes Aug 25, 2020

View reviewed changes

michaelDCurran dismissed LeonarddeR’s stale review via dc89550 August 26, 2020 05:19

michaelDCurran force-pushed the keepEventsForMSAAFocus branch from dc89550 to 68375d2 Compare August 31, 2020 06:34

IAccessibleHandler: add typing information to OrderedWinEventLimitor.…

1d5e5ce

…flushEvents and improve logic readability around skipping events due to exceeded count per thread.

michaelDCurran force-pushed the keepEventsForMSAAFocus branch from 7316138 to 1d5e5ce Compare August 31, 2020 07:42

Tests for limit of events per thread

c1d2b5c

feerrenrut mentioned this pull request Sep 3, 2020

Merge alpha into beta #11549

Merged

michaelDCurran added 3 commits September 4, 2020 14:23

Merge branch 'master' into keepEventsForMSAAFocus

bc3b1a4

OrderedWinEventLimiter: fix off-by-one error: 10 events per thread is…

43c1453

… emitted now, not 11.

OrderedWinEventLimiter unit tests: remove work-arounds for off-by-one…

b232238

… error for events per thread, and no longer expect certain failures

michaelDCurran commented Sep 4, 2020

View reviewed changes

Fix linting issue

b86d518

feerrenrut previously approved these changes Sep 4, 2020

View reviewed changes

Update what's new

702dcbe

michaelDCurran dismissed feerrenrut’s stale review via 702dcbe September 4, 2020 20:53

michaelDCurran merged commit c5aa20f into master Sep 4, 2020

michaelDCurran deleted the keepEventsForMSAAFocus branch September 4, 2020 21:16

-	def flushEvents(self, alwaysAllowedObjects=None):
+	AllowedObjectType = Tuple[
+		int,  # windowHandle
+		int,  # objectID
+		int,  # childID
+	]
+	def flushEvents(self, alwaysAllowedObjects: Optional[List[AllowedObjectType]] = None):

Uh oh!

Conversation

michaelDCurran commented Aug 25, 2020

Link to issue number:

Summary of the issue:

Description of how this pull request fixes the issue:

Testing performed:

Known issues with pull request:

Change log entry:

Uh oh!

AppVeyorBot commented Aug 25, 2020

Uh oh!

LeonarddeR left a comment

Choose a reason for hiding this comment

Uh oh!

LeonarddeR Aug 25, 2020

Choose a reason for hiding this comment

Uh oh!

michaelDCurran commented Aug 25, 2020 via email

Uh oh!

LeonarddeR left a comment

Choose a reason for hiding this comment

Uh oh!

lukaszgo1 commented Aug 25, 2020

Uh oh!

b-werner commented Aug 25, 2020

Uh oh!

feerrenrut left a comment

Choose a reason for hiding this comment

Uh oh!

feerrenrut Aug 25, 2020

Choose a reason for hiding this comment

Uh oh!

michaelDCurran Aug 31, 2020 • edited by feerrenrut Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

feerrenrut Aug 31, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

AppVeyorBot commented Aug 26, 2020

Uh oh!

AppVeyorBot commented Aug 31, 2020

Uh oh!

feerrenrut commented Aug 31, 2020

Uh oh!

feerrenrut commented Sep 3, 2020

Uh oh!

michaelDCurran Sep 4, 2020

Choose a reason for hiding this comment

Uh oh!

AppVeyorBot commented Sep 4, 2020

Uh oh!

michaelDCurran commented Sep 4, 2020

Uh oh!

feerrenrut Sep 4, 2020

Choose a reason for hiding this comment

Uh oh!

feerrenrut Sep 4, 2020

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

michaelDCurran Aug 31, 2020 •

edited by feerrenrut

Loading