Open Bug 1657404 Opened 2 years ago Updated 14 days ago

Cleanup Gamepad-related code

Categories

(Core :: DOM: Device Interfaces, task)

Product:
Core
Component:
DOM: Device Interfaces
Type:
task

Tracking

()

Status:
ASSIGNED

People

(Reporter: cmartin, Assigned: cmartin)

References

(Blocks 1 open bug)

Details

(Keywords: leave-open)

Attachments

(13 files, 9 obsolete files)

Bug 1657404 - Refactor gamepad monitoring - Part 1
47 bytes, text/x-phabricator-request
Details | Review
Bug 1657404 - Refactor gamepad monitoring - Part 2
47 bytes, text/x-phabricator-request
Details | Review
Bug 1657404 - Remove old IPDL workaround from GamepadTestChannelParent
47 bytes, text/x-phabricator-request
Details | Review
Bug 1657404 - Prepare GamepadTestChannelParent for "refcounted protocol"
47 bytes, text/x-phabricator-request
Details | Review
Bug 1657404 - Prepare GamepadTestChannelChild for "refcounted protocol"
47 bytes, text/x-phabricator-request
Details | Review
Bug 1657404 - Change PGamepadTestChannel to "refcounted protocol"
47 bytes, text/x-phabricator-request
Details | Review
Bug 1657404 - Use WeakPtr for observer pattern in GamepadPlatformService
47 bytes, text/x-phabricator-request
Details | Review
Bug 1657404 - Change PGamepadEventChannel to "refcounted protocol"
47 bytes, text/x-phabricator-request
Details | Review
Bug 1657404 - Change GamepadManager to explicitly have 1 event channel
47 bytes, text/x-phabricator-request
Details | Review
Bug 1657404 - Move gamepad test promise logic to GamepadServiceTest
47 bytes, text/x-phabricator-request
Details | Review
Bug 1657404 - Change how AndroidGamepadManager handles gamepad service IDs
47 bytes, text/x-phabricator-request
Details | Review
Bug 1657404 - Back out changeset 6792f28b99bd keeping 349854c5ad56
48 bytes, text/x-phabricator-request
Details | Review
Bug 1657404 - Add diag asserts to catch potential double IPDL shutdown
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
You need to log in before you can comment on or make changes to this bug.