Bug 147211 - AX: AccessibilityNodeObject::childrenChanged() generates too many AXLiveRegionChanged notifications
Summary: AX: AccessibilityNodeObject::childrenChanged() generates too many AXLiveRegio...
Alias: None
Product: WebKit
Classification: Unclassified
Component: Accessibility (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
Keywords: InRadar
Depends on:
Reported: 2015-07-22 17:09 PDT by Nan Wang
Modified: 2017-01-19 21:19 PST (History)
12 users (show)

See Also:

patch (12.81 KB, patch)
2015-07-22 19:01 PDT, Nan Wang
cfleizach: review-
Details | Formatted Diff | Diff
patch (13.51 KB, patch)
2015-07-23 12:23 PDT, Nan Wang
no flags Details | Formatted Diff | Diff
patch (13.99 KB, patch)
2015-07-23 12:43 PDT, Nan Wang
no flags Details | Formatted Diff | Diff
patch (14.21 KB, patch)
2015-07-23 12:58 PDT, Nan Wang
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews103 for mac-mavericks (759.36 KB, application/zip)
2015-07-23 13:27 PDT, Build Bot
no flags Details
patch (18.98 KB, patch)
2015-07-23 16:45 PDT, Nan Wang
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nan Wang 2015-07-22 17:09:16 PDT
Some websites like Facebook will recalculate the live region layout while navigating through VoiceOver, and causing WebKit to post tens of thousands of AXLiveRegionChanged notifications within second. This will make the VO hang for a long time.
Comment 1 Nan Wang 2015-07-22 17:09:42 PDT
Comment 2 Radar WebKit Bug Importer 2015-07-22 17:09:52 PDT
Comment 3 Nan Wang 2015-07-22 19:01:09 PDT
Created attachment 257322 [details]
Comment 4 chris fleizach 2015-07-23 09:05:11 PDT
Comment on attachment 257322 [details]

View in context: https://bugs.webkit.org/attachment.cgi?id=257322&action=review

> Source/WebCore/ChangeLog:9
> +        AccessibilityNodeObject::childrenChanged() being called many times in a short period of time may cause

change to 

AccessibilityNodeObject::childrenChanged() can be called repeatedly, generating a live region change notification each time. 
Sometimes, so many happen that VoiceOver hangs. We can use  a timer to make sure that we coalesce these notifications.

> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:106
> +    , liveRegionNotificationPostTimer(*this, &AccessibilityNodeObject::liveRegionChangedNotificationPostTimerFired)

i think this think should be on AXObjectCache. Right now this timer is being created for every AXObject, which also means that if you stop one then another object
will just as easily fire its own object. we want to have one instance of this timer per axOBjectCache

> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:162
> +            if (liveRegionNotificationPostTimer.isActive())

if the timer is active, we can probably do the opposite and just skip this code, since we now the active timer will now cover this case too?

> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:166
> +            liveRegionNotificationPostTimer.startOneShot(AccessibilityLiveRegionChangeNotificationInterval);

will this work just as well if we just use 0 as the time? i have a feeling that a lot of these are being generated within the same call stack so a 0 second timer may work just as well

> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:183
> +    for (AccessibilityObject* object : liveRegionObjectsSet)

this can be auto& object :  liverRegion..

> Source/WebCore/accessibility/AccessibilityNodeObject.h:210
> +    Timer liveRegionNotificationPostTimer;

internal ivars should all start with m_*

> Source/WebCore/accessibility/AccessibilityNodeObject.h:211
> +    HashSet<AccessibilityObject*> liveRegionObjectsSet;

this should be HashSet<RefPtr<AccessibilityObject>>

> LayoutTests/platform/mac/accessibility/aria-liveregions-notifications.html:48
> +        setTimeout("operation4()", 40);

if we make the timeout to be 0 seconds this may not be necessary

> LayoutTests/platform/mac/accessibility/aria-multiple-liveregions-notification.html:21
> +    description("This tests that ARIA live regions are sending out the correct notifications. We perform two operations to each aria region, at the end it should trigger 2 live region notifications");

2 -> two

> LayoutTests/platform/mac/accessibility/aria-multiple-liveregions-notification.html:31
> +            if (liveRegionChangeCount == 2) {

this doesn't necessarily catch if we only have two notifications.
we might have a 100 being fired but after 2 we cleanup.

i think in this case we should have a timeout to finish the test AFTER we get two notifications. It should be short obviously but
that will allow us to check if more notifications have come in

> LayoutTests/platform/mac/accessibility/aria-multiple-liveregions-notification.html:34
> +               window.testRunner.notifyDone();

use finishJSTest() instead of notifyDone

> LayoutTests/platform/mac/accessibility/aria-multiple-liveregions-notification.html:40
> +        window.testRunner.waitUntilDone();

use window.jsTestIsAsync = true instead of this

> LayoutTests/platform/mac/accessibility/aria-multiple-liveregions-notification.html:42
> +        document.getElementById("liveregion1").focus();

i don't like using the method of focus() then getting focusedElement.
instead use "accessibilityController.accessibleElementById('liveRegion1')

> LayoutTests/platform/mac/accessibility/aria-multiple-liveregions-notification.html:53
> +        operation1();

these should have meaningful names instead of operation*
Comment 5 Nan Wang 2015-07-23 12:23:54 PDT
Created attachment 257361 [details]
Comment 6 chris fleizach 2015-07-23 12:32:50 PDT
Comment on attachment 257361 [details]

View in context: https://bugs.webkit.org/attachment.cgi?id=257361&action=review

> Source/WebCore/accessibility/AXObjectCache.cpp:1164
> +    for (auto& object  : m_liveRegionObjectsSet)

spacing is wrong

> Source/WebCore/accessibility/AXObjectCache.h:297
> +    void liveRegionChangedNotificationPostTimerFired();

this should probably go with the other functions, so that the ivars stay together

> LayoutTests/platform/mac/accessibility/aria-liveregions-notifications-always-sent.html:40
> +            // Since there is a time delay for a AXLiveRegionChanged notification to be sent,

see comment below about comment

> LayoutTests/platform/mac/accessibility/aria-liveregions-notifications.html:43
> +        // Since there is a time delay for a AXLiveRegionChanged notification to be sent,

this comment should be "AXLiveRegionsChanged notifications are sent after a delay, so in order to ensure we get an notification for each operation we should avoid coalescing which can occur"

> LayoutTests/platform/mac/accessibility/aria-multiple-liveregions-notification.html:75
> +        if (liveRegionChangeCount == 2) {

you should add a shouldBeTrue(liveRegionChangeCount, 2)
Comment 7 Nan Wang 2015-07-23 12:43:04 PDT
Created attachment 257364 [details]
Comment 8 chris fleizach 2015-07-23 12:48:12 PDT
Comment on attachment 257364 [details]

View in context: https://bugs.webkit.org/attachment.cgi?id=257364&action=review

> Source/WebCore/accessibility/AXObjectCache.cpp:135
> +    , m_liveRegionChangedPostTimer(*this, &AXObjectCache::liveRegionChangedNotificationPostTimerFired)

when the AXObjectCache is torn down, we should probably stop this timer like

Comment 9 Nan Wang 2015-07-23 12:58:17 PDT
Created attachment 257369 [details]
Comment 10 Build Bot 2015-07-23 13:27:29 PDT
Comment on attachment 257369 [details]

Attachment 257369 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/4542849995505664

New failing tests:
Comment 11 Build Bot 2015-07-23 13:27:32 PDT
Created attachment 257372 [details]
Archive of layout-test-results from ews103 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-mavericks  Platform: Mac OS X 10.9.5
Comment 12 Nan Wang 2015-07-23 16:45:49 PDT
Created attachment 257406 [details]

* Stop the timer before adding object to HashSet, to avoid clearing and adding element to the HashSet happen at the same time.
* Added a delay to the timer.
* Fixed old tests to use JSTest.
Comment 13 WebKit Commit Bot 2015-07-23 17:33:54 PDT
Comment on attachment 257406 [details]

Clearing flags on attachment: 257406

Committed r187278: <http://trac.webkit.org/changeset/187278>
Comment 14 WebKit Commit Bot 2015-07-23 17:34:00 PDT
All reviewed patches have been landed.  Closing bug.
Comment 15 Alexey Proskuryakov 2015-07-25 18:00:32 PDT
These tests are super flaky, failing most of the time on debug bots:


Filed bug 147299 about that.