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




6 years ago
6 years ago


(Reporter: smaug, Assigned: smaug)


Firefox Tracking Flags

(Not tracked)



(1 attachment)



6 years ago
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

6 years ago
Looks like SVGPointListAndInfo has similar problem.


6 years ago
Assignee: nobody → Olli.Pettay
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

6 years ago
Created attachment 528940 [details] [diff] [review]

Hopefully this doesn't affect performance too badly.
Attachment #528940 - Flags: review?(dholbert)
Comment on attachment 528940 [details] [diff] [review]

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

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.
Attachment #528940 - Flags: review?(dholbert)
Attachment #528940 - Flags: review+
Attachment #528940 - Flags: feedback?(jwatt)
(In reply to comment #4)
> These probably both belong in a followup bug, though, which I'm happy to file.

Filed Bug 653571.
Comment on attachment 528940 [details] [diff] [review]

> 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 )
Attachment #528940 - Flags: feedback?(jwatt) → feedback+
(In reply to comment #6)
> I'd like us to keep a comment here (for all classes) noting:

Yeah, agreed.

Comment 8

6 years ago
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.