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 filter out MSAA events for the currently focused object, even if the winEvent limit has been exceeded for that thread #11520

Merged
merged 10 commits into from Sep 4, 2020

Conversation

michaelDCurran
Copy link
Member

@michaelDCurran michaelDCurran commented Aug 25, 2020

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.
@michaelDCurran michaelDCurran requested a review from feerrenrut Aug 25, 2020
@michaelDCurran michaelDCurran added this to the 2020.3 milestone Aug 25, 2020
@AppVeyorBot
Copy link

AppVeyorBot commented Aug 25, 2020

See test results for failed build of commit e941b52f9f

Copy link
Collaborator

@leonardder leonardder left a comment

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?

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

@leonardder leonardder Aug 25, 2020

Choose a reason for hiding this comment

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

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
Copy link
Member Author

michaelDCurran commented Aug 25, 2020

…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
leonardder previously approved these changes Aug 25, 2020
Copy link
Collaborator

@leonardder leonardder left a comment

Looks good to me now.

@lukaszgo1
Copy link
Contributor

lukaszgo1 commented Aug 25, 2020

It's possible this PR fixes #10484.

@b-werner
Copy link

b-werner commented Aug 25, 2020

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

Copy link
Member

@feerrenrut feerrenrut left a comment

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

@@ -75,10 +75,12 @@ def addEvent(
self._genericEventCache[(eventID, window, objectID, childID, threadID)] = next(self._eventCounter)
return True

def flushEvents(self):
def flushEvents(self, alwaysAllowedObjects=None):
Copy link
Member

@feerrenrut feerrenrut Aug 25, 2020

Choose a reason for hiding this comment

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

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):

Copy link
Member Author

@michaelDCurran michaelDCurran Aug 31, 2020

Choose a reason for hiding this comment

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

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

Copy link
Member

@feerrenrut feerrenrut Aug 31, 2020

Choose a reason for hiding this comment

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

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:

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

source/IAccessibleHandler/orderedWinEventLimiter.py Outdated Show resolved Hide resolved
@AppVeyorBot
Copy link

AppVeyorBot commented Aug 26, 2020

See test results for failed build of commit 90d327b48b

@AppVeyorBot
Copy link

AppVeyorBot commented Aug 31, 2020

See test results for failed build of commit 07d6218bd8

…flushEvents and improve logic readability around skipping events due to exceeded count per thread.
@feerrenrut
Copy link
Member

feerrenrut commented Aug 31, 2020

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
Copy link
Member

feerrenrut commented Sep 3, 2020

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


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.
Copy link
Member Author

@michaelDCurran michaelDCurran Sep 4, 2020

Choose a reason for hiding this comment

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

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
Copy link

AppVeyorBot commented Sep 4, 2020

See test results for failed build of commit cfd3f6e676

@michaelDCurran
Copy link
Member Author

michaelDCurran commented Sep 4, 2020

@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
feerrenrut previously approved these changes Sep 4, 2020
# equal to MAX_WINEVENTS_PER_THREAD=10
# Plus 4 focus events,
# Plus the last menu event.
# All totalling 15.
self.assertEqual(len(windowIds), 15)
Copy link
Member

@feerrenrut feerrenrut Sep 4, 2020

Choose a reason for hiding this comment

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

Great! 🥳

# 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),
Copy link
Member

@feerrenrut feerrenrut Sep 4, 2020

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

@michaelDCurran michaelDCurran merged commit c5aa20f into master Sep 4, 2020
1 check passed
@michaelDCurran michaelDCurran deleted the keepEventsForMSAAFocus branch Sep 4, 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

6 participants