[SVG] Leak in SVGAnimatedListPropertyTearOff
authorsvillar@igalia.com <svillar@igalia.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 11 Jul 2017 16:30:43 +0000 (16:30 +0000)
committersvillar@igalia.com <svillar@igalia.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 11 Jul 2017 16:30:43 +0000 (16:30 +0000)
https://bugs.webkit.org/show_bug.cgi?id=172545

Source/WebCore:

Reviewed by Said Abou-Hallawa.

SVGAnimatedListPropertyTearOff maintains a vector m_wrappers with references to
SVGPropertyTraits<PropertyType>::ListItemTearOff. Apart from that SVGPropertyTearOff has a
reference to SVGAnimatedProperty.

When SVGListProperty::getItemValuesAndWrappers() is called, it creates a
SVGPropertyTraits<PropertyType>::ListItemTearOff pointing to the same SVGAnimatedProperty (a
SVGAnimatedListPropertyTearOff) which stores the m_wrappers vector where the ListItemTearOff
is going to be added to. This effectively creates a reference cycle between the
SVGAnimatedListPropertyTearOff and all the ListItemTearOff it stores in m_wrappers.

We should detach those wrappers in propertyWillBeDeleted() in order to break the cycle.

* svg/properties/SVGAnimatedListPropertyTearOff.h:

LayoutTests:

Reviewed by Darin Adler.

* svg/animations/animation-leak-list-property-instances-expected.txt: Added.
* svg/animations/animation-leak-list-property-instances.html: Added.

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

LayoutTests/ChangeLog
LayoutTests/svg/animations/animation-leak-list-property-instances-expected.txt [new file with mode: 0644]
LayoutTests/svg/animations/animation-leak-list-property-instances.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/svg/properties/SVGAnimatedListPropertyTearOff.h

index 6a89d67..080431d 100644 (file)
@@ -1,3 +1,13 @@
+2017-07-11  Sergio Villar Senin  <svillar@igalia.com>
+
+        [SVG] Leak in SVGAnimatedListPropertyTearOff
+        https://bugs.webkit.org/show_bug.cgi?id=172545
+
+        Reviewed by Darin Adler.
+
+        * svg/animations/animation-leak-list-property-instances-expected.txt: Added.
+        * svg/animations/animation-leak-list-property-instances.html: Added.
+
 2017-07-11  Carlos Alberto Lopez Perez  <clopez@igalia.com>
 
         [GTK] Spin buttons on input type number appear over the value itself for small widths
diff --git a/LayoutTests/svg/animations/animation-leak-list-property-instances-expected.txt b/LayoutTests/svg/animations/animation-leak-list-property-instances-expected.txt
new file mode 100644 (file)
index 0000000..cfca71e
--- /dev/null
@@ -0,0 +1,7 @@
+This test checks that adding an animation to a SVG element does not leak the whole SVGDocument.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+PASS 0 is 0
+
diff --git a/LayoutTests/svg/animations/animation-leak-list-property-instances.html b/LayoutTests/svg/animations/animation-leak-list-property-instances.html
new file mode 100644 (file)
index 0000000..d55e6fd
--- /dev/null
@@ -0,0 +1,63 @@
+<!DOCTYPE html>
+<script src="../../resources/js-test-pre.js"></script>
+
+<body onload="test()">
+    <svg id="rootSVG" width="300" height="300" xmlns="http://www.w3.org/2000/svg" version="1.1"></svg>
+</body>
+
+<script>
+ description("This test checks that adding an animation to a SVG element does not leak the whole SVGDocument.")
+
+ function addRect()
+ {
+     var elem = document.createElementNS("http://www.w3.org/2000/svg", "rect");
+     elem.setAttribute("id", "rect");
+     elem.setAttribute("x", 50);
+     elem.setAttribute("y", 50);
+     elem.setAttribute("width", 50);
+     elem.setAttribute("height", 50);
+     elem.setAttribute("fill", "blue");
+
+     document.getElementById("rootSVG").appendChild(elem);
+ }
+
+ function applyTransform()
+ {
+     var svgroot = document.getElementById("rootSVG");
+     var transformList = document.getElementById("rect").transform.baseVal;
+     var rotate = svgroot.createSVGTransform();
+     rotate.setRotate(15,0,0);
+     transformList.appendItem(rotate);
+ }
+
+ function removeRect()
+ {
+     document.getElementById("rootSVG").removeChild(document.getElementById("rect"));
+ }
+
+ function test()
+ {
+     if (!window.internals || !window.GCController) {
+        testFailed("This test requires internals and GCController");
+        return;
+     }
+
+     testRunner.dumpAsText();
+
+     // One gc() call is not enough and cause flakiness in some platforms.
+     gc();
+     gc();
+     var originalLiveElements = internals.numberOfLiveNodes();
+
+     addRect();
+     applyTransform();
+     removeRect();
+
+     // One gc() call is not enough and cause flakiness in some platforms.
+     gc();
+     gc();
+     var delta = internals.numberOfLiveNodes() - originalLiveElements;
+     shouldBeZero(delta.toString());
+     var successfullyParsed = true;
+ }
+</script>
index f06165c..9fa28cd 100644 (file)
@@ -1,3 +1,24 @@
+2017-05-24  Sergio Villar Senin  <svillar@igalia.com>
+
+        [SVG] Leak in SVGAnimatedListPropertyTearOff
+        https://bugs.webkit.org/show_bug.cgi?id=172545
+
+        Reviewed by Said Abou-Hallawa.
+
+        SVGAnimatedListPropertyTearOff maintains a vector m_wrappers with references to
+        SVGPropertyTraits<PropertyType>::ListItemTearOff. Apart from that SVGPropertyTearOff has a
+        reference to SVGAnimatedProperty.
+
+        When SVGListProperty::getItemValuesAndWrappers() is called, it creates a
+        SVGPropertyTraits<PropertyType>::ListItemTearOff pointing to the same SVGAnimatedProperty (a
+        SVGAnimatedListPropertyTearOff) which stores the m_wrappers vector where the ListItemTearOff
+        is going to be added to. This effectively creates a reference cycle between the
+        SVGAnimatedListPropertyTearOff and all the ListItemTearOff it stores in m_wrappers.
+
+        We should detach those wrappers in propertyWillBeDeleted() in order to break the cycle.
+
+        * svg/properties/SVGAnimatedListPropertyTearOff.h:
+
 2017-07-11  Carlos Alberto Lopez Perez  <clopez@igalia.com>
 
         [GTK] Spin buttons on input type number appear over the value itself for small widths
index 0ce9937..bfa8e13 100644 (file)
@@ -73,6 +73,8 @@ public:
             m_baseVal = nullptr;
         else if (&property == m_animVal)
             m_animVal = nullptr;
+        if (!m_baseVal && !m_animVal)
+            detachListWrappers(m_values.size());
     }
 
     int findItem(SVGProperty* property)