[Font Loading] Crash when a single load request causes multiple fonts to fail loading
authormmaxfield@apple.com <mmaxfield@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 8 Mar 2016 22:22:40 +0000 (22:22 +0000)
committermmaxfield@apple.com <mmaxfield@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 8 Mar 2016 22:22:40 +0000 (22:22 +0000)
https://bugs.webkit.org/show_bug.cgi?id=155009

Reviewed by Simon Fraser.

Source/WebCore:

In JavaScript, the first promise fulfillment/failure wins. However, in C++, any
subsequent fulfillments/failures cause a crash.

Test: fast/text/font-face-set-document-multiple-failure.html

* css/CSSFontFace.cpp:
(WebCore::iterateClients): Notifying a client may cause some other client
to be destroyed, thereby modifying the clients set. This function allows
for notifying clients in a resilient manner.
(WebCore::CSSFontFace::setStyle): Update to use iterateClients().
(WebCore::CSSFontFace::setWeight): Ditto.
(WebCore::CSSFontFace::setUnicodeRange): Ditto.
(WebCore::CSSFontFace::setVariantLigatures): Ditto.
(WebCore::CSSFontFace::setVariantPosition): Ditto.
(WebCore::CSSFontFace::setVariantCaps): Ditto.
(WebCore::CSSFontFace::setVariantNumeric): Ditto.
(WebCore::CSSFontFace::setVariantAlternates): Ditto.
(WebCore::CSSFontFace::setVariantEastAsian): Ditto.
(WebCore::CSSFontFace::setFeatureSettings): Ditto.
(WebCore::CSSFontFace::setStatus): Ditto.
(WebCore::CSSFontFace::notifyClientsOfFontPropertyChange): Deleted.
* css/CSSFontFace.h: Adding a way for clients to make sure they don't register
or deregister another client.
* css/CSSFontFaceSet.cpp:
(WebCore::CSSFontFaceSet::guardAgainstClientRegistrationChanges): Simple
ref()/deref() pair.
(WebCore::CSSFontFaceSet::stopGuardingAgainstClientRegistrationChanges):
* css/CSSFontFaceSet.h:
* css/FontFace.cpp: Ditto.
(WebCore::FontFace::guardAgainstClientRegistrationChanges):
(WebCore::FontFace::stopGuardingAgainstClientRegistrationChanges):
* css/FontFace.h:
* css/FontFaceSet.cpp:
(WebCore::FontFaceSet::faceFinished): Make sure that we only fulfil or reject
a promise once.
* css/FontFaceSet.h:
* dom/Document.cpp:
(WebCore::Document::fonts): The CSSFontFaces inside the CSSFontSelector get
created during style recalc. We may be in a state where there is a style
recalc pending. In order to make sure the Javascript API sees the current
state of the world, force a style recalc here (but only if one is pending).

LayoutTests:

* fast/text/font-face-set-document-multiple-failure-expected.txt: Added.
* fast/text/font-face-set-document-multiple-failure.html: Added.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@197804 268f45cc-cd09-0410-ab3c-d52691b4dbfc

13 files changed:
LayoutTests/ChangeLog
LayoutTests/fast/text/font-face-set-document-multiple-failure-expected.txt [new file with mode: 0644]
LayoutTests/fast/text/font-face-set-document-multiple-failure.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/css/CSSFontFace.cpp
Source/WebCore/css/CSSFontFace.h
Source/WebCore/css/CSSFontFaceSet.cpp
Source/WebCore/css/CSSFontFaceSet.h
Source/WebCore/css/CSSSegmentedFontFace.h
Source/WebCore/css/FontFace.h
Source/WebCore/css/FontFaceSet.cpp
Source/WebCore/css/FontFaceSet.h
Source/WebCore/dom/Document.cpp

