Closed Bug 886213 Opened 6 years ago Closed 6 years ago
Leak getting navigator on removed iframe
My DOM fuzzer will ignore leaks involving frames or navigator until this is fixed.
159 // We seem to manually break cycles through most of our members in Invalidate. 160 // That said, if those members get set _after_ the window calls Invalidate on 161 // us, then what? Now we know what.
(In reply to Kyle Huey [:khuey] (email@example.com) from comment #1) > 159 // We seem to manually break cycles through most of our members in > Invalidate. > 160 // That said, if those members get set _after_ the window calls > Invalidate on > 161 // us, then what? > > Now we know what. Can you further explain how this may impact our users ?Is the fuzzer being used to catch regressions,leaks that we want to avoid shipping in a release ?
Yeah, there's a memory leak here that we should avoid shipping to users.
Sadly the obvious "cycle collect every field!" didn't fix this.
Okay, so the actual fix was easier: nsGlobalWindow wasn't traversing or unlinking mNavigator! But we should probably do the rest of this horror, too. I can try putting together some tests that leak for at least some of them.
Comment on attachment 768101 [details] [diff] [review] cycle collect more better Review of attachment 768101 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/base/Navigator.cpp @@ +160,5 @@ > + // mMimeTypes isn't cycle collected > + // mPlugins isn't cycle collected > + NS_IMPL_CYCLE_COLLECTION_UNLINK(mGeolocation) > + NS_IMPL_CYCLE_COLLECTION_UNLINK(mNotification) > + NS_IMPL_CYCLE_COLLECTION_UNLINK(mBatteryManager) I look at code like this and think "what could possibly go wrong?" :/
Don't we want to call Disconnect or some such when unlinking those member variables
(In reply to Nicholas Nethercote [:njn] from comment #6) > I look at code like this and think "what could possibly go wrong?" :/ It is quite awful. We have macros that would let you just list off the fields once, but this class has a bunch of conditional defines that keep that from working. (In reply to Olli Pettay [:smaug] from comment #7) > Don't we want to call Disconnect or some such when unlinking those member > variables Yeah, I should probably just make Unlink call Disconnect, or something.
I couldn't actually come up with a test case that required the changes from Navigator.cpp here, so maybe I don't need to do that. I can remove it if you think that's unnecessary complexity. I don't know anything about how Invalidate works.
Comment on attachment 768434 [details] [diff] [review] cycle collect more better >+NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN(Navigator) >+ // mMimeTypes isn't cycle collected >+ // mPlugins isn't cycle collected >+ NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mGeolocation) >+ NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mNotification) >+ NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mBatteryManager) >+ // mPowerManager isn't cycle collected oh dear. Could you file a followup to make it CCable.
Attachment #768434 - Flags: review?(bugs) → review+
mPowerManager? It doesn't look like it needs to be CCed. It looks like it is a weak pointer to a window plus an array of a class that has no fields, only a single method.
Okay, I guess there are a few implementations of wakelocklistener but none of them seem to hold anything interesting.
wakelocklistener can be implemented in JS
Ah, right. I filed bug 888389 for that.
https://hg.mozilla.org/integration/mozilla-inbound/rev/14b27ee025b8 try run was ok https://tbpl.mozilla.org/?tree=Try&rev=7b682ee7adf7
OS: Mac OS X → All
Hardware: x86_64 → All
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
Comment on attachment 768434 [details] [diff] [review] cycle collect more better [Approval Request Comment] Bug caused by (feature/regressing bug #): bug 885171 User impact if declined: leaks in some situations Testing completed (on m-c, etc.): m-c for a few days Risk to taking this patch (and alternatives if risky): low String or IDL/UUID changes made by this patch: none
Attachment #768434 - Flags: approval-mozilla-aurora?
(In reply to Jesse Ruderman from comment #0) > Created attachment 766527 [details] > testcase > > My DOM fuzzer will ignore leaks involving frames or navigator until this is > fixed. Hi Jesse, can you confirm this is fixed for you now ? Thanks !
Attachment #768434 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Yes, this is fixed for me. But note that bug 860482 also prevents me from testing for leaks involving frames.
Looks like this landed with tests, can someone please confirm?
Assuming this doesn't need QA.
Whiteboard: [MemShrink:P3] → [MemShrink:P3][qa-]
You need to log in before you can comment on or make changes to this bug.