- Revision
- 208889
- Author
- [email protected]
- Date
- 2016-11-18 12:56:36 -0800 (Fri, 18 Nov 2016)
Log Message
[CSS Font Loading] FontFaceSet.load() promises don't always fire
https://bugs.webkit.org/show_bug.cgi?id=164902
Reviewed by David Hyatt.
Source/WebCore:
Test: fast/text/fontfaceset-rebuild-during-loading.html
We currently handle web fonts in two phases. The first phase is building up
StyleRuleFontFace objects which reflect the style on the page. The second is creating
CSSFontFace objects from those StyleRuleFontFace objects. When script modifies the
style on the page, we can often update the CSSFontFace objects, but there are some
modifications which we don't know how to model. For these operations, we destroy the
CSSFontFace objects and rebuild them from the newly modified StyleRuleFontFace objects.
Normally, this is fine. However, with the CSS font loading API, the CSSFontFaces back
_javascript_ objects which will persist across the rebuilding step mentioned above. This
means that the FontFace objects need to adopt the new CSSFontFace objects and forget
the old CSSFontFace objects.
There was one bit of state which I forgot to update during this rebuilding phase. The
FontFaceSet object contains an internal HashMap where a reference to a CSSFontFace
is used as a key. After the rebuilding phase, this reference wasn't updated to point
to the new CSSFontFace.
The solution is to instead use a reference to the higher-level FontFace as the key to
the HashMap. This object is persistent across the rebuilding phase (and it adopts
the new CSSFontFaces). There is not a lifetime problem because the FontFace holds a
strong reference to its backing CSSFontFace object.
This bug didn't cause a memory problem because the HashMap was keeping the old
CSSFontFace alive because the key was a strong reference.
This patch also adds a lengthy comment explaining how the migration works.
* css/CSSFontFace.cpp:
(WebCore::CSSFontFace::initializeWrapper): This is another bit of state which didn't
survive the rebuilding phase. Moving it here causes it to survive.
(WebCore::CSSFontFace::wrapper):
* css/CSSFontSelector.cpp:
(WebCore::CSSFontSelector::addFontFaceRule):
* css/FontFaceSet.cpp:
(WebCore::FontFaceSet::load):
(WebCore::FontFaceSet::faceFinished):
* css/FontFaceSet.h:
LayoutTests:
* fast/text/fontfaceset-rebuild-during-loading-expected.txt: Added.
* fast/text/fontfaceset-rebuild-during-loading.html: Added.
Modified Paths
Added Paths
Diff
Modified: trunk/LayoutTests/ChangeLog (208888 => 208889)
--- trunk/LayoutTests/ChangeLog 2016-11-18 20:27:50 UTC (rev 208888)
+++ trunk/LayoutTests/ChangeLog 2016-11-18 20:56:36 UTC (rev 208889)
@@ -1,5 +1,15 @@
2016-11-18 Myles C. Maxfield <[email protected]>
+ [CSS Font Loading] FontFaceSet.load() promises don't always fire
+ https://bugs.webkit.org/show_bug.cgi?id=164902
+
+ Reviewed by David Hyatt.
+
+ * fast/text/fontfaceset-rebuild-during-loading-expected.txt: Added.
+ * fast/text/fontfaceset-rebuild-during-loading.html: Added.
+
+2016-11-18 Myles C. Maxfield <[email protected]>
+
[SVG -> OTF Font Converter] Fonts advances are not internally consistent inside the generated font file
https://bugs.webkit.org/show_bug.cgi?id=164846
<rdar://problem/29031509>
Added: trunk/LayoutTests/fast/text/fontfaceset-rebuild-during-loading-expected.txt (0 => 208889)
--- trunk/LayoutTests/fast/text/fontfaceset-rebuild-during-loading-expected.txt (rev 0)
+++ trunk/LayoutTests/fast/text/fontfaceset-rebuild-during-loading-expected.txt 2016-11-18 20:56:36 UTC (rev 208889)
@@ -0,0 +1,10 @@
+This test makes sure that FontFaceSet.load promises still fire when the CSSFontSelector has been rebuilt during a load.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+PASS Font loaded.
+PASS successfullyParsed is true
+
+TEST COMPLETE
+asdf
Added: trunk/LayoutTests/fast/text/fontfaceset-rebuild-during-loading.html (0 => 208889)
--- trunk/LayoutTests/fast/text/fontfaceset-rebuild-during-loading.html (rev 0)
+++ trunk/LayoutTests/fast/text/fontfaceset-rebuild-during-loading.html 2016-11-18 20:56:36 UTC (rev 208889)
@@ -0,0 +1,42 @@
+<!DOCTYPE html>
+<html>
+<head>
+<script src=""
+<style>
+@font-face {
+ font-family: "WebFont";
+ src: url("../../resources/Ahem.ttf") format("truetype");
+}
+</style>
+<style id="teststyle">
+.test {
+ font: 300px "WebFont";
+}
+</style>
+</head>
+<body>
+<div class="test">asdf</div>
+<script>
+if (window.internals) {
+ internals.clearMemoryCache();
+ internals.invalidateFontCache();
+}
+
+window.jsTestIsAsync = true;
+
+description("This test makes sure that FontFaceSet.load promises still fire when the CSSFontSelector has been rebuilt during a load.");
+
+document.fonts.load("300px 'WebFont'").then(function() {
+ testPassed("Font loaded.");
+ finishJSTest();
+}, function() {
+ testFailed("Font should load");
+ finishJSTest();
+});
+
+var testStyle = document.getElementById("teststyle");
+testStyle.media = "print";
+</script>
+<script src=""
+</body>
+</html>
\ No newline at end of file
Modified: trunk/Source/WebCore/ChangeLog (208888 => 208889)
--- trunk/Source/WebCore/ChangeLog 2016-11-18 20:27:50 UTC (rev 208888)
+++ trunk/Source/WebCore/ChangeLog 2016-11-18 20:56:36 UTC (rev 208889)
@@ -1,5 +1,52 @@
2016-11-18 Myles C. Maxfield <[email protected]>
+ [CSS Font Loading] FontFaceSet.load() promises don't always fire
+ https://bugs.webkit.org/show_bug.cgi?id=164902
+
+ Reviewed by David Hyatt.
+
+ Test: fast/text/fontfaceset-rebuild-during-loading.html
+
+ We currently handle web fonts in two phases. The first phase is building up
+ StyleRuleFontFace objects which reflect the style on the page. The second is creating
+ CSSFontFace objects from those StyleRuleFontFace objects. When script modifies the
+ style on the page, we can often update the CSSFontFace objects, but there are some
+ modifications which we don't know how to model. For these operations, we destroy the
+ CSSFontFace objects and rebuild them from the newly modified StyleRuleFontFace objects.
+
+ Normally, this is fine. However, with the CSS font loading API, the CSSFontFaces back
+ _javascript_ objects which will persist across the rebuilding step mentioned above. This
+ means that the FontFace objects need to adopt the new CSSFontFace objects and forget
+ the old CSSFontFace objects.
+
+ There was one bit of state which I forgot to update during this rebuilding phase. The
+ FontFaceSet object contains an internal HashMap where a reference to a CSSFontFace
+ is used as a key. After the rebuilding phase, this reference wasn't updated to point
+ to the new CSSFontFace.
+
+ The solution is to instead use a reference to the higher-level FontFace as the key to
+ the HashMap. This object is persistent across the rebuilding phase (and it adopts
+ the new CSSFontFaces). There is not a lifetime problem because the FontFace holds a
+ strong reference to its backing CSSFontFace object.
+
+ This bug didn't cause a memory problem because the HashMap was keeping the old
+ CSSFontFace alive because the key was a strong reference.
+
+ This patch also adds a lengthy comment explaining how the migration works.
+
+ * css/CSSFontFace.cpp:
+ (WebCore::CSSFontFace::initializeWrapper): This is another bit of state which didn't
+ survive the rebuilding phase. Moving it here causes it to survive.
+ (WebCore::CSSFontFace::wrapper):
+ * css/CSSFontSelector.cpp:
+ (WebCore::CSSFontSelector::addFontFaceRule):
+ * css/FontFaceSet.cpp:
+ (WebCore::FontFaceSet::load):
+ (WebCore::FontFaceSet::faceFinished):
+ * css/FontFaceSet.h:
+
+2016-11-18 Myles C. Maxfield <[email protected]>
+
[SVG -> OTF Font Converter] Fonts advances are not internally consistent inside the generated font file
https://bugs.webkit.org/show_bug.cgi?id=164846
<rdar://problem/29031509>
Modified: trunk/Source/WebCore/css/CSSFontFace.cpp (208888 => 208889)
--- trunk/Source/WebCore/css/CSSFontFace.cpp 2016-11-18 20:27:50 UTC (rev 208888)
+++ trunk/Source/WebCore/css/CSSFontFace.cpp 2016-11-18 20:56:36 UTC (rev 208889)
@@ -448,6 +448,7 @@
m_wrapper->fontStateChanged(*this, Status::Pending, Status::Failure);
break;
}
+ m_mayBePurged = false;
}
Ref<FontFace> CSSFontFace::wrapper()
@@ -458,7 +459,6 @@
auto wrapper = FontFace::create(*this);
m_wrapper = wrapper->createWeakPtr();
initializeWrapper();
- m_mayBePurged = false;
return wrapper;
}
Modified: trunk/Source/WebCore/css/CSSFontSelector.cpp (208888 => 208889)
--- trunk/Source/WebCore/css/CSSFontSelector.cpp 2016-11-18 20:27:50 UTC (rev 208888)
+++ trunk/Source/WebCore/css/CSSFontSelector.cpp 2016-11-18 20:56:36 UTC (rev 208889)
@@ -206,6 +206,16 @@
return;
if (RefPtr<CSSFontFace> existingFace = m_cssFontFaceSet->lookUpByCSSConnection(fontFaceRule)) {
+ // This adoption is fairly subtle. Script can trigger a purge of m_cssFontFaceSet at any time,
+ // which will cause us to just rely on the memory cache to retain the bytes of the file the next
+ // time we build up the CSSFontFaceSet. However, when the CSS Font Loading API is involved,
+ // the FontFace and FontFaceSet objects need to retain state. We create the new CSSFontFace object
+ // while the old one is still in scope so that the memory cache will be forced to retain the bytes
+ // of the resource. This means that the CachedFont will temporarily have two clients (until the
+ // old CSSFontFace goes out of scope, which should happen at the end of this "if" block). Because
+ // the CSSFontFaceSource objects will inspect their CachedFonts, the new CSSFontFace is smart enough
+ // to enter the correct state() during the next pump(). This approach of making a new CSSFontFace is
+ // simpler than computing and applying a diff of the StyleProperties.
m_cssFontFaceSet->remove(*existingFace);
if (auto* existingWrapper = existingFace->existingWrapper())
existingWrapper->adopt(fontFace.get());
Modified: trunk/Source/WebCore/css/FontFaceSet.cpp (208888 => 208889)
--- trunk/Source/WebCore/css/FontFaceSet.cpp 2016-11-18 20:27:50 UTC (rev 208888)
+++ trunk/Source/WebCore/css/FontFaceSet.cpp 2016-11-18 20:56:36 UTC (rev 208889)
@@ -155,7 +155,8 @@
if (face.get().status() == CSSFontFace::Status::Success)
continue;
waiting = true;
- m_pendingPromises.add(&face.get(), Vector<Ref<PendingPromise>>()).iterator->value.append(pendingPromise.copyRef());
+ ASSERT(face.get().existingWrapper());
+ m_pendingPromises.add(face.get().existingWrapper(), Vector<Ref<PendingPromise>>()).iterator->value.append(pendingPromise.copyRef());
}
if (!waiting)
@@ -209,7 +210,10 @@
void FontFaceSet::faceFinished(CSSFontFace& face, CSSFontFace::Status newStatus)
{
- auto iterator = m_pendingPromises.find(&face);
+ if (!face.existingWrapper())
+ return;
+
+ auto iterator = m_pendingPromises.find(face.existingWrapper());
if (iterator == m_pendingPromises.end())
return;
Modified: trunk/Source/WebCore/css/FontFaceSet.h (208888 => 208889)
--- trunk/Source/WebCore/css/FontFaceSet.h 2016-11-18 20:27:50 UTC (rev 208888)
+++ trunk/Source/WebCore/css/FontFaceSet.h 2016-11-18 20:56:36 UTC (rev 208889)
@@ -108,7 +108,7 @@
void derefEventTarget() final { deref(); }
Ref<CSSFontFaceSet> m_backing;
- HashMap<RefPtr<CSSFontFace>, Vector<Ref<PendingPromise>>> m_pendingPromises;
+ HashMap<RefPtr<FontFace>, Vector<Ref<PendingPromise>>> m_pendingPromises;
Optional<ReadyPromise> m_promise;
bool m_isReady { true };
};
