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.
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.
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?
I've added you to the original bug.
(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!
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.
(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.
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.
> 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.
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.
Attachment 104298 [details] was posted by a committer and has review+, assigning to Joseph Pecoraro for commit.
(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.
Comment on attachment 104298 [details] [PATCH] Proposed Fix That sounds like I should mark this r- then... meaning we still have this ref-cycle.
(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.
(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.
*** Bug 82445 has been marked as a duplicate of this bug. ***
Stephen, this fell off my plate a while ago. Do you mind taking a look at it?
*** Bug 83197 has been marked as a duplicate of this bug. ***
*** Bug 106709 has been marked as a duplicate of this bug. ***
Why can't we just clear m_fontElement in SVGFontFaceElement::removedFrom instead?
Created attachment 184427 [details] Fixes the leak
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.
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.
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.
(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.
Created attachment 184536 [details] Refined patch
Comment on attachment 184536 [details] Refined patch LGTM. r=me
Committed r140698: <http://trac.webkit.org/changeset/140698>
<rdar://problem/13079425>