Documents can be destroyed before their CSSFontFaceSet is destroyed
authormmaxfield@apple.com <mmaxfield@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 3 Apr 2019 21:46:55 +0000 (21:46 +0000)
committermmaxfield@apple.com <mmaxfield@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 3 Apr 2019 21:46:55 +0000 (21:46 +0000)
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.

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

LayoutTests/ChangeLog
LayoutTests/fast/text/font-face-set-destroy-document-expected.html [new file with mode: 0644]
LayoutTests/fast/text/font-face-set-destroy-document.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/css/CSSFontFace.h
Source/WebCore/css/CSSFontFaceSet.cpp
Source/WebCore/css/CSSFontFaceSet.h
Source/WebCore/css/CSSFontSelector.h

index 0896009..a7299b9 100644 (file)
@@ -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-04-03  Shawn Roberts  <sroberts@apple.com>
 
         http/tests/storageAccess/request-and-grant-access-cross-origin-sandboxed-iframe-from-prevalent-domain-with-user-interaction-but-access-from-wrong-frame.html is a flaky timeout
diff --git a/LayoutTests/fast/text/font-face-set-destroy-document-expected.html b/LayoutTests/fast/text/font-face-set-destroy-document-expected.html
new file mode 100644 (file)
index 0000000..9db8520
--- /dev/null
@@ -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
new file mode 100644 (file)
index 0000000..1a14400
--- /dev/null
@@ -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>
index d2b5d1c..1d57a38 100644 (file)
@@ -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-03  Sihui Liu  <sihui_liu@apple.com>
 
         Follow up fix for r243807: Use MarkedArgumentBuffer instead of Vector for JSValue
index bd7fc22..ad06d19 100644 (file)
@@ -189,7 +189,7 @@ private:
     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;
index b0b073b..0a8cf09 100644 (file)
@@ -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));
index 7d75f54..795d0a8 100644 (file)
@@ -120,7 +120,7 @@ private:
     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 };
 };
 
index eb4e063..9c31b36 100644 (file)
@@ -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)
     {