886213 - Leak getting navigator on removed iframe

Status: RESOLVED FIXED

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

Description

[Jesse Ruderman]

Attachment: testcase

My DOM fuzzer will ignore leaks involving frames or navigator until this is fixed.

Comment 1

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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.

Comment 2

[bhavana bajaj [:bajaj]]

(In reply to Kyle Huey [:khuey] ([e-mail]) 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 ?

Comment 3

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Yeah, there's a memory leak here that we should avoid shipping to users.

Comment 4

[Andrew McCreight [:mccr8]]

Sadly the obvious "cycle collect every field!" didn't fix this.

Comment 5

[Andrew McCreight [:mccr8]]

Attachment: cycle collect more better

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 6

[Nicholas Nethercote [inactive]]

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?"  :/

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Don't we want to call Disconnect or some such when unlinking those member variables

Comment 8

[Andrew McCreight [:mccr8]]

(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.

Comment 9

[Andrew McCreight [:mccr8]]

Attachment: cycle collect more better

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 10

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 11

[Andrew McCreight [:mccr8]]

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.

Comment 12

[Andrew McCreight [:mccr8]]

Okay, I guess there are a few implementations of wakelocklistener but none of them seem to hold anything interesting.

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

wakelocklistener can be implemented in JS

Comment 14

[Andrew McCreight [:mccr8]]

Ah, right.  I filed bug 888389 for that.

Comment 15

[Andrew McCreight [:mccr8]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/14b27ee025b8

try run was ok https://tbpl.mozilla.org/?tree=Try&rev=7b682ee7adf7

Comment 16

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/14b27ee025b8

Comment 17

[Andrew McCreight [:mccr8]]

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

Comment 18

[bhavana bajaj [:bajaj]]

(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 !

Comment 19

[Jesse Ruderman]

Yes, this is fixed for me.  But note that bug 860482 also prevents me from testing for leaks involving frames.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/605b49d10be6

Comment 21

[u279076]

Looks like this landed with tests, can someone please confirm?

Comment 22

[u279076]

Assuming this doesn't need QA.