Is FontFaceSet cycle collected?
It is. Does it looks right? https://hg.mozilla.org/mozilla-central/file/0a46652bd992/layout/style/FontFaceSet.cpp#l45
Flags: needinfo?(cam) → needinfo?(khuey)
What about the possible cycle between UserFontSet (mUserFontSet) and FontFaceSet?
FontFace is the root, so there is an unknown edge ->FontFace.
PresContext doesn't seem to traverse mFontFaceSet.
but traversing mFontFaceSet there doesn't seem to be enough.
(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.
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.
Assignee: nobody → cam
Status: NEW → ASSIGNED
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?
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 on attachment 8591427 [details] [diff] [review] patch Alright, sounds reasonable enough. Thanks for the explanation.
Attachment #8591427 - Flags: review+
Thanks for finding the cycle. https://hg.mozilla.org/integration/mozilla-inbound/rev/5632b533c8c8
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
status-firefox40: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
You need to log in before you can comment on or make changes to this bug.