1001134 - Gamepad service leaks windows if a page goes into bfcache

Status: RESOLVED FIXED

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

Description

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

Patch coming, I hope.

Comment 1

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

Attachment: patch

This seems to help after all.

Comment 2

[(not currently active) Ted Mielczarek]

Comment on attachment 8412175 [details] [diff] [review]
patch

Review of attachment 8412175 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry. :-/

Comment 3

[Andrew McCreight [:mccr8]]

Comment on attachment 8412175 [details] [diff] [review]
patch

Review of attachment 8412175 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for investigating and fixing this!

::: dom/base/nsGlobalWindow.cpp
@@ +1606,5 @@
>    }
>    mAudioContexts.Clear();
>  
>  #ifdef MOZ_GAMEPAD
> +  DisableGamepadUpdates();

Looks fine to me, though I wonder if these three things should be combined into a single function.  The other place that calls DisableGamepadUpdates() does mHasGamepad = false, but doesn't clear mGamePads.  It isn't a big deal either way, though, this patch is fine as is.

Comment 4

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

I decided to take the most conservative approach for the patch, so that it could land on branches too.

If we for some reason need to create new 29 builds, I think we could take the patch.

Comment 5

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8f4a613f6b66

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8f4a613f6b66

Comment 7

[Benjamin Kerensa [:bkerensa]]

(In reply to Olli Pettay [:smaug] from comment #4)
> I decided to take the most conservative approach for the patch, so that it
> could land on branches too.
> 
> If we for some reason need to create new 29 builds, I think we could take
> the patch.

Can you provide some justification for tracking? Also does this need uplift?

Comment 8

[Andrew McCreight [:mccr8]]

(In reply to Benjamin Kerensa [:bkerensa] from comment #7)
> Can you provide some justification for tracking? Also does this need uplift?

Leaking an entire page is bad because it uses a lot of memory, and can be very janky.  Ted initially noticed this problem because his browser was freezing for multiple seconds at a time, over and over.  It sounds like this will happen any time a page that uses the Gamepad API enters the bfcache, which means you navigated to another page.  There probably aren't many pages that use the gamepad API yet, but the behavior is pretty bad for those that do use it.

Comment 9

[Andrew McCreight [:mccr8]]

Comment on attachment 8412175 [details] [diff] [review]
patch

Too late for beta (we release on Tuesday).

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 878828 (enable gamepad API)
User impact if declined: memory leaks and possible multiple second pauses if you visit a page using the gamepad API
Testing completed (on m-c, etc.): It has been on 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 10

[Lukas Blakk [:lsblakk] use ?needinfo]

We're already done with building the final RC of FF29 so this is a wontfix on that release, we wouldn't take something like this in a ride-along for a chemspill.  Will have to ride the trains with FF30.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/92eb6a82595c

Comment 12

[u279076]

Olli, could you please advise QA what sites we know of to test this is working?

Comment 13

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

I don't know any sites. This would require enabling gamepad stuff on a page and then analyzing
cycle collector log after that page goes into bfcache.

I don't recall how ted first saw this.

Comment 14

[u279076]

(In reply to Olli Pettay [:smaug] from comment #13)
> I don't recall how ted first saw this.

Okay thanks. If we could get a minimized testcase it would be good to get that in one of our testsuites. Otherwise I'm deprioritizing this for QA testing.

Comment 15

[(not currently active) Ted Mielczarek]

I believe it was basically "use the gamepad API on a page, load another page" and doing that repeatedly, but the steps to figure out that the leak was present involved reading CC logs.