Last Comment Bug 653497 - Once bug 335998 is fixed, SVGPathDataAndOwner::mElement leaks documents
: Once bug 335998 is fixed, SVGPathDataAndOwner::mElement leaks documents
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: SVG (show other bugs)
: unspecified
: x86_64 Linux
: -- normal (vote)
: ---
Assigned To: Olli Pettay [:smaug] (vacation Aug 25-28)
:
Mentors:
Depends on:
Blocks: strongparent 515116
  Show dependency treegraph
 
Reported: 2011-04-28 11:03 PDT by Olli Pettay [:smaug] (vacation Aug 25-28)
Modified: 2011-04-29 06:12 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (6.30 KB, patch)
2011-04-28 13:09 PDT, Olli Pettay [:smaug] (vacation Aug 25-28)
dholbert: review+
jwatt: feedback+
Details | Diff | Splinter Review

Description Olli Pettay [:smaug] (vacation Aug 25-28) 2011-04-28 11:03:40 PDT
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] (vacation Aug 25-28) 2011-04-28 11:25:02 PDT
Looks like SVGPointListAndInfo has similar problem.
Comment 2 Daniel Holbert [:dholbert] 2011-04-28 11:49:14 PDT
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] (vacation Aug 25-28) 2011-04-28 13:09:59 PDT
Created attachment 528940 [details] [diff] [review]
patch

Hopefully this doesn't affect performance too badly.
Comment 4 Daniel Holbert [:dholbert] 2011-04-28 14:46:37 PDT
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] 2011-04-28 14:58:40 PDT
(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] (back in October - email directly if necessary) 2011-04-28 15:31:51 PDT
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] 2011-04-28 15:33:49 PDT
(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] (vacation Aug 25-28) 2011-04-29 06:12:33 PDT
http://hg.mozilla.org/mozilla-central/rev/b15803d8aee8

Note You need to log in before you can comment on or make changes to this bug.