653497 - Once bug 335998 is fixed, SVGPathDataAndOwner::mElement leaks documents

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Olli Pettay [:smaug][[e-mail]]]

Cycle collector doesn't know about SVGPathDataAndOwner::mElement, nor it is 
manually released. Once elements keep their parent and ownerDocument alive, 
SVGPathDataAndOwner::mElement causes the whole document to be leaked.

(This might be the last leak before the patch for bug 335998
 can run through the whole mochitest-plain without leaks)

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

Looks like SVGPointListAndInfo has similar problem.

Comment 2

[Daniel Holbert [:dholbert]]

Per bug 515116 comment 16, I don't think we *really* need any special pointer here, since nsSMILTargetIdentifier retains an owning pointer that will keep our Element alive long enough to prevent us from constructing a new one at the exact same address and fooling the element-equality-check.

However, raw pointers are still a bit scary, and it's probably best not to assume that the above will hold true forever.  So, smaug suggests in IRC that we replace the nsRefPtr with a nsWeakPtr (which won't keep the element alive, and which will be cleared when the element dies), and I agree that that's better than what we've got now.

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Attachment: patch

Hopefully this doesn't affect performance too badly.

Comment 4

[Daniel Holbert [:dholbert]]

Comment on attachment 528940 [details] [diff] [review]
patch

Looks good to me!  I'd like to get jwatt's thoughts on this change too, though, since he wrote this code.

MUSING
======
So, we should never actually end up doing math with one of these SVGXxxListAndYYY objects whose element has died, since these objects just last the length of a sample, except for ones used as a cached base-value (which only use for comparison, not for doing math).

So, this means that SVG*SMILType.cpp Add/Interpolate/ComputeDistance methods can assume that their Element() calls aren't returning null due to cleared nsWeakPtrs.

However, for clarity, we should probably
 (a) add IsIdentity() methods in the other SVGXxxListAndYYY classes, and
 (b) add some sanity-checking to Element() methods to be sure that, for nsWeakPtrs that have been initialized, they always have a non-null target.

These probably both belong in a followup bug, though, which I'm happy to file.

Comment 5

[Daniel Holbert [:dholbert]]

(In reply to comment #4)
> These probably both belong in a followup bug, though, which I'm happy to file.

Filed Bug 653571.

Comment 6

[Jonathan Watt [:jwatt]]

Comment on attachment 528940 [details] [diff] [review]
patch

> private:
>-  // We must keep a strong reference to our element because we may belong to a
>-  // cached baseVal nsSMILValue. See the comments starting at:
>-  // https://bugzilla.mozilla.org/show_bug.cgi?id=515116#c15
>-  nsRefPtr<nsSVGElement> mElement;
>+  nsWeakPtr mElement;

I'd like us to keep a comment here (for all classes) noting:

 * that we may be long lived because we belong to a cached baseVal nsSMILValue
 * why we're using a weak reference
 * the "our element died but another happened to get constructed at the same location" problem
   (or at least keep the link to https://bugzilla.mozilla.org/show_bug.cgi?id=515116#c15 )

Comment 7

[Daniel Holbert [:dholbert]]

(In reply to comment #6)
> I'd like us to keep a comment here (for all classes) noting:

Yeah, agreed.

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

http://hg.mozilla.org/mozilla-central/rev/b15803d8aee8