Resolve race between IOHIDManager and GameController framework

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Brady Eidson]

Resolve race between IOHIDManager and GameController framework

There's a race between the IOHIDService being available for an IOHIDDevice, and GameController framework publishing that same device.

This leads us to sometimes show the same gamepad twice - Once through IOHID and once through GCF.

We can shift our HID provider to use IOHIDService directly to resolve this.

[Brady Eidson]

rdar://problem/65554490

[Brady Eidson]

Created attachment 404292 [details]
Patch

[Brady Eidson]

Created attachment 404298 [details]
Patch

[Brady Eidson]

(In reply to Brady Eidson from comment #0)
> Resolve race between IOHIDManager and GameController framework
> 
> There's a race between the IOHIDService being available for an IOHIDDevice,
> and GameController framework publishing that same device.
> 
> This leads us to sometimes show the same gamepad twice - Once through IOHID
> and once through GCF.
> 
> We can shift our HID provider to use IOHIDService directly to resolve this.

BTW ^^^ That wasn't possible. So we're doing the approach in the patch

[Tim Horton]

Comment on attachment 404298 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=404298&action=review

> Source/WebCore/ChangeLog:16
> +        GameControler.framework uses IOHIDServices.

Controller

> Source/WebCore/ChangeLog:42
> +        In my testing, when the service publishes after the devices, it's always wihin 50ms, so the 1s
> +        delay seems sufficient.

Slightly frightening. Do you understand the source of the delay? (if it's, say, fetching something from disk, it might be much longer on some hardware).

> Source/WebCore/platform/gamepad/mac/HIDGamepadProvider.h:59
> +        Yes,
> +        No

Someone debugging later might get quite sad about the order of these values (say, if they cast to bool in logging). Maybe swap them?

> Source/WebCore/platform/gamepad/mac/HIDGamepadProvider.mm:191
> +    Yes,
> +    No,
> +    Maybe,

You've done it again

> Source/WebCore/platform/gamepad/mac/HIDGamepadProvider.mm:268
> +    [matchingAttributes addObject:@{(id)CFSTR(kIOHIDDeviceUsagePageKey) : @(kHIDPage_GenericDesktop), (id)CFSTR(kIOHIDDeviceUsageKey) : @(kHIDUsage_GD_GamePad)}];
> +    [matchingAttributes addObject:@{(id)CFSTR(kIOHIDDeviceUsagePageKey) : @(kHIDPage_GenericDesktop), (id)CFSTR(kIOHIDDeviceUsageKey) : @(kHIDUsage_GD_Joystick)}];

Why are you making CFStrings? Seems like NSString is fine, due to toll-free bridging, no? (and then you can just @() like you do for the what-I-assume-are-NSNumbers)

> Source/WebCore/platform/gamepad/mac/HIDGamepadProvider.mm:270
> +    IOHIDEventSystemClientSetMatchingMultiple(m_eventSystemClient.get(), (__bridge CFArrayRef)(matchingAttributes));

Weird extra parens around matchingAttributes

> Source/WebCore/platform/gamepad/mac/HIDGamepadProvider.mm:275
> +        HIDGamepadProvider::singleton().newGamePadServicePublished();

The leading "new" is triggering my "new or create" instinct. Maybe didPublishNewGamePadService? Or, since we aren't the ones publishing, some other verb?

[Brady Eidson]

(In reply to Tim Horton from comment #5)
> Comment on attachment 404298 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=404298&action=review
> 
> > Source/WebCore/ChangeLog:16
> > +        GameControler.framework uses IOHIDServices.
> 
> Controller
> 
> > Source/WebCore/ChangeLog:42
> > +        In my testing, when the service publishes after the devices, it's always wihin 50ms, so the 1s
> > +        delay seems sufficient.
> 
> Slightly frightening. Do you understand the source of the delay? (if it's,
> say, fetching something from disk, it might be much longer on some hardware).

It's not storage bound.

The "normal" race is simply mach messaging from the kernel coming up to user space through two different mach ports. A truly classic race.

A potential race *is* I/O bound as an atypically behaved physical gamepad device can get itself into this situation by being "weird" over USB/BT.

50ms is "a few ms roundtrip on one of those busses" plus a few kernel->user roundtrips.

We don't expect even that on common, well behaved hardware.

If the physical hardware itself causes a 1s delay...  It is PROBABLY not a piece of hardware GameController framework would handle (PS4, Xbox1, and MFi controllers) which are all known to behave.

tldr; I could construct a physical device to misbehave in this manner on purpose to exceed even the one second delay, but in the real world no such device would be handled by GCF

> 
> > Source/WebCore/platform/gamepad/mac/HIDGamepadProvider.h:59
> > +        Yes,
> > +        No
> 
> Someone debugging later might get quite sad about the order of these values
> (say, if they cast to bool in logging). Maybe swap them?

Yes.

> 
> > Source/WebCore/platform/gamepad/mac/HIDGamepadProvider.mm:191
> > +    Yes,
> > +    No,
> > +    Maybe,
> 
> You've done it again

It's as if my tired brain that fell into the anti-pattern once could fall into it again.

> 
> > Source/WebCore/platform/gamepad/mac/HIDGamepadProvider.mm:268
> > +    [matchingAttributes addObject:@{(id)CFSTR(kIOHIDDeviceUsagePageKey) : @(kHIDPage_GenericDesktop), (id)CFSTR(kIOHIDDeviceUsageKey) : @(kHIDUsage_GD_GamePad)}];
> > +    [matchingAttributes addObject:@{(id)CFSTR(kIOHIDDeviceUsagePageKey) : @(kHIDPage_GenericDesktop), (id)CFSTR(kIOHIDDeviceUsageKey) : @(kHIDUsage_GD_Joystick)}];
> 
> Why are you making CFStrings? Seems like NSString is fine, due to toll-free
> bridging, no? (and then you can just @() like you do for the
> what-I-assume-are-NSNumbers)

Yah 100%.
> 
> > Source/WebCore/platform/gamepad/mac/HIDGamepadProvider.mm:270
> > +    IOHIDEventSystemClientSetMatchingMultiple(m_eventSystemClient.get(), (__bridge CFArrayRef)(matchingAttributes));
> 
> Weird extra parens around matchingAttributes
> 

Yah 100%

> > Source/WebCore/platform/gamepad/mac/HIDGamepadProvider.mm:275
> > +        HIDGamepadProvider::singleton().newGamePadServicePublished();
> 
> The leading "new" is triggering my "new or create" instinct. Maybe
> didPublishNewGamePadService? Or, since we aren't the ones publishing, some
> other verb?

Sure.

[Brady Eidson]

Created attachment 404320 [details]
Those changes, plus some "make it build on Big Sur" changes

[EWS]

Committed r264389: <https://trac.webkit.org/changeset/264389>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 404320 [details].

[Brady Eidson]

This broke release builds on Catalina/Big Sur (due to unused variable),
and non-Apple-Internal builds (due to some missing SPI forward decls)

Fixing both soon.

[Brady Eidson]

Created attachment 404349 [details]
Other configuration build fixes

[Brady Eidson]

EWS isn't doing anything here...
So I doubt the CQ will either.

[Chris Dumez]

Reopening to attach new patch.

[Chris Dumez]

Created attachment 404352 [details]
Patch

[Brady Eidson]

https://trac.webkit.org/changeset/264402/webkit for the OSS build SPI fix
https://trac.webkit.org/changeset/264401/webkit for the release build fix