Bug 172545

Created attachment 311128 [details] Example Example demonstrating the leak. Navigate to this SVG from any HTML file and then go back to the original HTML file. The SVGDocument will be leaked.

Created attachment 311131 [details] Patch

I was totally unable to create a test case even using the example I attached above. It looks like window.internals.numberOfLiveNodes() does not catch the leak (pretty easy to see adding some instrumentation to SVGDocument or SVGAnimatedListPropertyTearOff).

Comment on attachment 311131 [details] Patch Attachment 311131 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/3808107 New failing tests: webrtc/peer-connection-audio-mute.html compositing/masks/compositing-clip-path-change-no-repaint.html

Created attachment 311143 [details] Archive of layout-test-results from ews126 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews126 Port: ios-simulator-wk2 Platform: Mac OS X 10.12.5

(In reply to Sergio Villar Senin from comment #3) > I was totally unable to create a test case even using the example I attached > above. It looks like window.internals.numberOfLiveNodes() does not catch the > leak (pretty easy to see adding some instrumentation to SVGDocument or > SVGAnimatedListPropertyTearOff). I think the issue is that we count DOM nodes, but the leak here is of other kinds of objects. Can we add another counter to make a regression test possible?

(In reply to Darin Adler from comment #6) > (In reply to Sergio Villar Senin from comment #3) > > I was totally unable to create a test case even using the example I attached > > above. It looks like window.internals.numberOfLiveNodes() does not catch the > > leak (pretty easy to see adding some instrumentation to SVGDocument or > > SVGAnimatedListPropertyTearOff). > > I think the issue is that we count DOM nodes, but the leak here is of other > kinds of objects. Can we add another counter to make a regression test > possible? Well, we could add a counter for SVGDocuments for example.

(In reply to Sergio Villar Senin from comment #7) > Well, we could add a counter for SVGDocuments for example. The node counter should count SVG documents, because an SVGDocument is a DOM node.

I would really like to see a test for this.

(In reply to Darin Adler from comment #9) > I would really like to see a test for this. Working on it

Said, you have touched this code a lot before. Could you please confirm that the patch is OK for you before I upload a test case?

(In reply to Sergio Villar Senin from comment #11) > Said, you have touched this code a lot before. Could you please confirm that > the patch is OK for you before I upload a test case? I think the approach is correct. SVGAnimatedListPropertyTearOff has the vector m_wrappers which holds Ref pointers to SVGPropertyTearOff. SVGPropertyTearOff holds a Ref pointer to SVGAnimatedProperty (or SVGAnimatedListPropertyTearOff in this case). To break this cycle you have to do it when SVGListPropertyTearOff is deleted. In the destructor of SVGListPropertyTearOff, we call SVGAnimatedListPropertyTearOff::propertyWillBeDeleted() which will detach the wrappers and hence break the cycle. The only thing that worries me is SVGAnimatedListPropertyTearOff::propertyWillBeDeleted() is called when either baseVal or animVal is deleted. So calling detachListWrappers() will detach all the wrappers of both the baseVal and the animVal regardless whether one of them is still alive or not. Maybe it is a better to check m_animVal and m_baseVal are both null before calling detachListWrappers if (!m_animVal && !m_baseVal) detachListWrappers();

Created attachment 314480 [details] Patch

Comment on attachment 314480 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=314480&action=review > LayoutTests/svg/animations/animation-leak-list-property-instances.html:6 > + function log(message) { > + var logDiv = document.getElementById('log'); > + logDiv.appendChild(document.createTextNode(message)); > + } Can't we use <script src="../resources/js-test-pre.js"> and <script src="../resources/js-test-post.js"> to avoid writing this function? > LayoutTests/svg/animations/animation-leak-list-property-instances.html:42 > + applyTransform("rootSVG", "rect"); Is there a difference between this chained calling sequence and a sequential calling sequence? function load() { applyTransform(); continueTest(); finishTest(); }

(In reply to Said Abou-Hallawa from comment #14) > Comment on attachment 314480 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=314480&action=review > > > LayoutTests/svg/animations/animation-leak-list-property-instances.html:6 > > + function log(message) { > > + var logDiv = document.getElementById('log'); > > + logDiv.appendChild(document.createTextNode(message)); > > + } > > Can't we use <script src="../resources/js-test-pre.js"> and <script > src="../resources/js-test-post.js"> to avoid writing this function? Not sure how I could avoid defining that function. There is nothing like that in those helpers. > > LayoutTests/svg/animations/animation-leak-list-property-instances.html:42 > > + applyTransform("rootSVG", "rect"); > > Is there a difference between this chained calling sequence and a sequential > calling sequence? > > function load() { > applyTransform(); > continueTest(); > finishTest(); > } Yeah, I'll definitely change that. The chain of calls was there because I was using longer timeouts for manual testing.

Committed r219193: <http://trac.webkit.org/changeset/219193>

The test svg/animations/animation-leak-list-property-instances.html added in this: https://trac.webkit.org/changeset/219193/webkit Is extremely flaky. It's starting to effect the testers. History: https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=svg%2Fanimations%2Fanimation-leak-list-property-instances.html https://build.webkit.org/results/Apple%20Sierra%20Release%20WK2%20(Tests)/r219211%20(2775)/results.html https://build.webkit.org/builders/Apple%20Sierra%20Release%20WK2%20(Tests)/builds/2775 Diff: --- /Volumes/Data/slave/sierra-release-tests-wk2/build/layout-test-results/svg/animations/animation-leak-list-property-instances-expected.txt +++ /Volumes/Data/slave/sierra-release-tests-wk2/build/layout-test-results/svg/animations/animation-leak-list-property-instances-actual.txt @@ -1,3 +1,3 @@ This test checks that transform animations in SVG elements do not cause the whole SVGDocument to leak. -PASS +FAIL: not enough live nodes freed: 13 vs 4 expected Is this something that is easily remedied?

Reverted r219193 for reason: The tests added with this revision were extreamly flaky on all platforms. Committed r219217: <http://trac.webkit.org/changeset/219217>

Well, looks like it was a good test. :)

(In reply to Matt Lewis from comment #18) > Reverted r219193 for reason: > > The tests added with this revision were extreamly flaky on all platforms. > > Committed r219217: <http://trac.webkit.org/changeset/219217> OK I'll take a look.

Created attachment 314852 [details] Patch Making the test less flaky

Committed r219257: <http://trac.webkit.org/changeset/219257>

The new patch unfortunately did not help the test to become less flaky. It is still failing on the same intermittently on the same testers. Debug seems to flake more, but it is still flaking on Release builds as well. Build: https://build.webkit.org/builders/Apple%20El%20Capitan%20Debug%20WK1%20(Tests)/builds/2334 https://build.webkit.org/results/Apple%20El%20Capitan%20Debug%20WK1%20(Tests)/r219259%20(2334)/results.html Diff: --- /Volumes/Data/slave/elcapitan-debug-tests-wk1/build/layout-test-results/svg/animations/animation-leak-list-property-instances-expected.txt +++ /Volumes/Data/slave/elcapitan-debug-tests-wk1/build/layout-test-results/svg/animations/animation-leak-list-property-instances-actual.txt @@ -1,2 +1,2 @@ -PASS: all nodes were properly removed. +FAIL: -9 nodes leaked.

Reverted r219257 for reason: The test added in the revision was still extreamly flaky on all testers. Committed r219264: <http://trac.webkit.org/changeset/219264>

(In reply to Matt Lewis from comment #23) > The new patch unfortunately did not help the test to become less flaky. It > is still failing on the same intermittently on the same testers. > Debug seems to flake more, but it is still flaking on Release builds as well. > > Build: > https://build.webkit.org/builders/ > Apple%20El%20Capitan%20Debug%20WK1%20(Tests)/builds/2334 > https://build.webkit.org/results/ > Apple%20El%20Capitan%20Debug%20WK1%20(Tests)/r219259%20(2334)/results.html > > > Diff: > --- > /Volumes/Data/slave/elcapitan-debug-tests-wk1/build/layout-test-results/svg/ > animations/animation-leak-list-property-instances-expected.txt > +++ > /Volumes/Data/slave/elcapitan-debug-tests-wk1/build/layout-test-results/svg/ > animations/animation-leak-list-property-instances-actual.txt > @@ -1,2 +1,2 @@ > -PASS: all nodes were properly removed. > +FAIL: -9 nodes leaked. Matt any chance you could test in the builders before landing if I send you a modified version of the test?

Do you know why the previous versions of your patch were flaky?

(In reply to Michael Catanzaro from comment #26) > Do you know why the previous versions of your patch were flaky? For the very same reason a lot of SVG tests are, they rely on the garbage collector sweeping objects and that's very difficult to predict, and what's more important to reliably reproduce mainly because the tests are run in a sequence and the results of the previous tests affect the following ones. There is a switch in run-webkit-tests to execute the GC before each test but I guess that would make the testing phase so long that is not a possibility in practice.

Comment on attachment 314852 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=314852&action=review Thanks so much for adding a test. Makes a big difference! I hope that adding the explicit calls to collect are sufficient. Maybe we should come back and change numberOfLiveNodes so that it automatically causes garbage collection. There is no use in counting nodes if you don’t force collection first. And then we can simplify this test and others. Also seems like numberOfLiveNodes should be an attribute, not an operation. > LayoutTests/svg/animations/animation-leak-list-property-instances.html:9 > + function log(message) { > + var logDiv = document.getElementById('log'); > + logDiv.appendChild(document.createTextNode(message)); > + } This function won’t work very well if you call it more than once. All the messages will be glued together into a single long string with no spaces or line breaks in between them. I agree with an earlier commenter’s suggestion that we make this a more normal test using the script-tests script and macros like shouldBe. Not critical, but less strange. > LayoutTests/svg/animations/animation-leak-list-property-instances.html:19 > + elem.setAttributeNS(null,"id", "rect"); > + elem.setAttributeNS(null,"x",50); > + elem.setAttributeNS(null,"y",50); > + elem.setAttributeNS(null,"width",50); > + elem.setAttributeNS(null,"height",50); > + elem.setAttributeNS(null,"fill", "blue"); These could use normal setAttribute; no need to use setAttributeNS. > LayoutTests/svg/animations/animation-leak-list-property-instances.html:38 > + var originalLiveElements = 0; No need for this global variable. > LayoutTests/svg/animations/animation-leak-list-property-instances.html:45 > + if (!window.testRunner) { > + log("This test requires testRunner"); > + return; > + } What this test actually requires is window.internals and window.GCController, not window.testRunner; but I suppose that’s a pedantic distinction. The use of testRunner.dumpAsText() is completely optional, but the use of numberOfLiveNodes and GCController is not. > LayoutTests/svg/animations/animation-leak-list-property-instances.html:48 > + testRunner.waitUntilDone(); This is not needed for a test that runs synchronously like this one does. > LayoutTests/svg/animations/animation-leak-list-property-instances.html:51 > + originalLiveElements = window.internals.numberOfLiveNodes(); Can just use internals, no need for window.internals. This can just be a "var", no need to use a global variable, since this is a synchronous test. > LayoutTests/svg/animations/animation-leak-list-property-instances.html:58 > + var delta = window.internals.numberOfLiveNodes() - originalLiveElements; Ditto. > LayoutTests/svg/animations/animation-leak-list-property-instances.html:64 > + testRunner.notifyDone(); This is not needed for a test that runs synchronously like this one does.

Committed r219325: <http://trac.webkit.org/changeset/219325>

You forgot to add a ChangeLog entry in the LayoutTests directory!

(In reply to Darin Adler from comment #28) > Comment on attachment 314852 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=314852&action=review > > Thanks so much for adding a test. Makes a big difference! I hope that adding > the explicit calls to collect are sufficient. Thanks for the suggestions the test looks much better after applying the suggested changes. I have just tried to call gc() twice instead of just one and the result is that the flakiness is gone. After trying with --iterations=100 on a mac machine I got no error at all (while it was easily failing at least once with --iterations=5 without the double call). I'm landing this modified version of the patch, fingers crossed.

Committed r219334: <http://trac.webkit.org/changeset/219334>

(In reply to Michael Catanzaro from comment #30) > You forgot to add a ChangeLog entry in the LayoutTests directory! Thanks for the reminder. Somehow it got deleted in the middle of this land/rollout fury :)

This caused bug 175023.

Re-opened since this is blocked by bug 175398

Created attachment 318591 [details] Patch

Comment on attachment 318591 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=318591&action=review > LayoutTests/svg/animations/animation-leak-list-property-instances.html:38 > + if (!window.internals || !window.GCController) { > + testFailed("This test requires internals and GCController"); > + return; > + } This is not indented correctly.

Comment on attachment 318591 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=318591&action=review > Source/WebCore/svg/properties/SVGAnimatedListPropertyTearOff.h:83 > + unsigned size = m_wrappers.size(); > + for (size_t i = 0; i < size; ++i) { > + if (&property == m_wrappers.at(i)) { > + m_wrappers.at(i) = nullptr; > + break; > + } > + } Mix of size_t and unsigned here is unusual. I would avoid that by writing this as either a modern for loop or using Vector::find. for (auto& wrapper : m_wrappers) { if (&property == wrapper) { wrapper = nullptr; break; } } Or: size_t i = m_wrappers.find(&property); if (i != notFound) m_wrappers[i] = nullptr; > LayoutTests/svg/animations/animation-leak-list-property-instances.html:42 > + // One gc() call is not enough and cause flakiness in some platforms. Confusing and annoying! Also a grammatical error: should be "causes".

Committed r221292: <http://trac.webkit.org/changeset/221292>

<rdar://problem/34129206>

This is causing a crash with the test svg/animations/svgtransform-animation-1.html. I'm seeing a regular crash on macOS as well as an assertion failure on Debug: https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=svg%2Fanimations%2Fsvgtransform-animation-1.html The regular crash: https://build.webkit.org/results/Apple%20El%20Capitan%20Release%20WK1%20(Tests)/r221384%20(4317)/svg/animations/svgtransform-animation-1-crash-log.txt The Assertion: https://build.webkit.org/builders/Apple%20El%20Capitan%20Debug%20WK2%20(Tests)/builds/2999 https://build.webkit.org/results/Apple%20El%20Capitan%20Debug%20WK2%20(Tests)/r221844%20(2999)/results.html https://build.webkit.org/results/Apple%20El%20Capitan%20Debug%20WK2%20(Tests)/r221844%20(2999)/svg/animations/svgtransform-animation-1-crash-log.txt stderr: ASSERTION FAILED: !m_deletionHasBegun /Volumes/Data/slave/elcapitan-debug/build/WebKitBuild/Debug/usr/local/include/wtf/RefCounted.h(43) : void WTF::RefCountedBase::ref() const 1 0x112f9c080 WTFCrash 2 0x112f9c0a9 WTFCrashWithSecurityImplication 3 0x104a503c2 WTF::RefCountedBase::ref() const 4 0x10646d82a void WTF::refIfNotNull<WebCore::SVGTransform>(WebCore::SVGTransform*) 5 0x10646d7e4 WTF::RefPtr<WebCore::SVGTransform>::RefPtr(WebCore::SVGTransform*) 6 0x10646d5cd WTF::RefPtr<WebCore::SVGTransform>::RefPtr(WebCore::SVGTransform*) 7 0x10660f98f WebCore::SVGListProperty<WebCore::SVGTransformListValues>::getItemValuesAndWrappers(WebCore::SVGAnimatedListPropertyTearOff<WebCore::SVGTransformListValues>&, unsigned int) 8 0x10660f807 WebCore::SVGListPropertyTearOff<WebCore::SVGTransformListValues>::getItem(unsigned int) 9 0x10660b703 WebCore::jsSVGTransformListPrototypeFunctionGetItemBody(JSC::ExecState*, WebCore::JSSVGTransformList*, JSC::ThrowScope&) 10 0x10660a007 long long WebCore::IDLOperation<WebCore::JSSVGTransformList>::call<&(WebCore::jsSVGTransformListPrototypeFunctionGetItemBody(JSC::ExecState*, WebCore::JSSVGTransformList*, JSC::ThrowScope&)), (WebCore::CastedThisErrorBehavior)0>(JSC::ExecState&, char const*) 11 0x1066093ac WebCore::jsSVGTransformListPrototypeFunctionGetItem(JSC::ExecState*) 12 0x271379601028 13 0x112b01016 llint_entry 14 0x112af93e7 vmEntryToJavaScript 15 0x1128c3e51 JSC::JITCode::execute(JSC::VM*, JSC::ProtoCallFrame*) 16 0x11286dc50 JSC::Interpreter::execute(JSC::EvalExecutable*, JSC::ExecState*, JSC::JSValue, JSC::JSScope*) 17 0x11286c914 JSC::eval(JSC::ExecState*) 18 0x112af2323 llint_slow_path_call_eval 19 0x112b018c3 llint_entry 20 0x112b01016 llint_entry 21 0x112b01016 llint_entry 22 0x112b01016 llint_entry 23 0x112af93e7 vmEntryToJavaScript 24 0x1128c3e51 JSC::JITCode::execute(JSC::VM*, JSC::ProtoCallFrame*) 25 0x1128715e5 JSC::Interpreter::executeCall(JSC::ExecState*, JSC::JSObject*, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&) 26 0x111fec9e0 JSC::call(JSC::ExecState*, JSC::JSValue, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&) 27 0x111fecab9 JSC::call(JSC::ExecState*, JSC::JSValue, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&, WTF::NakedPtr<JSC::Exception>&) 28 0x111feccbd JSC::profiledCall(JSC::ExecState*, JSC::ProfilingReason, JSC::JSValue, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&, WTF::NakedPtr<JSC::Exception>&) 29 0x105c739ab WebCore::JSMainThreadExecState::profiledCall(JSC::ExecState*, JSC::ProfilingReason, JSC::JSValue, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&, WTF::NakedPtr<JSC::Exception>&) 30 0x107166684 WebCore::ScheduledAction::executeFunctionInContext(JSC::JSGlobalObject*, JSC::JSValue, WebCore::ScriptExecutionContext&) 31 0x107166268 WebCore::ScheduledAction::execute(WebCore::Document&) LEAK: 27 WebProcessPool LEAK: 27 WebPageProxy

OK I'll try to take a look

(In reply to Sergio Villar Senin from comment #42) > OK I'll try to take a look What worries me more is it seems no one knows for sure the state of the SVG property tear off objects code. This code has all kinds of pointers: raw, WeakPtr and RefPtr. We use raw pointers to avoid leaks due to double referencing if we had used RefPtr. And use RefPtr to avoid use-after-free if we had used raw pointers. But we still see both the memory leaks and use-after-free in this code. So I am wondering it is still worthy keep investing in this code?

(In reply to Said Abou-Hallawa from comment #43) > (In reply to Sergio Villar Senin from comment #42) > > OK I'll try to take a look > > What worries me more is it seems no one knows for sure the state of the SVG > property tear off objects code. This code has all kinds of pointers: raw, > WeakPtr and RefPtr. We use raw pointers to avoid leaks due to double > referencing if we had used RefPtr. And use RefPtr to avoid use-after-free if > we had used raw pointers. But we still see both the memory leaks and > use-after-free in this code. So I am wondering it is still worthy keep > investing in this code? That's my concern as well. While working on it I had the impression that nobody actually knows exactly what's going on in the SVG tear off and properties code. It's quite complicated to know all the implications of changing that code, fortunatelly we have many tests although not enough as proven. I do definitely think that this code requires a plan, as you mention, to clean up all the RefPtr, raw pointer and WeakPtr chaotic soup. Unfortunatelly I don't currently have time for that, and I guess it is not a priority for Apple ATM either. I'll spend some time on it, the minimum required to discard that a trivial change could fix this new regression.