[Webkit-unassigned] [Bug 152759] SVG polyline and polygon leak page

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=152759

--- Comment #8 from Said Abou-Hallawa <sabouhallawa at apple.com> ---
(In reply to comment #5)
> I initially stated that polyline and polygon leak, I now believe in fact any
> svg node that creates a SVGAnimatedProperty will leak.  It just so happens
> that polyline and polygon always create it.  In older code based on based on
> 157437 the SVGAnimatedProperty would be created less often for polyline and
> polygon, additionally when it was created it didn't leak.
> 
> For now i'm using a workaround by restoring
> WebCore\rendering\svg\SVGPathData.cpp updatePathFromPolygonElement() and
> updatePathFromPolylineElement() to their older version in order to hit the
> failure case less often.  I'm also immediately releasing the reference kept
> on the "SVGElement* contextElement" in SVGAnimatedProperty constructor; so
> when the error case happens we don't leak the whole page (only the
> SVGAnimatedProperty).  
> 
> This greatly reduces the memory leaked and our soaks come out pretty good
> now. and I haven't encountered any issue with releasing the ref early.  I'm
> moving on to other more urgent issues for the time being, these workarounds
> I'm using i only consider temporary, I'd be interested in a better fix if
> one becomes available.
> 
> Chris

Yes the SVGAnimatedProperty leak happens because of the reference cycle between SVGListPropertyTearOff and SVGAnimatedListPropertyTearOff (which is derived form SVGAnimatedProperty). But the document leak happens because we cache the SVGAnimatedProperty in a global cache which SVGAnimatedProperty::animatedPropertyCache() returns. The entries of this cache never gets deleted. And because the SVGAnimatedProperty has "RefPtr<SVGElement> m_contextElement", the SVGElement ref count never get to zero so the whole document can't be deleted.

This cache is required since it is the place the property value keep changing while animation. But having it in a global cache without notification from the garbage collector that the property wrapper is no longer needed is wrong. I think the cached wrappers have to be stored with the SVGElement itself. Once all the wrappers are not referenced anymore, the SVGElement itself can be deleted and hence all the cached wrappers can be deleted also.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.webkit.org/pipermail/webkit-unassigned/attachments/20160201/754e4f48/attachment.html>