index 5967b66..0bf4ca0 100644 (file)
@@ -1,3 +1,13 @@
+2016-03-08  Myles C. Maxfield  <mmaxfield@apple.com>
+
+        [Font Loading] Crash when a single load request causes multiple fonts to fail loading
+        https://bugs.webkit.org/show_bug.cgi?id=155009
+
+        Reviewed by Simon Fraser.
+
+        * fast/text/font-face-set-document-multiple-failure-expected.txt: Added.
+        * fast/text/font-face-set-document-multiple-failure.html: Added.
+
 2016-03-08  Ryan Haddad  <ryanhaddad@apple.com>
 
         Skip fast/events/prevent-default-prevents-interaction-with-scrollbars.html on ios-simulator
diff --git a/LayoutTests/fast/text/font-face-set-document-multiple-failure-expected.txt b/LayoutTests/fast/text/font-face-set-document-multiple-failure-expected.txt
new file mode 100644 (file)
index 0000000..c891a57
--- /dev/null
@@ -0,0 +1,5 @@
+PASS globalX.code is globalX.NETWORK_ERR
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
diff --git a/LayoutTests/fast/text/font-face-set-document-multiple-failure.html b/LayoutTests/fast/text/font-face-set-document-multiple-failure.html
new file mode 100644 (file)
index 0000000..9ecdc71
--- /dev/null
@@ -0,0 +1,32 @@
+<!DOCTYPE html>
+<html>
+<head>
+<script src="../../resources/js-test-pre.js"></script>
+<style>
+@font-face {
+       font-family: "WebFont";
+       src: url("garbage");
+}
+@font-face {
+       font-family: "WebFont";
+       src: url("garbage");
+       font-variant: small-caps;
+}
+</style>
+</head>
+<body>
+<script>
+self.jsTestIsAsync = true;
+var globalX;
+document.fonts.load("16px WebFont").then(function() {
+    testFailed("Bogus URLs should not load");
+    finishJSTest();
+}, function(x) {
+       globalX = x;
+    shouldBe("globalX.code", "globalX.NETWORK_ERR");
+    finishJSTest();
+})
+</script>
+<script src="../../resources/js-test-post.js"></script>
+</body>
+</html>
\ No newline at end of file
index 8be4e0b..bda6324 100644 (file)
@@ -1,3 +1,52 @@
+2016-03-08  Myles C. Maxfield  <mmaxfield@apple.com>
+
+        [Font Loading] Crash when a single load request causes multiple fonts to fail loading
+        https://bugs.webkit.org/show_bug.cgi?id=155009
+
+        Reviewed by Simon Fraser.
+
+        In JavaScript, the first promise fulfillment/failure wins. However, in C++, any
+        subsequent fulfillments/failures cause a crash.
+
+        Test: fast/text/font-face-set-document-multiple-failure.html
+
+        * css/CSSFontFace.cpp:
+        (WebCore::iterateClients): Notifying a client may cause some other client
+        to be destroyed, thereby modifying the clients set. This function allows
+        for notifying clients in a resilient manner.
+        (WebCore::CSSFontFace::setStyle): Update to use iterateClients().
+        (WebCore::CSSFontFace::setWeight): Ditto.
+        (WebCore::CSSFontFace::setUnicodeRange): Ditto.
+        (WebCore::CSSFontFace::setVariantLigatures): Ditto.
+        (WebCore::CSSFontFace::setVariantPosition): Ditto.
+        (WebCore::CSSFontFace::setVariantCaps): Ditto.
+        (WebCore::CSSFontFace::setVariantNumeric): Ditto.
+        (WebCore::CSSFontFace::setVariantAlternates): Ditto.
+        (WebCore::CSSFontFace::setVariantEastAsian): Ditto.
+        (WebCore::CSSFontFace::setFeatureSettings): Ditto.
+        (WebCore::CSSFontFace::setStatus): Ditto.
+        (WebCore::CSSFontFace::notifyClientsOfFontPropertyChange): Deleted.
+        * css/CSSFontFace.h: Adding a way for clients to make sure they don't register
+        or deregister another client.
+        * css/CSSFontFaceSet.cpp:
+        (WebCore::CSSFontFaceSet::guardAgainstClientRegistrationChanges): Simple
+        ref()/deref() pair.
+        (WebCore::CSSFontFaceSet::stopGuardingAgainstClientRegistrationChanges):
+        * css/CSSFontFaceSet.h:
+        * css/FontFace.cpp: Ditto.
+        (WebCore::FontFace::guardAgainstClientRegistrationChanges):
+        (WebCore::FontFace::stopGuardingAgainstClientRegistrationChanges):
+        * css/FontFace.h:
+        * css/FontFaceSet.cpp:
+        (WebCore::FontFaceSet::faceFinished): Make sure that we only fulfil or reject
+        a promise once.
+        * css/FontFaceSet.h:
+        * dom/Document.cpp:
+        (WebCore::Document::fonts): The CSSFontFaces inside the CSSFontSelector get
+        created during style recalc. We may be in a state where there is a style
+        recalc pending. In order to make sure the Javascript API sees the current
+        state of the world, force a style recalc here (but only if one is pending).
+
 2016-03-08  Commit Queue  <commit-queue@webkit.org>
 
         Unreviewed, rolling out r197793 and r197799.
