Closed
Bug 1254298
Opened 9 years ago
Closed 9 years ago
Platform specific gamepad services can leak on shutdown in e10s
Categories
(Core :: DOM: Device Interfaces, defect, P3)
Core
DOM: Device Interfaces
Tracking
()
RESOLVED
FIXED
mozilla48
People
(Reporter: qdot, Assigned: qdot)
References
Details
(Whiteboard: btpp-active)
Attachments
(1 file)
(deleted),
patch
|
ted
:
review+
cleu
:
review+
|
Details | Diff | Splinter Review |
In the process of landing bug 1156957, a leak of nsTArray_base was found on shutdown. This resolved back to the mGamepads member of LinuxGamepadService, meaning that the Service was still alive at time of shutdown, which it shouldn't be.
The logs show that the GamepadListenerRemoved IPC message is sent after the shutdown of the child actor, meaning the parent never receives the message, and therefore never shuts down the platform service. We should make sure that gamepad platform services are cleanly shut down both when all listeners are removed, and on XPCOM shutdown.
Comment 1•9 years ago
|
||
I think this leak can be reproduced under the following process.
1. Open firefox nightly (I use ./mach run)
2. Use the FIRST tab to open https://html5gamepad.com and plug a gamepad in
3. Open a new tab but don't do anything about it
4. Close the first tab (The gamepad is still plugged in), waiting for 5 seconds
5. Go to https://html5gamepad.com with the Tab created in 3.
You will find that this website cannot detect gamepad unless you reconnect it.
It is because gamepad service doesn't shutdown after the first tab is closed,
the gamepad service thinks your gamepad is always attached so it doesn't fire
GamepadAdded event to the new tab.
This problem only exists in Nightly build under Linux and Mac OSX, Windows is not affected.
Interestingly, only the first tab has this issue,
So I think it is something goes wrong when firefox startup.
Assignee | ||
Comment 2•9 years ago
|
||
I think this may have something to do with the Cleanup Timer in GamepadService. Whenever we lose our last nsGlobalWindow listener, a 2 second timer is started to clean up the service. This is ok behavior in-process, but out of process this means that we can try to clean up after the child has closed. I bet this is leading to other problems too, like the one mentioned in Comment 1.
Would like to find a minimum viable fix for this, so we can concentrate on the PBackground implementation. I think it might just be shutting down directly in e10s situations, and only using the timer on non-e10s? Will give that a shot.
Assignee | ||
Comment 3•9 years ago
|
||
Ok, since it seems to work locally, here's the try for aforementioned mfv (don't run cleanup timer on e10s):
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4ca2ae89c7bc
Assignee | ||
Comment 4•9 years ago
|
||
This patch makes GamepadService run the IPC cleanup code directly on the last listener detach in the e10s case. In the in-process case, we still keep the usual Timer solution.
Attachment #8728703 -
Flags: review?(ted)
Attachment #8728703 -
Flags: review?(cleu)
Comment 5•9 years ago
|
||
Comment on attachment 8728703 [details] [diff] [review]
Patch 1 (v1) - Bypass GamepadService cleanup timer on e10s
Review of attachment 8728703 [details] [diff] [review]:
-----------------------------------------------------------------
I see no potential hazard to remove cleanup timer under e10s.
So I think this patch is OK.
Attachment #8728703 -
Flags: review?(cleu) → review+
Updated•9 years ago
|
tracking-e10s:
--- → +
Priority: -- → P3
Comment 6•9 years ago
|
||
Comment on attachment 8728703 [details] [diff] [review]
Patch 1 (v1) - Bypass GamepadService cleanup timer on e10s
Review of attachment 8728703 [details] [diff] [review]:
-----------------------------------------------------------------
This seems pretty sensible.
Attachment #8728703 -
Flags: review?(ted) → review+
Updated•9 years ago
|
Whiteboard: btpp-active
Comment 8•9 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in
before you can comment on or make changes to this bug.
Description
•