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?
Attachment #8591427 - Flags: review?(continuation)
(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.