Changeset 243239 in webkit

Timestamp:

Mar 20, 2019, 2:23:02 PM (7 years ago)

Author:

rniwa@webkit.org

Message:

[CSS OM] StyledElementInlineStylePropertyMap creates a Ref cycle with its owner element
​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.

Location:

trunk

Files:

• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/css-typedom/attribute-style-map-should-not-leak-every-element-expected.txt (added)
• LayoutTests/css-typedom/attribute-style-map-should-not-leak-every-element.html (added)
• Source/WebCore/ChangeLog (modified) (1 diff)
• Source/WebCore/css/typedom/StylePropertyMap.h (modified) (1 diff)
• Source/WebCore/dom/Element.cpp (modified) (1 diff)
• Source/WebCore/dom/StyledElement.cpp (modified) (2 diffs)
• Source/WebCore/platform/graphics/CustomPaintImage.cpp (modified) (1 diff)

trunk/LayoutTests/ChangeLog

@@ +1 @@
+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>
 

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+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>
 

trunk/Source/WebCore/css/typedom/StylePropertyMap.h

@@ -36 +36 @@
 
 class StylePropertyMap : public StylePropertyMapReadOnly {
+public:
+    virtual void clearElement() = 0;
 };
 

trunk/Source/WebCore/dom/Element.cpp

@@ -204 +204 @@
         detachAllAttrNodesFromElement();
 
+#if ENABLE(CSS_TYPED_OM)
+    if (hasRareData()) {
+        if (auto* map = elementRareData()->attributeStyleMap())
+            map->clearElement();
+    }
+#endif
+
     if (hasPendingResources()) {
         document().accessSVGExtensions().removeElementFromPendingResources(*this);

trunk/Source/WebCore/dom/StyledElement.cpp

@@ -89 +89 @@
     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)
@@ -115 +120 @@
     }
 
-    Ref<StyledElement> m_element;
+    StyledElement* m_element { nullptr };
 };
 

trunk/Source/WebCore/platform/graphics/CustomPaintImage.cpp

@@ -106 +106 @@
     }
 
+    void clearElement() override { }
+
     RefPtr<TypedOMCSSStyleValue> get(const String& property) const final { return makeRefPtr(m_map.get(property)); }