Open Bug 1657404 Opened 5 years ago Updated 2 months ago

Cleanup Gamepad-related code

Categories

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

task

Tracking

()

People

(Reporter: cmartin, Unassigned)

References

(Blocks 2 open bugs)

Details

(Keywords: leave-open, webcompat:platform-bug, Whiteboard: [cloud-gaming])

Attachments

(14 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
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.

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.

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

Assignee: nobody → cmartin
Status: NEW → ASSIGNED
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

"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

Attachment #9168848 - Attachment is obsolete: true

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

Attachment #9168860 - Attachment is obsolete: true
Regressions: 1660660
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
Pushed by cmartin@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/5729967eff6d Use WeakPtr for observer pattern in GamepadPlatformService r=handyman

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

Flags: needinfo?(cmartin)
Pushed by cmartin@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/8132dd939023 Change PGamepadEventChannel to "refcounted protocol" r=handyman

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.

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
Pushed by cmartin@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/5ea464cd1c26 Change GamepadManager to explicitly have 1 event channel r=handyman

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

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

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.

  • 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

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

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

Pushed by cmartin@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/0ce25a58f2f4 Move gamepad test promise logic to GamepadServiceTest r=handyman
Attachment #9186391 - Attachment is obsolete: true
Pushed by cmartin@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/ef842e07dffa Change how AndroidGamepadManager handles gamepad service IDs r=geckoview-reviewers,agi

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

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
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
Regressions: 1680955
Regressions: 1681748
Attachment #9181868 - Attachment is obsolete: true
Attachment #9181869 - Attachment is obsolete: true
Attachment #9181870 - Attachment is obsolete: true

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

Flags: needinfo?(jcristau)

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.

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.

Flags: needinfo?(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.

Flags: needinfo?(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.

Flags: needinfo?(cmartin)

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.

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?

Flags: needinfo?(jcristau)

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

Flags: needinfo?(jcristau) → needinfo?(cmartin)
Pushed by cmartin@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/f737176fe4c7 Back out changeset 6792f28b99bd keeping 349854c5ad56 r=handyman

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.

Flags: needinfo?(cmartin) → needinfo?(jcristau)

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.

Pushed by cmartin@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/d31313f495aa Add diag asserts to catch potential double IPDL shutdown r=handyman

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

Flags: needinfo?(cmartin)

Will land this eventually when I get more Gamepad cycles.

Flags: needinfo?(cmartin)
See Also: → 1671957

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

Flags: needinfo?(cmartin)
Flags: needinfo?(cmartin)
Attachment #9186393 - Attachment is obsolete: true
Attachment #9187063 - Attachment is obsolete: true
Attachment #9187062 - Attachment is obsolete: true

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.

Flags: needinfo?(cmartin)
Flags: needinfo?(cmartin)

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

Flags: needinfo?(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.

Flags: needinfo?(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

Pushed by rjesup@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/4d4247338bf6 Back out changeset 349854c5ad56 (gamepad) r=cmartin
Pushed by nfay@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/6f01a26c0296 Gamepad: Make a singleton to hold monitoring observers r=handyman

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

Flags: needinfo?(rjesup)

to keep it on cmartin's radar

Flags: needinfo?(rjesup) → needinfo?(cmartin)
Assignee: cmartin → nobody
Severity: -- → S3
Status: ASSIGNED → NEW
Flags: needinfo?(cmartin)
Priority: -- → P3
Whiteboard: [gaming]
Whiteboard: [gaming] → [cloud-gaming]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: