Bug 151810 – Reference cycle between SVGPathElement and SVGPathSegWithContext leaks Document

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

[Said Abou-Hallawa]

The issue is that "RefPtr<SVGPathElement> m_element" in SVGPathSegWithContext is causing reference cycle by using RefPtr to the element.

Specifically, SVGPathElement::m_pathSegList is a SVGPathSegList (subclass of a Vector of RefPtr<SVGPathSeg>), so it strongly references the SVGPathSeg. In practice, I think all SVGPathSeg objects are SVGPathSegWithContext (maybe that subclass should be folded into SVGPathSeg?) SVGPathSegWithContext::m_contextElement strongly references the SVGPathElement, and so we have a reference cycle.

[Said Abou-Hallawa]

Created attachment 266538 [details]
Patch

[Andreas Kling]

Comment on attachment 266538 [details]
Patch

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

> Source/WebCore/svg/SVGPathSegWithContext.h:73
> -    RefPtr<SVGPathElement> m_element;
> +    SVGPathElement* m_element;

Can't this pointer become stale if we have a reachable JS wrapper for the SVGPathSeg and the SVGPathElement goes away?

[Darin Adler]

Comment on attachment 266538 [details]
Patch

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

>> Source/WebCore/svg/SVGPathSegWithContext.h:73
>> +    SVGPathElement* m_element;
> 
> Can't this pointer become stale if we have a reachable JS wrapper for the SVGPathSeg and the SVGPathElement goes away?

Yes, this looks wrong. What’s the actual strategy for these references? What should null out this m_element pointer?

[Said Abou-Hallawa]

Created attachment 268343 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 266538 [details]
Patch

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

>>> Source/WebCore/svg/SVGPathSegWithContext.h:73
>>> +    SVGPathElement* m_element;
>> 
>> Can't this pointer become stale if we have a reachable JS wrapper for the SVGPathSeg and the SVGPathElement goes away?
> 
> Yes, this looks wrong. What’s the actual strategy for these references? What should null out this m_element pointer?

Yes this can happen. Here is an example:

<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" version="1.1">
    <script>
        var path = document.createElementNS("http://www.w3.org/2000/svg", "path");
        var seg = path.createSVGPathSegMovetoAbs(100, 100);
        path.remove();
        alert(seg);
        seg.x = 10;
    </script>
</svg>

[Build Bot]

Comment on attachment 268343 [details]
Patch

Attachment 268343 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/655402

New failing tests:
svg/custom/js-update-path-removal.svg
svg/custom/textPath-path-change-using-svg-dom-pattern.svg
svg/W3C-SVG-1.1-SE/paths-dom-02-f.svg
svg/custom/textPath-path-change-using-svg-dom.svg
svg/dom/svglist-exception-on-out-bounds-error.html
svg/dom/SVGPathSegList-clear-and-initialize.xhtml
svg/dom/SVGPathSegList-removeItem.xhtml
svg/dom/SVGPathSegList-replaceItem.xhtml
svg/dom/SVGPathSegList-crash.html

[Build Bot]

Created attachment 268348 [details]
Archive of layout-test-results from ews114 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 268343 [details]
Patch

Attachment 268343 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/655480

New failing tests:
svg/custom/js-update-path-removal.svg
svg/custom/textPath-path-change-using-svg-dom-pattern.svg
svg/W3C-SVG-1.1-SE/paths-dom-02-f.svg
svg/custom/textPath-path-change-using-svg-dom.svg
svg/dom/SVGPathSegList-clear-and-initialize.xhtml
svg/dom/SVGPathSegList-crash.html
svg/dom/SVGPathSegList-replaceItem.xhtml
svg/dom/svglist-exception-on-out-bounds-error.html
svg/dom/SVGPathSegList-removeItem.xhtml

[Build Bot]

Created attachment 268349 [details]
Archive of layout-test-results from ews102 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 268343 [details]
Patch

Attachment 268343 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/655492

New failing tests:
svg/custom/js-update-path-removal.svg
svg/custom/textPath-path-change-using-svg-dom-pattern.svg
svg/W3C-SVG-1.1-SE/paths-dom-02-f.svg
svg/custom/textPath-path-change-using-svg-dom.svg
svg/dom/SVGPathSegList-clear-and-initialize.xhtml
svg/dom/SVGPathSegList-crash.html
svg/dom/SVGPathSegList-replaceItem.xhtml
svg/dom/svglist-exception-on-out-bounds-error.html
svg/dom/SVGPathSegList-removeItem.xhtml

[Build Bot]

Created attachment 268350 [details]
Archive of layout-test-results from ews106 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Said Abou-Hallawa]

Created attachment 268386 [details]
Patch

[Said Abou-Hallawa]

Some tests were crashing because I changed the function SVGPathSegWithContext::setContextAndRole() to set m_element to element->createWeakPtr(). But since some callers, which remove SVGPathSegWithContext, pass a nullptr for SVGPathElement. So I needed to handle this case by assigning a null WeakPtr to m_element if element is nullptr.

[Chris Vienneau]

I was directed here from https://bugs.webkit.org/show_bug.cgi?id=152759, do you believe the patch here will resolve my problem too?  I plan to watch this bug and apply the patch when it is ready.

Thanks Chris

[Said Abou-Hallawa]

(In reply to comment #14)
> I was directed here from https://bugs.webkit.org/show_bug.cgi?id=152759, do
> you believe the patch here will resolve my problem too?  I plan to watch
> this bug and apply the patch when it is ready.
> 
> Thanks Chris

No. The reasons of the two bugs are just similar. We need two patches to fix them. Once the approach of the attached patch is approved I can upload a similar patch for the other one 152759.

[WebKit Commit Bot]

Comment on attachment 268386 [details]
Patch

Clearing flags on attachment: 268386

Committed r194964: <http://trac.webkit.org/changeset/194964>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Said Abou-Hallawa]

<rdar://problem/23453491>