Bug 172545 – [SVG] Leak in SVGAnimatedListPropertyTearOff

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Sergio Villar Senin]

[SVG] Leak in SVGAnimatedListPropertyTearOff

[Sergio Villar Senin]

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.

[Sergio Villar Senin]

Created attachment 311131 [details]
Patch

[Sergio Villar Senin]

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).

[Build Bot]

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

[Build Bot]

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

[Darin Adler]

(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?

[Sergio Villar Senin]

(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.

[Darin Adler]

(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.

[Darin Adler]

I would really like to see a test for this.

[Sergio Villar Senin]

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

Working on it

[Sergio Villar Senin]

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?

[Said Abou-Hallawa]

(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();

[Sergio Villar Senin]

Created attachment 314480 [details]
Patch

[Said Abou-Hallawa]

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();
}

[Sergio Villar Senin]

(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.

[Sergio Villar Senin]

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

[Matt Lewis]

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?

[Matt Lewis]

Reverted r219193 for reason:

The tests added with this revision were extreamly flaky on all platforms.

Committed r219217: <http://trac.webkit.org/changeset/219217>

[Michael Catanzaro]

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

[Sergio Villar Senin]

(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.

[Sergio Villar Senin]

Created attachment 314852 [details]
Patch

Making the test less flaky

[Sergio Villar Senin]

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

[Matt Lewis]

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 Lewis]

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>

[Sergio Villar Senin]

(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?

[Michael Catanzaro]

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

[Sergio Villar Senin]

(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.

[Darin Adler]

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.

[Sergio Villar Senin]

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

[Michael Catanzaro]

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

[Sergio Villar Senin]

(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.

[Sergio Villar Senin]

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

[Sergio Villar Senin]

(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 :)

[Alexey Proskuryakov]

This caused bug 175023.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 175398

[Sergio Villar Senin]

Created attachment 318591 [details]
Patch

[Michael Catanzaro]

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.

[Darin Adler]

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".

[Sergio Villar Senin]

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

[Radar WebKit Bug Importer]

<rdar://problem/34129206>

[Matt Lewis]

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

[Sergio Villar Senin]

OK I'll try to take a look

[Said Abou-Hallawa]

(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?

[Sergio Villar Senin]

(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.