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.
Created attachment 90495 [details] Patch
This is a generic problem for SVGElement. So changing title.
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!
(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...
This is leaking SVG documents too (due to elements holding guard refs), https://bugs.webkit.org/show_bug.cgi?id=58889.
Created attachment 90528 [details] New approach This should fix the problem, Antti is going to try.
*** Bug 59084 has been marked as a duplicate of this bug. ***
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?
Comment on attachment 90528 [details] New approach Wrong approach, doesn't fix anything. I'm investigating...
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.
(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
(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?
(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.
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.
(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.
(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.
(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...
(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.
Comment on attachment 90692 [details] Patch r-. See the comments of Niko.
Created attachment 91253 [details] Patch v2
Created attachment 91254 [details] Patch v3 Oops, forgot to update the ChangeLog to reflect the refcounting change.
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)?
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.
(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..
Created attachment 91822 [details] Patch v4 Fixed Leos & Dirks comments.
Comment on attachment 91822 [details] Patch v4 r=me. You're right with renaming the function.
Landed in r85413.
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