1153628 - Leak with document.fonts, event, SVG

Status: RESOLVED FIXED

Description

[Jesse Ruderman]

Created attachment 8591342 [details]
testcase

Leaks nsGlobalWindow & nsDocument.

Comment 1

[Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary)]

Is FontFaceSet cycle collected?

Comment 2

[Cameron McCormack (:heycam)]

It is.  Does it looks right?  https://hg.mozilla.org/mozilla-central/file/0a46652bd992/layout/style/FontFaceSet.cpp#l45

Comment 3

[Andrew McCreight [:mccr8] (PTO-ish 12-22 to -30)]

What about the possible cycle between UserFontSet (mUserFontSet) and FontFaceSet?

Comment 4

[Olli Pettay [:smaug] (high review load)]

FontFace is the root, so there is an unknown edge ->FontFace.

Comment 5

[Olli Pettay [:smaug] (high review load)]

PresContext doesn't seem to traverse mFontFaceSet.

Comment 6

[Olli Pettay [:smaug] (high review load)]

but traversing mFontFaceSet there doesn't seem to be enough.

Comment 7

[Cameron McCormack (:heycam)]

(In reply to Andrew McCreight [:mccr8] from comment #3)
> What about the possible cycle between UserFontSet (mUserFontSet) and
> FontFaceSet?

Yes I think this is it.

Comment 8

[Cameron McCormack (:heycam)]

Created attachment 8591427 [details] [diff] [review]
patch

Break the strong reference from the FontFaceSet::UserFontSet to the FontFaceSet when the pres context is destroying the FontFaceSet.

Comment 9

[Andrew McCreight [:mccr8] (PTO-ish 12-22 to -30)]

Can anybody else take a strong reference to the mUserFontSet?  Is there some guarantee such that this DestroyUserFont() is always called and you are okay with stuff being held alive always until then?

Comment 10

[Cameron McCormack (:heycam)]

The user font set object should live until the document loses its pres shell, at least.  We should always call DestroyUserFontSet, since we do that from within nsPresContext::SetShell.  (Although that makes me wonder whether we really should be keeping the same FontFaceSet object on document.fonts if we twiddle its iframe to display:none and back again.)

I see two other strong references to gfxUserFontSet (which is what FontFaceSet::UserFontSet inherits from): on gfxFontGroup and gfxFcFontSet (which lives inside a gfxPangoFontGroup).  A gfxFontGroup can be held on to by a CanvasRenderingContext2D, so that could make it outlive the document's pres shell.  We still would have called DestroyUserFontSet though.

Comment 11

[Andrew McCreight [:mccr8] (PTO-ish 12-22 to -30)]

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

Alright, sounds reasonable enough.  Thanks for the explanation.

Comment 12

[Cameron McCormack (:heycam)]

Thanks for finding the cycle.

https://hg.mozilla.org/integration/mozilla-inbound/rev/5632b533c8c8

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5632b533c8c8