<!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>[218157] 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/218157">218157</a></dd>
<dt>Author</dt> <dd>mmaxfield@apple.com</dd>
<dt>Date</dt> <dd>2017-06-12 17:05:55 -0700 (Mon, 12 Jun 2017)</dd>
</dl>

<h3>Log Message</h3>
<pre>@font-face rules with invalid primary fonts never download their secondary fonts
https://bugs.webkit.org/show_bug.cgi?id=173138
<rdar://problem/32554450>

Reviewed by Simon Fraser.

Source/WebCore:

We have logic in CSSFontAccessor::font() which disallows downloading a CSSFontFace if that CSSFontFace
is already in the Succeeded state. However, it was possible for a succeeded CSSFontFace to still fail
to create a font. In this situation, we wouldn't be able to use the downloaded font, and we wouldn't
try to download the next item in the src: list because the CSSFontFace is succeeded.

This patch strengthens the meaning of the Succeeded state. Previously, it just meant that the bytes
in the file were downloaded successfully. This patch extends this to also mean that the bytes in the
file can be successfully interpreted as a font. This way, the CSSFontFace in the example above won't be
set to the Succeeded state, so we will continue follow the src: list and download the secondary fonts.

This has an added benefit that the CSS Font Loading API's promises will be called more appropriately.
The transition to the Succeeded state will trigger a resolve of the promise. Now, these promises will
only be resolved if the fonts are actually parsed and understood by our text system.

Test: fast/text/font-fallback-invalid-load.html

* css/CSSFontFaceSource.cpp:
(WebCore::CSSFontFaceSource::fontLoaded): Move to the failed state if we can't understand the font
data. This is the crux of this patch.
(WebCore::CSSFontFaceSource::font): This function should only be called if we are in the Succeeded
state, which means now we know we should always be able to understand the bytes of the file. Therefore,
we can change some if statements into ASSERT()s.
* loader/cache/CachedSVGFont.cpp:
(WebCore::CachedSVGFont::createFont): Ditto.
(WebCore::CachedSVGFont::platformDataFromCustomData): Similarly to CSSFontFaceSource::fontLoaded(), this
adds another check to our criteria for transitioning into the Succeeded state, which will guarantee that
later we will always be able to create the font object.

LayoutTests:

* fast/text/font-fallback-invalid-load-expected.html: Added.
* fast/text/font-fallback-invalid-load.html: Added.
* fast/text/resources/bogus.svg: 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="#trunkSourceWebCorecssCSSFontFaceSourcecpp">trunk/Source/WebCore/css/CSSFontFaceSource.cpp</a></li>
<li><a href="#trunkSourceWebCoreloadercacheCachedSVGFontcpp">trunk/Source/WebCore/loader/cache/CachedSVGFont.cpp</a></li>
</ul>

<h3>Added Paths</h3>
<ul>
<li><a href="#trunkLayoutTestsfasttextfontfallbackinvalidloadexpectedhtml">trunk/LayoutTests/fast/text/font-fallback-invalid-load-expected.html</a></li>
<li><a href="#trunkLayoutTestsfasttextfontfallbackinvalidloadhtml">trunk/LayoutTests/fast/text/font-fallback-invalid-load.html</a></li>
<li><a href="#trunkLayoutTestsfasttextresourcesbogussvg">trunk/LayoutTests/fast/text/resources/bogus.svg</a></li>
</ul>

