Merge r243828 - Documents can be destroyed before their CSSFontFaceSe…

…t is destroyed https://bugs.webkit.org/show_bug.cgi?id=195830 Reviewed by Darin Adler. Source/WebCore: CSSFontFaceSet has a raw pointer to its owning document. JS can keep the CSSFontFaceSet alive (by using FontFaceSet) and can destroy the document at any time. When the document is destroyed, the link between the two objects needs to be severed. Test: fast/text/font-face-set-destroy-document.html * css/CSSFontFace.cpp: (WebCore::CSSFontFace::CSSFontFace): * css/CSSFontFace.h: * css/CSSFontFaceSet.cpp: (WebCore::CSSFontFaceSet::CSSFontFaceSet): (WebCore::CSSFontFaceSet::ensureLocalFontFacesForFamilyRegistered): * css/CSSFontFaceSet.h: * css/CSSFontSelector.cpp: (WebCore::CSSFontSelector::CSSFontSelector): (WebCore::CSSFontSelector::addFontFaceRule): * css/CSSFontSelector.h: * css/FontFace.cpp: (WebCore::FontFace::FontFace): LayoutTests: * fast/text/font-face-set-destroy-document-expected.html: Added. * fast/text/font-face-set-destroy-document.html: Added.
WebKit · Apr 8, 2019 · c401022 · c401022
1 parent 523a2ae
commit c401022
Show file tree

Hide file tree

Showing 8 changed files with 60 additions and 5 deletions.
diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
@@ -1,3 +1,13 @@
+2019-04-03  Myles C. Maxfield  <mmaxfield@apple.com>
+
+        Documents can be destroyed before their CSSFontFaceSet is destroyed
+        https://bugs.webkit.org/show_bug.cgi?id=195830
+
+        Reviewed by Darin Adler.
+
+        * fast/text/font-face-set-destroy-document-expected.html: Added.
+        * fast/text/font-face-set-destroy-document.html: Added.
+
 2019-03-26  Dean Jackson  <dino@apple.com>
 
         vertexAttribPointer must restrict offset parameter

diff --git a/LayoutTests/fast/text/font-face-set-destroy-document-expected.html b/LayoutTests/fast/text/font-face-set-destroy-document-expected.html
@@ -0,0 +1,3 @@
+<html>
+This test makes sure FontFaceSets don't access deleted documents. The test passes if there is no crash when running under ASan.
+</html>
diff --git a/LayoutTests/fast/text/font-face-set-destroy-document.html b/LayoutTests/fast/text/font-face-set-destroy-document.html
@@ -0,0 +1,15 @@
+<html>
+This test makes sure FontFaceSets don't access deleted documents. The test passes if there is no crash when running under ASan.
+<script>
+
+d = document.implementation.createDocument(null, '');
+f = new FontFace('f', 'local(F)', {});
+ffs = d.fonts;
+delete d;
+// gc();
+GCController.collect();
+
+// trigger use after free
+ffs.add(f);
+</script>
+</html>
diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
@@ -1,3 +1,30 @@
+2019-04-03  Myles C. Maxfield  <mmaxfield@apple.com>
+
+        Documents can be destroyed before their CSSFontFaceSet is destroyed
+        https://bugs.webkit.org/show_bug.cgi?id=195830
+
+        Reviewed by Darin Adler.
+
+        CSSFontFaceSet has a raw pointer to its owning document. JS can keep the CSSFontFaceSet alive (by using FontFaceSet)
+        and can destroy the document at any time. When the document is destroyed, the link between the two objects needs to
+        be severed.
+
+        Test: fast/text/font-face-set-destroy-document.html
+
+        * css/CSSFontFace.cpp:
+        (WebCore::CSSFontFace::CSSFontFace):
+        * css/CSSFontFace.h:
+        * css/CSSFontFaceSet.cpp:
+        (WebCore::CSSFontFaceSet::CSSFontFaceSet):
+        (WebCore::CSSFontFaceSet::ensureLocalFontFacesForFamilyRegistered):
+        * css/CSSFontFaceSet.h:
+        * css/CSSFontSelector.cpp:
+        (WebCore::CSSFontSelector::CSSFontSelector):
+        (WebCore::CSSFontSelector::addFontFaceRule):
+        * css/CSSFontSelector.h:
+        * css/FontFace.cpp:
+        (WebCore::FontFace::FontFace):
+
 2019-04-02  Ryosuke Niwa  <rniwa@webkit.org>
 
         Crash in HTMLCanvasElement::createContext2d after the element got adopted to a new document

diff --git a/Source/WebCore/css/CSSFontFace.h b/Source/WebCore/css/CSSFontFace.h
@@ -189,7 +189,7 @@ class CSSFontFace final : public RefCounted<CSSFontFace> {
     FontLoadingBehavior m_loadingBehavior { FontLoadingBehavior::Auto };
 
     Vector<std::unique_ptr<CSSFontFaceSource>, 0, CrashOnOverflow, 0> m_sources;
-    RefPtr<CSSFontSelector> m_fontSelector;
+    RefPtr<CSSFontSelector> m_fontSelector; // FIXME: https://bugs.webkit.org/show_bug.cgi?id=196437 There's a retain cycle: CSSFontSelector -> CSSFontFaceSet -> CSSFontFace -> CSSFontSelector
     RefPtr<StyleRuleFontFace> m_cssConnection;
     HashSet<Client*> m_clients;
     WeakPtr<FontFace> m_wrapper;

diff --git a/Source/WebCore/css/CSSFontFaceSet.cpp b/Source/WebCore/css/CSSFontFaceSet.cpp
@@ -42,7 +42,7 @@
 namespace WebCore {
 
 CSSFontFaceSet::CSSFontFaceSet(CSSFontSelector* owningFontSelector)
-    : m_owningFontSelector(owningFontSelector)
+    : m_owningFontSelector(makeWeakPtr(owningFontSelector))
 {
 }
 
@@ -113,7 +113,7 @@ void CSSFontFaceSet::ensureLocalFontFacesForFamilyRegistered(const String& famil
 
     Vector<Ref<CSSFontFace>> faces;
     for (auto item : capabilities) {
-        Ref<CSSFontFace> face = CSSFontFace::create(m_owningFontSelector, nullptr, nullptr, true);
+        Ref<CSSFontFace> face = CSSFontFace::create(m_owningFontSelector.get(), nullptr, nullptr, true);
 
         Ref<CSSValueList> familyList = CSSValueList::createCommaSeparated();
         familyList->append(CSSValuePool::singleton().createFontFamilyValue(familyName));

diff --git a/Source/WebCore/css/CSSFontFaceSet.h b/Source/WebCore/css/CSSFontFaceSet.h
@@ -120,7 +120,7 @@ class CSSFontFaceSet final : public RefCounted<CSSFontFaceSet>, public CSSFontFa
     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;
-    CSSFontSelector* m_owningFontSelector;
+    WeakPtr<CSSFontSelector> m_owningFontSelector;
     unsigned m_activeCount { 0 };
 };
 

diff --git a/Source/WebCore/css/CSSFontSelector.h b/Source/WebCore/css/CSSFontSelector.h
@@ -47,7 +47,7 @@ class CachedFont;
 class Document;
 class StyleRuleFontFace;
 
-class CSSFontSelector final : public FontSelector, public CSSFontFaceSetClient {
+class CSSFontSelector final : public FontSelector, public CSSFontFaceSetClient, public CanMakeWeakPtr<CSSFontSelector> {
 public:
     static Ref<CSSFontSelector> create(Document& document)
     {