- Revision
- 243239
- Author
- [email protected]
- Date
- 2019-03-20 14:23:02 -0700 (Wed, 20 Mar 2019)
Log 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.
Modified Paths
Added Paths
Diff
Modified: trunk/LayoutTests/ChangeLog (243238 => 243239)
--- trunk/LayoutTests/ChangeLog 2019-03-20 21:22:08 UTC (rev 243238)
+++ trunk/LayoutTests/ChangeLog 2019-03-20 21:23:02 UTC (rev 243239)
@@ -1,3 +1,15 @@
+2019-03-19 Ryosuke Niwa <[email protected]>
+
+ [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 <[email protected]>
REGRESSION(r240634): Element::hasPointerCapture() passes a JS-controlled value directly into a HashMap as a key
Added: trunk/LayoutTests/css-typedom/attribute-style-map-should-not-leak-every-element-expected.txt (0 => 243239)
--- trunk/LayoutTests/css-typedom/attribute-style-map-should-not-leak-every-element-expected.txt (rev 0)
+++ trunk/LayoutTests/css-typedom/attribute-style-map-should-not-leak-every-element-expected.txt 2019-03-20 21:23:02 UTC (rev 243239)
@@ -0,0 +1,3 @@
+This tests allocating 1000 elements and triggering GC. GC should collect some elements.
+
+PASS
Added: trunk/LayoutTests/css-typedom/attribute-style-map-should-not-leak-every-element.html (0 => 243239)
--- trunk/LayoutTests/css-typedom/attribute-style-map-should-not-leak-every-element.html (rev 0)
+++ trunk/LayoutTests/css-typedom/attribute-style-map-should-not-leak-every-element.html 2019-03-20 21:23:02 UTC (rev 243239)
@@ -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>
Modified: trunk/Source/WebCore/ChangeLog (243238 => 243239)
--- trunk/Source/WebCore/ChangeLog 2019-03-20 21:22:08 UTC (rev 243238)
+++ trunk/Source/WebCore/ChangeLog 2019-03-20 21:23:02 UTC (rev 243239)
@@ -1,5 +1,34 @@
2019-03-19 Ryosuke Niwa <[email protected]>
+ [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 <[email protected]>
+
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
Modified: trunk/Source/WebCore/css/typedom/StylePropertyMap.h (243238 => 243239)
--- trunk/Source/WebCore/css/typedom/StylePropertyMap.h 2019-03-20 21:22:08 UTC (rev 243238)
+++ trunk/Source/WebCore/css/typedom/StylePropertyMap.h 2019-03-20 21:23:02 UTC (rev 243239)
@@ -35,6 +35,8 @@
namespace WebCore {
class StylePropertyMap : public StylePropertyMapReadOnly {
+public:
+ virtual void clearElement() = 0;
};
} // namespace WebCore
Modified: trunk/Source/WebCore/dom/Element.cpp (243238 => 243239)
--- trunk/Source/WebCore/dom/Element.cpp 2019-03-20 21:22:08 UTC (rev 243238)
+++ trunk/Source/WebCore/dom/Element.cpp 2019-03-20 21:23:02 UTC (rev 243239)
@@ -203,6 +203,13 @@
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());
Modified: trunk/Source/WebCore/dom/StyledElement.cpp (243238 => 243239)
--- trunk/Source/WebCore/dom/StyledElement.cpp 2019-03-20 21:22:08 UTC (rev 243238)
+++ trunk/Source/WebCore/dom/StyledElement.cpp 2019-03-20 21:23:02 UTC (rev 243239)
@@ -88,14 +88,19 @@
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 @@
return StylePropertyMapReadOnly::reifyValue(value.get(), element.document(), &element);
}
- Ref<StyledElement> m_element;
+ StyledElement* m_element { nullptr };
};
StylePropertyMap& StyledElement::ensureAttributeStyleMap()
Modified: trunk/Source/WebCore/platform/graphics/CustomPaintImage.cpp (243238 => 243239)
--- trunk/Source/WebCore/platform/graphics/CustomPaintImage.cpp 2019-03-20 21:22:08 UTC (rev 243238)
+++ trunk/Source/WebCore/platform/graphics/CustomPaintImage.cpp 2019-03-20 21:23:02 UTC (rev 243239)
@@ -105,6 +105,8 @@
{
}
+ void clearElement() override { }
+
RefPtr<TypedOMCSSStyleValue> get(const String& property) const final { return makeRefPtr(m_map.get(property)); }
HashMap<String, RefPtr<TypedOMCSSStyleValue>> m_map;
