- Revision
- 266148
- Author
- beid...@apple.com
- Date
- 2020-08-25 15:08:51 -0700 (Tue, 25 Aug 2020)
Log Message
Font loads quickly followed by navigations may fail indefinitely
<rdar://problem/65560550> and https://bugs.webkit.org/show_bug.cgi?id=215435
Reviewed by Myles C. Maxfield.
Source/WebCore:
Second take at this.
Myles took the first swipe at this, but a conflict with SuspendableTimer caused issues
in the form of layout test asserts with
http/tests/security/navigate-when-restoring-cached-page.html
His original ChangeLog entry:
Font loads are coalesced using a zero-delay timer. However, that zero-delay timer
can fire while the page is in the middle of a navigation, which will cause the font
loads to fail. Then, the second page can request those same fonts, which are marked
as failed, and as such will never actually load/use the desired web font.
This patch just stops the zero-delay timer during navigations, and resumes it
when resuming the document. This means:
1. The second page in the above story will not see that the font has failed, or
even started, and will then re-request the font and load it successfully
2. If the user goes "back" to the previous page, the zero-delay timer is restarted,
the CachedFont realizes it's already succeeded, and the previous page is rendered
as expected.
Test: fast/loader/font-load-timer.html
---
Now the explanation of the failure it caused:
The font loading timer was a SuspendableTimer, which is an ActiveDOMObject.
An ActiveDOMObject was used to make sure the delayed font loads play well with the
page cache, which is still necessary.
But we also still need to suspend the timer manually when "stopLoading()" is called,
which doesn't play well with ActiveDOMObject's automatic suspend/resume.
My solution:
- Make the timer "just a normal timer"
- Make CSSFontSelector itself the ActiveDOMObject
- Let DocumentLoader explicitly pause the font load timer
- Rely on ActiveDOMObject to resume the timer
These keep the bug fixed and resolve the layout test ASSERT seen with
http/tests/security/navigate-when-restoring-cached-page.html
* css/CSSFontSelector.cpp:
(WebCore::CSSFontSelector::CSSFontSelector):
(WebCore::CSSFontSelector::clearDocument):
(WebCore::CSSFontSelector::beginLoadingFontSoon):
(WebCore::CSSFontSelector::suspendFontLoadingTimer):
(WebCore::CSSFontSelector::fontLoadingTimerFired):
(WebCore::CSSFontSelector::stop):
(WebCore::CSSFontSelector::suspend):
(WebCore::CSSFontSelector::resume):
(WebCore::CSSFontSelector::beginLoadTimerFired): Deleted.
* css/CSSFontSelector.h:
* loader/DocumentLoader.cpp:
(WebCore::DocumentLoader::stopLoading):
LayoutTests:
1) The page has some content that has “font-family: WebFont” but there are no @font-face blocks on the page
2) In script, after the page has loaded, add an @font-face rule to the page with “font-family: WebFont” and some valid font URL
3) Synchronously, within the same turn of the run loop, trigger a synchronous layout of the element (using offsetWidth or something). This will add the font to the 0-delay time work list.
4) Synchronously, within the same turn of the run loop, navigate to a second page that doesn’t use the web font.
5) The second page waits some small-but-positive amount of time. This will cause the 0-delay timer to fire, but because the page is in the middle of navigating, the font load should fail.
6) The second page adds the same @font-face rule to itself using script. This should pull the same (failed) CachedResource object out of the memory cache.
7) Use the CSS Font Loading API to wait for the font load to complete
8) Make sure that the font is used on the second page (as a reference test). Today, the second page’s font load will fail because it pulled the failed font out of the memory cache. The test makes sure the second page’s font load succeeds.
* fast/loader/font-load-timer-expected.html: Added.
* fast/loader/font-load-timer.html: Added.
* fast/loader/resources/font-load-timer-navigation-destination.html: Added.
Modified Paths
Added Paths
Diff
Modified: trunk/LayoutTests/ChangeLog (266147 => 266148)
--- trunk/LayoutTests/ChangeLog 2020-08-25 22:00:06 UTC (rev 266147)
+++ trunk/LayoutTests/ChangeLog 2020-08-25 22:08:51 UTC (rev 266148)
@@ -1,3 +1,23 @@
+2020-08-25 Brady Eidson <beid...@apple.com>
+
+ Font loads quickly followed by navigations may fail indefinitely
+ <rdar://problem/65560550> and https://bugs.webkit.org/show_bug.cgi?id=215435
+
+ Reviewed by Myles C. Maxfield.
+
+ 1) The page has some content that has “font-family: WebFont” but there are no @font-face blocks on the page
+ 2) In script, after the page has loaded, add an @font-face rule to the page with “font-family: WebFont” and some valid font URL
+ 3) Synchronously, within the same turn of the run loop, trigger a synchronous layout of the element (using offsetWidth or something). This will add the font to the 0-delay time work list.
+ 4) Synchronously, within the same turn of the run loop, navigate to a second page that doesn’t use the web font.
+ 5) The second page waits some small-but-positive amount of time. This will cause the 0-delay timer to fire, but because the page is in the middle of navigating, the font load should fail.
+ 6) The second page adds the same @font-face rule to itself using script. This should pull the same (failed) CachedResource object out of the memory cache.
+ 7) Use the CSS Font Loading API to wait for the font load to complete
+ 8) Make sure that the font is used on the second page (as a reference test). Today, the second page’s font load will fail because it pulled the failed font out of the memory cache. The test makes sure the second page’s font load succeeds.
+
+ * fast/loader/font-load-timer-expected.html: Added.
+ * fast/loader/font-load-timer.html: Added.
+ * fast/loader/resources/font-load-timer-navigation-destination.html: Added.
+
2020-08-25 Hector Lopez <hector_i_lo...@apple.com>
[ macOS Mojave wk1 ] fast/layers/hidpi-transform-on-child-content-is-mispositioned.html is a flaky failure
Added: trunk/LayoutTests/fast/loader/font-load-timer-expected.html (0 => 266148)
--- trunk/LayoutTests/fast/loader/font-load-timer-expected.html (rev 0)
+++ trunk/LayoutTests/fast/loader/font-load-timer-expected.html 2020-08-25 22:08:51 UTC (rev 266148)
@@ -0,0 +1,14 @@
+<!DOCTYPE html>
+<html>
+<head>
+<style id="style">
+@font-face {
+ font-family: 'WebFont';
+ src: url('../../resources/Ahem.ttf') format('truetype');
+}
+</style>
+</head>
+<body>
+<span id="target" style="font: 48px 'WebFont';">HelloWorld!</span>
+</body>
+</html>
Added: trunk/LayoutTests/fast/loader/font-load-timer.html (0 => 266148)
--- trunk/LayoutTests/fast/loader/font-load-timer.html (rev 0)
+++ trunk/LayoutTests/fast/loader/font-load-timer.html 2020-08-25 22:08:51 UTC (rev 266148)
@@ -0,0 +1,22 @@
+<!DOCTYPE html>
+<html>
+<head>
+<style id="style"></style>
+</head>
+<body>
+<span id="target" style="font: 48px 'WebFont';">Hello, World!</span>
+<script>
+if (window.testRunner)
+ testRunner.waitUntilDone();
+
+if (window.internals) {
+ internals.invalidateFontCache();
+ internals.clearMemoryCache();
+}
+
+document.getElementById("style").sheet.insertRule("@font-face { font-family: 'WebFont'; src: url('../../resources/Ahem.ttf') format('truetype'); }");
+document.getElementById("target").offsetWidth;
+window.location = "resources/font-load-timer-navigation-destination.html";
+</script>
+</body>
+</html>
Added: trunk/LayoutTests/fast/loader/resources/font-load-timer-navigation-destination.html (0 => 266148)
--- trunk/LayoutTests/fast/loader/resources/font-load-timer-navigation-destination.html (rev 0)
+++ trunk/LayoutTests/fast/loader/resources/font-load-timer-navigation-destination.html 2020-08-25 22:08:51 UTC (rev 266148)
@@ -0,0 +1,21 @@
+<!DOCTYPE html>
+<html>
+<head>
+<style id="style"></style>
+</head>
+<body>
+<span id="target" style="font: 48px 'WebFont';">HelloWorld!</span>
+<script>
+window.setTimeout(function() {
+ document.getElementById("style").sheet.insertRule("@font-face { font-family: 'WebFont'; src: url('../../../resources/Ahem.ttf') format('truetype'); }");
+ document.fonts.entries().next().value.load().then(function() {
+ if (window.testRunner)
+ testRunner.notifyDone();
+ }, function() {
+ if (window.testRunner)
+ testRunner.notifyDone();
+ });
+}, 100);
+</script>
+</body>
+</html>
Modified: trunk/Source/WebCore/ChangeLog (266147 => 266148)
--- trunk/Source/WebCore/ChangeLog 2020-08-25 22:00:06 UTC (rev 266147)
+++ trunk/Source/WebCore/ChangeLog 2020-08-25 22:08:51 UTC (rev 266148)
@@ -1,3 +1,68 @@
+2020-08-25 Brady Eidson <beid...@apple.com>
+
+ Font loads quickly followed by navigations may fail indefinitely
+ <rdar://problem/65560550> and https://bugs.webkit.org/show_bug.cgi?id=215435
+
+ Reviewed by Myles C. Maxfield.
+
+ Second take at this.
+
+ Myles took the first swipe at this, but a conflict with SuspendableTimer caused issues
+ in the form of layout test asserts with
+ http/tests/security/navigate-when-restoring-cached-page.html
+
+ His original ChangeLog entry:
+
+ Font loads are coalesced using a zero-delay timer. However, that zero-delay timer
+ can fire while the page is in the middle of a navigation, which will cause the font
+ loads to fail. Then, the second page can request those same fonts, which are marked
+ as failed, and as such will never actually load/use the desired web font.
+
+ This patch just stops the zero-delay timer during navigations, and resumes it
+ when resuming the document. This means:
+
+ 1. The second page in the above story will not see that the font has failed, or
+ even started, and will then re-request the font and load it successfully
+ 2. If the user goes "back" to the previous page, the zero-delay timer is restarted,
+ the CachedFont realizes it's already succeeded, and the previous page is rendered
+ as expected.
+
+ Test: fast/loader/font-load-timer.html
+
+ ---
+
+ Now the explanation of the failure it caused:
+ The font loading timer was a SuspendableTimer, which is an ActiveDOMObject.
+
+ An ActiveDOMObject was used to make sure the delayed font loads play well with the
+ page cache, which is still necessary.
+
+ But we also still need to suspend the timer manually when "stopLoading()" is called,
+ which doesn't play well with ActiveDOMObject's automatic suspend/resume.
+
+ My solution:
+ - Make the timer "just a normal timer"
+ - Make CSSFontSelector itself the ActiveDOMObject
+ - Let DocumentLoader explicitly pause the font load timer
+ - Rely on ActiveDOMObject to resume the timer
+
+ These keep the bug fixed and resolve the layout test ASSERT seen with
+ http/tests/security/navigate-when-restoring-cached-page.html
+
+ * css/CSSFontSelector.cpp:
+ (WebCore::CSSFontSelector::CSSFontSelector):
+ (WebCore::CSSFontSelector::clearDocument):
+ (WebCore::CSSFontSelector::beginLoadingFontSoon):
+ (WebCore::CSSFontSelector::suspendFontLoadingTimer):
+ (WebCore::CSSFontSelector::fontLoadingTimerFired):
+ (WebCore::CSSFontSelector::stop):
+ (WebCore::CSSFontSelector::suspend):
+ (WebCore::CSSFontSelector::resume):
+ (WebCore::CSSFontSelector::beginLoadTimerFired): Deleted.
+ * css/CSSFontSelector.h:
+ * loader/DocumentLoader.cpp:
+ (WebCore::DocumentLoader::stopLoading):
+
2020-08-25 Peng Liu <peng.l...@apple.com>
When using airplay with Youtube, the Youtube tab becomes completely empty and is unresponsive for an extended period of time if we switch the tab
Modified: trunk/Source/WebCore/css/CSSFontSelector.cpp (266147 => 266148)
--- trunk/Source/WebCore/css/CSSFontSelector.cpp 2020-08-25 22:00:06 UTC (rev 266147)
+++ trunk/Source/WebCore/css/CSSFontSelector.cpp 2020-08-25 22:08:51 UTC (rev 266148)
@@ -62,9 +62,10 @@
static unsigned fontSelectorId;
CSSFontSelector::CSSFontSelector(Document& document)
- : m_document(makeWeakPtr(document))
+ : ActiveDOMObject(document)
+ , m_document(makeWeakPtr(document))
, m_cssFontFaceSet(CSSFontFaceSet::create(this))
- , m_beginLoadingTimer(&document, *this, &CSSFontSelector::beginLoadTimerFired)
+ , m_fontLoadingTimer(*this, &CSSFontSelector::fontLoadingTimerFired)
, m_uniqueId(++fontSelectorId)
, m_version(0)
{
@@ -73,7 +74,7 @@
m_cssFontFaceSet->addClient(*this);
LOG(Fonts, "CSSFontSelector %p ctor", this);
- m_beginLoadingTimer.suspendIfNeeded();
+ suspendIfNeeded();
}
CSSFontSelector::~CSSFontSelector()
@@ -333,12 +334,12 @@
void CSSFontSelector::clearDocument()
{
if (!m_document) {
- ASSERT(!m_beginLoadingTimer.isActive());
+ ASSERT(!m_fontLoadingTimer.isActive());
ASSERT(m_fontsToBeginLoading.isEmpty());
return;
}
- m_beginLoadingTimer.cancel();
+ m_fontLoadingTimer.stop();
CachedResourceLoader& cachedResourceLoader = m_document->cachedResourceLoader();
for (auto& fontHandle : m_fontsToBeginLoading) {
@@ -361,14 +362,20 @@
m_fontsToBeginLoading.append(&font);
// Increment the request count now, in order to prevent didFinishLoad from being dispatched
// after this font has been requested but before it began loading. Balanced by
- // decrementRequestCount() in beginLoadTimerFired() and in clearDocument().
+ // decrementRequestCount() in fontLoadingTimerFired() and in clearDocument().
m_document->cachedResourceLoader().incrementRequestCount(font);
- m_beginLoadingTimer.startOneShot(0_s);
+ if (!m_fontLoadingTimerIsSuspended)
+ m_fontLoadingTimer.startOneShot(0_s);
}
-void CSSFontSelector::beginLoadTimerFired()
+void CSSFontSelector::suspendFontLoadingTimer()
{
+ suspend(ReasonForSuspension::PageWillBeSuspended);
+}
+
+void CSSFontSelector::fontLoadingTimerFired()
+{
Vector<CachedResourceHandle<CachedFont>> fontsToBeginLoading;
fontsToBeginLoading.swap(m_fontsToBeginLoading);
@@ -417,4 +424,31 @@
return font;
}
+void CSSFontSelector::stop()
+{
+ m_fontLoadingTimer.stop();
}
+
+void CSSFontSelector::suspend(ReasonForSuspension)
+{
+ if (m_fontLoadingTimerIsSuspended) {
+ ASSERT(!m_fontLoadingTimer.isActive());
+ return;
+ }
+
+ m_fontLoadingTimerIsSuspended = m_fontLoadingTimer.isActive();
+ m_fontLoadingTimer.stop();
+}
+
+void CSSFontSelector::resume()
+{
+ if (!m_fontLoadingTimerIsSuspended)
+ return;
+
+ if (!m_fontsToBeginLoading.isEmpty())
+ m_fontLoadingTimer.startOneShot(0_s);
+
+ m_fontLoadingTimerIsSuspended = false;
+}
+
+}
Modified: trunk/Source/WebCore/css/CSSFontSelector.h (266147 => 266148)
--- trunk/Source/WebCore/css/CSSFontSelector.h 2020-08-25 22:00:06 UTC (rev 266147)
+++ trunk/Source/WebCore/css/CSSFontSelector.h 2020-08-25 22:08:51 UTC (rev 266148)
@@ -25,12 +25,13 @@
#pragma once
+#include "ActiveDOMObject.h"
#include "CSSFontFace.h"
#include "CSSFontFaceSet.h"
#include "CachedResourceHandle.h"
#include "Font.h"
#include "FontSelector.h"
-#include "SuspendableTimer.h"
+#include "Timer.h"
#include <memory>
#include <wtf/Forward.h>
#include <wtf/HashSet.h>
@@ -47,7 +48,7 @@
class Document;
class StyleRuleFontFace;
-class CSSFontSelector final : public FontSelector, public CSSFontFaceSetClient, public CanMakeWeakPtr<CSSFontSelector> {
+class CSSFontSelector final : public FontSelector, public CSSFontFaceSetClient, public CanMakeWeakPtr<CSSFontSelector>, public ActiveDOMObject {
public:
static Ref<CSSFontSelector> create(Document& document)
{
@@ -80,6 +81,7 @@
Document* document() const { return m_document.get(); }
void beginLoadingFontSoon(CachedFont&);
+ void suspendFontLoadingTimer();
FontFaceSet* fontFaceSetIfExists();
FontFaceSet& fontFaceSet();
@@ -96,8 +98,14 @@
void fontModified() final;
- void beginLoadTimerFired();
+ void fontLoadingTimerFired();
+ // ActiveDOMObject
+ void stop() final;
+ void suspend(ReasonForSuspension) final;
+ void resume() final;
+ const char* activeDOMObjectName() const final { return "CSSFontSelector"_s; }
+
struct PendingFontFaceRule {
StyleRuleFontFace& styleRuleFontFace;
bool isInitiatingElementInUserAgentShadowTree;
@@ -112,8 +120,10 @@
Vector<CachedResourceHandle<CachedFont>> m_fontsToBeginLoading;
HashSet<RefPtr<CSSFontFace>> m_cssConnectionsPossiblyToRemove;
HashSet<RefPtr<StyleRuleFontFace>> m_cssConnectionsEncounteredDuringBuild;
- SuspendableTimer m_beginLoadingTimer;
+ Timer m_fontLoadingTimer;
+ bool m_fontLoadingTimerIsSuspended { false };
+
unsigned m_uniqueId;
unsigned m_version;
unsigned m_computingRootStyleFontCount { 0 };
Modified: trunk/Source/WebCore/loader/DocumentLoader.cpp (266147 => 266148)
--- trunk/Source/WebCore/loader/DocumentLoader.cpp 2020-08-25 22:00:06 UTC (rev 266147)
+++ trunk/Source/WebCore/loader/DocumentLoader.cpp 2020-08-25 22:08:51 UTC (rev 266148)
@@ -33,6 +33,7 @@
#include "ApplicationCacheHost.h"
#include "Archive.h"
#include "ArchiveResourceCollection.h"
+#include "CSSFontSelector.h"
#include "CachedPage.h"
#include "CachedRawResource.h"
#include "CachedResourceLoader.h"
@@ -313,6 +314,9 @@
// Always cancel multipart loaders
cancelAll(m_multipartSubresourceLoaders);
+ if (auto* document = m_frame->document())
+ document->fontSelector().suspendFontLoadingTimer();
+
// Appcache uses ResourceHandle directly, DocumentLoader doesn't count these loads.
m_applicationCacheHost->stopLoadingInFrame(*m_frame);
