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

RESOLVED FIXED 205168

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

https://bugs.webkit.org/show_bug.cgi?id=205168

Summary REGRESSION: [Mac wk1] imported/w3c/web-platform-tests/mathml/presentation-mar...

Truitt Savell

Reported 2019-12-12 09:36:20 PST

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

Attachments
WIP (506 bytes, patch) 2020-01-17 22:29 PST, Ryosuke Niwa	no flags	Details Formatted Diff Diff
WIP2 (710 bytes, patch) 2020-01-27 20:22 PST, Ryosuke Niwa	no flags	Details Formatted Diff Diff
Fixes the bug and add a test (8.94 KB, patch) 2020-01-28 16:49 PST, Ryosuke Niwa	no flags	Details Formatted Diff Diff
Patch for landing (9.11 KB, patch) 2020-01-29 16:29 PST, Ryosuke Niwa	no flags	Details Formatted Diff Diff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.

Radar WebKit Bug Importer

Comment 1 2019-12-12 09:37:22 PST

<rdar://problem/57880452>

Truitt Savell

Comment 2 2019-12-12 10:19:29 PST

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

Truitt Savell

Comment 3 2019-12-12 10:26:58 PST

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

youenn fablet

Comment 4 2019-12-12 22:36:14 PST

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

Frédéric Wang (:fredw)

Comment 5 2019-12-12 22:53:17 PST

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

Comment 6 2020-01-13 23:28:49 PST

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

Comment 7 2020-01-14 21:15:54 PST

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

Ryosuke Niwa

Comment 8 2020-01-17 21:50:01 PST

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

Ryosuke Niwa

Comment 9 2020-01-17 21:52:21 PST

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

Comment 10 2020-01-17 22:29:27 PST

Created attachment 388129 [details] WIP

Ryosuke Niwa

Comment 11 2020-01-17 22:31:24 PST

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)

Comment 12 2020-01-21 11:52:18 PST

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

Ryosuke Niwa

Comment 13 2020-01-21 21:29:58 PST

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 14 2020-01-21 21:37:30 PST

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

Comment 15 2020-01-27 17:12:23 PST

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

Comment 16 2020-01-27 20:22:43 PST

Created attachment 388963 [details] WIP2

Ryosuke Niwa

Comment 17 2020-01-28 16:49:59 PST

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

Frédéric Wang (:fredw)

Comment 18 2020-01-28 23:30:52 PST

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 19 2020-01-29 00:55:02 PST

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

Comment 20 2020-01-29 14:33:45 PST

(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

Comment 21 2020-01-29 16:29:48 PST

Created attachment 389199 [details] Patch for landing

Ryosuke Niwa

Comment 22 2020-01-29 17:24:08 PST

Comment on attachment 389199 [details] Patch for landing Wait for EWS.

WebKit Commit Bot

Comment 23 2020-01-29 17:26:38 PST

The commit-queue encountered the following flaky tests while processing attachment 389199 [details]: editing/spelling/spellcheck-async-remove-frame.html bug 158401 (authors: morrita@google.com, rniwa@webkit.org, and tony@chromium.org) The commit-queue is continuing to process your patch.

Ryosuke Niwa

Comment 24 2020-01-29 20:15:53 PST

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

Ryosuke Niwa

Comment 25 2020-01-30 20:21:41 PST

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

Note You need to log in before you can comment on or make changes to this bug.

Status RESOLVED

Resolution FIXED

Priority P2

Severity Normal

Classification Unclassified

Version WebKit Nightly Build

Hardware Unspecified

OS Unspecified

Product WebKit

Component Tools / Tests

Assignee

Ryosuke Niwa

Reported

2019-12-12 09:36 PST

Modified

2020-01-30 20:21 PST History

CC List

19 users Show

URL

Keywords InRadar

Depends on

174030

Blocks

Dependencies

tree graph