RESOLVED FIXED 189091
Flaky IntersectionObserver web platform tests involving style updates 
https://bugs.webkit.org/show_bug.cgi?id=189091
Summary Flaky IntersectionObserver web platform tests involving style updates
Ali Juma
Reported 2018-08-29 11:35:04 PDT
These tests are flaky on Mac WK1: web-platform-tests/intersection-observer/bounding-box.html web-platform-tests/intersection-observer/display-none.html web-platform-tests/intersection-observer/containing-block.html My suspicion is that the Timer-based scheduling we're using doesn't always generate updates when the tests expect, since the tests are written in a way that assumes HTMLEventLoop-driven scheduling.
Attachments
Suppress flaky failures (1.65 KB, patch)
2018-08-29 11:42 PDT, Ali Juma 		no flags
DetailsFormatted DiffDiff
Generalize suppression (2.77 KB, patch)
2018-08-29 14:18 PDT, Ali Juma 		no flags
DetailsFormatted DiffDiff
Patch (21.21 KB, patch)
2018-10-01 10:31 PDT, Ali Juma 		no flags
DetailsFormatted DiffDiff
Patch for landing (21.16 KB, patch)
2018-10-16 13:29 PDT, Ali Juma 		no flags
DetailsFormatted DiffDiff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.
Ali Juma
Comment 1 2018-08-29 11:42:20 PDT
Created attachment 348417 [details] Suppress flaky failures
Ali Juma
Comment 2 2018-08-29 11:44:30 PDT
Comment on attachment 348417 [details] Suppress flaky failures Clearing flags on attachment: 348417 Committed r235471: <https://trac.webkit.org/changeset/235471>
Ali Juma
Comment 3 2018-08-29 13:59:53 PDT
What's interesting about these three test is they all involve changes to style which change intersection, and in the flaky failures we're not getting an intersection update at the point where the test expects one. For changes to style, we first need a style update to happen, which then triggers a layout update, which then schedules an intersection update, and that in turn schedules notifications. The WPTs expect layout and style to be updated right after the next rAF, and intersections to be updated right after that. One thing we can do to reduce the delay for notifications is to fire them immediately after the intersection update rather than scheduled on zero-second Timer (since the intersection update itself is already scheduled using a Timer). These tests are also flaking on WK2, so I'm going to generalize the suppression until we have a fix landed and the tests are no longer flaking.
Ali Juma
Comment 4 2018-08-29 14:18:31 PDT
Created attachment 348432 [details] Generalize suppression
Ali Juma
Comment 5 2018-08-29 14:20:25 PDT
Comment on attachment 348432 [details] Generalize suppression Clearing flags on attachment: 348432 Committed r235479: <https://trac.webkit.org/changeset/235479>
Darin Adler
Comment 6 2018-08-29 18:16:17 PDT
Given the description above I suspect these flaky results reflect a real bug that could adversely affect interoperability with other browser engines. Even though these timing properties are not well specified, I think they could possibly be important for compatibility, not just for the test harness.
Ali Juma
Comment 7 2018-08-30 13:40:36 PDT
(In reply to Darin Adler from comment #6) > Given the description above I suspect these flaky results reflect a real bug > that could adversely affect interoperability with other browser engines. > Even though these timing properties are not well specified, I think they > could possibly be important for compatibility, not just for the test harness. Yes, trying to make the timing of these events more predictable (and more consistent with other browsers) seems worth doing. The approach I'm considering is scheduling these updates as part of the layer update lifecycle (e.g., scheduling them right after flushLayers()). At that point we know that layout and style are up-to-date, so it's safe to compute intersections.
Ali Juma
Comment 8 2018-10-01 10:31:57 PDT
Created attachment 351259 [details] Patch
Simon Fraser (smfr)
Comment 9 2018-10-11 16:33:40 PDT
Comment on attachment 351259 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=351259&action=review > Source/WebKit/WebProcess/WebPage/AcceleratedDrawingArea.cpp:258 > +#if ENABLE(INTERSECTION_OBSERVER) > + m_webPage.updateIntersectionObservations(); > +#endif Would be nice to add a "willDisplayPage" bottleneck to WebPage and/or Page and move the #ifdef lower down.
Ali Juma
Comment 10 2018-10-16 13:29:07 PDT
Created attachment 352494 [details] Patch for landing
Ali Juma
Comment 11 2018-10-16 13:30:26 PDT
(In reply to Simon Fraser (smfr) from comment #9) > Comment on attachment 351259 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=351259&action=review > > > Source/WebKit/WebProcess/WebPage/AcceleratedDrawingArea.cpp:258 > > +#if ENABLE(INTERSECTION_OBSERVER) > > + m_webPage.updateIntersectionObservations(); > > +#endif > > Would be nice to add a "willDisplayPage" bottleneck to WebPage and/or Page > and move the #ifdef lower down. Done, added willDisplayPage to WebPage and Page and moved the #if down to Page.
WebKit Commit Bot
Comment 12 2018-10-17 06:52:38 PDT
Comment on attachment 352494 [details] Patch for landing Clearing flags on attachment: 352494 Committed r237218: <https://trac.webkit.org/changeset/237218>
WebKit Commit Bot
Comment 13 2018-10-17 06:52:40 PDT
All reviewed patches have been landed. Closing bug.
Radar WebKit Bug Importer
Comment 14 2018-10-17 06:53:26 PDT
<rdar://problem/45337305>
Note You need to log in before you can comment on or make changes to this bug.