Bug 66438

Summary: Abandoned Memory: SVGFontElement and Corresponding SVGDocument Never Deconstructed
Product: WebKit Reporter: Joseph Pecoraro <joepeck>
Component: SVGAssignee: Ryosuke Niwa <rniwa>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, bashi, cachobot, darin, d-r, eric, fmalita, inferno, jochen, joepeck, jschuh, mitz, pdr, rniwa, schenney, simon.fraser, thorton, webkit-bug-importer, webkit.review.bot, zherczeg, zimmermann
Priority: P1 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 106716    
Attachments:
Description Flags
[TEST] Test Case
none
[PATCH] Proposed Fix
none
Fixes the leak
none
Refined patch krit: review+

Joseph Pecoraro
Reported 2011-08-17 17:45:36 PDT
Created attachment 104285 [details] [TEST] Test Case It looks like there is a ref/retain cycle between the SVGFontFaceElement and the SVGFontElement. This cycle prevents the SVGDocument, and the corresponding SVG Font data from ever being released. I attached a zip of a an html+svg font test (might be possible to reduce further for those experienced with SVG). What I'm seeing is: 1. CachedFont creates the SVGDocument and assigns it into m_externalSVGDocument. 2. When the CachedFont is evicted from WebCore's MemoryCache the SVGDocument gets its refCount dropped to 0 => removedLastRef 3. Document::removeLastRef (for the SVGDocument) attempts to remove all Nodes, but doesn't delete the SVGFontElement subtree because the SVGFontElement has a ref (held by its containing SVGFontFaceElement). So the node keeps the document alive, and no-one holds a reference to the document, and presumably not the font anymore. A possible fix is to just give the SVGFontFaceElement a weak reference to the SVGFontElement. It doesn't seem to need a full RefPtr, but I'm not familiar with the area. I'll run some tests and propose a patch soon. Steps to reproduce with the attached test: 1. Load the test page 2. Close the test page 3. Cause the CachedFont to be evicted gdb> WebCore::memoryCache()->setDisabled(true); // works like a charm 4. See if the SVGDocument was actually deconstructed You can also use memory analysis tools to see if there were leftover allocations for the SVGElements (Glyphs, etc) that are still alive.
Attachments
[TEST] Test Case (55.06 KB, application/zip)
2011-08-17 17:45 PDT, Joseph Pecoraro 		no flags
Details
[PATCH] Proposed Fix (2.29 KB, patch)
2011-08-17 19:11 PDT, Joseph Pecoraro 		no flags
DetailsFormatted DiffDiff
Fixes the leak (3.56 KB, patch)
2013-01-24 00:48 PST, Ryosuke Niwa 		no flags
DetailsFormatted DiffDiff
Refined patch (3.90 KB, patch)
2013-01-24 11:13 PST, Ryosuke Niwa 		krit: review+
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Joseph Pecoraro
Comment 1 2011-08-17 19:11:25 PDT
Created attachment 104298 [details] [PATCH] Proposed Fix This fixes the problem and didn't cause any tests to crash. I can't think of a scenario where someone would like to keep an SVGFontFace element around without its corresponding SVGFontElement. Maybe we can add some kind of ASSERTs to catch something like that early.
Joseph Pecoraro
Comment 2 2011-08-18 22:20:39 PDT
Looks like this the RefPtr causing the cycle was added in: <http://webkit.org/b/53270> We should hold RefPtrs to SVG font faces Eric, Justin, do you have any comments on this patch? I don't have access to bug 53270 because it was marked as security. However, even with this patch which does not use RefPtr the test case added with that change seems to pass: svg/custom/use-multiple-on-nested-disallowed-font.html Is it safe to make this change?
Eric Seidel (no email)
Comment 3 2011-08-19 10:13:06 PDT
I've added you to the original bug.
Joseph Pecoraro
Comment 4 2011-08-19 10:38:16 PDT
(In reply to comment #3) > I've added you to the original bug. Yah, the original bug doesn't have much, but its a start. The original test case passes now without the RefPtr so I wonder how important that really was. I'm hoping an SVG expert will comment. The last thing I want to do is reopen a security bug!
Eric Seidel (no email)
Comment 5 2011-08-19 10:53:26 PDT
Comment on attachment 104298 [details] [PATCH] Proposed Fix The reality is you're the expert here, as you looked at this code most recently. I suspect either Nico or I wrote this code, but I certainly don't remember. Nico might.
Abhishek Arya
Comment 6 2011-08-19 20:29:26 PDT
(In reply to comment #4) > (In reply to comment #3) > > I've added you to the original bug. > > Yah, the original bug doesn't have much, but its a start. The original test > case passes now without the RefPtr so I wonder how important that really was. > I'm hoping an SVG expert will comment. The last thing I want to do is reopen > a security bug! Did you run the testcase with libgmalloc on ?? It should definitely crash with that. Also try running with old-run-webkit-tests --repeat-each 1000 ? We should have waited for Justin's response than rushing away to revert a security fix.
Joseph Pecoraro
Comment 7 2011-08-19 22:24:59 PDT
I plan on testing a bunch. Esp since Eric denoted me as An expert now :P. I discovered the security bug after I put this up for review and annotated the file to check The history. I will give your suggestions a shot, I don't Plan on landing anytime soon so if anyone investigates In the meantime that is great.
Joseph Pecoraro
Comment 8 2011-09-20 13:29:15 PDT
> Did you run the testcase with libgmalloc on ?? It should definitely crash with that. Also > try running with old-run-webkit-tests --repeat-each 1000 ? We should have waited for > Justin's response than rushing away to revert a security fix. Abhishek Arya, I tried this on my machine (10.7 Lion) and I didn't encounter a crash (with my patch and libgmalloc). If you are still concerned could you try on your machine and let me know if it still crashes? shell> export MALLOC_FILL_SPACE=YES shell> ./Tools/Scripts/old-run-webkit-tests --debug --guard-malloc --repeat-each 1000 svg/custom/use-multiple-on-nested-disallowed-font.html ... succeeded ... shell> ./Tools/Scripts/old-run-webkit-tests --debug --guard-malloc svg 1846 test cases (99%) succeeded 4 test cases (<1%) had incorrect layout <-- negligable I think 3 test cases (<1%) had stderr output I still plan to investigate this some more, so I won't be landing yet.
Justin Schuh
Comment 9 2011-09-20 16:10:08 PDT
Yeah, my first patch does create a cycle (sorry about that). However, I don't see any changes to the calling code that would prevent the stale pointer from being left behind. I expect that the layout test I created just doesn't tickle a crash anymore due to changes in the use element. I think the correct way to fix this is for the SVGFontElement to have a set of all the SVGFontFaceElements that point to it. On removal of either element it would just clear the respective back pointer.
Eric Seidel (no email)
Comment 10 2011-12-21 14:30:03 PST
Attachment 104298 [details] was posted by a committer and has review+, assigning to Joseph Pecoraro for commit.
Justin Schuh
Comment 11 2011-12-21 14:45:20 PST
(In reply to comment #10) > Attachment 104298 [details] was posted by a committer and has review+, assigning to Joseph Pecoraro for commit. That's just going to reopen a security bug. I explained in comment #9 why this isn't the correct fix.
Eric Seidel (no email)
Comment 12 2011-12-21 14:51:56 PST
Comment on attachment 104298 [details] [PATCH] Proposed Fix That sounds like I should mark this r- then... meaning we still have this ref-cycle.
Justin Schuh
Comment 13 2011-12-21 15:00:14 PST
(In reply to comment #12) > (From update of attachment 104298 [details]) > That sounds like I should mark this r- then... meaning we still have this ref-cycle. Yeah, as a non-reviewer I'm not comfortable marking something r-. I'll try to make time over the holidays to put together the fix. It's easy enough, but I always seem to have higher priority issues on my list.
Joseph Pecoraro
Comment 14 2011-12-21 15:03:41 PST
(In reply to comment #13) > I'll try to make time over the holidays to put together the fix. It's easy > enough, but I always seem to have higher priority issues on my list. That would be great. Feel free to take the bug and "assign" it to yourself.
Justin Schuh
Comment 15 2012-04-04 17:15:56 PDT
*** Bug 82445 has been marked as a duplicate of this bug. ***
Justin Schuh
Comment 16 2012-04-04 17:24:25 PDT
Stephen, this fell off my plate a while ago. Do you mind taking a look at it?
Tim Horton
Comment 17 2012-04-20 15:44:30 PDT
*** Bug 83197 has been marked as a duplicate of this bug. ***
Stephen Chenney
Comment 18 2013-01-17 08:58:29 PST
*** Bug 106709 has been marked as a duplicate of this bug. ***
Ryosuke Niwa
Comment 19 2013-01-23 21:46:57 PST
Why can't we just clear m_fontElement in SVGFontFaceElement::removedFrom instead?
Ryosuke Niwa
Comment 20 2013-01-24 00:48:28 PST
Created attachment 184427 [details] Fixes the leak
Eric Seidel (no email)
Comment 21 2013-01-24 00:49:28 PST
Comment on attachment 184427 [details] Fixes the leak We have a WeakPtr class now. :) But it's a thread-safe weakptr, and possibly more than you need for this case.
Stephen Chenney
Comment 22 2013-01-24 06:25:15 PST
Comment on attachment 184427 [details] Fixes the leak View in context: https://bugs.webkit.org/attachment.cgi?id=184427&action=review Apart from my concern above, this does seem like the right fix. I don't see how the parentNode() can be deleted with removedFrom being called on the child, except in the case of fast document destruction when everything is being deleted anyway. > Source/WebCore/svg/SVGFontFaceElement.cpp:337 > + m_fontElement = 0; I don't understand why you are not clearing the pointer when it is equal to the parentNode(). As far as I can tell, the m_fontElement pointer must be the parent, because the only place I can see it set is when it is set to the parent: bool describesParentFont = parentNode()->hasTagName(SVGNames::fontTag); if (describesParentFont) { m_fontElement = static_cast<SVGFontElement*>(parentNode()); So why not always clear the pointer in removedFrom? You could even assert that the pointer is either null or equal to the parentNode() at the time removedFrom is called.
Stephen Chenney
Comment 23 2013-01-24 06:26:33 PST
Sorry, concern "above" was actually "below" (In reply to comment #22) > (From update of attachment 184427 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=184427&action=review > > Apart from my concern below, this does seem like the right fix. I don't see how the parentNode() can be deleted with removedFrom being called on the child, except in the case of fast document destruction when everything is being deleted anyway. > > > Source/WebCore/svg/SVGFontFaceElement.cpp:337 > > + m_fontElement = 0; > > I don't understand why you are not clearing the pointer when it is equal to the parentNode(). As far as I can tell, the m_fontElement pointer must be the parent, because the only place I can see it set is when it is set to the parent: > > bool describesParentFont = parentNode()->hasTagName(SVGNames::fontTag); > if (describesParentFont) { > m_fontElement = static_cast<SVGFontElement*>(parentNode()); > > So why not always clear the pointer in removedFrom? You could even assert that the pointer is either null or equal to the parentNode() at the time removedFrom is called.
Ryosuke Niwa
Comment 24 2013-01-24 10:31:35 PST
(In reply to comment #22) > > Source/WebCore/svg/SVGFontFaceElement.cpp:337 > > + m_fontElement = 0; > > I don't understand why you are not clearing the pointer when it is equal to the parentNode(). As far as I can tell, the m_fontElement pointer must be the parent, because the only place I can see it set is when it is set to the parent: Sorry, I should have written as "m_fontElement && !parentNode()" but it's probably even better to do: if (rootParent->inDocument()) { m_fontElement = 0; … } else ASSERT(!m_fontElement); given the semantics. Let me try that. > So why not always clear the pointer in removedFrom? You could even assert that the pointer is either null or equal to the parentNode() at the time removedFrom is called. removedFrom might be called upon a node that's a part of a detached tree. So parentNode() is not always 0. But given that we're already calling unregisterSVGFontFaceElement when that happens, we can simply clear m_fontElement in that case as well. Furthermore, when the node is not in the document, we should never have non-zero m_fontElement so we should probably assert that instead of assigning 0.
Ryosuke Niwa
Comment 25 2013-01-24 11:13:23 PST
Created attachment 184536 [details] Refined patch
Dirk Schulze
Comment 26 2013-01-24 11:16:23 PST
Comment on attachment 184536 [details] Refined patch LGTM. r=me
Ryosuke Niwa
Comment 27 2013-01-24 11:19:43 PST
Committed r140698: <http://trac.webkit.org/changeset/140698>
Radar WebKit Bug Importer
Comment 28 2013-01-24 11:20:58 PST
<rdar://problem/13079425>
Note You need to log in before you can comment on or make changes to this bug.