Bug 147211 – AX: AccessibilityNodeObject::childrenChanged() generates too many AXLiveRegionChanged notifications

• Status: RESOLVED
• Resolution: FIXED
• Component: Accessibility

[Nan Wang]

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.

[Nan Wang]

<rdar://problem/19908029>

[Radar WebKit Bug Importer]

<rdar://problem/21952653>

[Nan Wang]

Created attachment 257322 [details]
patch

[chris fleizach]

Comment on attachment 257322 [details]
patch

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

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

[Nan Wang]

Created attachment 257361 [details]
patch

[chris fleizach]

Comment on attachment 257361 [details]
patch

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

[Nan Wang]

Created attachment 257364 [details]
patch

[chris fleizach]

Comment on attachment 257364 [details]
patch

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

AXObjectCache::~AXObjectCache()
{
    m_notificationPostTimer.stop();

[Nan Wang]

Created attachment 257369 [details]
patch

[Build Bot]

Comment on attachment 257369 [details]
patch

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

New failing tests:
platform/mac/accessibility/aria-multiple-liveregions-notification.html
platform/mac/accessibility/aria-liveregions-notifications.html

[Build Bot]

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

[Nan Wang]

Created attachment 257406 [details]
patch

Updates:
* 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.

[WebKit Commit Bot]

Comment on attachment 257406 [details]
patch

Clearing flags on attachment: 257406

Committed r187278: <http://trac.webkit.org/changeset/187278>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Alexey Proskuryakov]

These tests are super flaky, failing most of the time on debug bots:

https://webkit-test-results.appspot.com/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=platform%2Fmac%2Faccessibility%2Faria-liveregions-notifications

Filed bug 147299 about that.