index 0e81d79..0f3ce58 100644 (file)
 
 namespace WebCore {
 
+template<typename T> void iterateClients(HashSet<CSSFontFace::Client*>& clients, T callback)
+{
+    Vector<Ref<CSSFontFace::Client>> clientsCopy;
+    clientsCopy.reserveInitialCapacity(clients.size());
+    for (auto* client : clients)
+        clientsCopy.uncheckedAppend(*client);
+
+    for (auto* client : clients)
+        callback(*client);
+}
+
 void CSSFontFace::appendSources(CSSFontFace& fontFace, CSSValueList& srcList, Document* document, bool isInitiatingElementInUserAgentShadowTree)
 {
     for (auto& src : srcList) {
@@ -89,15 +100,6 @@ CSSFontFace::~CSSFontFace()
 {
 }
 
-void CSSFontFace::notifyClientsOfFontPropertyChange()
-{
-    auto clientsCopy = m_clients;
-    for (auto* client : clientsCopy) {
-        if (m_clients.contains(client))
-            client->fontPropertyChanged(*this);
-    }
-}
-
 bool CSSFontFace::setFamilies(CSSValue& family)
 {
     if (!is<CSSValueList>(family))
@@ -110,11 +112,9 @@ bool CSSFontFace::setFamilies(CSSValue& family)
     RefPtr<CSSValueList> oldFamilies = m_families;
     m_families = &familyList;
 
-    auto clientsCopy = m_clients;
-    for (auto* client : clientsCopy) {
-        if (m_clients.contains(client))
-            client->fontPropertyChanged(*this, oldFamilies.get());
-    }
+    iterateClients(m_clients, [&](Client& client) {
+        client.fontPropertyChanged(*this, oldFamilies.get());
+    });
 
     return true;
 }
@@ -142,7 +142,9 @@ bool CSSFontFace::setStyle(CSSValue& style)
     if (auto mask = calculateStyleMask(style)) {
         m_traitsMask = static_cast<FontTraitsMask>((static_cast<unsigned>(m_traitsMask) & (~FontStyleMask)) | mask.value());
 
-        notifyClientsOfFontPropertyChange();
+        iterateClients(m_clients, [&](Client& client) {
+            client.fontPropertyChanged(*this);
+        });
 
         return true;
     }
@@ -189,7 +191,9 @@ bool CSSFontFace::setWeight(CSSValue& weight)
     if (auto mask = calculateWeightMask(weight)) {
         m_traitsMask = static_cast<FontTraitsMask>((static_cast<unsigned>(m_traitsMask) & (~FontWeightMask)) | mask.value());
 
-        notifyClientsOfFontPropertyChange();
+        iterateClients(m_clients, [&](Client& client) {
+            client.fontPropertyChanged(*this);
+        });
 
         return true;
     }
@@ -209,7 +213,9 @@ bool CSSFontFace::setUnicodeRange(CSSValue& unicodeRange)
         m_ranges.append(UnicodeRange(range.from(), range.to()));
     }
 
-    notifyClientsOfFontPropertyChange();
+    iterateClients(m_clients, [&](Client& client) {
+        client.fontPropertyChanged(*this);
+    });
 
     return true;
 }
@@ -222,7 +228,9 @@ bool CSSFontFace::setVariantLigatures(CSSValue& variantLigatures)
     m_variantSettings.historicalLigatures = ligatures.historicalLigatures;
     m_variantSettings.contextualAlternates = ligatures.contextualAlternates;
 
-    notifyClientsOfFontPropertyChange();
+    iterateClients(m_clients, [&](Client& client) {
+        client.fontPropertyChanged(*this);
+    });
 
     return true;
 }
