Closed
Bug 1183484
Opened 9 years ago
Closed 9 years ago
Leak with document.fonts.entries()
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla42
People
(Reporter: jruderman, Assigned: heycam)
References
Details
(Keywords: memory-leak, regression, testcase)
Attachments
(2 files, 1 obsolete file)
57 bytes,
text/html
|
Details | |
2.91 KB,
patch
|
bzbarsky
:
review+
lmandel
:
approval-mozilla-aurora+
lmandel
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
No description provided.
Comment 1•9 years ago
|
||
Maybe a regression from bug 1161413?
Comment 2•9 years ago
|
||
[Tracking Requested - why for this release]: Leak in new functionality in Firefox 40. Uh, yeah, so.. FontFaceSetIterator is not cycle collected. But it holds a strong ref to the FontFaceSet, which holds a strong ref to the document, and so forth. This is basically a regression from bug 1072101. We need cycle collection here. Andrew, what's the right way to handle that for a non-refcounted and non-wrapper-cached class like this?
Blocks: 1072101
tracking-firefox40:
--- → ?
tracking-firefox41:
--- → ?
tracking-firefox42:
--- → ?
Keywords: regression
Comment 3•9 years ago
|
||
If a class A uniquely owns an instance of class B, and A is CCed and B is not, then you can Traverse/Unlink the strong references of B from A.
Comment 4•9 years ago
|
||
You can put Traverse and Unlink helper methods on B, and call them from the Traverse and Unlink of A.
Assignee | ||
Comment 5•9 years ago
|
||
(In reply to Andrew McCreight [:mccr8] from comment #3) > If a class A uniquely owns an instance of class B, and A is CCed and B is > not, then you can Traverse/Unlink the strong references of B from A. I think instead we have class A uniquely owning an instance of class B, where A is not CCed but B is, with A = FontFaceSetIterator and B = FontFaceSet. FontFaceSetIterator is the one that's not wrapper cached or refcounted. Since there's no way to get from the FontFaceSet to any FontFaceSetIterators created for it, I don't see how we can avoid making FontFaceSetIterator CCed, can we?
What owns FontFaceSetIterator?
Assignee | ||
Comment 7•9 years ago
|
||
The JS wrapper. The methods that produce FontFaceSetIterators are all [NewObject].
If you have an object owned by JS that can itself own cycle collected objects, it must be cycle collected.
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → cam
Status: NEW → ASSIGNED
Assignee | ||
Comment 9•9 years ago
|
||
This is what I'm trying, but it doesn't seem to help the leak. Am I doing the right thing?
Comment 10•9 years ago
|
||
Do you have to change some bindings stuff? I'm not sure how that works, but the JS object that owns this font thing needs to know to trace it.
Comment 11•9 years ago
|
||
> Do you have to change some bindings stuff?
The bindings stuff doesn't do anything with tracing, actually. I was hoping you knew how this stuff gets traced (presumably by xpconnect somehow?). Let's hope Peter knows!
We do have existing DOM classes that use NS_IMPL_CYCLE_COLLECTION_ROOT_NATIVE. But at least the ones I spot-checked are wrappercached, in case that matters.
Comment 12•9 years ago
|
||
For WebIDL, it won't be XPConnect. I'll take a look through the bindings code tomorrow if Peter doesn't get back to us first.
Flags: needinfo?(continuation)
Comment 13•9 years ago
|
||
Oh, I see how this works. The DOMJSClass stores a nsCycleCollectionParticipant* gotten via GetCCParticipant<T>::Get() on the concrete type. In this case the concrete type would be FontFaceSetIterator. The template magic should then end up doing the right thing. Then later CycleCollectedJSRuntime::NoteGCThingXPCOMChildren uses that pointer to call NoteNativeChild. Cameron, did you do a toplevel rebuild after applying your patch, or did you build just in layout/style? I assume you need to rebuild in dom/bindings too for the template bits with SFINAE to notice that now your class is cycle-collected...
Flags: needinfo?(cam)
Assignee | ||
Comment 14•9 years ago
|
||
I did do a top level build. I'll try a clobber...
Flags: needinfo?(cam)
Assignee | ||
Comment 15•9 years ago
|
||
Still leaking. How do I dump out the known CC edges and number of unknown ones again? (It's always so long between needing to do that that I forget how.)
Comment 16•9 years ago
|
||
But I can confirm that applying Cameron's patch and then recompiling still leaks. It leaks because the template argument deduction stuff fails and SFINAE, so we think this class is just not cycle collectible. Making the two CC macros in the header public instead of private makes things work correctly as far as I can tell.
Flags: needinfo?(continuation)
Comment 17•9 years ago
|
||
Oh, sorry, I should have caught that!
Assignee | ||
Comment 18•9 years ago
|
||
Awful, sorry about that! As I pointed out on IRC, if I'd put NS_INLINE_DECL_CYCLE_COLLECTING_NATIVE_REFCOUNTING and NS_DECL_CYCLE_COLLECTION_NATIVE_CLASS the other way around it would've worked since the former includes a public: thingo at its end.
Attachment #8634525 -
Attachment is obsolete: true
Attachment #8634575 -
Flags: review?(bzbarsky)
Comment 19•9 years ago
|
||
Comment on attachment 8634575 [details] [diff] [review] patch r=me
Attachment #8634575 -
Flags: review?(bzbarsky) → review+
Comment 21•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/18e90871e991
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
Comment 22•9 years ago
|
||
Do we want this on branches?
Comment 23•9 years ago
|
||
I think we do, yes. It's low risk, and a new API, so would be better to not have it leak.
Assignee | ||
Comment 24•9 years ago
|
||
Comment on attachment 8634575 [details] [diff] [review] patch Approval Request Comment [Feature/regressing bug #]: bug 1072101 [User impact if declined]: memory leak with certain usage of Font Loading API [Describe test coverage new/current, TreeHerder]: green on m-c, tested locally [Risks and why]: low, small change [String/UUID change made/needed]: N/A While this applies to mozilla-beta too, the API pref is not turned on by default there, so it's less likely someone will run into the leak.
Attachment #8634575 -
Flags: approval-mozilla-beta?
Attachment #8634575 -
Flags: approval-mozilla-aurora?
Comment 25•9 years ago
|
||
(In reply to Cameron McCormack (:heycam) from comment #24) > While this applies to mozilla-beta too, the API pref is not turned on by > default there, so it's less likely someone will run into the leak. I'm marking 40's status as disabled and not tracking for 40. I also take it this means there we don't really need to take the fix in 40. Please correct me if this assumption is wrong.
status-firefox40:
--- → disabled
status-firefox41:
--- → affected
Comment 26•9 years ago
|
||
Comment on attachment 8634575 [details] [diff] [review] patch bz tells me that the risk of this fix is really low. Although not required for 40, it is still preferable to take the fix in case the feature is enabled. Let's take this in beta6. Beta+ Aurora+
Attachment #8634575 -
Flags: approval-mozilla-beta?
Attachment #8634575 -
Flags: approval-mozilla-beta+
Attachment #8634575 -
Flags: approval-mozilla-aurora?
Attachment #8634575 -
Flags: approval-mozilla-aurora+
Updated•9 years ago
|
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•