- Revision
- 255414
- Author
- rn...@webkit.org
- Date
- 2020-01-29 20:15:51 -0800 (Wed, 29 Jan 2020)
Log Message
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
Reviewed by Simon Fraser.
Source/WebCore:
The flakiness of the test was caused by document.fonts.ready resolving without waiting for the document
to be loaded if FontFaceSet::FontFaceSet is created after the initial layout had been done.
r249295 introduced this racy code to address the previous behavior of WebKit, which didn't wait for
either document to finish loading or the initial layout to happen at all, and the WebKit workaround in
the offending WPT test was subsequentially removed, resulting in the flaky failure after the test was
synced up in r249760.
This patch address the underlying bug by making ready promise wait until Document::implicitClose is called.
Unfortunately, the specification is extremely vague in terms of when this promise is resolved but new
behavior of WebKit is more or less with Firefox (Chrome seems to have a similar race condition as WebKit).
See also: https://github.com/w3c/csswg-drafts/issues/4248.
Test: fast/css/font-face-set-ready-after-document-load.html
* css/CSSFontSelector.cpp:
(WebCore::CSSFontSelector::fontFaceSetIfExists): Renamed from optionalFontFaceSet for consistency.
* css/CSSFontSelector.h:
* css/FontFaceSet.cpp:
(WebCore::FontFaceSet::FontFaceSet): Fixed the bug.
(WebCore::FontFaceSet::documentDidFinishLoading): Renamed from didFirstLayout to reflect the new semantics.
(WebCore::FontFaceSet::completedLoading):
* css/FontFaceSet.h:
* dom/Document.cpp:
(WebCore::Document::implicitClose): Deployed makeRefPtr to be safe.
LayoutTests:
* fast/css/font-face-set-ready-after-document-load-expected.txt: Added.
* fast/css/font-face-set-ready-after-document-load.html: Added.
Modified Paths
Added Paths
Diff
Modified: trunk/LayoutTests/ChangeLog (255413 => 255414)
--- trunk/LayoutTests/ChangeLog 2020-01-30 04:12:57 UTC (rev 255413)
+++ trunk/LayoutTests/ChangeLog 2020-01-30 04:15:51 UTC (rev 255414)
@@ -1,3 +1,13 @@
+2020-01-29 Ryosuke Niwa <rn...@webkit.org>
+
+ 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
+
+ Reviewed by Simon Fraser.
+
+ * fast/css/font-face-set-ready-after-document-load-expected.txt: Added.
+ * fast/css/font-face-set-ready-after-document-load.html: Added.
+
2020-01-29 Sunny He <sunny...@apple.com>
Clamp paddingBoxWidth/Height to a minimum of zero
Added: trunk/LayoutTests/fast/css/font-face-set-ready-after-document-load-expected.txt (0 => 255414)
--- trunk/LayoutTests/fast/css/font-face-set-ready-after-document-load-expected.txt (rev 0)
+++ trunk/LayoutTests/fast/css/font-face-set-ready-after-document-load-expected.txt 2020-01-30 04:15:51 UTC (rev 255414)
@@ -0,0 +1,8 @@
+This test records the order by which load and DOMContentLoaded evnets fire relative to when document.fonts.ready is resolved.
+fonts.ready should resolve only after DOMContentLoaded and load event are fired, and fonts.check should return true for Ahem at that point.
+
+Before setting font-family - fonts.check Ahem: false
+DOMContentLoaded
+load
+fonts.ready - fonts.check Ahem: true
+hello
Added: trunk/LayoutTests/fast/css/font-face-set-ready-after-document-load.html (0 => 255414)
--- trunk/LayoutTests/fast/css/font-face-set-ready-after-document-load.html (rev 0)
+++ trunk/LayoutTests/fast/css/font-face-set-ready-after-document-load.html 2020-01-30 04:15:51 UTC (rev 255414)
@@ -0,0 +1,47 @@
+<!DOCTYPE html>
+<html>
+<head>
+<meta charset="utf-8">
+<style>
+@font-face {
+ font-family: Ahem;
+ src: url("../../resources/fonts/Ahem.ttf");
+}
+</style>
+</head>
+<body>
+<p>This test records the order by which load and DOMContentLoaded evnets fire relative to when document.fonts.ready is resolved.<br>
+fonts.ready should resolve only after DOMContentLoaded and load event are fired, and fonts.check should return true for Ahem at that point.</p>
+<pre id="log"></pre>
+<div id="element1">hello</div>
+<script>
+
+if (window.testRunner) {
+ testRunner.dumpAsText();
+ testRunner.waitUntilDone();
+}
+
+function log(message) {
+ document.getElementById('log').textContent += message + '\n';
+}
+
+function checkAhem(description) {
+ const check = document.fonts.check('10px Ahem');
+ log(`${description} - fonts.check Ahem: ${check}`);
+}
+
+window.addEventListener('DOMContentLoaded', () => log('DOMContentLoaded'));
+window.addEventListener('load', () => log('load'));
+
+checkAhem('Before setting font-family');
+element1.getBoundingClientRect();
+element1.style.fontFamily = 'Ahem';
+document.fonts.ready.then(() => {
+ checkAhem('fonts.ready');
+ if (window.testRunner)
+ testRunner.notifyDone();
+});
+
+</script>
+</body>
+</html>
Modified: trunk/Source/WebCore/ChangeLog (255413 => 255414)
--- trunk/Source/WebCore/ChangeLog 2020-01-30 04:12:57 UTC (rev 255413)
+++ trunk/Source/WebCore/ChangeLog 2020-01-30 04:15:51 UTC (rev 255414)
@@ -1,3 +1,38 @@
+2020-01-29 Ryosuke Niwa <rn...@webkit.org>
+
+ 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
+
+ Reviewed by Simon Fraser.
+
+ The flakiness of the test was caused by document.fonts.ready resolving without waiting for the document
+ to be loaded if FontFaceSet::FontFaceSet is created after the initial layout had been done.
+
+ r249295 introduced this racy code to address the previous behavior of WebKit, which didn't wait for
+ either document to finish loading or the initial layout to happen at all, and the WebKit workaround in
+ the offending WPT test was subsequentially removed, resulting in the flaky failure after the test was
+ synced up in r249760.
+
+ This patch address the underlying bug by making ready promise wait until Document::implicitClose is called.
+
+ Unfortunately, the specification is extremely vague in terms of when this promise is resolved but new
+ behavior of WebKit is more or less with Firefox (Chrome seems to have a similar race condition as WebKit).
+
+ See also: https://github.com/w3c/csswg-drafts/issues/4248.
+
+ Test: fast/css/font-face-set-ready-after-document-load.html
+
+ * css/CSSFontSelector.cpp:
+ (WebCore::CSSFontSelector::fontFaceSetIfExists): Renamed from optionalFontFaceSet for consistency.
+ * css/CSSFontSelector.h:
+ * css/FontFaceSet.cpp:
+ (WebCore::FontFaceSet::FontFaceSet): Fixed the bug.
+ (WebCore::FontFaceSet::documentDidFinishLoading): Renamed from didFirstLayout to reflect the new semantics.
+ (WebCore::FontFaceSet::completedLoading):
+ * css/FontFaceSet.h:
+ * dom/Document.cpp:
+ (WebCore::Document::implicitClose): Deployed makeRefPtr to be safe.
+
2020-01-29 Sunny He <sunny...@apple.com>
Clamp paddingBoxWidth/Height to a minimum of zero
Modified: trunk/Source/WebCore/css/CSSFontSelector.cpp (255413 => 255414)
--- trunk/Source/WebCore/css/CSSFontSelector.cpp 2020-01-30 04:12:57 UTC (rev 255413)
+++ trunk/Source/WebCore/css/CSSFontSelector.cpp 2020-01-30 04:15:51 UTC (rev 255414)
@@ -83,7 +83,7 @@
FontCache::singleton().removeClient(*this);
}
-FontFaceSet* CSSFontSelector::optionalFontFaceSet()
+FontFaceSet* CSSFontSelector::fontFaceSetIfExists()
{
return m_fontFaceSet.get();
}
Modified: trunk/Source/WebCore/css/CSSFontSelector.h (255413 => 255414)
--- trunk/Source/WebCore/css/CSSFontSelector.h 2020-01-30 04:12:57 UTC (rev 255413)
+++ trunk/Source/WebCore/css/CSSFontSelector.h 2020-01-30 04:15:51 UTC (rev 255414)
@@ -81,7 +81,7 @@
void beginLoadingFontSoon(CachedFont&);
- FontFaceSet* optionalFontFaceSet();
+ FontFaceSet* fontFaceSetIfExists();
FontFaceSet& fontFaceSet();
void incrementIsComputingRootStyleFont() { ++m_computingRootStyleFontCount; }
Modified: trunk/Source/WebCore/css/FontFaceSet.cpp (255413 => 255414)
--- trunk/Source/WebCore/css/FontFaceSet.cpp 2020-01-30 04:12:57 UTC (rev 255413)
+++ trunk/Source/WebCore/css/FontFaceSet.cpp 2020-01-30 04:15:51 UTC (rev 255414)
@@ -71,9 +71,9 @@
, m_readyPromise(makeUniqueRef<ReadyPromise>(*this, &FontFaceSet::readyPromiseResolve))
{
if (document.frame())
- m_isFirstLayoutDone = document.frame()->loader().stateMachine().firstLayoutDone();
+ m_isDocumentLoaded = document.loadEventFinished() && !document.processingLoadEvent();
- if (m_isFirstLayoutDone && !backing.hasActiveFontFaces())
+ if (m_isDocumentLoaded && !backing.hasActiveFontFaces())
m_readyPromise->resolve(*this);
m_backing->addClient(*this);
@@ -196,9 +196,9 @@
// FIXME: Fire a "loading" event asynchronously.
}
-void FontFaceSet::didFirstLayout()
+void FontFaceSet::documentDidFinishLoading()
{
- m_isFirstLayoutDone = true;
+ m_isDocumentLoaded = true;
if (!m_backing->hasActiveFontFaces() && !m_readyPromise->isFulfilled())
m_readyPromise->resolve(*this);
}
@@ -205,7 +205,7 @@
void FontFaceSet::completedLoading()
{
- if (m_isFirstLayoutDone && !m_readyPromise->isFulfilled())
+ if (m_isDocumentLoaded && !m_readyPromise->isFulfilled())
m_readyPromise->resolve(*this);
}
Modified: trunk/Source/WebCore/css/FontFaceSet.h (255413 => 255414)
--- trunk/Source/WebCore/css/FontFaceSet.h 2020-01-30 04:12:57 UTC (rev 255413)
+++ trunk/Source/WebCore/css/FontFaceSet.h 2020-01-30 04:15:51 UTC (rev 255414)
@@ -60,7 +60,7 @@
using ReadyPromise = DOMPromiseProxyWithResolveCallback<IDLInterface<FontFaceSet>>;
ReadyPromise& ready() { return m_readyPromise.get(); }
- void didFirstLayout();
+ void documentDidFinishLoading();
CSSFontFaceSet& backing() { return m_backing; }
@@ -118,7 +118,8 @@
Ref<CSSFontFaceSet> m_backing;
HashMap<RefPtr<FontFace>, Vector<Ref<PendingPromise>>> m_pendingPromises;
UniqueRef<ReadyPromise> m_readyPromise;
- bool m_isFirstLayoutDone { true };
+
+ bool m_isDocumentLoaded { true };
};
}
Modified: trunk/Source/WebCore/dom/Document.cpp (255413 => 255414)
--- trunk/Source/WebCore/dom/Document.cpp 2020-01-30 04:12:57 UTC (rev 255413)
+++ trunk/Source/WebCore/dom/Document.cpp 2020-01-30 04:15:51 UTC (rev 255414)
@@ -3086,8 +3086,8 @@
m_processingLoadEvent = false;
- if (auto* fontFaceSet = fontSelector().optionalFontFaceSet())
- fontFaceSet->didFirstLayout();
+ if (auto fontFaceSet = makeRefPtr(fontSelector().fontFaceSetIfExists()))
+ fontFaceSet->documentDidFinishLoading();
#if PLATFORM(COCOA) || PLATFORM(WIN) || PLATFORM(GTK)
if (f && hasLivingRenderTree() && AXObjectCache::accessibilityEnabled()) {