@@ -233,7 +241,9 @@ bool CSSFontFace::setVariantPosition(CSSValue& variantPosition)
         return false;
     m_variantSettings.position = downcast<CSSPrimitiveValue>(variantPosition);
 
-    notifyClientsOfFontPropertyChange();
+    iterateClients(m_clients, [&](Client& client) {
+        client.fontPropertyChanged(*this);
+    });
 
     return true;
 }
@@ -244,7 +254,9 @@ bool CSSFontFace::setVariantCaps(CSSValue& variantCaps)
         return false;
     m_variantSettings.caps = downcast<CSSPrimitiveValue>(variantCaps);
 
-    notifyClientsOfFontPropertyChange();
+    iterateClients(m_clients, [&](Client& client) {
+        client.fontPropertyChanged(*this);
+    });
 
     return true;
 }
@@ -258,7 +270,9 @@ bool CSSFontFace::setVariantNumeric(CSSValue& variantNumeric)
     m_variantSettings.numericOrdinal = numeric.ordinal;
     m_variantSettings.numericSlashedZero = numeric.slashedZero;
 
-    notifyClientsOfFontPropertyChange();
+    iterateClients(m_clients, [&](Client& client) {
+        client.fontPropertyChanged(*this);
+    });
 
     return true;
 }
@@ -269,7 +283,9 @@ bool CSSFontFace::setVariantAlternates(CSSValue& variantAlternates)
         return false;
     m_variantSettings.alternates = downcast<CSSPrimitiveValue>(variantAlternates);
 
-    notifyClientsOfFontPropertyChange();
+    iterateClients(m_clients, [&](Client& client) {
+        client.fontPropertyChanged(*this);
+    });
 
     return true;
 }
@@ -281,7 +297,9 @@ bool CSSFontFace::setVariantEastAsian(CSSValue& variantEastAsian)
     m_variantSettings.eastAsianWidth = eastAsian.width;
     m_variantSettings.eastAsianRuby = eastAsian.ruby;
 
-    notifyClientsOfFontPropertyChange();
+    iterateClients(m_clients, [&](Client& client) {
+        client.fontPropertyChanged(*this);
+    });
 
     return true;
 }
@@ -298,7 +316,9 @@ bool CSSFontFace::setFeatureSettings(CSSValue& featureSettings)
         m_featureSettings.insert(FontFeature(feature.tag(), feature.value()));
     }
 
-    notifyClientsOfFontPropertyChange();
+    iterateClients(m_clients, [&](Client& client) {
+        client.fontPropertyChanged(*this);
+    });
 
     return true;
 }
@@ -380,8 +400,9 @@ void CSSFontFace::setStatus(Status newStatus)
         break;
     }
 
-    for (auto* client : m_clients)
-        client->fontStateChanged(*this, m_status, newStatus);
+    iterateClients(m_clients, [&](Client& client) {
+        client.fontStateChanged(*this, m_status, newStatus);
+    });
 
     m_status = newStatus;
 }
