1657404 - Cleanup Gamepad-related code

Status: NEW

(Core :: DOM: Device Interfaces, task, P3)

Description

[Chris Martin [:cmartin]]

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.

Comment 1

[Chris Martin [:cmartin]]

Attachment: Bug 1657404 - Refactor gamepad monitoring - Part 1

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.

Comment 2

[Chris Martin [:cmartin]]

Attachment: Bug 1657404 - Refactor gamepad monitoring - Part 2

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

Comment 3

[Pulsebot]

Pushed by [e-mail]:
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

Comment 4

[Chris Martin [:cmartin]]

Attachment: Bug 1657404 - Remove "pending events" from gamepad service

"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

Comment 5

[Chris Martin [:cmartin]]

Attachment: Bug 1657404 - Remove "pending events" from gamepad service

"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.

Comment 6

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/550630983bc5
https://hg.mozilla.org/mozilla-central/rev/b9e6371846e0

Comment 7

[Chris Martin [:cmartin]]

Attachment: Bug 1657404 - Remove old IPDL workaround from GamepadTestChannelParent

Comment 8

[Chris Martin [:cmartin]]

Attachment: Bug 1657404 - Prepare GamepadTestChannelParent for "refcounted protocol"

Depends on D93019

Comment 9

[Chris Martin [:cmartin]]

Attachment: Bug 1657404 - Prepare GamepadTestChannelChild for "refcounted protocol"

Depends on D93021

Comment 10

[Chris Martin [:cmartin]]

Attachment: Bug 1657404 - Change PGamepadTestChannel to "refcounted protocol"

Depends on D93022

Comment 11

[Chris Martin [:cmartin]]

Attachment: Bug 1657404 - Use WeakPtr for observer pattern in GamepadPlatformService

Comment 12

[Pulsebot]

Pushed by [e-mail]:
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

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5729967eff6d
Use WeakPtr for observer pattern in GamepadPlatformService r=handyman

Comment 14

[Chris Martin [:cmartin]]

Attachment: Bug 1657404 - Change PGamepadEventChannel to "refcounted protocol"

Comment 15

[Razvan Maries]

Backed out for perma failures.

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&selectedTaskRun=NAY3KMgJTL2l_ZCkfFNYng.0&resultStatus=testfailed%2Cbusted%2Cexception&revision=248dbb293545604293bec69833d84030f48c7eca&searchStr=mochitests&failure_classification_id=2

Log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=318206064&repo=autoland&lineNumber=5873

Backout: https://hg.mozilla.org/integration/autoland/rev/d4576538182ea054b03253f1c8601bba26eaf1c6

Comment 16

[Chris Martin [:cmartin]]

Thanks for the backout -- Will have to re-think the lifetimes of the object that caused the crash.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8132dd939023
Change PGamepadEventChannel to "refcounted protocol" r=handyman

Comment 18

[Chris Martin [:cmartin]]

Attachment: Bug 1657404 - Change GamepadManager to explicitly have 1 event channel

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

[Pulsebot]

Pushed by [e-mail]:
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

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/8132dd939023

Comment 21

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/c5a399870fba
https://hg.mozilla.org/mozilla-central/rev/29eb6e041917
https://hg.mozilla.org/mozilla-central/rev/219491bd4447
https://hg.mozilla.org/mozilla-central/rev/e818ba719e0f

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5ea464cd1c26
Change GamepadManager to explicitly have 1 event channel r=handyman

Comment 23

[Chris Martin [:cmartin]]

Attachment: Bug 1657404 - GamepadPlatformService mechanical refactor

This is a mechanical refactor in prep for some logic changes to
GamepadPlatformService. It makes the logic changes more obviously correct.

Comment 24

[Chris Martin [:cmartin]]

Attachment: Bug 1657404 - Refactor gamepad event channel logic into a single class

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

Comment 25

[Chris Martin [:cmartin]]

Attachment: Bug 1657404 - Simplify several pieces of logic in GamepadPlatformService

Depends on D93695

Comment 26

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/mozilla-central/rev/5ea464cd1c26

Comment 27

[Chris Martin [:cmartin]]

Attachment: Bug 1657404 - Move gamepad test promise logic to GamepadServiceTest

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.

Comment 28

[Chris Martin [:cmartin]]

Attachment: Bug 1657404 - Gamepad: Minor fixes for naming/style/logic

• 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

Comment 29

[Chris Martin [:cmartin]]

Attachment: Bug 1657404 - Change how AndroidGamepadManager handles gamepad service IDs

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

Comment 30

[Chris Martin [:cmartin]]

Attachment: Bug 1657404 - Implement a strongly-typed, service-dependent gamepad handle

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

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0ce25a58f2f4
Move gamepad test promise logic to GamepadServiceTest r=handyman

Comment 32

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ef842e07dffa
Change how AndroidGamepadManager handles gamepad service IDs r=geckoview-reviewers,agi

Comment 33

[Chris Martin [:cmartin]]

