Bug 205168 – REGRESSION: [Mac wk1] imported/w3c/web-platform-tests/mathml/presentation-markup/scripts/underover-parameters-3.html is a flakey failure

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Truitt Savell]

imported/w3c/web-platform-tests/mathml/presentation-markup/scripts/underover-parameters-3.html

This test is a flakey failure on Mac

History:
https://results.webkit.org/?suite=layout-tests&test=imported%2Fw3c%2Fweb-platform-tests%2Fmathml%2Fpresentation-markup%2Fscripts%2Funderover-parameters-3.html

Diff:
--- /Volumes/Data/slave/mojave-release-tests-wk1/build/layout-test-results/imported/w3c/web-platform-tests/mathml/presentation-markup/scripts/underover-parameters-3-expected.txt
+++ /Volumes/Data/slave/mojave-release-tests-wk1/build/layout-test-results/imported/w3c/web-platform-tests/mathml/presentation-markup/scripts/underover-parameters-3-actual.txt
@@ -1,10 +1,10 @@
 
 PASS Baseline alignment 
 PASS Heights of bases 
-PASS AccentBaseHeight, UnderbarExtraDescender 
-PASS AccentBaseHeight, UnderbarVerticalGap 
-PASS AccentBaseHeight, OverbarExtraAscender 
-PASS AccentBaseHeight, OverbarVerticalGap 
+FAIL AccentBaseHeight, UnderbarExtraDescender assert_approx_equals: munderover: accent over short base expected 40 +/- 2 but got 30.5
+FAIL AccentBaseHeight, UnderbarVerticalGap assert_approx_equals: munderover: accent over short base expected 40 +/- 2 but got 30.5
+FAIL AccentBaseHeight, OverbarExtraAscender assert_approx_equals: mover: accent over short base expected 40 +/- 2 but got 30.5
+FAIL AccentBaseHeight, OverbarVerticalGap assert_approx_equals: mover: nonaccent over short base expected 140 +/- 2 but got 31.5
 
 
It looks like this test began failing around r252125

[Radar WebKit Bug Importer]

<rdar://problem/57880452>

[Truitt Savell]

Due to a lack of builds to test there is a regression range of r252087-r252115

[Truitt Savell]

marked test as failing while investigating in https://trac.webkit.org/changeset/253434/webkit

[youenn fablet]

Fred, do you have any idea what might be the issue?

[Frédéric Wang (:fredw)]

This test involves loading web fonts with specific math parameters. Not sure why the flakiness happens only on macOS WK1. Might be related to https://github.com/web-platform-tests/wpt/pull/18937 and https://bugs.webkit.org/show_bug.cgi?id=174030

[Ryosuke Niwa]

Hm... I can't reproduce this with:
./Tools/Scripts/run-webkit-tests --release -1 --iterations 100 --exit-after-n-failures 1 imported/w3c/web-platform-tests/mathml/presentation-markup/

[Ryosuke Niwa]

I've tried 100 iterations on Mac mini with debug build but can't reproduce it either.

[Ryosuke Niwa]

Now I have access to a Mac mini on which the flakiness reproduces.

[Ryosuke Niwa]

There is a race condition between when Web fonts start loading and when load event is dispatched on window. Specifically, the failure happens when the layout which triggers font loading happens after window’s load event had been dispatched. Now I'm figuring out why that happens. In theory, we should be updating the layout at least once before load event fires.

[Ryosuke Niwa]

Created attachment 388129 [details]
WIP

[Ryosuke Niwa]

Alright, updating the layout before deciding whether load event needs to be fired or not solves this problem. This is a bit problematic from perf perspective though because we'd end up triggering new layout whenever a sub resource finishes loading. We probably need to move it to right before document's ready state changes. I'd also have to check this behavior is mandated by the specification or not.

[Simon Fraser (smfr)]

Do we need to force layout synchronously, or we can we just trigger a rendering update?

[Ryosuke Niwa]

Hm... there is something strange here. Even if "load" event on the window were to fire before fonts load, "document.fonts" should be updating the style and trigger font loads.

