Bug 59072 – LEAK: SVGElement leaks when detaching it in a pending resource state

• Component: SVG
• Status: RESOLVED FIXED

[Leo Yang]

SVG <use> element is leaking when detaching from pending on resource state.

<?xml version="1.0" standalone="no"?>
<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" version="1.0" width="400" height="400">
    <use xlink:href="#undefined" id="use" />
</svg>

the above document will cause <use> be leaking.

patch is coming soon.

[Leo Yang]

Created attachment 90495 [details]
Patch

[Leo Yang]

This is a generic problem for SVGElement. So changing title.

[Nikolas Zimmermann]

As discussed on IRC:
SVGElement::removedFromDocument() needs to be implemented, that clears any pending resource state.
Aka removes the offending element from the SVGDocumentExtensions HashSet.


    typedef HashSet<RefPtr<SVGStyledElement> > SVGPendingElements;

Some time ago this HashSet was changed to contain RefPtr, as we saw dangling pointer crashes. The reason is now clear:

<rect fill="url(#foo)"> causes the <rect> to be present in the HashSet. If foo never appears, and we remove the <rect> from DOM, or destruct the document, the RefPtr in SVGPendingElements is still present!

The RefCounting itself is relatively new, but now the reason is clear why we had dangling pointer crashes in first place: we're not cleaning up this SVGPendingElements HashSet properly!

[Nikolas Zimmermann]

(In reply to comment #3)
> As discussed on IRC:
> SVGElement::removedFromDocument() needs to be implemented, that clears any pending resource state.
> Aka removes the offending element from the SVGDocumentExtensions HashSet.
> 
> 
>     typedef HashSet<RefPtr<SVGStyledElement> > SVGPendingElements;
> 
> Some time ago this HashSet was changed to contain RefPtr, as we saw dangling pointer crashes. The reason is now clear:
> 
> <rect fill="url(#foo)"> causes the <rect> to be present in the HashSet. If foo never appears, and we remove the <rect> from DOM, or destruct the document, the RefPtr in SVGPendingElements is still present!
> 
> The RefCounting itself is relatively new, but now the reason is clear why we had dangling pointer crashes in first place: we're not cleaning up this SVGPendingElements HashSet properly!

As discussed with Leo on IRC, all we need to do to fix is:
implement SVGElement::removedFromDocument, similar to insertedIntoDocument, that just removes the pending resource...

[Antti Koivisto]

This is leaking SVG documents too (due to elements holding guard refs), https://bugs.webkit.org/show_bug.cgi?id=58889.

[Nikolas Zimmermann]

Created attachment 90528 [details]
New approach

This should fix the problem, Antti is going to try.

[Nikolas Zimmermann]

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

[Eric Seidel (no email)]

Comment on attachment 90528 [details]
New approach

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

> Source/WebCore/svg/SVGElement.cpp:67
> +static inline void clearPendingResourceState(SVGDocumentExtensions* extensions, const String& id)

I'm confused.  Why wouldnt this go on SVGDocumentExtensions.  And can we add an ASSERT that would catch the leak?

[Nikolas Zimmermann]

Comment on attachment 90528 [details]
New approach

Wrong approach, doesn't fix anything. I'm investigating...

[Nikolas Zimmermann]

Created attachment 90692 [details]
Patch

New approach, the previous patch was just plain wrong :-) Included several testcases that leak in trunk and stop with that patch.

[Leo Yang]

(In reply to comment #10)
> Created an attachment (id=90692) [details]
> Patch
> 
> New approach, the previous patch was just plain wrong :-) Included several testcases that leak in trunk and stop with that patch.

