Closed Bug 886213 Opened 6 years ago Closed 6 years ago

Leak getting navigator on removed iframe

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla25
Tracking Status
firefox23 --- unaffected
firefox24 + fixed
firefox25 --- fixed

People

(Reporter: jruderman, Assigned: mccr8)

References

(Blocks 2 open bugs)

Details

(Keywords: memory-leak, regression, testcase, Whiteboard: [MemShrink:P3][qa-])

Attachments

(2 files, 1 obsolete file)

Attached file testcase
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.
Whiteboard: [MemShrink] → [MemShrink:P3]
(In reply to Kyle Huey [:khuey] (khuey@mozilla.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.
Assignee: nobody → continuation
Sadly the obvious "cycle collect every field!" didn't fix this.
Attached patch cycle collect more better (obsolete) — Splinter Review
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.
Attachment #768101 - Attachment is obsolete: true
Attachment #768434 - Flags: review?(bugs)
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/mozilla-central/rev/14b27ee025b8
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?
Flags: in-testsuite?
Flags: in-testsuite? → in-testsuite+
Assuming this doesn't need QA.
Whiteboard: [MemShrink:P3] → [MemShrink:P3][qa-]
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.