Closed Bug 1153628 Opened 10 years ago Closed 10 years ago

Leak with document.fonts, event, SVG

Categories

(Core :: DOM: Core & HTML, defect)

x86_64
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox40 --- fixed

People

(Reporter: jruderman, Assigned: heycam)

Details

(Keywords: memory-leak, testcase)

Attachments

(2 files)

Attached image testcase
Leaks nsGlobalWindow & nsDocument.
Is FontFaceSet cycle collected?
Flags: needinfo?(cam)
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.
Attached patch patchSplinter Review
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)
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+
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: