Bug 151810 - Reference cycle between SVGPathElement and SVGPathSegWithContext leaks Document
Summary: Reference cycle between SVGPathElement and SVGPathSegWithContext leaks Document
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Said Abou-Hallawa
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2015-12-03 10:47 PST by Said Abou-Hallawa
Modified: 2016-01-13 09:08 PST (History)
9 users (show)

See Also:


Attachments
Patch (2.24 KB, patch)
2015-12-03 11:30 PST, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (39.47 KB, patch)
2016-01-05 18:33 PST, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews114 for mac-yosemite (1.58 MB, application/zip)
2016-01-05 19:17 PST, Build Bot
no flags Details
Archive of layout-test-results from ews102 for mac-yosemite (1.47 MB, application/zip)
2016-01-05 19:25 PST, Build Bot
no flags Details
Archive of layout-test-results from ews106 for mac-yosemite-wk2 (1.08 MB, application/zip)
2016-01-05 19:32 PST, Build Bot
no flags Details
Patch (41.32 KB, patch)
2016-01-06 12:16 PST, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Said Abou-Hallawa 2015-12-03 10:47:05 PST
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.
Comment 1 Said Abou-Hallawa 2015-12-03 11:30:55 PST
Created attachment 266538 [details]
Patch
Comment 2 Andreas Kling 2015-12-03 14:02:09 PST
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?
Comment 3 Darin Adler 2015-12-08 09:35:55 PST
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?
Comment 4 Said Abou-Hallawa 2016-01-05 18:33:28 PST
Created attachment 268343 [details]
Patch
Comment 5 Said Abou-Hallawa 2016-01-05 18:35:09 PST
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>
Comment 6 Build Bot 2016-01-05 19:17:23 PST
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
Comment 7 Build Bot 2016-01-05 19:17:26 PST
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
Comment 8 Build Bot 2016-01-05 19:25:53 PST
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
Comment 9 Build Bot 2016-01-05 19:25:56 PST
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
Comment 10 Build Bot 2016-01-05 19:32:22 PST
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
Comment 11 Build Bot 2016-01-05 19:32:26 PST
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
Comment 12 Said Abou-Hallawa 2016-01-06 12:16:27 PST
Created attachment 268386 [details]
Patch
Comment 13 Said Abou-Hallawa 2016-01-06 12:21:23 PST
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.
Comment 14 Chris Vienneau 2016-01-12 11:27:39 PST
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
Comment 15 Said Abou-Hallawa 2016-01-12 11:40:14 PST
(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.
Comment 16 WebKit Commit Bot 2016-01-13 09:04:51 PST
Comment on attachment 268386 [details]
Patch

Clearing flags on attachment: 268386

Committed r194964: <http://trac.webkit.org/changeset/194964>
Comment 17 WebKit Commit Bot 2016-01-13 09:04:55 PST
All reviewed patches have been landed.  Closing bug.
Comment 18 Said Abou-Hallawa 2016-01-13 09:08:22 PST
<rdar://problem/23453491>