[Ryosuke Niwa]

Comment on attachment 388129 [details]
WIP

Can't land this or any variant. It would be too risky & affects too many things at least for now.

[Ryosuke Niwa]

Finally I know why this is happening. The issue that when FontFaceSet’s constructor is invoked, it could immediately resolve its promise as long as the first layout has been updated. This is problematic since the first layout may happen way before the document finishes loading.

Youeen added this code in https://bugs.webkit.org/show_bug.cgi?id=174030 in order to address the even more aggressive behavior we once had but this is racy & inadaquete for the purpose of satisfying the following condition in the specification:

In fact, the bug which landed this fix mentions that there were previously a WebKit specific fix for this:
https://bugs.webkit.org/show_bug.cgi?id=174030#c15

[Ryosuke Niwa]

Created attachment 388963 [details]
WIP2

[Ryosuke Niwa]

Created attachment 389088 [details]
Fixes the bug and add a test

[Frédéric Wang (:fredw)]

Comment on attachment 389088 [details]
Fixes the bug and add a test

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

> LayoutTests/fast/css/font-face-set-ready-after-document-load-expected.txt:1
> +This test records the order by which load and DOMContentLoaded evnets fire relative to when document.fonts.ready is resolved.

events*

> LayoutTests/fast/css/font-face-set-ready-after-document-load.html:13
> +<p>This test records the order by which load and DOMContentLoaded evnets fire relative to when document.fonts.ready is resolved.<br>

events*

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 389088 [details]
Fixes the bug and add a test

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

So WebKit doesn't have a way to deal with the case where onload has fired and you add an stylesheet with @font-face to the document, right?

Blink and Gecko do deal with that, thought I'm not a fan of the forced layout, it seems like WebKit behavior is somewhat simpler.

Though I believe such a behavior is required by the spec per https://drafts.csswg.org/css-font-loading/#ref-for-dom-fontfaceset-readypromise-slot%E2%91%A0

It doesn't seem like WebKit implements such a thing when new font loads are triggered.

> Source/WebCore/css/FontFaceSet.h:122
> +    bool m_isDocumentLoaded { true };

This looks like it'd deserve a comment on why it's true by default... If there's no frame() there's no layout to be done, but presumably the document does load?

[Ryosuke Niwa]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #19)
> Comment on attachment 389088 [details]
> Fixes the bug and add a test
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=389088&action=review
> 
> So WebKit doesn't have a way to deal with the case where onload has fired
> and you add an stylesheet with @font-face to the document, right?
> 
> Blink and Gecko do deal with that, thought I'm not a fan of the forced
> layout, it seems like WebKit behavior is somewhat simpler.
> 
> Though I believe such a behavior is required by the spec per
> https://drafts.csswg.org/css-font-loading/#ref-for-dom-fontfaceset-
> readypromise-slot%E2%91%A0
> 
> It doesn't seem like WebKit implements such a thing when new font loads are
> triggered.

As far as I can tell, no. But the spec is super vague about things so it's unclear.

> > Source/WebCore/css/FontFaceSet.h:122
> > +    bool m_isDocumentLoaded { true };
> 
> This looks like it'd deserve a comment on why it's true by default... If
> there's no frame() there's no layout to be done, but presumably the document
> does load?

I'm gonna leave it as is since I don't think I understand this code enough to provide a good / useful comment.

[Ryosuke Niwa]

Created attachment 389199 [details]
Patch for landing

[Ryosuke Niwa]

Comment on attachment 389199 [details]
Patch for landing

Wait for EWS.

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 389199 [details]:

editing/spelling/spellcheck-async-remove-frame.html bug 158401 (authors: [e-mail], [e-mail], and [e-mail])
The commit-queue is continuing to process your patch.

[Ryosuke Niwa]

Committed r255414: <https://trac.webkit.org/changeset/255414>

[Ryosuke Niwa]

Committed r255483: <https://trac.webkit.org/changeset/255483>