Bug 151810

Summary: Reference cycle between SVGPathElement and SVGPathSegWithContext leaks Document
Product: WebKit Reporter: Said Abou-Hallawa <sabouhallawa>
Component: SVGAssignee: Said Abou-Hallawa <sabouhallawa>
Status: RESOLVED FIXED    
Severity: Normal CC: buildbot, chris.vno, commit-queue, dino, kling, nakim, nikos.andronikos, rniwa, zimmermann
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=152759
Attachments:
Description Flags
Patch
none
Patch
none
Archive of layout-test-results from ews114 for mac-yosemite
none
Archive of layout-test-results from ews102 for mac-yosemite
none
Archive of layout-test-results from ews106 for mac-yosemite-wk2
none
Patch none

Said Abou-Hallawa
Reported 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.
Attachments
Patch (2.24 KB, patch)
2015-12-03 11:30 PST, Said Abou-Hallawa 		no flags
DetailsFormatted DiffDiff
Patch (39.47 KB, patch)
2016-01-05 18:33 PST, Said Abou-Hallawa 		no flags
DetailsFormatted DiffDiff
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
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Said Abou-Hallawa
Comment 1 2015-12-03 11:30:55 PST
Created attachment 266538 [details] Patch
Andreas Kling
Comment 2 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?
Darin Adler
Comment 3 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?
Said Abou-Hallawa
Comment 4 2016-01-05 18:33:28 PST
Created attachment 268343 [details] Patch
Said Abou-Hallawa
Comment 5 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>
Build Bot
Comment 6 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
Build Bot
Comment 7 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
Build Bot
Comment 8 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
Build Bot
Comment 9 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
Build Bot
Comment 10 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
Build Bot
Comment 11 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
Said Abou-Hallawa
Comment 12 2016-01-06 12:16:27 PST
Created attachment 268386 [details] Patch
Said Abou-Hallawa
Comment 13 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.
Chris Vienneau
Comment 14 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
Said Abou-Hallawa
Comment 15 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.
WebKit Commit Bot
Comment 16 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>
WebKit Commit Bot
Comment 17 2016-01-13 09:04:55 PST
All reviewed patches have been landed. Closing bug.
Said Abou-Hallawa
Comment 18 2016-01-13 09:08:22 PST
<rdar://problem/23453491>
Note You need to log in before you can comment on or make changes to this bug.