Closed Bug 1183484 Opened 4 years ago Closed 4 years ago

Leak with document.fonts.entries()

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox40 - fixed
firefox41 + fixed
firefox42 + fixed

People

(Reporter: jruderman, Assigned: heycam)

References

(Blocks 1 open bug)

Details

(Keywords: memory-leak, regression, testcase)

Attachments

(2 files, 1 obsolete file)

Attached file testcase
No description provided.
Maybe a regression from bug 1161413?
[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
Keywords: regression
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.
You can put Traverse and Unlink helper methods on B, and call them from the Traverse and Unlink of A.
(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?
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: nobody → cam
Status: NEW → ASSIGNED
Attached patch patch v0.1 (obsolete) — Splinter Review
This is what I'm trying, but it doesn't seem to help the leak.  Am I doing the right thing?
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.
> 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.
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)
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)
I did do a top level build.  I'll try a clobber...
Flags: needinfo?(cam)
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.)
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)
Oh, sorry, I should have caught that!
Attached patch patchSplinter Review
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 on attachment 8634575 [details] [diff] [review]
patch

r=me
Attachment #8634575 - Flags: review?(bzbarsky) → review+
https://hg.mozilla.org/mozilla-central/rev/18e90871e991
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
Do we want this on branches?
I think we do, yes.  It's low risk, and a new API, so would be better to not have it leak.
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?
(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.
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+
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.