Gamepad service leaks windows if a page goes into bfcache

RESOLVED FIXED in Firefox 30

Status

()

defect
RESOLVED FIXED
5 years ago
3 months ago

People

(Reporter: smaug, Assigned: smaug)

Tracking

Trunk
mozilla31
x86_64
Linux
Points:
---

Firefox Tracking Flags

(firefox29- wontfix, firefox30+ fixed, firefox31+ fixed)

Details

(Whiteboard: [MemShrink][qa-])

Attachments

(1 attachment)

Assignee

Description

5 years ago
Patch coming, I hope.
Whiteboard: [MemShrink]
Assignee

Comment 1

5 years ago
Posted patch patchSplinter Review
This seems to help after all.
Attachment #8412175 - Flags: review?(ted)
Attachment #8412175 - Flags: review?(continuation)
Comment on attachment 8412175 [details] [diff] [review]
patch

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

Sorry. :-/
Attachment #8412175 - Flags: review?(ted) → review+
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.
Attachment #8412175 - Flags: review?(continuation) → review+
Assignee

Comment 4

5 years ago
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.
Assignee

Updated

5 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/8f4a613f6b66
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
(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?
(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 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
Attachment #8412175 - Flags: approval-mozilla-aurora?
Attachment #8412175 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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.
Olli, could you please advise QA what sites we know of to test this is working?
Flags: needinfo?(bugs)
Assignee

Comment 13

5 years ago
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.
Flags: needinfo?(bugs)
(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.
Whiteboard: [MemShrink] → [MemShrink][qa-]
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.
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.