From f9200ceecc884822554a5d4e6100a361aa886346 Mon Sep 17 00:00:00 2001 From: "mmaxfield@apple.com" Date: Tue, 8 Mar 2016 22:22:40 +0000 Subject: [PATCH] [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. 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 --- LayoutTests/ChangeLog | 10 +++ ...face-set-document-multiple-failure-expected.txt | 5 ++ .../font-face-set-document-multiple-failure.html | 32 +++++++++ Source/WebCore/ChangeLog | 49 ++++++++++++++ Source/WebCore/css/CSSFontFace.cpp | 78 ++++++++++++++-------- Source/WebCore/css/CSSFontFace.h | 8 ++- Source/WebCore/css/CSSFontFaceSet.cpp | 4 +- Source/WebCore/css/CSSFontFaceSet.h | 6 +- Source/WebCore/css/CSSSegmentedFontFace.h | 12 +++- Source/WebCore/css/FontFace.h | 4 ++ Source/WebCore/css/FontFaceSet.cpp | 10 ++- Source/WebCore/css/FontFaceSet.h | 1 + Source/WebCore/dom/Document.cpp | 1 + 13 files changed, 181 insertions(+), 39 deletions(-) create mode 100644 LayoutTests/fast/text/font-face-set-document-multiple-failure-expected.txt create mode 100644 LayoutTests/fast/text/font-face-set-document-multiple-failure.html diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog index 5967b66..0bf4ca0 100644 --- a/LayoutTests/ChangeLog +++ b/LayoutTests/ChangeLog @@ -1,3 +1,13 @@ +2016-03-08 Myles C. Maxfield + + [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 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 index 0000000..c891a57 --- /dev/null +++ b/LayoutTests/fast/text/font-face-set-document-multiple-failure-expected.txt @@ -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 index 0000000..9ecdc71 --- /dev/null +++ b/LayoutTests/fast/text/font-face-set-document-multiple-failure.html @@ -0,0 +1,32 @@ + + + + + + + + + + + \ No newline at end of file diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog index 8be4e0b..bda6324 100644 --- a/Source/WebCore/ChangeLog +++ b/Source/WebCore/ChangeLog @@ -1,3 +1,52 @@ +2016-03-08 Myles C. Maxfield + + [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 Unreviewed, rolling out r197793 and r197799. diff --git a/Source/WebCore/css/CSSFontFace.cpp b/Source/WebCore/css/CSSFontFace.cpp index 0e81d79..0f3ce58 100644 --- a/Source/WebCore/css/CSSFontFace.cpp +++ b/Source/WebCore/css/CSSFontFace.cpp @@ -48,6 +48,17 @@ namespace WebCore { +template void iterateClients(HashSet& clients, T callback) +{ + Vector> 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(family)) @@ -110,11 +112,9 @@ bool CSSFontFace::setFamilies(CSSValue& family) RefPtr 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((static_cast(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((static_cast(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(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(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(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() diff --git a/Source/WebCore/css/CSSFontFace.h b/Source/WebCore/css/CSSFontFace.h index a48eff5..2fc76b7 100644 --- a/Source/WebCore/css/CSSFontFace.h +++ b/Source/WebCore/css/CSSFontFace.h @@ -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 diff --git a/Source/WebCore/css/CSSFontFaceSet.cpp b/Source/WebCore/css/CSSFontFaceSet.cpp index dbcfb7a..9b7a7b6 100644 --- a/Source/WebCore/css/CSSFontFaceSet.cpp +++ b/Source/WebCore/css/CSSFontFaceSet.cpp @@ -398,13 +398,13 @@ CSSSegmentedFontFace* CSSFontFaceSet::getFontFace(FontTraitsMask traitsMask, con return nullptr; auto& familyFontFaces = iterator->value; - auto& segmentedFontFaceCache = m_cache.add(family, HashMap>()).iterator->value; + auto& segmentedFontFaceCache = m_cache.add(family, HashMap>()).iterator->value; auto& face = segmentedFontFaceCache.add(traitsMask, nullptr).iterator->value; if (face) return face.get(); - face = std::make_unique(); + face = CSSSegmentedFontFace::create(); Vector, 32> candidateFontFaces; for (int i = familyFontFaces.size() - 1; i >= 0; --i) { diff --git a/Source/WebCore/css/CSSFontFaceSet.h b/Source/WebCore/css/CSSFontFaceSet.h index 1b58ad6..00458f0 100644 --- a/Source/WebCore/css/CSSFontFaceSet.h +++ b/Source/WebCore/css/CSSFontFaceSet.h @@ -75,6 +75,10 @@ public: Vector> matchingFaces(const String& font, const String& text, ExceptionCode&); + // CSSFontFace::Client needs to be able to be held in a RefPtr. + void ref() override { RefCounted::ref(); } + void deref() override { RefCounted::deref(); } + private: CSSFontFaceSet(); @@ -95,7 +99,7 @@ private: Vector> m_faces; // We should investigate moving m_faces to FontFaceSet and making it reference FontFaces. This may clean up the font loading design. HashMap>, ASCIICaseInsensitiveHash> m_facesLookupTable; HashMap>, ASCIICaseInsensitiveHash> m_locallyInstalledFacesLookupTable; - HashMap>, ASCIICaseInsensitiveHash> m_cache; + HashMap>, 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 m_clients; diff --git a/Source/WebCore/css/CSSSegmentedFontFace.h b/Source/WebCore/css/CSSSegmentedFontFace.h index 9179a09..946dfbc 100644 --- a/Source/WebCore/css/CSSSegmentedFontFace.h +++ b/Source/WebCore/css/CSSSegmentedFontFace.h @@ -39,10 +39,13 @@ namespace WebCore { class CSSFontSelector; class FontDescription; -class CSSSegmentedFontFace final : public CSSFontFace::Client { +class CSSSegmentedFontFace final : public RefCounted, public CSSFontFace::Client { WTF_MAKE_FAST_ALLOCATED; public: - CSSSegmentedFontFace(); + static Ref create() + { + return adoptRef(*new CSSSegmentedFontFace()); + } ~CSSSegmentedFontFace(); void appendFontFace(Ref&&); @@ -51,7 +54,12 @@ public: Vector, 1>& constituentFaces() { return m_fontFaces; } + // CSSFontFace::Client needs to be able to be held in a RefPtr. + void ref() override { RefCounted::ref(); } + void deref() override { RefCounted::deref(); } + private: + CSSSegmentedFontFace(); void fontLoaded(CSSFontFace&) override; HashMap> m_cache; diff --git a/Source/WebCore/css/FontFace.h b/Source/WebCore/css/FontFace.h index 9aa5d12..d6c19a3 100644 --- a/Source/WebCore/css/FontFace.h +++ b/Source/WebCore/css/FontFace.h @@ -82,6 +82,10 @@ public: WeakPtr createWeakPtr() const; + // CSSFontFace::Client needs to be able to be held in a RefPtr. + void ref() override { RefCounted::ref(); } + void deref() override { RefCounted::deref(); } + private: FontFace(JSC::ExecState&, CSSFontSelector&); FontFace(JSC::ExecState&, CSSFontFace&); diff --git a/Source/WebCore/css/FontFaceSet.cpp b/Source/WebCore/css/FontFaceSet.cpp index ad5aa48..3f9f58b 100644 --- a/Source/WebCore/css/FontFaceSet.cpp +++ b/Source/WebCore/css/FontFaceSet.cpp @@ -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; + } } } diff --git a/Source/WebCore/css/FontFaceSet.h b/Source/WebCore/css/FontFaceSet.h index b30cd45..870d133 100644 --- a/Source/WebCore/css/FontFaceSet.h +++ b/Source/WebCore/css/FontFaceSet.h @@ -101,6 +101,7 @@ private: public: Vector> faces; Promise promise; + bool hasReachedTerminalState { false }; }; FontFaceSet(Document&, const Vector>&); diff --git a/Source/WebCore/dom/Document.cpp b/Source/WebCore/dom/Document.cpp index 63d8ba0..c1d43fc4 100644 --- a/Source/WebCore/dom/Document.cpp +++ b/Source/WebCore/dom/Document.cpp @@ -6706,6 +6706,7 @@ Document& Document::ensureTemplateDocument() Ref Document::fonts() { + updateStyleIfNeeded(); return fontSelector().fontFaceSet(); } -- 1.8.3.1