I did something like this today. But I think we maybe do this more efficient. My thought is the following.

  add a function like
  virtual Vector<AtomticString> SVGElement::collectPendingResourceId()

  add a resource id parameter to SVGDocumentExtensions::removeElementFromPendingResources, make it like
  void SVGDocumentExtensions::removeElementFromPendingResources(const AtomicString& resurctId SVGElement* element)

  In SVGElement::removedFromDocument, we can call removeElementFromPendingResources for each resource which are collected by collectPendingResourceId().

  This enable us to implement removeElementFromPendingResources more efficient because 1) we can quick reject to call removeElementFromPendingResources if the vector is empty; 2)otherwise, we can use resourceId to find pending elements quickly.

  The implementation of SVGElement::collectPendingResourceId() like

  {
       return SVGResources::collectPendingResourceId(this); //static function
  }

  Vector<AtomicString> SVGResources::collectPendingResourceId(SVGElement* element)
  {
       // Do something like SVGResources::buildCachedResource()
       // use element's style to collect pending resource id.
  }

  For other elements like SVGUseElement, the implmentation of collectPendingResourceId like

   {
       Vector<AtomicString> vector = Super::collectPendingResourceId();
       if (m_isPendingResource) {
           vector.append(m_resourceId);
           // Do other clear things;
       }
       return vector;
   }

  One thing that I'm not sure is that I don't know if SVGElement's style is
  available when we call collectPendingResourceId

  The above is just my thought, but might be wrong :p

[Nikolas Zimmermann]

