Diff
Modified: trunk/LayoutTests/ChangeLog (221291 => 221292)
--- trunk/LayoutTests/ChangeLog 2017-08-29 09:10:52 UTC (rev 221291)
+++ trunk/LayoutTests/ChangeLog 2017-08-29 09:32:34 UTC (rev 221292)
@@ -1,3 +1,23 @@
+2017-08-19 Sergio Villar Senin <[email protected]>
+
+ [SVG] Leak in SVGAnimatedListPropertyTearOff
+ https://bugs.webkit.org/show_bug.cgi?id=172545
+
+ Reviewed by Darin Adler.
+
+ The list of new added tests includes the one for the original bug, a new test for the
+ regression and a couple of tests imported from Blink which verify that
+ SVGAnimatedListPropertyTearOff does not crash after the context element goes out of scope.
+
+ * svg/animations/animation-leak-list-property-instances-expected.txt: Added.
+ * svg/animations/animation-leak-list-property-instances.html: Added.
+ * svg/dom/SVGAnimatedListPropertyTearOff-crash-2-expected.txt: Added. Imported from Blink.
+ * svg/dom/SVGAnimatedListPropertyTearOff-crash-2.html: Added. Imported from Blink.
+ * svg/dom/SVGAnimatedListPropertyTearOff-crash-expected.txt: Added. Imported from Blink.
+ * svg/dom/SVGAnimatedListPropertyTearOff-crash.html: Added. Imported from Blink.
+ * svg/dom/SVGAnimatedListPropertyTearOff-leak-expected.txt: Added.
+ * svg/dom/SVGAnimatedListPropertyTearOff-leak.html: Added.
+
2017-08-28 Per Arne Vollan <[email protected]>
Rebaseline accessibility/menu-list-crash2.html after r220930.
Added: trunk/LayoutTests/svg/animations/animation-leak-list-property-instances-expected.txt (0 => 221292)
--- trunk/LayoutTests/svg/animations/animation-leak-list-property-instances-expected.txt (rev 0)
+++ trunk/LayoutTests/svg/animations/animation-leak-list-property-instances-expected.txt 2017-08-29 09:32:34 UTC (rev 221292)
@@ -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
+
Property changes on: trunk/LayoutTests/svg/animations/animation-leak-list-property-instances-expected.txt
___________________________________________________________________
Added: svn:eol-style
+LF
\ No newline at end of property
Added: trunk/LayoutTests/svg/animations/animation-leak-list-property-instances.html (0 => 221292)
--- trunk/LayoutTests/svg/animations/animation-leak-list-property-instances.html (rev 0)
+++ trunk/LayoutTests/svg/animations/animation-leak-list-property-instances.html 2017-08-29 09:32:34 UTC (rev 221292)
@@ -0,0 +1,61 @@
+<!DOCTYPE html>
+<script src=""
+<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 causes flakiness in some platforms.
+ gc();
+ gc();
+ var originalLiveElements = internals.numberOfLiveNodes();
+
+ addRect();
+ applyTransform();
+ removeRect();
+
+ // One gc() call is not enough and causes flakiness in some platforms.
+ gc();
+ gc();
+ var delta = internals.numberOfLiveNodes() - originalLiveElements;
+ shouldBeZero(delta.toString());
+ var successfullyParsed = true;
+ }
+</script>
+<body _onload_="test()">
+ <svg id="rootSVG" width="300" height="300" xmlns="http://www.w3.org/2000/svg" version="1.1"></svg>
+</body>
Property changes on: trunk/LayoutTests/svg/animations/animation-leak-list-property-instances.html
___________________________________________________________________
Added: svn:eol-style
+LF
\ No newline at end of property
Added: svn:mime-type
+text/html
\ No newline at end of property
Added: trunk/LayoutTests/svg/dom/SVGAnimatedListPropertyTearOff-crash-2-expected.txt (0 => 221292)
--- trunk/LayoutTests/svg/dom/SVGAnimatedListPropertyTearOff-crash-2-expected.txt (rev 0)
+++ trunk/LayoutTests/svg/dom/SVGAnimatedListPropertyTearOff-crash-2-expected.txt 2017-08-29 09:32:34 UTC (rev 221292)
@@ -0,0 +1,10 @@
+This tests SVGAnimatedListPropertyTearOff containing SVGTransform don't crash if modified after contextElement goes out of scope.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+This test passes if we don't crash.
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
Property changes on: trunk/LayoutTests/svg/dom/SVGAnimatedListPropertyTearOff-crash-2-expected.txt
___________________________________________________________________
Added: svn:eol-style
+LF
\ No newline at end of property
Added: trunk/LayoutTests/svg/dom/SVGAnimatedListPropertyTearOff-crash-2.html (0 => 221292)
--- trunk/LayoutTests/svg/dom/SVGAnimatedListPropertyTearOff-crash-2.html (rev 0)
+++ trunk/LayoutTests/svg/dom/SVGAnimatedListPropertyTearOff-crash-2.html 2017-08-29 09:32:34 UTC (rev 221292)
@@ -0,0 +1,17 @@
+<!DOCTYPE html>
+<script src=""
+<script>
+ description("This tests SVGAnimatedListPropertyTearOff containing SVGTransform don't crash if modified after contextElement goes out of scope.");
+
+ var pattern = document.createElementNS("http://www.w3.org/2000/svg","pattern");
+ var baseVal = pattern.patternTransform.baseVal;
+ pattern = null;
+ gc();
+
+ var svgTransform = document.createElementNS("http://www.w3.org/2000/svg","svg").createSVGTransform();
+ baseVal.insertItemBefore(svgTransform, 0);
+ gc();
+
+ debug("This test passes if we don't crash.");
+</script>
+<script src=""
Property changes on: trunk/LayoutTests/svg/dom/SVGAnimatedListPropertyTearOff-crash-2.html
___________________________________________________________________
Added: svn:eol-style
+LF
\ No newline at end of property
Added: svn:mime-type
+text/html
\ No newline at end of property
Added: trunk/LayoutTests/svg/dom/SVGAnimatedListPropertyTearOff-crash-expected.txt (0 => 221292)
--- trunk/LayoutTests/svg/dom/SVGAnimatedListPropertyTearOff-crash-expected.txt (rev 0)
+++ trunk/LayoutTests/svg/dom/SVGAnimatedListPropertyTearOff-crash-expected.txt 2017-08-29 09:32:34 UTC (rev 221292)
@@ -0,0 +1,10 @@
+This tests SVGAnimatedListPropertyTearOff don't crash if modified after contextElement goes out of scope.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+This test passes if we don't crash.
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
Property changes on: trunk/LayoutTests/svg/dom/SVGAnimatedListPropertyTearOff-crash-expected.txt
___________________________________________________________________
Added: svn:eol-style
+LF
\ No newline at end of property
Added: trunk/LayoutTests/svg/dom/SVGAnimatedListPropertyTearOff-crash.html (0 => 221292)
--- trunk/LayoutTests/svg/dom/SVGAnimatedListPropertyTearOff-crash.html (rev 0)
+++ trunk/LayoutTests/svg/dom/SVGAnimatedListPropertyTearOff-crash.html 2017-08-29 09:32:34 UTC (rev 221292)
@@ -0,0 +1,17 @@
+<!DOCTYPE html>
+<script src=""
+<script>
+ description("This tests SVGAnimatedListPropertyTearOff don't crash if modified after contextElement goes out of scope.");
+
+ var text = document.createElementNS("http://www.w3.org/2000/svg","text");
+ var baseVal = text.dx.baseVal;
+ text = null;
+ gc();
+
+ var svgLength = document.createElementNS("http://www.w3.org/2000/svg","svg").createSVGLength();
+ baseVal.appendItem(svgLength);
+ gc();
+
+ debug("This test passes if we don't crash.");
+</script>
+<script src=""
Property changes on: trunk/LayoutTests/svg/dom/SVGAnimatedListPropertyTearOff-crash.html
___________________________________________________________________
Added: svn:eol-style
+LF
\ No newline at end of property
Added: svn:mime-type
+text/html
\ No newline at end of property
Added: trunk/LayoutTests/svg/dom/SVGAnimatedListPropertyTearOff-leak-expected.txt (0 => 221292)
--- trunk/LayoutTests/svg/dom/SVGAnimatedListPropertyTearOff-leak-expected.txt (rev 0)
+++ trunk/LayoutTests/svg/dom/SVGAnimatedListPropertyTearOff-leak-expected.txt 2017-08-29 09:32:34 UTC (rev 221292)
@@ -0,0 +1,16 @@
+This test checks that custom data attributes are not lost due GC while still referenced.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+PASS successfullyParsed is true
+
+TEST COMPLETE
+PASS 0.5000000000000001 is within 1e-9 of 0.5
+PASS 0 is 0
+PASS 0 is 0
+PASS 0.5000000000000001 is within 1e-9 of 0.5
+PASS 0 is 0
+PASS 0 is 0
+0.5000000000000001,0,0,0.5000000000000001,0,0
+
Property changes on: trunk/LayoutTests/svg/dom/SVGAnimatedListPropertyTearOff-leak-expected.txt
___________________________________________________________________
Added: svn:eol-style
+LF
\ No newline at end of property
Added: trunk/LayoutTests/svg/dom/SVGAnimatedListPropertyTearOff-leak.html (0 => 221292)
--- trunk/LayoutTests/svg/dom/SVGAnimatedListPropertyTearOff-leak.html (rev 0)
+++ trunk/LayoutTests/svg/dom/SVGAnimatedListPropertyTearOff-leak.html 2017-08-29 09:32:34 UTC (rev 221292)
@@ -0,0 +1,108 @@
+<!DOCTYPE html>
+<style>
+ body {
+ overflow: hidden;
+ }
+ #svg {
+ width: 100%;
+ height: 100%;
+ }
+ #current-transform {
+ height: 30px;
+ }
+ #is-same-instance {
+ width: 30px;
+ height: 30px;
+ background-color: green;
+ }
+</style>
+
+<p id="description"></p>
+<div id="console"></div>
+
+<body _onload_="runTest()">
+<div id="current-transform">translate(0, 0)</div>
+<div id="is-same-instance"></div>
+<div id="svg-container">
+ <svg id="svg" xmlns="http://www.w3.org/2000/svg" version="1.1">
+ <g transform="translate(0, 0)">
+ <rect width="100" height="100" fill="green"></rect>
+ </g>
+ </svg>
+</div>
+</body>
+
+<script src=""
+
+<script>
+ description("This test checks that custom data attributes are not lost due GC while still referenced.");
+
+ let iterations = 0;
+ let currentScaling = 1
+ const gElement = document.getElementById('svg').firstElementChild
+ const transform = document.getElementById('current-transform')
+
+ if (window.testRunner) {
+ testRunner.dumpAsText();
+ testRunner.waitUntilDone();
+ }
+
+ function checkCache() {
+ let cache = gElement["data-transform-cache"];
+ shouldBeCloseTo(cache.matrix.a.toString(), 0.5, 0.000000001);
+ shouldBe(cache.matrix.b.toString(), "0");
+ shouldBe(cache.matrix.c.toString(), "0");
+ shouldBeCloseTo(cache.matrix.d.toString(), 0.5, 0.000000001);
+ shouldBe(cache.matrix.e.toString(), "0");
+ shouldBe(cache.matrix.f.toString(), "0");
+ }
+
+ function runTest() {
+ currentScaling -= 0.1;
+ const matrix = {
+ elements: [currentScaling, 0, 0, currentScaling, 0, 0]
+ }
+ setMatrix(gElement, matrix);
+ transform.innerHTML = matrix.elements.toString();
+
+ if (++iterations < 5) {
+ gc();
+ gc();
+ runTest();
+ } else {
+ checkCache();
+ if (window.testRunner)
+ testRunner.notifyDone();
+ }
+ }
+
+ function setMatrix(element, matrix) {
+ const elements = matrix.elements;
+ let cache = element["data-transform-cache"];
+ if (!cache) {
+ cache = element.transform.baseVal.getItem(0);
+ const m = element.transform.baseVal.getItem(0).matrix;
+ m.a = elements[0];
+ m.b = elements[1];
+ m.c = elements[2];
+ m.d = elements[3];
+ m.e = elements[4];
+ m.f = elements[5];
+ element["data-transform-cache"] = cache
+ } else {
+ const m = cache.matrix;
+ shouldBe
+ if (cache !== element.transform.baseVal.getItem(0)) {
+ console.log("FAIL: " + cache + " should be " + element.transform.baseVal.getItem(0) + " but is not.");
+ document.getElementById('is-same-instance').style.backgroundColor = 'red';
+ }
+ m.a = elements[0];
+ m.b = elements[1];
+ m.c = elements[2];
+ m.d = elements[3];
+ m.e = elements[4];
+ m.f = elements[5];
+ }
+ }
+</script>
+<script src=""
Property changes on: trunk/LayoutTests/svg/dom/SVGAnimatedListPropertyTearOff-leak.html
___________________________________________________________________
Added: svn:eol-style
+LF
\ No newline at end of property
Added: svn:mime-type
+text/html
\ No newline at end of property
Modified: trunk/Source/WebCore/ChangeLog (221291 => 221292)
--- trunk/Source/WebCore/ChangeLog 2017-08-29 09:10:52 UTC (rev 221291)
+++ trunk/Source/WebCore/ChangeLog 2017-08-29 09:32:34 UTC (rev 221292)
@@ -1,3 +1,40 @@
+2017-08-19 Sergio Villar Senin <[email protected]>
+
+ [SVG] Leak in SVGAnimatedListPropertyTearOff
+ https://bugs.webkit.org/show_bug.cgi?id=172545
+
+ Reviewed by Darin Adler.
+
+ 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.
+
+ In order to effectively break the cycle without freeing too many wrappers we should take two
+ measures:
+ 1) Break the reference cycle by storing raw pointers in the m_wrappers Vector
+ 2) Remove the ListItemTearOff which is being deleted (it notifies the animated property by
+ calling propertyWillBeDeleted) from the m_wrappers Vector.
+
+ This is a re-land of r219334 which caused early releases of custom data attribute objects
+ added to SVG elements (wkb.ug/175023).
+
+ Tests: svg/animations/animation-leak-list-property-instances.html
+ svg/dom/SVGAnimatedListPropertyTearOff-crash-2.html
+ svg/dom/SVGAnimatedListPropertyTearOff-crash.html
+ svg/dom/SVGAnimatedListPropertyTearOff-leak.html
+
+ * svg/properties/SVGAnimatedListPropertyTearOff.h:
+ * svg/properties/SVGListProperty.h:
+ (WebCore::SVGListProperty::getItemValuesAndWrappers):
+ * svg/properties/SVGListPropertyTearOff.h:
+ (WebCore::SVGListPropertyTearOff::removeItemFromList):
+
2017-08-29 Andy Estes <[email protected]>
[Mac] Upstream WKSetMetadataURL() from WebKitSystemInterface
Modified: trunk/Source/WebCore/svg/properties/SVGAnimatedListPropertyTearOff.h (221291 => 221292)
--- trunk/Source/WebCore/svg/properties/SVGAnimatedListPropertyTearOff.h 2017-08-29 09:10:52 UTC (rev 221291)
+++ trunk/Source/WebCore/svg/properties/SVGAnimatedListPropertyTearOff.h 2017-08-29 09:32:34 UTC (rev 221292)
@@ -34,7 +34,7 @@
public:
using ListItemType = typename SVGPropertyTraits<PropertyType>::ListItemType;
using ListItemTearOff = typename SVGPropertyTraits<PropertyType>::ListItemTearOff;
- using ListWrapperCache = Vector<RefPtr<ListItemTearOff>>;
+ using ListWrapperCache = Vector<ListItemTearOff*>;
using ListProperty = SVGListProperty<PropertyType>;
using ListPropertyTearOff = typename SVGPropertyTraits<PropertyType>::ListPropertyTearOff;
using ContentType = PropertyType;
@@ -73,6 +73,11 @@
m_baseVal = nullptr;
else if (&property == m_animVal)
m_animVal = nullptr;
+ else {
+ size_t i = m_wrappers.find(&property);
+ if (i != notFound)
+ m_wrappers[i] = nullptr;
+ }
}
int findItem(SVGProperty* property)
Modified: trunk/Source/WebCore/svg/properties/SVGListProperty.h (221291 => 221292)
--- trunk/Source/WebCore/svg/properties/SVGListProperty.h 2017-08-29 09:10:52 UTC (rev 221291)
+++ trunk/Source/WebCore/svg/properties/SVGListProperty.h 2017-08-29 09:32:34 UTC (rev 221292)
@@ -206,7 +206,7 @@
// It is also associated with our animated property, so it can notify the SVG Element which holds the SVGAnimated*List
// that it has been modified (and thus can call svgAttributeChanged(associatedAttributeName)).
wrapper = ListItemTearOff::create(animatedList, UndefinedRole, m_values->at(index));
- m_wrappers->at(index) = wrapper;
+ m_wrappers->at(index) = wrapper.get();
}
return wrapper.releaseNonNull();
Modified: trunk/Source/WebCore/svg/properties/SVGListPropertyTearOff.h (221291 => 221292)
--- trunk/Source/WebCore/svg/properties/SVGListPropertyTearOff.h 2017-08-29 09:10:52 UTC (rev 221291)
+++ trunk/Source/WebCore/svg/properties/SVGListPropertyTearOff.h 2017-08-29 09:32:34 UTC (rev 221292)
@@ -66,7 +66,7 @@
ASSERT(m_values->size() == m_wrappers->size());
ASSERT_WITH_SECURITY_IMPLICATION(itemIndex < m_wrappers->size());
- RefPtr<ListItemTearOff>& item = m_wrappers->at(itemIndex);
+ RefPtr<ListItemTearOff> item = m_wrappers->at(itemIndex);
item->detachWrapper();
m_wrappers->remove(itemIndex);
m_values->remove(itemIndex);
@@ -141,7 +141,7 @@
unsigned size = m_wrappers->size();
ASSERT(size == m_values->size());
for (unsigned i = 0; i < size; ++i) {
- ListItemTearOff* item = m_wrappers->at(i).get();
+ ListItemTearOff* item = m_wrappers->at(i);
if (!item)
continue;
item->setAnimatedProperty(m_animatedProperty.ptr());
