<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.1//EN"
"http://www.w3.org/TR/xhtml11/DTD/xhtml11.dtd">
<html xmlns="http://www.w3.org/1999/xhtml">
<head><meta http-equiv="content-type" content="text/html; charset=utf-8" />
<title>[208889] trunk</title>
</head>
<body>
<style type="text/css"><!--
#msg dl.meta { border: 1px #006 solid; background: #369; padding: 6px; color: #fff; }
#msg dl.meta dt { float: left; width: 6em; font-weight: bold; }
#msg dt:after { content:':';}
#msg dl, #msg dt, #msg ul, #msg li, #header, #footer, #logmsg { font-family: verdana,arial,helvetica,sans-serif; font-size: 10pt; }
#msg dl a { font-weight: bold}
#msg dl a:link { color:#fc3; }
#msg dl a:active { color:#ff0; }
#msg dl a:visited { color:#cc6; }
h3 { font-family: verdana,arial,helvetica,sans-serif; font-size: 10pt; font-weight: bold; }
#msg pre { overflow: auto; background: #ffc; border: 1px #fa0 solid; padding: 6px; }
#logmsg { background: #ffc; border: 1px #fa0 solid; padding: 1em 1em 0 1em; }
#logmsg p, #logmsg pre, #logmsg blockquote { margin: 0 0 1em 0; }
#logmsg p, #logmsg li, #logmsg dt, #logmsg dd { line-height: 14pt; }
#logmsg h1, #logmsg h2, #logmsg h3, #logmsg h4, #logmsg h5, #logmsg h6 { margin: .5em 0; }
#logmsg h1:first-child, #logmsg h2:first-child, #logmsg h3:first-child, #logmsg h4:first-child, #logmsg h5:first-child, #logmsg h6:first-child { margin-top: 0; }
#logmsg ul, #logmsg ol { padding: 0; list-style-position: inside; margin: 0 0 0 1em; }
#logmsg ul { text-indent: -1em; padding-left: 1em; }#logmsg ol { text-indent: -1.5em; padding-left: 1.5em; }
#logmsg > ul, #logmsg > ol { margin: 0 0 1em 0; }
#logmsg pre { background: #eee; padding: 1em; }
#logmsg blockquote { border: 1px solid #fa0; border-left-width: 10px; padding: 1em 1em 0 1em; background: white;}
#logmsg dl { margin: 0; }
#logmsg dt { font-weight: bold; }
#logmsg dd { margin: 0; padding: 0 0 0.5em 0; }
#logmsg dd:before { content:'\00bb';}
#logmsg table { border-spacing: 0px; border-collapse: collapse; border-top: 4px solid #fa0; border-bottom: 1px solid #fa0; background: #fff; }
#logmsg table th { text-align: left; font-weight: normal; padding: 0.2em 0.5em; border-top: 1px dotted #fa0; }
#logmsg table td { text-align: right; border-top: 1px dotted #fa0; padding: 0.2em 0.5em; }
#logmsg table thead th { text-align: center; border-bottom: 1px solid #fa0; }
#logmsg table th.Corner { text-align: left; }
#logmsg hr { border: none 0; border-top: 2px dashed #fa0; height: 1px; }
#header, #footer { color: #fff; background: #636; border: 1px #300 solid; padding: 6px; }
#patch { width: 100%; }
#patch h4 {font-family: verdana,arial,helvetica,sans-serif;font-size:10pt;padding:8px;background:#369;color:#fff;margin:0;}
#patch .propset h4, #patch .binary h4 {margin:0;}
#patch pre {padding:0;line-height:1.2em;margin:0;}
#patch .diff {width:100%;background:#eee;padding: 0 0 10px 0;overflow:auto;}
#patch .propset .diff, #patch .binary .diff {padding:10px 0;}
#patch span {display:block;padding:0 10px;}
#patch .modfile, #patch .addfile, #patch .delfile, #patch .propset, #patch .binary, #patch .copfile {border:1px solid #ccc;margin:10px 0;}
#patch ins {background:#dfd;text-decoration:none;display:block;padding:0 10px;}
#patch del {background:#fdd;text-decoration:none;display:block;padding:0 10px;}
#patch .lines, .info {color:#888;background:#fff;}
--></style>
<div id="msg">
<dl class="meta">
<dt>Revision</dt> <dd><a href="http://trac.webkit.org/projects/webkit/changeset/208889">208889</a></dd>
<dt>Author</dt> <dd>mmaxfield@apple.com</dd>
<dt>Date</dt> <dd>2016-11-18 12:56:36 -0800 (Fri, 18 Nov 2016)</dd>
</dl>
<h3>Log Message</h3>
<pre>[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.</pre>
<h3>Modified Paths</h3>
<ul>
<li><a href="#trunkLayoutTestsChangeLog">trunk/LayoutTests/ChangeLog</a></li>
<li><a href="#trunkSourceWebCoreChangeLog">trunk/Source/WebCore/ChangeLog</a></li>
<li><a href="#trunkSourceWebCorecssCSSFontFacecpp">trunk/Source/WebCore/css/CSSFontFace.cpp</a></li>
<li><a href="#trunkSourceWebCorecssCSSFontSelectorcpp">trunk/Source/WebCore/css/CSSFontSelector.cpp</a></li>
<li><a href="#trunkSourceWebCorecssFontFaceSetcpp">trunk/Source/WebCore/css/FontFaceSet.cpp</a></li>
<li><a href="#trunkSourceWebCorecssFontFaceSeth">trunk/Source/WebCore/css/FontFaceSet.h</a></li>
</ul>
<h3>Added Paths</h3>
<ul>
<li><a href="#trunkLayoutTestsfasttextfontfacesetrebuildduringloadingexpectedtxt">trunk/LayoutTests/fast/text/fontfaceset-rebuild-during-loading-expected.txt</a></li>
<li><a href="#trunkLayoutTestsfasttextfontfacesetrebuildduringloadinghtml">trunk/LayoutTests/fast/text/fontfaceset-rebuild-during-loading.html</a></li>
</ul>
</div>
<div id="patch">
<h3>Diff</h3>
<a id="trunkLayoutTestsChangeLog"></a>
<div class="modfile"><h4>Modified: trunk/LayoutTests/ChangeLog (208888 => 208889)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/LayoutTests/ChangeLog 2016-11-18 20:27:50 UTC (rev 208888)
+++ trunk/LayoutTests/ChangeLog 2016-11-18 20:56:36 UTC (rev 208889)
</span><span class="lines">@@ -1,5 +1,15 @@
</span><span class="cx"> 2016-11-18 Myles C. Maxfield <mmaxfield@apple.com>
</span><span class="cx">
</span><ins>+ [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 <mmaxfield@apple.com>
+
</ins><span class="cx"> [SVG -> OTF Font Converter] Fonts advances are not internally consistent inside the generated font file
</span><span class="cx"> https://bugs.webkit.org/show_bug.cgi?id=164846
</span><span class="cx"> <rdar://problem/29031509>
</span></span></pre></div>
<a id="trunkLayoutTestsfasttextfontfacesetrebuildduringloadingexpectedtxt"></a>
<div class="addfile"><h4>Added: trunk/LayoutTests/fast/text/fontfaceset-rebuild-during-loading-expected.txt (0 => 208889)</h4>
<pre class="diff"><span>
<span class="info">--- 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)
</span><span class="lines">@@ -0,0 +1,10 @@
</span><ins>+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
</ins></span></pre></div>
<a id="trunkLayoutTestsfasttextfontfacesetrebuildduringloadinghtml"></a>
<div class="addfile"><h4>Added: trunk/LayoutTests/fast/text/fontfaceset-rebuild-during-loading.html (0 => 208889)</h4>
<pre class="diff"><span>
<span class="info">--- 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)
</span><span class="lines">@@ -0,0 +1,42 @@
</span><ins>+<!DOCTYPE html>
+<html>
+<head>
+<script src="../../resources/js-test-pre.js"></script>
+<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="../../resources/js-test-post.js"></script>
+</body>
+</html>
</ins><span class="cx">\ No newline at end of file
</span></span></pre></div>
<a id="trunkSourceWebCoreChangeLog"></a>
<div class="modfile"><h4>Modified: trunk/Source/WebCore/ChangeLog (208888 => 208889)</h4>
<pre class="diff"><span>
<span class="info">--- 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)
</span><span class="lines">@@ -1,5 +1,52 @@
</span><span class="cx"> 2016-11-18 Myles C. Maxfield <mmaxfield@apple.com>
</span><span class="cx">
</span><ins>+ [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 <mmaxfield@apple.com>
+
</ins><span class="cx"> [SVG -> OTF Font Converter] Fonts advances are not internally consistent inside the generated font file
</span><span class="cx"> https://bugs.webkit.org/show_bug.cgi?id=164846
</span><span class="cx"> <rdar://problem/29031509>
</span></span></pre></div>
<a id="trunkSourceWebCorecssCSSFontFacecpp"></a>
<div class="modfile"><h4>Modified: trunk/Source/WebCore/css/CSSFontFace.cpp (208888 => 208889)</h4>
<pre class="diff"><span>
<span class="info">--- 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)
</span><span class="lines">@@ -448,6 +448,7 @@
</span><span class="cx"> m_wrapper->fontStateChanged(*this, Status::Pending, Status::Failure);
</span><span class="cx"> break;
</span><span class="cx"> }
</span><ins>+ m_mayBePurged = false;
</ins><span class="cx"> }
</span><span class="cx">
</span><span class="cx"> Ref<FontFace> CSSFontFace::wrapper()
</span><span class="lines">@@ -458,7 +459,6 @@
</span><span class="cx"> auto wrapper = FontFace::create(*this);
</span><span class="cx"> m_wrapper = wrapper->createWeakPtr();
</span><span class="cx"> initializeWrapper();
</span><del>- m_mayBePurged = false;
</del><span class="cx"> return wrapper;
</span><span class="cx"> }
</span><span class="cx">
</span></span></pre></div>
<a id="trunkSourceWebCorecssCSSFontSelectorcpp"></a>
<div class="modfile"><h4>Modified: trunk/Source/WebCore/css/CSSFontSelector.cpp (208888 => 208889)</h4>
<pre class="diff"><span>
<span class="info">--- 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)
</span><span class="lines">@@ -206,6 +206,16 @@
</span><span class="cx"> return;
</span><span class="cx">
</span><span class="cx"> if (RefPtr<CSSFontFace> existingFace = m_cssFontFaceSet->lookUpByCSSConnection(fontFaceRule)) {
</span><ins>+ // 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.
</ins><span class="cx"> m_cssFontFaceSet->remove(*existingFace);
</span><span class="cx"> if (auto* existingWrapper = existingFace->existingWrapper())
</span><span class="cx"> existingWrapper->adopt(fontFace.get());
</span></span></pre></div>
<a id="trunkSourceWebCorecssFontFaceSetcpp"></a>
<div class="modfile"><h4>Modified: trunk/Source/WebCore/css/FontFaceSet.cpp (208888 => 208889)</h4>
<pre class="diff"><span>
<span class="info">--- 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)
</span><span class="lines">@@ -155,7 +155,8 @@
</span><span class="cx"> if (face.get().status() == CSSFontFace::Status::Success)
</span><span class="cx"> continue;
</span><span class="cx"> waiting = true;
</span><del>- m_pendingPromises.add(&face.get(), Vector<Ref<PendingPromise>>()).iterator->value.append(pendingPromise.copyRef());
</del><ins>+ ASSERT(face.get().existingWrapper());
+ m_pendingPromises.add(face.get().existingWrapper(), Vector<Ref<PendingPromise>>()).iterator->value.append(pendingPromise.copyRef());
</ins><span class="cx"> }
</span><span class="cx">
</span><span class="cx"> if (!waiting)
</span><span class="lines">@@ -209,7 +210,10 @@
</span><span class="cx">
</span><span class="cx"> void FontFaceSet::faceFinished(CSSFontFace& face, CSSFontFace::Status newStatus)
</span><span class="cx"> {
</span><del>- auto iterator = m_pendingPromises.find(&face);
</del><ins>+ if (!face.existingWrapper())
+ return;
+
+ auto iterator = m_pendingPromises.find(face.existingWrapper());
</ins><span class="cx"> if (iterator == m_pendingPromises.end())
</span><span class="cx"> return;
</span><span class="cx">
</span></span></pre></div>
<a id="trunkSourceWebCorecssFontFaceSeth"></a>
<div class="modfile"><h4>Modified: trunk/Source/WebCore/css/FontFaceSet.h (208888 => 208889)</h4>
<pre class="diff"><span>
<span class="info">--- 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)
</span><span class="lines">@@ -108,7 +108,7 @@
</span><span class="cx"> void derefEventTarget() final { deref(); }
</span><span class="cx">
</span><span class="cx"> Ref<CSSFontFaceSet> m_backing;
</span><del>- HashMap<RefPtr<CSSFontFace>, Vector<Ref<PendingPromise>>> m_pendingPromises;
</del><ins>+ HashMap<RefPtr<FontFace>, Vector<Ref<PendingPromise>>> m_pendingPromises;
</ins><span class="cx"> Optional<ReadyPromise> m_promise;
</span><span class="cx"> bool m_isReady { true };
</span><span class="cx"> };
</span></span></pre>
</div>
</div>
</body>
</html>