@@ -397,8 +418,9 @@ void CSSFontFace::fontLoaded(CSSFontFaceSource&)
     ASSERT(m_fontSelector);
     m_fontSelector->fontLoaded();
 
-    for (auto* client : m_clients)
-        client->fontLoaded(*this);
+    iterateClients(m_clients, [&](Client& client) {
+        client.fontLoaded(*this);
+    });
 }
 
 size_t CSSFontFace::pump()
index a48eff5..2fc76b7 100644 (file)
@@ -108,9 +108,11 @@ public:
     class Client {
     public:
         virtual ~Client() { }
-        virtual void fontLoaded(CSSFontFace&) { };
-        virtual void fontStateChanged(CSSFontFace&, Status oldState, Status newState) { UNUSED_PARAM(oldState); UNUSED_PARAM(newState); };
-        virtual void fontPropertyChanged(CSSFontFace&, CSSValueList* oldFamilies = nullptr) { UNUSED_PARAM(oldFamilies); };
+        virtual void fontLoaded(CSSFontFace&) { }
+        virtual void fontStateChanged(CSSFontFace&, Status oldState, Status newState) { UNUSED_PARAM(oldState); UNUSED_PARAM(newState); }
+        virtual void fontPropertyChanged(CSSFontFace&, CSSValueList* oldFamilies = nullptr) { UNUSED_PARAM(oldFamilies); }
+        virtual void ref() = 0;
+        virtual void deref() = 0;
     };
 
     // Pending => Loading  => TimedOut