(In reply to comment #11)
> (In reply to comment #10)
> > Created an attachment (id=90692) [details] [details]
> > Patch
> > 
> > New approach, the previous patch was just plain wrong :-) Included several testcases that leak in trunk and stop with that patch.
> 
> I did something like this today. But I think we maybe do this more efficient. 
>   Vector<AtomicString> SVGResources::collectPendingResourceId(SVGElement* element)
>   {
>        // Do something like SVGResources::buildCachedResource()
>        // use element's style to collect pending resource id.
>   }

This is problematic, we need to remove the pending resources that were initially registered.
Think of <rect fill="url(#foo)", if I change the url to #bar using JS, how would you handle that?
Your collectPendingResourceId would return #bar, and we'd miss to remove the #foo reference from the HashSet in the SVGDocumentExtensions.

> 
>   One thing that I'm not sure is that I don't know if SVGElement's style is
>   available when we call collectPendingResourceId
Not necessarily...
> 
>   The above is just my thought, but might be wrong :p
What do you think about the approach in my patch? you have speed concerns?

[Leo Yang]

(In reply to comment #12)
> (In reply to comment #11)
> > (In reply to comment #10)
> > > Created an attachment (id=90692) [details] [details] [details]
> > > Patch
> > > 
> > > New approach, the previous patch was just plain wrong :-) Included several testcases that leak in trunk and stop with that patch.
> > 
> > I did something like this today. But I think we maybe do this more efficient. 
> >   Vector<AtomicString> SVGResources::collectPendingResourceId(SVGElement* element)
> >   {
> >        // Do something like SVGResources::buildCachedResource()
> >        // use element's style to collect pending resource id.
> >   }
> 
> This is problematic, we need to remove the pending resources that were initially registered.
> Think of <rect fill="url(#foo)", if I change the url to #bar using JS, how would you handle that?
> Your collectPendingResourceId would return #bar, and we'd miss to remove the #foo reference from the HashSet in the SVGDocumentExtensions.
> 
> > 
> >   One thing that I'm not sure is that I don't know if SVGElement's style is
> >   available when we call collectPendingResourceId
> Not necessarily...
> > 
> >   The above is just my thought, but might be wrong :p
> What do you think about the approach in my patch? you have speed concerns?
The approach will work. But every SVGElement costs time which is taken by removeElementFromPendingReources even if it has no pending resource. And elements which have no pending resource are very common. Maybe adding a bit to SVGElement as a hint of pending on resources can reduce most uncessary calls.

[Dirk Schulze]

Comment on attachment 90692 [details]
Patch

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

> Source/WebCore/svg/SVGDocumentExtensions.cpp:214
> +void SVGDocumentExtensions::addPendingResource(const AtomicString& id, PassRefPtr<SVGElement> obj)

I wonder why you need this change. Or better: why did it work before? None-styleable elements can be pending resources or could need pending resources.

> Source/WebCore/svg/SVGDocumentExtensions.cpp:242
> +    if (m_pendingResources.isEmpty())
> +        return;

To Leo's comment. We have to call SVGDocumentExtensions for animations anyway. And if the element doesn't have pending resources we return earlier.

[Nikolas Zimmermann]

(In reply to comment #14)
> (From update of attachment 90692 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=90692&action=review
> 
> > Source/WebCore/svg/SVGDocumentExtensions.cpp:214
> > +void SVGDocumentExtensions::addPendingResource(const AtomicString& id, PassRefPtr<SVGElement> obj)
> 
> I wonder why you need this change. Or better: why did it work before? None-styleable elements can be pending resources or could need pending resources.
Nope only SVGStyledElements use this framework. The question is correct, the stuff calling add/removePendingResources should be in SVGStyledElements dtor, not in SVGElement.
I'm looking into it...

> > Source/WebCore/svg/SVGDocumentExtensions.cpp:242
> > +    if (m_pendingResources.isEmpty())
> > +        return;
> 
> To Leo's comment. We have to call SVGDocumentExtensions for animations anyway. And if the element doesn't have pending resources we return earlier.

[Leo Yang]

(In reply to comment #14)
> (From update of attachment 90692 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=90692&action=review
> 
> > Source/WebCore/svg/SVGDocumentExtensions.cpp:214
> > +void SVGDocumentExtensions::addPendingResource(const AtomicString& id, PassRefPtr<SVGElement> obj)
> 
> I wonder why you need this change. Or better: why did it work before? None-styleable elements can be pending resources or could need pending resources.
> 
> > Source/WebCore/svg/SVGDocumentExtensions.cpp:242
> > +    if (m_pendingResources.isEmpty())
> > +        return;
> 
> To Leo's comment. We have to call SVGDocumentExtensions for animations anyway. And if the element doesn't have pending resources we return earlier.
How about m_pendingResources is not empty? Consider a few SVGStyledElements in a document are pending on resources, which means m_pendingResources is not empty, but some other SVGStyledElements in the document aren't. Every SVGStyledElement not in the pending map also will cost the time of the loop.

[Nikolas Zimmermann]

(In reply to comment #13)
> (In reply to comment #12)
> > (In reply to comment #11)
> > > (In reply to comment #10)
> > > > Created an attachment (id=90692) [details] [details] [details] [details]
> > > > Patch
> > > > 
> > > > New approach, the previous patch was just plain wrong :-) Included several testcases that leak in trunk and stop with that patch.
> > > 
> > > I did something like this today. But I think we maybe do this more efficient. 
> > >   Vector<AtomicString> SVGResources::collectPendingResourceId(SVGElement* element)
> > >   {
> > >        // Do something like SVGResources::buildCachedResource()
> > >        // use element's style to collect pending resource id.
> > >   }
> > 
> > This is problematic, we need to remove the pending resources that were initially registered.
> > Think of <rect fill="url(#foo)", if I change the url to #bar using JS, how would you handle that?
> > Your collectPendingResourceId would return #bar, and we'd miss to remove the #foo reference from the HashSet in the SVGDocumentExtensions.
> > 
> > > 
> > >   One thing that I'm not sure is that I don't know if SVGElement's style is
> > >   available when we call collectPendingResourceId
> > Not necessarily...
> > > 
> > >   The above is just my thought, but might be wrong :p
> > What do you think about the approach in my patch? you have speed concerns?
> The approach will work. But every SVGElement costs time which is taken by removeElementFromPendingReources even if it has no pending resource. And elements which have no pending resource are very common. Maybe adding a bit to SVGElement as a hint of pending on resources can reduce most uncessary calls.
You're right, though adding anything to SVGElement/SVGStyledElement is a bad idea, as it enlarges the size of all these classes, even though it's mostly unused. so I've used SVGElementRareData as place to track whether the SVGStyledElement is pending or not.

Updating patch soon...

[Nikolas Zimmermann]

(In reply to comment #14)
> (From update of attachment 90692 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=90692&action=review
> 
> > Source/WebCore/svg/SVGDocumentExtensions.cpp:214
> > +void SVGDocumentExtensions::addPendingResource(const AtomicString& id, PassRefPtr<SVGElement> obj)
> 
> I wonder why you need this change. Or better: why did it work before? None-styleable elements can be pending resources or could need pending resources.
It turns out all resources that can be pending are SVGStyledElements. I'm reverting that part of the patch, and move the code where it belongs (from SVGElement to SVGStyledElement).

> 
> > Source/WebCore/svg/SVGDocumentExtensions.cpp:242
> > +    if (m_pendingResources.isEmpty())
> > +        return;
> 
> To Leo's comment. We have to call SVGDocumentExtensions for animations anyway. And if the element doesn't have pending resources we return earlier.
Nope, that was exactly Leos point. We only exit earlier if needsPendingResourceHandling() is false.
And by-default we return true there for every SVGStyledElement, only false for a few (filter, marker, mask, etc..). So we have to track pending resources state explicitely in SVGStyledElement oherwhise we're always doing extra work, so Leo is right.

[Dirk Schulze]

Comment on attachment 90692 [details]
Patch

r-. See the comments of Niko.

[Nikolas Zimmermann]

Created attachment 91253 [details]
Patch v2

[Nikolas Zimmermann]

Created attachment 91254 [details]
Patch v3

Oops, forgot to update the ChangeLog to reflect the refcounting change.

[Leo Yang]

Comment on attachment 91254 [details]
Patch v3

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

I just add my comments, not a review :)

> Source/WebCore/svg/SVGElementRareData.h:64
> +    bool isPendingResource() const { return m_isPendingResource; }

Is hasPendingResource() better?

> Source/WebCore/svg/SVGElementRareData.h:65
> +    void setIsPendingResource(bool value) { m_isPendingResource = value; }

Is setHasPendingResource() better?

> Source/WebCore/svg/SVGElementRareData.h:78
> +    bool m_isPendingResource : 1;

ditto: m_hasPendingResource?

> Source/WebCore/svg/SVGStyledElement.cpp:454
> +}

ditto

> Source/WebCore/svg/SVGStyledElement.cpp:459
> +}

ditto

> Source/WebCore/svg/SVGStyledElement.h:56
> +    void setIsPendingResource(bool);

ditto

> Source/WebCore/svg/SVGUseElement.cpp:176
> +            }

I can't understand why this <use>'s href affects the other <use>'s. Shall we call document()->accessSVGExtensions()->removeElementFromPendingResources(this)?

[Dirk Schulze]

Comment on attachment 91254 [details]
Patch v3

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

Mainly r- because of the wrong style of the tests. Still some open questions on the patch itself.

> Source/WebCore/svg/SVGDocumentExtensions.cpp:251
> +    HashMap<AtomicString, SVGPendingElements*>::iterator end = m_pendingResources.end();

Why can we have multiple resources for one id? Even if the user sets the same id to two different elements, we don't end up with two resources. What am I missing? Ah you try to catch the scenario where the user removes one of the elements and we take the other resource?

>> Source/WebCore/svg/SVGElementRareData.h:64
>> +    bool isPendingResource() const { return m_isPendingResource; }
> 
> Is hasPendingResource() better?

First unofficial reviews. Great! really.

>> Source/WebCore/svg/SVGElementRareData.h:65
>> +    void setIsPendingResource(bool value) { m_isPendingResource = value; }
> 
> Is setHasPendingResource() better?

The naming is correct here. The affected element _is_ a pending resource. But it may have it self pending resources of course.

>> Source/WebCore/svg/SVGElementRareData.h:78
>> +    bool m_isPendingResource : 1;
> 
> ditto: m_hasPendingResource?

Ditto.

>> Source/WebCore/svg/SVGUseElement.cpp:176
>> +            }
> 
> I can't understand why this <use>'s href affects the other <use>'s. Shall we call document()->accessSVGExtensions()->removeElementFromPendingResources(this)?

the use element can have pending resources it self: xlink:hreaf="foo". We need to remove foo from the pending lists. Nevertheless I agree. Why are you doing it if the use element is itself a pending resource? Should this if condition get removed?

> LayoutTests/svg/custom/pending-resource-leak-2.svg:6
> +			layoutTestController.waitUntilDone();

No tabs please. For some reason this is not checked by the style bot :-(

> LayoutTests/svg/custom/pending-resource-leak-3.svg:6
> +			layoutTestController.waitUntilDone();

ditto. The following files have the same problem.

[Nikolas Zimmermann]

(In reply to comment #23)
> (From update of attachment 91254 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=91254&action=review
> 
> Mainly r- because of the wrong style of the tests. Still some open questions on the patch itself.
> 
> > Source/WebCore/svg/SVGDocumentExtensions.cpp:251
> > +    HashMap<AtomicString, SVGPendingElements*>::iterator end = m_pendingResources.end();
> 
> Why can we have multiple resources for one id? Even if the user sets the same id to two different elements, we don't end up with two resources. What am I missing? Ah you try to catch the scenario where the user removes one of the elements and we take the other resource?
<rect fill="url(#foo)" stroke="url(#foo)"> that's the scenario :-)

> 
> >> Source/WebCore/svg/SVGElementRareData.h:64
> >> +    bool isPendingResource() const { return m_isPendingResource; }
> > 
> > Is hasPendingResource() better?
> 
> First unofficial reviews. Great! really.
Yeah, it's better, I changed the name.

> 
> >> Source/WebCore/svg/SVGElementRareData.h:65
> >> +    void setIsPendingResource(bool value) { m_isPendingResource = value; }
> > 
> > Is setHasPendingResource() better?
> 
> The naming is correct here. The affected element _is_ a pending resource. But it may have it self pending resources of course.
Wait Leo is right, when we have
<rect fill="url(#foo)" id="rect"/>
Then we call addPendingResource("url(#foo), rectElement).
That doesn't mean rectElement is a pending resource, but it _has_ pending resources, so Leos suggestions is good.

> 
> >> Source/WebCore/svg/SVGUseElement.cpp:176
> >> +            }
> > 
> > I can't understand why this <use>'s href affects the other <use>'s. Shall we call document()->accessSVGExtensions()->removeElementFromPendingResources(this)?
> 
> the use element can have pending resources it self: xlink:hreaf="foo". We need to remove foo from the pending lists. Nevertheless I agree. Why are you doing it if the use element is itself a pending resource? Should this if condition get removed?
Use is special, my changes only mimic the "old way" use handles resources. I tried to change this, but it's really tricky -- we have to postpone cleaning use & pending resources until the point where we use the new shadow DOM creation. Currently RenderSVGShadowTreeRootContainer _manually_ handles pending resources, and calls useElement->buildPendingResource() on its own, that's why we have the check in there and can't remove the condition. This only affects use.

> 
> > LayoutTests/svg/custom/pending-resource-leak-2.svg:6
> > +			layoutTestController.waitUntilDone();
> 
> No tabs please. For some reason this is not checked by the style bot :-(
Fixed.

Uploading a new patch soon..

[Nikolas Zimmermann]

Created attachment 91822 [details]
Patch v4

Fixed Leos & Dirks comments.

[Dirk Schulze]

Comment on attachment 91822 [details]
Patch v4

r=me. You're right with renaming the function.

[Nikolas Zimmermann]

Landed in r85413.

[Adam Roben (:aroben)]

It's hard to tell if this had any effect on the leaks bot:

r85411, 5342 leaks
http://build.webkit.org/builders/SnowLeopard%20Intel%20Leaks/builds/16613

r85413, 6266 leaks
http://build.webkit.org/builders/SnowLeopard%20Intel%20Leaks/builds/16614

r85427, 5221 leaks
http://build.webkit.org/builders/SnowLeopard%20Intel%20Leaks/builds/16619