Bug 83856 – SMIL animation causes leak of the related Document (and many elements)

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[Tim Horton]

Created attachment 137031 [details]
repro

Steps to Reproduce:

1. Open the attached test case.
2. "heap WebProcess | grep HTMLDocument"
3. Refresh the attached test case a few times.
4. "heap WebProcess | grep HTMLDocument"

Notice that there are a number of still-allocated HTMLDocuments equalling the number of times the page loaded.

It seems like the cache in SVGAnimatedProperty.h is the primary problem. SVGAnimatedProperties are added into it on creation, and take a RefPtr to the element they're animating. The cache takes a RefPtr to the SVGAnimatedProperty, However, this cache is only pruned when an SVGAnimatedProperty is destroyed -- which will never happen, because the cache still has a reference to it. Since the SVGAnimatedProperty still has a reference to the element, the element doesn't get destroyed when the page is torn down, preventing the document and animated element (at least) from being destroyed.

<rdar://problem/11216047>

[Tim Horton]

Technically this is probably a regression from http://trac.webkit.org/changeset/70223, but I haven't checked since it's so old.

[Tim Horton]

I have a patch that switches the cache around to store raw pointers, but there's another issue:

After doing that, the SVGAnimatedProperties are created and immediately destroyed underneath findAnimatedPropertiesForAttributeName, which creates them, sticking them into a Vector<RefPtr<SVGAnimatedProperty> >, then stuffs them into a vector of raw pointers, then destroys the vector full of references, leaving *nothing* referencing the SVGAnimatedProperties. So then it goes to use the vector of raw pointers, and they've all already been destroyed.

I'm looking into SVGAnimateElement keeping ahold of its properties to solve this - I'm working on that right now, seems to be going OK.

I have a different patch that just nulls out m_contextElement on each of the SVGAnimatedProperty objects from SVGAnimateElement::targetElementWillChange (and also checks for null m_contextElement and creates a new wrapper if necessary in lookupOrCreateWrapper). This immediately fixes the document/elements leak, but isn't a root-cause fix, just a bandaid for the large leak.

[Tim Horton]

I'm looking into SVGAnimateElement keeping ahold of its properties to solve this - I'm working on that right now, seems to be going OK.

This is *almost* working, but the tearoff is not being destroyed and is keeping *the last* refptr to the SVGAnimatedProperty.

[Tim Horton]

(In reply to comment #3)
> I'm looking into SVGAnimateElement keeping ahold of its properties to solve this - I'm working on that right now, seems to be going OK.
> 
> This is *almost* working, but the tearoff is not being destroyed and is keeping *the last* refptr to the SVGAnimatedProperty.

Ok, there's another cycle, I think: SVGAnimatedPropertyTearOff (a.k.a. our SVGAnimatedProperty that we're now taking a reference to on the AnimateElement) has two refptrs to "PropertyTearOffs", which is a template parameter. "PropertyTearOff"s are a subclass of SVGPropertyTearOff, which hold a refptr to the SVGAnimatedProperty(TearOff). Making that a raw pointer seems reasonable and cuts the cycle.

Then, after running the test and either forcing a GC or waiting long enough for one to happen (the GC delay seems to have been turned up significantly at some point? maybe 109956):

> heap WebProcess | grep HTMLDocument
        1      4608    4608.0   WebCore::HTMLDocument

Time to run tests and read the patch carefully and clean things up.

[Tim Horton]

