[CSS OM] StyledElementInlineStylePropertyMap creates a Ref cycle with its owner element
authorrniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 20 Mar 2019 21:23:02 +0000 (21:23 +0000)
committerrniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 20 Mar 2019 21:23:02 +0000 (21:23 +0000)
https://bugs.webkit.org/show_bug.cgi?id=195987

Reviewed by Simon Fraser.

Source/WebCore:

StyledElementInlineStylePropertyMap was leaking every element for which it was created because due to
a reference cycle. The StyledElementInlineStylePropertyMap holds onto its element using Ref and
the element also stores StyledElementInlineStylePropertyMap in ElementRareData using RefPtr.

Fixed the cycle by making the reference from StyledElementInlineStylePropertyMap weak. For now we use
a raw pointer because we can't create a WeakPtr of an element yet.

Test: css-typedom/attribute-style-map-should-not-leak-every-element.html

* css/typedom/StylePropertyMap.h:
(WebCore::StylePropertyMap): Added clearElement as a virtual function.
* dom/Element.cpp:
(WebCore::Element::~Element): Clear the element pointer in StyledElementInlineStylePropertyMap.
* dom/StyledElement.cpp:
(WebCore::StyledElementInlineStylePropertyMap::get): Added a null check for m_element.
(WebCore::StyledElementInlineStylePropertyMap::StyledElementInlineStylePropertyMap):
(WebCore::StyledElementInlineStylePropertyMap::clearElement): Added.
(WebCore::StyledElementInlineStylePropertyMap): Use a raw pointer instead of Ref to StyledElement
to avoid the leak.
* platform/graphics/CustomPaintImage.cpp:
(WebCore::HashMapStylePropertyMap::clearElement): Added.

LayoutTests:

Added a regression test.

* css-typedom/attribute-style-map-should-not-leak-every-element-expected.txt: Added.
* css-typedom/attribute-style-map-should-not-leak-every-element.html: Added.

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

LayoutTests/ChangeLog
LayoutTests/css-typedom/attribute-style-map-should-not-leak-every-element-expected.txt [new file with mode: 0644]
LayoutTests/css-typedom/attribute-style-map-should-not-leak-every-element.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/css/typedom/StylePropertyMap.h
Source/WebCore/dom/Element.cpp
Source/WebCore/dom/StyledElement.cpp
Source/WebCore/platform/graphics/CustomPaintImage.cpp

index 596d4ca..3188574 100644 (file)
@@ -1,3 +1,15 @@
+2019-03-19  Ryosuke Niwa  <rniwa@webkit.org>
+
+        [CSS OM] StyledElementInlineStylePropertyMap creates a Ref cycle with its owner element
+        https://bugs.webkit.org/show_bug.cgi?id=195987
+
+        Reviewed by Simon Fraser.
+
+        Added a regression test.
+
+        * css-typedom/attribute-style-map-should-not-leak-every-element-expected.txt: Added.
+        * css-typedom/attribute-style-map-should-not-leak-every-element.html: Added.
+
 2019-03-20  Antoine Quint  <graouts@apple.com>
 
         REGRESSION(r240634): Element::hasPointerCapture() passes a JS-controlled value directly into a HashMap as a key
diff --git a/LayoutTests/css-typedom/attribute-style-map-should-not-leak-every-element-expected.txt b/LayoutTests/css-typedom/attribute-style-map-should-not-leak-every-element-expected.txt
new file mode 100644 (file)
index 0000000..e1acec0
--- /dev/null
@@ -0,0 +1,3 @@
+This tests allocating 1000 elements and triggering GC. GC should collect some elements.
+
+PASS
diff --git a/LayoutTests/css-typedom/attribute-style-map-should-not-leak-every-element.html b/LayoutTests/css-typedom/attribute-style-map-should-not-leak-every-element.html
new file mode 100644 (file)
index 0000000..5e33baf
--- /dev/null
@@ -0,0 +1,29 @@
+<!DOCTYPE html>
+<html>
+<body>
+<p>This tests allocating 1000 elements and triggering GC. GC should collect some elements.</p>
+<script>
+
+if (!window.internals)
+    document.write('This test requires internals');
+else {
+    testRunner.dumpAsText();
+
+    const initialNodeCount = internals.numberOfLiveNodes();
+
+    const testCount = 1000;
+    (() => {
+        for (let i = 0; i < testCount; ++i)
+            document.createElement('div').attributeStyleMap;
+    })();
+
+    if (window.GCController)
+        GCController.collect();
+
+    const nodeCountDiff = internals.numberOfLiveNodes() - initialNodeCount;
+    document.write(nodeCountDiff < testCount / 1.5 ? 'PASS' : `FAIL - ${nodeCountDiff} nodes alive after triggering GC`);
+}
+
+</script>
+</body>
+</html>
index 1841b9b..978b211 100644 (file)
@@ -1,5 +1,34 @@
 2019-03-19  Ryosuke Niwa  <rniwa@webkit.org>
 
