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.
rdar://problem/65554490
Created attachment 404292 [details] Patch
Created attachment 404298 [details] Patch
(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
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?
(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.
Created attachment 404320 [details] Those changes, plus some "make it build on Big Sur" changes
Committed r264389: <https://trac.webkit.org/changeset/264389> All reviewed patches have been landed. Closing bug and clearing flags on attachment 404320 [details].
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.
Created attachment 404349 [details] Other configuration build fixes
EWS isn't doing anything here... So I doubt the CQ will either.
Reopening to attach new patch.
Created attachment 404352 [details] Patch
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