(In reply to comment #4)
> Making that a raw pointer seems reasonable and cuts the cycle.

I'm not so sure anymore. It makes svg/animations/force-use-shadow-tree-recreation-while-animating crash; it looks like maybe the SVGAnimatedProperty is getting destroyed while we recreate the shadow tree (because elements are coming and going?), since the tearoff no longer has a reference to its SVGAnimatedProperty and is therefore no longer keeping it alive.

[Tim Horton]

(In reply to comment #5)
> (In reply to comment #4)
> > Making that a raw pointer seems reasonable and cuts the cycle.
> 
> I'm not so sure anymore. It makes svg/animations/force-use-shadow-tree-recreation-while-animating crash; it looks like maybe the SVGAnimatedProperty is getting destroyed while we recreate the shadow tree (because elements are coming and going?), since the tearoff no longer has a reference to its SVGAnimatedProperty and is therefore no longer keeping it alive.

I can get around the crash (there were two more callsites not taking references to the return value of lookupOrCreateWrapper, but horrible things are happening re: <use/> and SVGAnimatedProperty's m_isAnimating state (the SVGAnimated property still thinks it's being animated when it's being destroyed, others think they're not being animated when they should, etc.).

<use/> and SMIL, my favorite parts of SVG :D

[Tim Horton]

OK, they're actually still getting discarded at the wrong time.

collectAnimatedPropertiesFromInstances creates more SVGAnimatedProperties, for each shadowTreeElement (instance). However, said shadowTreeElement never takes its reference to those SVGAnimatedProperties, and neither does the original element.

[Tim Horton]

(In reply to comment #7)
> OK, they're actually still getting discarded at the wrong time.
> 
> collectAnimatedPropertiesFromInstances creates more SVGAnimatedProperties, for each shadowTreeElement (instance). However, said shadowTreeElement never takes its reference to those SVGAnimatedProperties, and neither does the original element.

Hmm, the shadowTreeElement shouldn't, since it's an instance of the animated element, not of the <animate>. I'm assuming this means the shadow tree doesn't duplicate <animate> and friends? Then we need to keep the SVGAnimatedProperties of instances alive on the main SVGAnimateElement and clean them up when instances disappear, etc. This seems really annoying.

[Tim Horton]

There are some vaguely related notes in SVGElementInstance::invalidateAllInstancesOfElement.

I'm going to post a WIP patch in a minute, as I have to run away for a few hours. The way it makes SVGAnimateElement hold a reference to its SVGAnimationProperties is obviously a *complete* hack, I did it just to see if it'd work, we'll probably want to switch up a lot of the users of m_animatedProperties to use Vector<RefPtr<t>> & everywhere to prevent that dirtiness.

I need to think about how to fix the <use/> case; I'll be back later tonight with updates.

[Tim Horton]

Created attachment 137111 [details]
hacky wip patch

[Tim Horton]

(In reply to comment #9)
> There are some vaguely related notes in SVGElementInstance::invalidateAllInstancesOfElement.
> 
> I'm going to post a WIP patch in a minute, as I have to run away for a few hours. The way it makes SVGAnimateElement hold a reference to its SVGAnimationProperties is obviously a *complete* hack, I did it just to see if it'd work, we'll probably want to switch up a lot of the users of m_animatedProperties to use Vector<RefPtr<t>> & everywhere to prevent that dirtiness.
> 
> I need to think about how to fix the <use/> case; I'll be back later tonight with updates.

Keeping a reference to the <use> properties (another Vector<RefPtr<SVGAnimatedProperty> > on SVGAnimateElement) seems to work as expected. Updated in SVGAnimateElement::resetToBaseValue and SVGAnimatedTypeAnimator::executeAction (this is a little awkward, I'd like to play with it more) and cleared alongside m_animatedProperties in SVGAnimateElement::targetElementWillChange.

Passes all the tests; I want to try running them all in a window in sequence, forcing GC, and checking Document count, tomorrow, then I'll post a patch.

[Tim Horton]

Created attachment 137379 [details]
patch

Overzealous find and replace probably needs paring down in some cases; this doesn't crash/assert or leak, though. Need to know more about how to update when <use/> instances change, but right now the worst that will happen is we hold on to some extra elements while the page is up, and let them go when the page is torn down, I think. Needs a test (how?!) and a changelog -- if anyone wants to pick it up while I'm on vacation, that's totally cool with me.

[Tim Horton]

(In reply to comment #12)
> Need to know more about how to update when <use/> instances change, but right now the worst that will happen is we hold on to some extra elements while the page is up, and let them go when the page is torn down, I think.

False; if you add a new instance during an animation, we hit an assert now, because m_animatedInstanceProperties is out of date. We really need to keep it in sync with the current set of instances.

[Tim Horton]

(In reply to comment #13)
> (In reply to comment #12)
> > Need to know more about how to update when <use/> instances change, but right now the worst that will happen is we hold on to some extra elements while the page is up, and let them go when the page is torn down, I think.
> 
> False; if you add a new instance during an animation, we hit an assert now, because m_animatedInstanceProperties is out of date. We really need to keep it in sync with the current set of instances.

Nevermind, that was https://bugs.webkit.org/show_bug.cgi?id=84657.

[Tim Horton]

Created attachment 138490 [details]
patch

[Antti Koivisto]

I think it should be possible to test this using window.internals.numberOfLiveDocuments and/or window.internals.numberOfLiveNodes

[Nikolas Zimmermann]

Comment on attachment 138490 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=138490&action=review

Thanks Tim for taking this! The changes to the storage in SVGPropertyTearOff and SVGAnimatedProperty are fine, I'm just worried that both properties and instanceProperties are passed onto the SVGAnimatedTypeAnimators. That should be avoided:

> Source/WebCore/svg/SVGAnimatedTypeAnimator.h:80
> +    SVGElementAnimatedPropertyListMap findAnimatedPropertiesFromInstancesForAttributeName(SVGElement* targetElement, const QualifiedName& attributeName)

A seperated code-path for collecting the instances, returning a HashMap<> seems hacky, I'd rather unfiy this code with findAnimatedPropertiesForAttributeName:
void collectAnimatedPropertiesForAttributeName(SVGElement* targetElement, const QualifiedName& attributeName,  HashMap<SVGElement*, RefPtr<SVGAnimatedProperty> >& map)

The map could be filled as follows:
- key: targetElement | value: properties returned by targetElement->localAttributeToPropertyMap().animatedPropertiesForAttribute(...)
- key: shadowTreeElement1 | value: properties returned by shadowTreeElement->localAttributeToPropertyMap()...
- key: shadowTreeElement2 | value: ditto.
...

This way we can continue to use a single call to collect all properties from the target element and its instances.
The classes deriving from SVGAnimatedTypeAnimator don't need to know about the origin of the SVGAnimatedProperty (if they belong to an instance in the shadow tree element or a regular element), thus exposing both "properties" and "instanceProperties" should be avoided.

What do you think?

[Tim Horton]

(In reply to comment #17)
> (From update of attachment 138490 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=138490&action=review
> 
> This way we can continue to use a single call to collect all properties from the target element and its instances.
> The classes deriving from SVGAnimatedTypeAnimator don't need to know about the origin of the SVGAnimatedProperty (if they belong to an instance in the shadow tree element or a regular element), thus exposing both "properties" and "instanceProperties" should be avoided.
> 
> What do you think?

You're right, there's no reason to keep them separate! I'll fix that up and post another patch today.

[Tim Horton]

Created attachment 138952 [details]
patch with niko's change

I'll look into making a test in the morning, but it'd be nice to hear from Niko again :D

[Nikolas Zimmermann]

Comment on attachment 138952 [details]
patch with niko's change

View in context: https://bugs.webkit.org/attachment.cgi?id=138952&action=review

> Source/WebCore/svg/SVGAnimatedTypeAnimator.h:32
> +typedef pair<SVGElement*, Vector<RefPtr<SVGAnimatedProperty> > > SVGElementAnimatedPropertyPair;

I'd propose to use a struct here, to have descriptive names instead of having to use "properties[0].second" - hard to tell what that is from first sight.
struct SVGElementAnimatedProperties {
    SVGElement* element;
    Vector<RefPtr<SVGAnimatedProperty> > properties;
};
typedef Vector<SVGElementAnimatedProperties> SVElementAnimatedPropertyList;

> Source/WebCore/svg/SVGAnimatedTypeAnimator.h:124
> +    typename AnimValType::ContentType* constructFromBaseValue(const SVGElementAnimatedPropertyList& properties)

s/properties/animatedTypes/ - the now name fits better.

> Source/WebCore/svg/SVGAnimatedTypeAnimator.h:126
> +        ASSERT(properties[0].second.size() == 1);

ASSERT(animatedTypes[0].properties.size() == 1);

> Source/WebCore/svg/SVGAnimatedTypeAnimator.h:135
> +    void resetFromBaseValue(const SVGElementAnimatedPropertyList& properties, SVGAnimatedType* type, typename AnimValType::ContentType& (SVGAnimatedType::*getter)())

s/properties/animatedTypes/.

> Source/WebCore/svg/SVGAnimatedTypeAnimator.h:144
> +    void stopAnimValAnimationForType(const SVGElementAnimatedPropertyList& properties)

s/properties/animatedTypes/. (Stopping to list that one)

> Source/WebCore/svg/SVGAnimatedTypeAnimator.h:170
> +        const typename AnimValType1::ContentType& firstType = castAnimatedPropertyToActualType<AnimValType1>(properties[0].second[0])->currentBaseValue();
> +        const typename AnimValType2::ContentType& secondType = castAnimatedPropertyToActualType<AnimValType2>(properties[0].second[1])->currentBaseValue();

This snippet made me come up with the struct idea :-) animatedTypes[0].properties[0] is really easier to read than "properties[0].second[0]" :-)

> Source/WebCore/svg/SVGAnimatedTypeAnimator.h:186
> +        animatedTypeValue.first = castAnimatedPropertyToActualType<AnimValType1>(properties[0].second[0])->currentBaseValue();

You're passing RefPtr<SVGAnimatedProperty> to castAnimatedPropertyToActualType which takes a PassRefPtr<>, but you're not using .release() here to create a PassRefPtr from the RefPtr and null the underlying pointer. So why not switch back castAnimatedPropertyToActualType to take a SVGAnimatedProperty* for this function?

> Source/WebCore/svg/SVGAnimatedTypeAnimator.h:215
> +    AnimValType* castAnimatedPropertyToActualType(PassRefPtr<SVGAnimatedProperty> property)
...
> +        return static_cast<AnimValType*>(property.get());

See above.


> Source/WebCore/svg/properties/SVGPropertyTearOff.h:120
> -    RefPtr<SVGAnimatedProperty> m_animatedProperty;
> +    SVGAnimatedProperty* m_animatedProperty;

The initial idea was that: rect.x.baseVal.value (the SVGPropertyTearOff<SVGLength> which represents the modifiable SVGLength object in SVG DOM) kept rect.x.baseVal (the SVGAnimatedPropertyTearOff<SVGLength>) alive.
We need a test to ensure this really works. (Not sure if we have existing tests for that). Example given:
<svg onload="loaded()"><rect width="100" height="100" fill="green"/>
<script>
function loaded() {
    rect = document.getElementsByTagName("rect")[0];
    var baseValReference = rect.x.baseVal; // That's a SVGLength, aka. SVGPropertyTearOff<SVGLength>
    rect.parentNode.removeChild(rect);
    // Force GC here. Is 'baseValReference' preventing SVGRectElements destruction? That's expected here.
    alert(baseValReference.value);
}
</script>
</svg>

You can repeat the testcase with "var animatedSVGLengthReference = rect.x", which is a SVGAnimatedLength, aka. SVGAnimatedPropertyTearOff<SVGLength>.
It should also keep the <rect> alive. I'm asking for these test cases, as I'm not 100% sure your current approach takes care of that.

[Tim Horton]

(In reply to comment #20)
> (From update of attachment 138952 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=138952&action=review
> You can repeat the testcase with "var animatedSVGLengthReference = rect.x", which is a SVGAnimatedLength, aka. SVGAnimatedPropertyTearOff<SVGLength>.
> It should also keep the <rect> alive. I'm asking for these test cases, as I'm not 100% sure your current approach takes care of that.

Ooh, you're right:

<svg xmlns="http://www.w3.org/2000/svg" onload="loaded()"><rect x="10" y="10" width="100" height="100" fill="green"/>
<script>
<![CDATA[
function loaded() {
    rect = document.getElementsByTagName("rect")[0];
    var baseValReference = rect.x.baseVal; // That's a SVGLength, aka. SVGPropertyTearOff<SVGLength>
    rect.parentNode.removeChild(rect);
    rect = 0;
    GCController.collect();
    alert(baseValReference.value);
}
]]>
</script>
</svg>

crashes in DRT. But, we can't have this cycle. I'll have to look into how to break it without breaking this case.

[Tim Horton]

Created attachment 139080 [details]
patch fixing niko's suggestions plus adding some tests

[Nikolas Zimmermann]

Comment on attachment 139080 [details]
patch fixing niko's suggestions plus adding some tests

View in context: https://bugs.webkit.org/attachment.cgi?id=139080&action=review

Excellent move forward! I only have a few code nitpicks, and some issues with missing tests, see below:
(The patch is really ready to r+, I'll  leave the r? state, so cr-linux EWS eventually builds this patch later, so we can see at least chromium test results).

> LayoutTests/ChangeLog:13
> +

These tests are great, but I think we should extend them for following scenarios:

#1)
<defs>
<rect id="rect" width="10" height="100"/>
</defs>
<use xlink:href="#rect">
    <animate attributeName="width" by="90" begin="0s" dur="10s"/>
</use>
<use ...

- one <rect>, with eg. 5 instances via <use>. All start animating at 0s, for a dur of 10s.
- Keep a base value reference to eg. rect.width.baseVal around
- Record the numberOfLiveNodes
- Remove one or several of the <use> instances, while the animation is running
- Assure the numberOfLIveNodes shrinked by the removed instances, even though a reference to the <rect> they're cloned from is held.
(Note that there is no way to access the clonedRect.width.baseVal, cloned elements are never exposed directly to the DOM, only via SVGElementInstance, but the 'shadowTreeElement' that we track internally can't be accessed.)

I want to assure here, that we don't block instances to destruct, just because a reference to the original <rect> is hold from js.

#2) The same test without keeping a base value reference around

#3) The same test as #2), but adding a <use> referencing the <rect> dynamically via JS, while the animations is running, verifying that the newly added instance also animates.

I'm sure I could think of more cases, but I think these are the most tricky things, we should definitely get right.

> LayoutTests/svg/animations/smil-leak-elements-expected.txt:1
> +CONSOLE MESSAGE: line 35: PASS

I hope the line number reporting is consistent across ports.

> LayoutTests/svg/animations/smil-leak-elements.svg:31
> +    GCController.collect();

I'd check if (!windw.internals) first, and early exit, so this test would show no warnings in Safari.

> LayoutTests/svg/animations/smil-leak-elements.svg:33
> +    var liveDelta = window.internals.numberOfLiveNodes() - originalLiveElements;

Wow this is extremely useful, didn't know about numberOfLiveNodes so far!

> LayoutTests/svg/animations/svglength-element-removed-crash.svg:13
> +    rect.parentNode.removeChild(rect);

Can't we use the same numberOfLiveNodes logic here, to assure the rect is still alive? This way we wouldn't need gmalloc to find the problem, if it regresses in future.

> LayoutTests/svg/animations/svglength-element-removed-crash.svg:17
> +    for (var i = 0; i < 10000; ++i) 
> +        var s = new String("abc");

Isn't this only needed if GCController is not available? Do we also need to check if (window.GCController) to make this work in Safari?

> Source/WebCore/svg/SVGAnimatedTypeAnimator.h:32
> +struct SVGElementAnimatedProperties {

We could add a default actor, that nulls element for safety?

> Source/WebCore/svg/properties/SVGPropertyTearOff.h:72
> +        return m_contextElement.get();

Interessting approach, to hold a RefPtr<> to just the original element. If i understand correctly, this RefPtr<> will keep the SVGAnimatedProperty alive, which keeps the property tearoffs alive, right?

[WebKit Review Bot]

Comment on attachment 139080 [details]
patch fixing niko's suggestions plus adding some tests

Attachment 139080 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12548202

New failing tests:
svg/animations/smil-leak-elements.svg

[WebKit Review Bot]

Created attachment 139186 [details]
Archive of layout-test-results from ec2-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-03  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Darin Adler]

(In reply to comment #23)
> (The patch is really ready to r+, I'll  leave the r? state, so cr-linux EWS eventually builds this patch later, so we can see at least chromium test results).

Does cr-linux EWS skip patches once they have been r+'d? I have never encountered that before.

[Tim Horton]

(In reply to comment #23)
> (From update of attachment 139080 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=139080&action=review

> #3) The same test as #2), but adding a <use> referencing the <rect> dynamically via JS, while the animations is running, verifying that the newly added instance also animates.

#1 and #2 sound good; #3 isn't feasible right now because of https://bugs.webkit.org/show_bug.cgi?id=84657; this is a quite high-priority fix (blocking others from doing testing), so I'd not like to get weighted down trying to fix that in this bug as well, please.

[Tim Horton]

(In reply to comment #26)
> (In reply to comment #23)
> > (The patch is really ready to r+, I'll  leave the r? state, so cr-linux EWS eventually builds this patch later, so we can see at least chromium test results).
> 
> Does cr-linux EWS skip patches once they have been r+'d? I have never encountered that before.

I don't think so, though I've seen multiple bots skip testing a patch before -- maybe only for r-?

[Maciej Stachowiak]

(In reply to comment #28)
> (In reply to comment #26)
> > (In reply to comment #23)
> > > (The patch is really ready to r+, I'll  leave the r? state, so cr-linux EWS eventually builds this patch later, so we can see at least chromium test results).
> > 
> > Does cr-linux EWS skip patches once they have been r+'d? I have never encountered that before.
> 
> I don't think so, though I've seen multiple bots skip testing a patch before -- maybe only for r-?

All the bots will now do r+ patches as well as r?, so it should be fine to r+ this now. It looks like cr-linux has run already in any case, but it failed because the tree was already red.

[Tim Horton]

(In reply to comment #29)
> (In reply to comment #28)
> > (In reply to comment #26)
> > > (In reply to comment #23)
> > > > (The patch is really ready to r+, I'll  leave the r? state, so cr-linux EWS eventually builds this patch later, so we can see at least chromium test results).
> > > 
> > > Does cr-linux EWS skip patches once they have been r+'d? I have never encountered that before.
> > 
> > I don't think so, though I've seen multiple bots skip testing a patch before -- maybe only for r-?
> 
> All the bots will now do r+ patches as well as r?, so it should be fine to r+ this now. It looks like cr-linux has run already in any case, but it failed because the tree was already red.

Actually, it failed because (as Niko was worried) the line numbers from the console.logs didn't match, as you can see in the output that the bot uploaded. I'm fixing the test to add text nodes for logging instead and touching up Niko's other suggestions (and new tests).

[Tim Horton]

Created attachment 139284 [details]
patch

[Dean Jackson]

Comment on attachment 139284 [details]
patch

Seems that everyone agrees that this is r+. We still don't have the latest cr-linux results, but I'm confident thorton fixed it (and we'll know soon enough).

[Tim Horton]

Landed in http://trac.webkit.org/changeset/115518; Niko, if you have any further concerns, can we follow-up in an additional bug? Hopefully I've addressed everything, though!

[Nikolas Zimmermann]

(In reply to comment #33)
> Landed in http://trac.webkit.org/changeset/115518; Niko, if you have any further concerns, can we follow-up in an additional bug? Hopefully I've addressed everything, though!
The patch is excellent as-is! Thanks Dino for reviewing it officially.
We can add the other test I asked for after 84657 is fixed. Do you plan to work on that?

(In reply to comment #29)
> All the bots will now do r+ patches as well as r?, so it should be fine to r+ this now. It looks like cr-linux has run already in any case, but it failed because the tree was already red.
Ah okay, thanks for the hint.

[Tim Horton]

(In reply to comment #34)
> (In reply to comment #33)
> > Landed in http://trac.webkit.org/changeset/115518; Niko, if you have any further concerns, can we follow-up in an additional bug? Hopefully I've addressed everything, though!
> The patch is excellent as-is! Thanks Dino for reviewing it officially.
> We can add the other test I asked for after 84657 is fixed. Do you plan to work on that?

I got it, actually (that one is smil-leak-dynamically-added-element-instances.svg), I'm not sure why I didn't run into 84657.

Thanks for all your many reviews, Niko!!

[Ken Buchanan]

*** Bug 84732 has been marked as a duplicate of this bug. ***