Bug 189091 – Flaky IntersectionObserver web platform tests involving style updates

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Ali Juma]

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.

[Ali Juma]

Created attachment 348417 [details]
Suppress flaky failures

[Ali Juma]

Comment on attachment 348417 [details]
Suppress flaky failures

Clearing flags on attachment: 348417

Committed r235471: <https://trac.webkit.org/changeset/235471>

[Ali Juma]

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]

Created attachment 348432 [details]
Generalize suppression

[Ali Juma]

Comment on attachment 348432 [details]
Generalize suppression

Clearing flags on attachment: 348432

Committed r235479: <https://trac.webkit.org/changeset/235479>

[Darin Adler]

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]

(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]

Created attachment 351259 [details]
Patch

[Simon Fraser (smfr)]

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]

Created attachment 352494 [details]
Patch for landing

[Ali Juma]

(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 on attachment 352494 [details]
Patch for landing

Clearing flags on attachment: 352494

Committed r237218: <https://trac.webkit.org/changeset/237218>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/45337305>