Bug 66438 – Abandoned Memory: SVGFontElement and Corresponding SVGDocument Never Deconstructed

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

[Joseph Pecoraro]

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.

[Joseph Pecoraro]

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]

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)]

I've added you to the original bug.

[Joseph Pecoraro]

(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 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]

(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]

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]

> 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]

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)]

Attachment 104298 [details] was posted by a committer and has review+, assigning to Joseph Pecoraro for commit.

[Justin Schuh]

(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 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]

(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]

(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]

*** Bug 82445 has been marked as a duplicate of this bug. ***

[Justin Schuh]

Stephen, this fell off my plate a while ago. Do you mind taking a look at it?

[Tim Horton]

*** Bug 83197 has been marked as a duplicate of this bug. ***

[Stephen Chenney]

*** Bug 106709 has been marked as a duplicate of this bug. ***

[Ryosuke Niwa]

Why can't we just clear m_fontElement in SVGFontFaceElement::removedFrom instead?

[Ryosuke Niwa]

Created attachment 184427 [details]
Fixes the leak

[Eric Seidel (no email)]

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 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]

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]

(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]

Created attachment 184536 [details]
Refined patch

[Dirk Schulze]

Comment on attachment 184536 [details]
Refined patch

LGTM. r=me

[Ryosuke Niwa]

Committed r140698: <http://trac.webkit.org/changeset/140698>

[Radar WebKit Bug Importer]

<rdar://problem/13079425>