+        [CSS OM] StyledElementInlineStylePropertyMap creates a Ref cycle with its owner element
+        https://bugs.webkit.org/show_bug.cgi?id=195987
+
+        Reviewed by Simon Fraser.
+
+        StyledElementInlineStylePropertyMap was leaking every element for which it was created because due to
+        a reference cycle. The StyledElementInlineStylePropertyMap holds onto its element using Ref and
+        the element also stores StyledElementInlineStylePropertyMap in ElementRareData using RefPtr.
+
+        Fixed the cycle by making the reference from StyledElementInlineStylePropertyMap weak. For now we use
+        a raw pointer because we can't create a WeakPtr of an element yet.
+
+        Test: css-typedom/attribute-style-map-should-not-leak-every-element.html
+
+        * css/typedom/StylePropertyMap.h:
+        (WebCore::StylePropertyMap): Added clearElement as a virtual function.
+        * dom/Element.cpp:
+        (WebCore::Element::~Element): Clear the element pointer in StyledElementInlineStylePropertyMap.
+        * dom/StyledElement.cpp:
+        (WebCore::StyledElementInlineStylePropertyMap::get): Added a null check for m_element.
+        (WebCore::StyledElementInlineStylePropertyMap::StyledElementInlineStylePropertyMap):
+        (WebCore::StyledElementInlineStylePropertyMap::clearElement): Added.
+        (WebCore::StyledElementInlineStylePropertyMap): Use a raw pointer instead of Ref to StyledElement
+        to avoid the leak.
+        * platform/graphics/CustomPaintImage.cpp:
+        (WebCore::HashMapStylePropertyMap::clearElement): Added.
+
+2019-03-19  Ryosuke Niwa  <rniwa@webkit.org>
+
         appendChild should throw when inserting an ancestor of a template into its content adopted to another document
         https://bugs.webkit.org/show_bug.cgi?id=195984
 
index 7b19f95..cf59d02 100644 (file)
@@ -35,6 +35,8 @@
 namespace WebCore {
 
 class StylePropertyMap : public StylePropertyMapReadOnly {
+public:
+    virtual void clearElement() = 0;
 };
 
 } // namespace WebCore
index 6e84493..9f8a381 100644 (file)
@@ -203,6 +203,13 @@ Element::~Element()
     if (hasSyntheticAttrChildNodes())
         detachAllAttrNodesFromElement();
 
+#if ENABLE(CSS_TYPED_OM)
+    if (hasRareData()) {
+        if (auto* map = elementRareData()->attributeStyleMap())
+            map->clearElement();
+    }
+#endif
+
     if (hasPendingResources()) {
         document().accessSVGExtensions().removeElementFromPendingResources(*this);
         ASSERT(!hasPendingResources());
index 38685df..92a973a 100644 (file)
@@ -88,14 +88,19 @@ public:
 private:
     RefPtr<TypedOMCSSStyleValue> get(const String& property) const final
     {
-        return extractInlineProperty(property, m_element.get());
+        ASSERT(m_element); // Hitting this assertion would imply a GC bug. Element is collected while this property map is alive.
+        if (!m_element)
+            return nullptr;
+        return extractInlineProperty(property, *m_element);
     }
 
     explicit StyledElementInlineStylePropertyMap(StyledElement& element)
-        : m_element(makeRef(element))
+        : m_element(&element)
     {
     }
 
+    void clearElement() override { m_element = nullptr; }
+
     static RefPtr<TypedOMCSSStyleValue> extractInlineProperty(const String& name, StyledElement& element)
     {
         if (!element.inlineStyle())
@@ -114,7 +119,7 @@ private:
         return StylePropertyMapReadOnly::reifyValue(value.get(), element.document(), &element);
     }
 
-    Ref<StyledElement> m_element;
+    StyledElement* m_element { nullptr };
 };
 
 StylePropertyMap& StyledElement::ensureAttributeStyleMap()
index c32e4f0..df156f8 100644 (file)
@@ -105,6 +105,8 @@ private:
     {
     }
 
+    void clearElement() override { }
+
     RefPtr<TypedOMCSSStyleValue> get(const String& property) const final { return makeRefPtr(m_map.get(property)); }
 
     HashMap<String, RefPtr<TypedOMCSSStyleValue>> m_map;