Cleanup Gamepad-related code
Categories
(Core :: DOM: Device Interfaces, task)
Tracking
()
People
(Reporter: cmartin, Assigned: cmartin)
References
(Blocks 1 open bug)
Details
(Keywords: leave-open)
Attachments
(13 files, 9 obsolete files)
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review |
As Daosheng and I are going through this gamepad code, there are places that could stand a little refactoring. We can just use this bug for general cleanup that isn't attached to a particular feature.
Assignee | ||
Comment 1•2 years ago
|
||
The starting/stopping of gamepad monitoring is probably a decision that
should be made at the level of the GamepadPlatformService, not in the IPC
actor. This is step 1, later I will expand some of these functions because
it makes the code easier to understand.
Assignee | ||
Comment 2•2 years ago
|
||
Substitute and eliminate MaybeStopGamepadMonitoring(). The logic to do this
is so dependent on GamepadPlatformService that it should probably just be a
member.
Depends on D86266
Updated•2 years ago
|
Pushed by cmartin@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/550630983bc5 Refactor gamepad monitoring - Part 1 r=daoshengmu https://hg.mozilla.org/integration/autoland/rev/b9e6371846e0 Refactor gamepad monitoring - Part 2 r=daoshengmu
Assignee | ||
Comment 4•2 years ago
|
||
"Pending events" is supposed to save all of the events that occurred before
a child process starts listening, and then replay them so each child has
the same controller state.
However, the current implementation does not do this (it clears it after
the first child connects), and writing a full-blown implementation that does
would require a different design, as just using a list that never clears would
result in unbounded memory usage.
Since we are replacing all of this with shared memory anyway, let's get rid
of this and solve it with shared memory instead.
Depends on D86267
Updated•2 years ago
|
Assignee | ||
Comment 5•2 years ago
|
||
"Pending events" is supposed to save all of the events that occurred before
a child process starts listening, and then replay them so each child has
the same controller state.
However, the current implementation does not do this (it clears it after
the first child connects), and writing a full-blown implementation that does
would require a different design, as just using a list that never clears would
result in unbounded memory usage.
Since we are replacing all of this with shared memory anyway, let's get rid
of this and solve it with shared memory instead.
Updated•2 years ago
|
Comment 6•2 years ago
|
||
bugherder |
Assignee | ||
Comment 7•2 years ago
|
||
Assignee | ||
Comment 8•2 years ago
|
||
Depends on D93019
Assignee | ||
Comment 9•2 years ago
|
||
Depends on D93021
Assignee | ||
Comment 10•2 years ago
|
||
Depends on D93022
Assignee | ||
Comment 11•2 years ago
|
||
Comment 12•2 years ago
|
||
Pushed by cmartin@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/e6e1924fb688 Remove old IPDL workaround from GamepadTestChannelParent r=handyman https://hg.mozilla.org/integration/autoland/rev/b225f6a01afe Prepare GamepadTestChannelParent for "refcounted protocol" r=handyman https://hg.mozilla.org/integration/autoland/rev/ae904c76b6cc Prepare GamepadTestChannelChild for "refcounted protocol" r=handyman https://hg.mozilla.org/integration/autoland/rev/a79143c550d7 Change PGamepadTestChannel to "refcounted protocol" r=handyman
Comment 13•2 years ago
|
||
Pushed by cmartin@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/5729967eff6d Use WeakPtr for observer pattern in GamepadPlatformService r=handyman
Assignee | ||
Comment 14•2 years ago
|
||
Comment 15•2 years ago
|
||
Backed out for perma failures.
Log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=318206064&repo=autoland&lineNumber=5873
Backout: https://hg.mozilla.org/integration/autoland/rev/d4576538182ea054b03253f1c8601bba26eaf1c6
Assignee | ||
Comment 16•2 years ago
|
||
Thanks for the backout -- Will have to re-think the lifetimes of the object that caused the crash.
Comment 17•2 years ago
|
||
Pushed by cmartin@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/8132dd939023 Change PGamepadEventChannel to "refcounted protocol" r=handyman
Assignee | ||
Comment 18•2 years ago
|
||
Currently, the GamepadManager contains an nsTArray of event channels. However,
logically the code only ever allows this array to have either 0 or 1 entries.
This change replaces the nsTArray with a nullable pointer.
Comment 19•2 years ago
|
||
Pushed by cmartin@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/c5a399870fba Remove old IPDL workaround from GamepadTestChannelParent r=handyman https://hg.mozilla.org/integration/autoland/rev/29eb6e041917 Prepare GamepadTestChannelParent for "refcounted protocol" r=handyman https://hg.mozilla.org/integration/autoland/rev/219491bd4447 Prepare GamepadTestChannelChild for "refcounted protocol" r=handyman https://hg.mozilla.org/integration/autoland/rev/e818ba719e0f Change PGamepadTestChannel to "refcounted protocol" r=handyman
Comment 20•2 years ago
|
||
bugherder |
Comment 21•2 years ago
|
||
bugherder |
Comment 22•2 years ago
|
||
Pushed by cmartin@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/5ea464cd1c26 Change GamepadManager to explicitly have 1 event channel r=handyman
Assignee | ||
Comment 23•2 years ago
|
||
This is a mechanical refactor in prep for some logic changes to
GamepadPlatformService. It makes the logic changes more obviously correct.
Assignee | ||
Comment 24•2 years ago
|
||
This is another mechanical tranformation. It actually makes a few things more
obvious about this class that I will take advantage of in the next change.
Depends on D93694
Assignee | ||
Comment 25•2 years ago
|
||
Depends on D93695
Comment 26•2 years ago
|
||
bugherder |
Assignee | ||
Comment 27•2 years ago
|
||
Currently, this promise is being created at one level of abstraction but
fulfilled at another. This will be important soon, as this promise is about
to become more complex.
Assignee | ||
Comment 28•2 years ago
|
||
- Inline a few functions in GamepadPlatformService that are only used
in one place - Switch GamepadManager to use an nsTArray<T> to hold gamepad data instead
of hash tables (Hash tables are slower for small arrays, plus I don't want
to make a hash function for my future handle type) - Switch nsGlobalWindowInner to use a single nsTArray<T> instead of a separate
set and hash table - Fix broken logic in GamepadManager::SetLightIndicatorColor()
- Change IPC channels that use promises to call their parameter "aPromiseID"
because "aID" is not a great name
Depends on D96270
Assignee | ||
Comment 29•2 years ago
|
||
Currently, the AndroidGamepadManager uses a single function to both add and
remove gamepad service entries (Even though their functionality is different),
and it also uses a JNI callback to set the ID rather than simply returning it
This fixes both of those things.
Depends on D96271
Assignee | ||
Comment 30•2 years ago
|
||
Currently, the gamepad code uses a uint32_t as a handle and does some trickery
with it by trying to create a unique ID and adding an offset to it for VR code.
This can (and has) led to errors where the developer forgets to perform the
arithmetic and sends the wrong number to the wrong manager.
This change created a strongly-typed handle that remembers which service it
belongs to. This should eliminate such accidents.
Depends on D96272
Comment 31•2 years ago
|
||
Pushed by cmartin@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/0ce25a58f2f4 Move gamepad test promise logic to GamepadServiceTest r=handyman
Updated•2 years ago
|
Comment 32•2 years ago
|
||
Pushed by cmartin@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/ef842e07dffa Change how AndroidGamepadManager handles gamepad service IDs r=geckoview-reviewers,agi
Assignee | ||
Comment 33•2 years ago
|
||
Currently, the list of monitoring observers is stored in the
GamepadPlatformService. But it's possible for testing to start before an
event channel has been created, or to exist longer. That could result in the
GamepadPlatformManager being created-destroyed multiple times along with
this list.
Probably the simplest thing to do here is just have the list be its own
indepedent entity
Assignee | ||
Comment 34•2 years ago
|
||
A substantial refactor of GamepadPlatformService:
- Easier to understand lifetime
- Correct usage of mutexes to protect shared state
- Clear separation of "service owner" and "service user"
- Simplify logic in some places
- Better variable names
Comment 35•2 years ago
|
||
bugherder |
Comment 36•2 years ago
|
||
Pushed by cmartin@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/349854c5ad56 Gamepad: Make a singleton to hold monitoring observers r=handyman https://hg.mozilla.org/integration/autoland/rev/6792f28b99bd Refactor GamepadPlatformService r=handyman https://hg.mozilla.org/integration/autoland/rev/ec6a64b639a8 Implement a strongly-typed, service-dependent gamepad handle r=handyman,aklotz
Comment 37•2 years ago
|
||
bugherder |
Updated•2 years ago
|
Updated•2 years ago
|
Updated•2 years ago
|
Comment 38•2 years ago
|
||
ni?self to back this out of beta 85 per bug 1681682 comment 1
Assignee | ||
Comment 39•2 years ago
|
||
Julien - I have fixes coming down the pipeline for this. If you're still okay with it being fixed in early beta, I'd prefer to give it a few more days.
Comment 40•2 years ago
|
||
I can give it a couple of days but we're building the last beta before January on Sunday, and I'd rather not let the crashes unaddressed for that 2-week break.
Comment 41•2 years ago
|
||
Where do we stand now? I'd like to get the regressions resolved today if at all possible, via either fixes or a backout.
Assignee | ||
Comment 42•2 years ago
•
|
||
Hi Julien,
I let my bugfix commits ride the trains a bit, and it looks like the signature in Bug 1681682 has completely disappeared.
The signature in Bug 1681750 was fixed, but it also transformed into the signature from Bug 1682589. You can see the frequency has been greatly reduced. I'm going to work on reducing it to zero, but that will likely take time that we don't have and it would be too high-risk for Beta.
I think I'm going to attempt a backout of D96664 without backing out D96273, as losing D96273 would be pretty rough for me - I spent a lot of time running on the "rebasing treadmill" to finally land that change, and I'd prefer not to go back on again. It should be very low-risk as the 2 changes are barely dependent on each other.
I will NI you in this ticket when I've landed the partial-backout.
Assignee | ||
Comment 43•2 years ago
|
||
Comment 44•2 years ago
|
||
We could back out D96273 from beta but not central, if that helps buy us time, and doesn't require more rebasing for you. There's somewhat less urgency to get a fix for 86 given how long there is before that goes to beta, and the lower crash volume in the remaining bug 1682589.
Assignee | ||
Comment 45•2 years ago
•
|
||
I guess that makes sense to do a straight backout on Beta, since it's the lowest risk and both changes are mostly just refactors and there's no feature really hanging on it.
As for Nightly, I will likely follow through with this backout of D96664 without backing out D96273, just because D96664 is fairly straightforward, and I can just file a bug to re-apply it once the dependent components are fixed to work properly with it.
Do you need anything from me to do the backout on Beta, or is it something you do?
Comment 46•2 years ago
|
||
I can do it. To confirm this would be D96273 + D96664? Or also D96663?
Comment 47•2 years ago
|
||
Pushed by cmartin@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/f737176fe4c7 Back out changeset 6792f28b99bd keeping 349854c5ad56 r=handyman
Assignee | ||
Comment 48•2 years ago
|
||
You might as well back out all 3 -- They're all just refactors, and since they were all landed together the safest thing is to remove them together.
Assignee | ||
Updated•2 years ago
|
Comment 49•2 years ago
|
||
beta backout:
https://hg.mozilla.org/releases/mozilla-beta/rev/0f0e8d040ea71e2ab86e80215fc0e9a7275d1e2a
Comment 50•2 years ago
|
||
bugherder |
Assignee | ||
Comment 51•2 years ago
|
||
Based on evidence from crash reports in Bug 1682589, there may be a bug where
IPDL intermittently calls ActorDestroy()
twice on GamepadEventChannelParent,
leading to a crash.
This attempts to catch this behavior in Nightly (assuming that I'm correct and
it does exist). If not, I will have to find some other explanation for the
crash behavior from that bug.
Comment 52•2 years ago
|
||
Pushed by cmartin@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/d31313f495aa Add diag asserts to catch potential double IPDL shutdown r=handyman
Comment 53•2 years ago
|
||
bugherder |
Comment 54•1 year ago
|
||
The leave-open keyword is there and there is no activity for 6 months.
:cmartin, maybe it's time to close this bug?
Assignee | ||
Comment 55•11 months ago
|
||
Will land this eventually when I get more Gamepad cycles.
Comment 56•4 months ago
|
||
The leave-open keyword is there and there is no activity for 6 months.
:cmartin, maybe it's time to close this bug?
Assignee | ||
Updated•4 months ago
|
Updated•29 days ago
|
Updated•14 days ago
|
Updated•14 days ago
|
Description
•