474041 - [FIX]Leak with two elements whose ID matches an <svg:use xlink:href>

Status: RESOLVED FIXED

(Core :: SVG, defect, P3)

Description

[Jesse Ruderman]

Attachment: testcase

Steps to reproduce:
1. Run Firefox debug with trace-refcnt enabled (e.g. XPCOM_MEM_LEAK_LOG=2).
2. Load the testcase.
3. Quit Firefox.

Result: trace-refcnt reports that a bunch of stuff leaked, including nsGenericElements, for a total of 221907 bytes of instrumented objects.

Comment 1

[Jesse Ruderman]

Attachment: trace-refcnt leak log

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: Fix

No tests included yet because I haven't figured out the best way to test this reliably.

What I see happen is that we do CreateAnonymousContent on the <use> twice in this testcase.  The first time presumably during initial load, the second time when we move it before the first rect.  I'm not sure why we don't do it the second time we move the <use>; I need to dig some more into that.

In any case, the second time we're in CreateAnonymousContent we call LookupHref(), which calls nsReferencedElement::Reset, which calls Unlink().  There is an mPendingNotification triggered by the same DOM mutation that moved the <use>, so we Clear() it but do NOT drop it (without this patch).  Now we go ahead and set up our new anonymous content.  Then when the <use> is moved again, we end up calling nsReferencedElement::Observe for the change in which content maps to the given id.  Now mPendingNotification is non-null, so we give it a reference to the new content (in this case the outer rect).  But it has a null mOwner, so it'll never drop the ref to this content (and in fact it might have Run() already; since mOwner is null it can't tell the owner to drop it).  So now the nsReferencedElement holds a ref to the outer rect, the outer rect holds a ref to the <use>, and we have a reference cycle.  Since mPendingNotification doesn't participate in cycle collection, we never collect the cycle.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Oh, and we might want to take this in 1.9.1.

Comment 4

[Boris Zbarsky [:bzbarsky]]

> I'm not sure why we don't do it the second time we move the <use>

Because <rect> gets a leaf frame, so no frame construction for the <use> at that point.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Pushed http://hg.mozilla.org/mozilla-central/rev/6f1fd905afff and http://hg.mozilla.org/releases/mozilla-1.9.1/rev/5da591c58078.