Closed
Bug 1001134
Opened 10 years ago
Closed 10 years ago
Gamepad service leaks windows if a page goes into bfcache
Categories
(Core :: DOM: Core & HTML, defect)
Tracking
()
RESOLVED
FIXED
mozilla31
People
(Reporter: smaug, Assigned: smaug)
Details
(Whiteboard: [MemShrink][qa-])
Attachments
(1 file)
668 bytes,
patch
|
ted
:
review+
mccr8
:
review+
lsblakk
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
Patch coming, I hope.
Updated•10 years ago
|
Whiteboard: [MemShrink]
Assignee | ||
Comment 1•10 years ago
|
||
This seems to help after all.
Attachment #8412175 -
Flags: review?(ted)
Attachment #8412175 -
Flags: review?(continuation)
Comment 2•10 years ago
|
||
Comment on attachment 8412175 [details] [diff] [review] patch Review of attachment 8412175 [details] [diff] [review]: ----------------------------------------------------------------- Sorry. :-/
Attachment #8412175 -
Flags: review?(ted) → review+
Comment 3•10 years ago
|
||
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•10 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.
status-firefox29:
--- → affected
status-firefox30:
--- → affected
status-firefox31:
--- → affected
tracking-firefox29:
--- → ?
tracking-firefox30:
--- → ?
tracking-firefox31:
--- → ?
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 5•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/8f4a613f6b66
Keywords: checkin-needed
Comment 6•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/8f4a613f6b66
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
Comment 7•10 years ago
|
||
(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•10 years ago
|
||
(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•10 years ago
|
||
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?
Updated•10 years ago
|
Updated•10 years ago
|
Attachment #8412175 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 10•10 years ago
|
||
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 12•10 years ago
|
||
Olli, could you please advise QA what sites we know of to test this is working?
Flags: needinfo?(bugs)
Assignee | ||
Comment 13•10 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)
Comment 14•10 years ago
|
||
(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-]
Comment 15•10 years ago
|
||
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.
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•