Bug 149510

Summary: accessibility/mac/aria-expanded-notifications.html is flaky
Product: WebKit Reporter: Beth Dakin <bdakin>
Component: AccessibilityAssignee: Myles C. Maxfield <mmaxfield>
Status: RESOLVED FIXED    
Severity: Normal CC: aboxhall, apinheiro, ap, cfleizach, commit-queue, dmazzoni, jcraig, jdiggs, mario, mmaxfield, samuel_white, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=149218
Attachments:
Description Flags
Test List
none
Failure diff
none
Patch cfleizach: review+

Beth Dakin
Reported 2015-09-23 13:29:27 PDT
https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=accessibility%2Fmac%2Faria-expanded-notifications.html shows that this test has been flaky on the Mavericks and Yosemite bots.
Attachments
Test List (518 bytes, text/plain)
2015-09-29 14:13 PDT, Myles C. Maxfield 		no flags
Details
Failure diff (499 bytes, text/plain)
2015-09-29 14:33 PDT, Myles C. Maxfield 		no flags
Details
Patch (3.78 KB, patch)
2015-09-30 23:27 PDT, Myles C. Maxfield 		cfleizach: review+
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1 2015-09-23 13:29:40 PDT
<rdar://problem/22826005>
Beth Dakin
Comment 2 2015-09-23 13:35:29 PDT
Marked it flaky: http://trac.webkit.org/changeset/190183
Radar WebKit Bug Importer
Comment 3 2015-09-23 13:36:05 PDT
<rdar://problem/22826112>
Myles C. Maxfield
Comment 4 2015-09-29 14:12:31 PDT
Almost certainly caused by http://trac.webkit.org/changeset/189668
Myles C. Maxfield
Comment 5 2015-09-29 14:12:57 PDT
Can reproducing after running a few dozen times in a row with the attached test list.
Myles C. Maxfield
Comment 6 2015-09-29 14:13:23 PDT
Created attachment 262106 [details] Test List
Myles C. Maxfield
Comment 7 2015-09-29 14:33:16 PDT
Attaching the failure diff
Myles C. Maxfield
Comment 8 2015-09-29 14:33:43 PDT
Created attachment 262109 [details] Failure diff
Myles C. Maxfield
Comment 9 2015-09-30 13:15:47 PDT
(In reply to comment #4) > Almost certainly caused by http://trac.webkit.org/changeset/189668 Verified this.
Myles C. Maxfield
Comment 10 2015-09-30 13:21:21 PDT
(In reply to comment #9) > (In reply to comment #4) > > Almost certainly caused by http://trac.webkit.org/changeset/189668 > > Verified this. By "verified," I mean that, when the patch is reverted, I can no longer repro the issue.
Myles C. Maxfield
Comment 11 2015-09-30 22:08:57 PDT
Breaking in AccessibilityController::addNotificationListener() seems to make this reproduce 100%. Seems like a race.
Myles C. Maxfield
Comment 12 2015-09-30 23:13:41 PDT
The problem is triggered by our resumable parser. if (elapsedTime > m_parserTimeLimit) session.needsYield = true; AXLoadComplete gets called synchronously from DocumentLoader::finishedLoading(). This means that the order of the AXLoadComplete message isn't FIFO: If the parser yielded previously, the AXLoadComplete will be handled after some AX messages had been handled. If the parser hadn't yielded, the AXLoadComplete will be the first message processed (no matter what had been queued up before it). I assume that making AXLoadComplete asynchronous is not possible. Therefore, it seems the test should be relaxed to handle the case of various orderings.
Myles C. Maxfield
Comment 13 2015-09-30 23:27:10 PDT
Created attachment 262236 [details] Patch
Alexey Proskuryakov
Comment 14 2015-09-30 23:27:17 PDT
> I assume that making AXLoadComplete asynchronous is not possible. This actually sounds like it should be good. Loading is asynchronous in nature, so queueing the notification should not make an observable difference other than in this edge case. Chris, does that make sense?
Alexey Proskuryakov
Comment 15 2015-09-30 23:29:19 PDT
Comment on attachment 262236 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=262236&action=review > LayoutTests/accessibility/mac/aria-expanded-notifications-expected.txt:12 > TEST COMPLETE > -Notification: AXLoadComplete > Notification: AXRowCountChanged This should be fixed by properly using js-test functionality - TEST COMPLETE is supposed to be last. > LayoutTests/platform/mac/TestExpectations:-1334 > -webkit.org/b/149510 accessibility/mac/aria-expanded-notifications.html [ Pass Failure ] It seems like we would need a code change anyway for the other test that asserts.
chris fleizach
Comment 16 2015-09-30 23:30:57 PDT
Yea this sounds good. The test is not dependent on the order of these notifications If we didn't get to the bottom of this I was going to suggest filtering out the load notification Thanks!
Myles C. Maxfield
Comment 17 2015-09-30 23:31:12 PDT
Comment on attachment 262236 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=262236&action=review >> LayoutTests/platform/mac/TestExpectations:-1334 >> -webkit.org/b/149510 accessibility/mac/aria-expanded-notifications.html [ Pass Failure ] > > It seems like we would need a code change anyway for the other test that asserts. Unfortunately, it seems that other test is a separate issue :(
Myles C. Maxfield
Comment 18 2015-10-01 10:46:26 PDT
Committed r190417: <http://trac.webkit.org/changeset/190417>
Note You need to log in before you can comment on or make changes to this bug.