Closed
Bug 1153628
Opened 10 years ago
Closed 10 years ago
Leak with document.fonts, event, SVG
Categories
(Core :: DOM: Core & HTML, defect)
Tracking
()
RESOLVED
FIXED
mozilla40
Tracking | Status | |
---|---|---|
firefox40 | --- | fixed |
People
(Reporter: jruderman, Assigned: heycam)
Details
(Keywords: memory-leak, testcase)
Attachments
(2 files)
133 bytes,
image/svg+xml
|
Details | |
657 bytes,
patch
|
mccr8
:
review+
|
Details | Diff | Splinter Review |
Leaks nsGlobalWindow & nsDocument.
Is FontFaceSet cycle collected?
Flags: needinfo?(cam)
Assignee | ||
Comment 2•10 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)
Comment 3•10 years ago
|
||
What about the possible cycle between UserFontSet (mUserFontSet) and FontFaceSet?
Comment 4•10 years ago
|
||
FontFace is the root, so there is an unknown edge ->FontFace.
Comment 5•10 years ago
|
||
PresContext doesn't seem to traverse mFontFaceSet.
Comment 6•10 years ago
|
||
but traversing mFontFaceSet there doesn't seem to be enough.
Assignee | ||
Comment 7•10 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•10 years ago
|
||
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)
Comment 9•10 years ago
|
||
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?
Updated•10 years ago
|
Attachment #8591427 -
Flags: review?(continuation)
Assignee | ||
Comment 10•10 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 11•10 years ago
|
||
Comment on attachment 8591427 [details] [diff] [review]
patch
Alright, sounds reasonable enough. Thanks for the explanation.
Attachment #8591427 -
Flags: review+
Assignee | ||
Comment 12•10 years ago
|
||
Thanks for finding the cycle.
https://hg.mozilla.org/integration/mozilla-inbound/rev/5632b533c8c8
Comment 13•10 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•