Attachment: Bug 1657404 - Gamepad: Make a singleton to hold monitoring observers

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

Comment 34

[Chris Martin [:cmartin]]

Attachment: Bug 1657404 - Refactor GamepadPlatformService

A substantial refactor of GamepadPlatformService:

1. Easier to understand lifetime
2. Correct usage of mutexes to protect shared state
3. Clear separation of "service owner" and "service user"
4. Simplify logic in some places
5. Better variable names

Comment 35

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/0ce25a58f2f4
https://hg.mozilla.org/mozilla-central/rev/ef842e07dffa

Comment 36

[Pulsebot]

Pushed by [e-mail]:
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

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/349854c5ad56
https://hg.mozilla.org/mozilla-central/rev/6792f28b99bd
https://hg.mozilla.org/mozilla-central/rev/ec6a64b639a8

Comment 38

[Julien Cristau [:jcristau]]

ni?self to back this out of beta 85 per bug 1681682 comment 1

Comment 39

[Chris Martin [:cmartin]]

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

[Julien Cristau [:jcristau]]

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

[Julien Cristau [:jcristau]]

Where do we stand now? I'd like to get the regressions resolved today if at all possible, via either fixes or a backout.

Comment 42

[Chris Martin [:cmartin]]

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.

Comment 43

[Chris Martin [:cmartin]]

Attachment: Bug 1657404 - Back out changeset 6792f28b99bd keeping 349854c5ad56

Comment 44

[Julien Cristau [:jcristau]]

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.

Comment 45

[Chris Martin [:cmartin]]

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

[Julien Cristau [:jcristau]]

I can do it. To confirm this would be D96273 + D96664? Or also D96663?

Comment 47

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f737176fe4c7
Back out changeset 6792f28b99bd keeping 349854c5ad56 r=handyman

Comment 48

[Chris Martin [:cmartin]]

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.

Comment 49

[Julien Cristau [:jcristau]]

beta backout:
https://hg.mozilla.org/releases/mozilla-beta/rev/0f0e8d040ea71e2ab86e80215fc0e9a7275d1e2a

Comment 50

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/f737176fe4c7

Comment 51

[Chris Martin [:cmartin]]

Attachment: Bug 1657404 - Add diag asserts to catch potential double IPDL shutdown

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

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d31313f495aa
Add diag asserts to catch potential double IPDL shutdown r=handyman

Comment 53

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/d31313f495aa

Comment 54

[BugBot [:suhaib / :marco/ :calixte]]

The leave-open keyword is there and there is no activity for 6 months.
:cmartin, maybe it's time to close this bug?

Comment 55

[Chris Martin [:cmartin]]

Will land this eventually when I get more Gamepad cycles.

Comment 56

[BugBot [:suhaib / :marco/ :calixte]]

The leave-open keyword is there and there is no activity for 6 months.
:cmartin, maybe it's time to close this bug?

Comment 57

[BugBot [:suhaib / :marco/ :calixte]]

The leave-open keyword is there and there is no activity for 6 months.
:cmartin, maybe it's time to close this bug?
For more information, please visit auto_nag documentation.

Comment 58

[Randell Jesup [:jesup] (needinfo me)]

I found that my WIndows 10 desktop could not see any gamepad controller, and mozregression traced it back to this landing-and-partial-backout. Note that my win10 laptop (Lenovo P53) does work, and so does Linux.

I did a win32 build locally with a backout of 349854c5ad56, and it works on the desktop. Removing the backout makes it fail again. (Both with a new profile).

Hardware is:
AMD Ryzen 5 3600
32GB
Tuf Gaming motherboard
USB cable from a rear slot, connected to standard XBox One controller. Same with an Afterglow Xbox wired controller.
Win10 Pro, 22H2, Windows Feature Experience Pack 1000.19060.1000.0
AMD Radeon 6700XT

Comment 59

[Chris Martin [:cmartin]]

Thank you very much for taking the time to investigate this, Randell :)

I guess the solution is fairly straightforward then -- I will upload a patch that backs out 349854c5ad56.

Comment 60

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Bug 1657404: Back out changeset 349854c5ad56 (gamepad) r=cmartin!

This changeset was left in when the rest of bug 1657404 was backed out in
2020, and causes at least some Windows machines to not see gamepads

Comment 61

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4d4247338bf6
Back out changeset 349854c5ad56 (gamepad) r=cmartin

Comment 62

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6f01a26c0296
Gamepad: Make a singleton to hold monitoring observers r=handyman

Comment 63

[Norisz Fay [:noriszfay]]

The backout has caused multiple GamepadPlatformService.cpp related failures and so it was backed out

Backout link

Push with failures

Failure log // Failure log 2

Comment 64

[Norisz Fay [:noriszfay]]

https://hg.mozilla.org/mozilla-central/rev/4d4247338bf6
https://hg.mozilla.org/mozilla-central/rev/6f01a26c0296

Comment 65

[Randell Jesup [:jesup] (needinfo me)]

to keep it on cmartin's radar