Leak with document.fonts, event, SVG

RESOLVED FIXED in Firefox 40

Status

()

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: jruderman, Assigned: heycam)

Tracking

(Blocks: 1 bug, {memory-leak, testcase})

Trunk
mozilla40
x86_64
Mac OS X
memory-leak, testcase
Points:
---

Firefox Tracking Flags

(firefox40 fixed)

Details

Attachments

(2 attachments)

(Reporter)

Description

4 years ago
Created attachment 8591342 [details]
testcase

Leaks nsGlobalWindow & nsDocument.
Is FontFaceSet cycle collected?
Flags: needinfo?(cam)
(Assignee)

Comment 2

4 years ago
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.
(Assignee)

Comment 7

4 years ago
(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.
(Assignee)

Comment 8

4 years ago
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
Flags: needinfo?(khuey)
Attachment #8591427 - Flags: review?(continuation)
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?
(Assignee)

Comment 10

4 years ago
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+
https://hg.mozilla.org/mozilla-central/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.