</div>
<div id="patch">
<h3>Diff</h3>
<a id="trunkLayoutTestsChangeLog"></a>
<div class="modfile"><h4>Modified: trunk/LayoutTests/ChangeLog (218156 => 218157)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/LayoutTests/ChangeLog      2017-06-13 00:04:33 UTC (rev 218156)
+++ trunk/LayoutTests/ChangeLog 2017-06-13 00:05:55 UTC (rev 218157)
</span><span class="lines">@@ -1,3 +1,15 @@
</span><ins>+2017-06-12  Myles C. Maxfield  <mmaxfield@apple.com>
+
+        @font-face rules with invalid primary fonts never download their secondary fonts
+        https://bugs.webkit.org/show_bug.cgi?id=173138
+        <rdar://problem/32554450>
+
+        Reviewed by Simon Fraser.
+
+        * fast/text/font-fallback-invalid-load-expected.html: Added.
+        * fast/text/font-fallback-invalid-load.html: Added.
+        * fast/text/resources/bogus.svg: Added.
+
</ins><span class="cx"> 2017-06-12  Daniel Bates  <dabates@apple.com>
</span><span class="cx"> 
</span><span class="cx">         Implement W3C Secure Contexts Draft Specification
</span></span></pre></div>
<a id="trunkLayoutTestsfasttextfontfallbackinvalidloadexpectedhtml"></a>
<div class="addfile"><h4>Added: trunk/LayoutTests/fast/text/font-fallback-invalid-load-expected.html (0 => 218157)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/LayoutTests/fast/text/font-fallback-invalid-load-expected.html                             (rev 0)
+++ trunk/LayoutTests/fast/text/font-fallback-invalid-load-expected.html        2017-06-13 00:05:55 UTC (rev 218157)
</span><span class="lines">@@ -0,0 +1,15 @@
</span><ins>+<!DOCTYPE html>
+<html>
+<head>
+<style>
+@font-face {
+    font-family: "WebFont";
+    src: url("../../resources/Ahem.ttf") format("truetype");
+}
+</style>
+</head>
+<body>
+This test makes sure that an @font-face with an invalid primary font file is rendered. The test passes if you see something other than this text on the page.
+<div style="font: 48px 'WebFont';">Hello</div>
+</body>
+</html>
</ins></span></pre></div>
<a id="trunkLayoutTestsfasttextfontfallbackinvalidloadhtml"></a>
<div class="addfile"><h4>Added: trunk/LayoutTests/fast/text/font-fallback-invalid-load.html (0 => 218157)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/LayoutTests/fast/text/font-fallback-invalid-load.html                              (rev 0)
+++ trunk/LayoutTests/fast/text/font-fallback-invalid-load.html 2017-06-13 00:05:55 UTC (rev 218157)
</span><span class="lines">@@ -0,0 +1,33 @@
</span><ins>+<!DOCTYPE html>
+<html>
+<head>
+<script>
+if (window.testRunner)
+    testRunner.waitUntilDone();
+</script>
+<style>
+@font-face {
+    font-family: "WebFont";
+    src: url("resources/bogus.svg") format("svg"), url("../../resources/Ahem.ttf") format("truetype");
+}
+</style>
+</head>
+<body>
+This test makes sure that an @font-face with an invalid primary font file is rendered. The test passes if you see something other than this text on the page.
+<div style="font: 48px 'WebFont', Helvetica;">Hello</div>
+<div id="error" style="font-size: 48px; color: red;"></div>
+<script>
+// We're waiting for Ahem to be loaded. Unfortunately, the WK API says that the load is complete when the number of concurrent in-flight subresources
+// hits 0, which occurs before we hit our second layout and realize we need to load Ahem. So, without this, the test would complete before Ahem is
+// requested.
+if (window.testRunner) {
+    document.fonts.keys().next().value.loaded.then(function() {
+        testRunner.notifyDone();
+    }, function() {
+        document.getElementById("error").textContent = "Error loading font.";
+        testRunner.notifyDone();
+    });
+}
+</script>
+</body>
+</html>
</ins></span></pre></div>
<a id="trunkLayoutTestsfasttextresourcesbogussvg"></a>
<div class="addfile"><h4>Added: trunk/LayoutTests/fast/text/resources/bogus.svg (0 => 218157)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/LayoutTests/fast/text/resources/bogus.svg                          (rev 0)
+++ trunk/LayoutTests/fast/text/resources/bogus.svg     2017-06-13 00:05:55 UTC (rev 218157)
</span><span class="lines">@@ -0,0 +1 @@
</span><ins>+bogus
</ins></span></pre></div>
<a id="trunkSourceWebCoreChangeLog"></a>
<div class="modfile"><h4>Modified: trunk/Source/WebCore/ChangeLog (218156 => 218157)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/WebCore/ChangeLog   2017-06-13 00:04:33 UTC (rev 218156)
+++ trunk/Source/WebCore/ChangeLog      2017-06-13 00:05:55 UTC (rev 218157)
</span><span class="lines">@@ -1,3 +1,39 @@
</span><ins>+2017-06-12  Myles C. Maxfield  <mmaxfield@apple.com>
+
+        @font-face rules with invalid primary fonts never download their secondary fonts
+        https://bugs.webkit.org/show_bug.cgi?id=173138
+        <rdar://problem/32554450>
+
+        Reviewed by Simon Fraser.
+
+        We have logic in CSSFontAccessor::font() which disallows downloading a CSSFontFace if that CSSFontFace
+        is already in the Succeeded state. However, it was possible for a succeeded CSSFontFace to still fail
+        to create a font. In this situation, we wouldn't be able to use the downloaded font, and we wouldn't
+        try to download the next item in the src: list because the CSSFontFace is succeeded.
+
+        This patch strengthens the meaning of the Succeeded state. Previously, it just meant that the bytes
+        in the file were downloaded successfully. This patch extends this to also mean that the bytes in the
+        file can be successfully interpreted as a font. This way, the CSSFontFace in the example above won't be
+        set to the Succeeded state, so we will continue follow the src: list and download the secondary fonts.
+
+        This has an added benefit that the CSS Font Loading API's promises will be called more appropriately.
+        The transition to the Succeeded state will trigger a resolve of the promise. Now, these promises will
+        only be resolved if the fonts are actually parsed and understood by our text system.
+
+        Test: fast/text/font-fallback-invalid-load.html
+
+        * css/CSSFontFaceSource.cpp:
+        (WebCore::CSSFontFaceSource::fontLoaded): Move to the failed state if we can't understand the font
+        data. This is the crux of this patch.
+        (WebCore::CSSFontFaceSource::font): This function should only be called if we are in the Succeeded
+        state, which means now we know we should always be able to understand the bytes of the file. Therefore,
+        we can change some if statements into ASSERT()s.
+        * loader/cache/CachedSVGFont.cpp:
+        (WebCore::CachedSVGFont::createFont): Ditto.
+        (WebCore::CachedSVGFont::platformDataFromCustomData): Similarly to CSSFontFaceSource::fontLoaded(), this
+        adds another check to our criteria for transitioning into the Succeeded state, which will guarantee that
+        later we will always be able to create the font object.
+
</ins><span class="cx"> 2017-06-12  Chris Dumez  <cdumez@apple.com>
</span><span class="cx"> 
</span><span class="cx">         Add Arabic Kasra to list of blacklisted characters when puny-decoding URL
</span></span></pre></div>
<a id="trunkSourceWebCorecssCSSFontFaceSourcecpp"></a>
<div class="modfile"><h4>Modified: trunk/Source/WebCore/css/CSSFontFaceSource.cpp (218156 => 218157)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/WebCore/css/CSSFontFaceSource.cpp   2017-06-13 00:04:33 UTC (rev 218156)
+++ trunk/Source/WebCore/css/CSSFontFaceSource.cpp      2017-06-13 00:05:55 UTC (rev 218157)
</span><span class="lines">@@ -120,7 +120,7 @@
</span><span class="cx">     if (m_face.webFontsShouldAlwaysFallBack())
</span><span class="cx">         return;
</span><span class="cx"> 
</span><del>-    if (m_font->errorOccurred())
</del><ins>+    if (m_font->errorOccurred() || !m_font->ensureCustomFontData(m_familyNameOrURI))
</ins><span class="cx">         setStatus(Status::Failure);
</span><span class="cx">     else
</span><span class="cx">         setStatus(Status::Success);
</span><span class="lines">@@ -193,10 +193,13 @@
</span><span class="cx">     }
</span><span class="cx"> 
</span><span class="cx">     if (m_font) {
</span><del>-        if (!m_font->ensureCustomFontData(m_familyNameOrURI))
-            return nullptr;
-
-        return m_font->createFont(fontDescription, m_familyNameOrURI, syntheticBold, syntheticItalic, fontFaceFeatures, fontFaceVariantSettings, fontFaceCapabilities);
</del><ins>+        auto success = m_font->ensureCustomFontData(m_familyNameOrURI);
+        ASSERT_UNUSED(success, success);
+            
+        ASSERT(status() == Status::Success);
+        auto result = m_font->createFont(fontDescription, m_familyNameOrURI, syntheticBold, syntheticItalic, fontFaceFeatures, fontFaceVariantSettings, fontFaceCapabilities);
+        ASSERT(result);
+        return result;
</ins><span class="cx">     }
</span><span class="cx"> 
</span><span class="cx">     // In-Document SVG Fonts
</span></span></pre></div>
<a id="trunkSourceWebCoreloadercacheCachedSVGFontcpp"></a>
<div class="modfile"><h4>Modified: trunk/Source/WebCore/loader/cache/CachedSVGFont.cpp (218156 => 218157)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/WebCore/loader/cache/CachedSVGFont.cpp      2017-06-13 00:04:33 UTC (rev 218156)
+++ trunk/Source/WebCore/loader/cache/CachedSVGFont.cpp 2017-06-13 00:05:55 UTC (rev 218157)
</span><span class="lines">@@ -54,9 +54,8 @@
</span><span class="cx"> 
</span><span class="cx"> RefPtr<Font> CachedSVGFont::createFont(const FontDescription& fontDescription, const AtomicString& remoteURI, bool syntheticBold, bool syntheticItalic, const FontFeatureSettings& fontFaceFeatures, const FontVariantSettings& fontFaceVariantSettings, FontSelectionSpecifiedCapabilities fontFaceCapabilities)
</span><span class="cx"> {
</span><del>-    if (firstFontFace(remoteURI))
-        return CachedFont::createFont(fontDescription, remoteURI, syntheticBold, syntheticItalic, fontFaceFeatures, fontFaceVariantSettings, fontFaceCapabilities);
-    return nullptr;
</del><ins>+    ASSERT(firstFontFace(remoteURI));
+    return CachedFont::createFont(fontDescription, remoteURI, syntheticBold, syntheticItalic, fontFaceFeatures, fontFaceVariantSettings, fontFaceCapabilities);
</ins><span class="cx"> }
</span><span class="cx"> 
</span><span class="cx"> FontPlatformData CachedSVGFont::platformDataFromCustomData(const FontDescription& fontDescription, bool bold, bool italic, const FontFeatureSettings& fontFaceFeatures, const FontVariantSettings& fontFaceVariantSettings, FontSelectionSpecifiedCapabilities fontFaceCapabilities)
</span><span class="lines">@@ -86,7 +85,7 @@
</span><span class="cx">             m_externalSVGDocument = nullptr;
</span><span class="cx">         if (m_externalSVGDocument)
</span><span class="cx">             maybeInitializeExternalSVGFontElement(remoteURI);
</span><del>-        if (!m_externalSVGFontElement)
</del><ins>+        if (!m_externalSVGFontElement || !firstFontFace(remoteURI))
</ins><span class="cx">             return false;
</span><span class="cx">         if (auto convertedFont = convertSVGToOTFFont(*m_externalSVGFontElement))
</span><span class="cx">             m_convertedFont = SharedBuffer::create(WTFMove(convertedFont.value()));
</span></span></pre>
</div>
</div>

</body>
</html>