index dbcfb7a..9b7a7b6 100644 (file)
@@ -398,13 +398,13 @@ CSSSegmentedFontFace* CSSFontFaceSet::getFontFace(FontTraitsMask traitsMask, con
         return nullptr;
     auto& familyFontFaces = iterator->value;
 
-    auto& segmentedFontFaceCache = m_cache.add(family, HashMap<unsigned, std::unique_ptr<CSSSegmentedFontFace>>()).iterator->value;
+    auto& segmentedFontFaceCache = m_cache.add(family, HashMap<unsigned, RefPtr<CSSSegmentedFontFace>>()).iterator->value;
 
     auto& face = segmentedFontFaceCache.add(traitsMask, nullptr).iterator->value;
     if (face)
         return face.get();
 
-    face = std::make_unique<CSSSegmentedFontFace>();
+    face = CSSSegmentedFontFace::create();
 
     Vector<std::reference_wrapper<CSSFontFace>, 32> candidateFontFaces;
     for (int i = familyFontFaces.size() - 1; i >= 0; --i) {
index 1b58ad6..00458f0 100644 (file)
@@ -75,6 +75,10 @@ public:
 
     Vector<std::reference_wrapper<CSSFontFace>> matchingFaces(const String& font, const String& text, ExceptionCode&);
 
+    // CSSFontFace::Client needs to be able to be held in a RefPtr.
+    void ref() override { RefCounted<CSSFontFaceSet>::ref(); }
+    void deref() override { RefCounted<CSSFontFaceSet>::deref(); }
+
 private:
     CSSFontFaceSet();
 
@@ -95,7 +99,7 @@ private:
     Vector<Ref<CSSFontFace>> m_faces; // We should investigate moving m_faces to FontFaceSet and making it reference FontFaces. This may clean up the font loading design.
     HashMap<String, Vector<Ref<CSSFontFace>>, ASCIICaseInsensitiveHash> m_facesLookupTable;
     HashMap<String, Vector<Ref<CSSFontFace>>, ASCIICaseInsensitiveHash> m_locallyInstalledFacesLookupTable;
-    HashMap<String, HashMap<unsigned, std::unique_ptr<CSSSegmentedFontFace>>, ASCIICaseInsensitiveHash> m_cache;
+    HashMap<String, HashMap<unsigned, RefPtr<CSSSegmentedFontFace>>, ASCIICaseInsensitiveHash> m_cache;
     size_t m_facesPartitionIndex { 0 }; // All entries in m_faces before this index are CSS-connected.
     Status m_status { Status::Loaded };
     HashSet<CSSFontFaceSetClient*> m_clients;
index 9179a09..946dfbc 100644 (file)
@@ -39,10 +39,13 @@ namespace WebCore {
 class CSSFontSelector;
 class FontDescription;
 
-class CSSSegmentedFontFace final : public CSSFontFace::Client {
+class CSSSegmentedFontFace final : public RefCounted<CSSSegmentedFontFace>, public CSSFontFace::Client {
     WTF_MAKE_FAST_ALLOCATED;
 public:
-    CSSSegmentedFontFace();
+    static Ref<CSSSegmentedFontFace> create()
+    {
+        return adoptRef(*new CSSSegmentedFontFace());
+    }
     ~CSSSegmentedFontFace();
 
     void appendFontFace(Ref<CSSFontFace>&&);
@@ -51,7 +54,12 @@ public:
 
     Vector<Ref<CSSFontFace>, 1>& constituentFaces() { return m_fontFaces; }
 
+    // CSSFontFace::Client needs to be able to be held in a RefPtr.
+    void ref() override { RefCounted<CSSSegmentedFontFace>::ref(); }
+    void deref() override { RefCounted<CSSSegmentedFontFace>::deref(); }
+
 private:
+    CSSSegmentedFontFace();
     void fontLoaded(CSSFontFace&) override;
 
     HashMap<FontDescriptionKey, FontRanges, FontDescriptionKeyHash, WTF::SimpleClassHashTraits<FontDescriptionKey>> m_cache;
index 9aa5d12..d6c19a3 100644 (file)
@@ -82,6 +82,10 @@ public:
 
     WeakPtr<FontFace> createWeakPtr() const;
 
+    // CSSFontFace::Client needs to be able to be held in a RefPtr.
+    void ref() override { RefCounted<FontFace>::ref(); }
+    void deref() override { RefCounted<FontFace>::deref(); }
+
 private:
     FontFace(JSC::ExecState&, CSSFontSelector&);
     FontFace(JSC::ExecState&, CSSFontFace&);
index ad5aa48..3f9f58b 100644 (file)
@@ -236,12 +236,16 @@ void FontFaceSet::faceFinished(CSSFontFace& face, CSSFontFace::Status newStatus)
 
     for (auto& pendingPromise : iterator->value) {
         if (newStatus == CSSFontFace::Status::Success) {
-            if (pendingPromise->hasOneRef())
+            if (pendingPromise->hasOneRef() && !pendingPromise->hasReachedTerminalState) {
                 pendingPromise->promise.resolve(pendingPromise->faces);
+                pendingPromise->hasReachedTerminalState = true;
+            }
         } else {
             ASSERT(newStatus == CSSFontFace::Status::Failure);
-            // The first resolution wins, so we can just reject early now.
-            pendingPromise->promise.reject(DOMCoreException::create(ExceptionCodeDescription(NETWORK_ERR)));
+            if (!pendingPromise->hasReachedTerminalState) {
+                pendingPromise->promise.reject(DOMCoreException::create(ExceptionCodeDescription(NETWORK_ERR)));
+                pendingPromise->hasReachedTerminalState = true;
+            }
         }
     }
 
index b30cd45..870d133 100644 (file)
@@ -101,6 +101,7 @@ private:
     public:
         Vector<RefPtr<FontFace>> faces;
         Promise promise;
+        bool hasReachedTerminalState { false };
     };
 
     FontFaceSet(Document&, const Vector<RefPtr<FontFace>>&);
index 63d8ba0..c1d43fc 100644 (file)
@@ -6706,6 +6706,7 @@ Document& Document::ensureTemplateDocument()
 
 Ref<FontFaceSet> Document::fonts()
 {
+    updateStyleIfNeeded();
     return fontSelector().fontFaceSet();
 }