RESOLVED FIXED 214245
Resolve race between IOHIDManager and GameController framework
https://bugs.webkit.org/show_bug.cgi?id=214245
Summary Resolve race between IOHIDManager and GameController framework
Brady Eidson
Reported 2020-07-12 19:47:18 PDT
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.
Attachments
Patch (18.69 KB, patch)
2020-07-14 15:27 PDT, Brady Eidson
no flags
Patch (18.74 KB, patch)
2020-07-14 15:49 PDT, Brady Eidson
no flags
Those changes, plus some "make it build on Big Sur" changes (19.07 KB, patch)
2020-07-14 23:26 PDT, Brady Eidson
no flags
Other configuration build fixes (3.30 KB, patch)
2020-07-15 09:19 PDT, Brady Eidson
no flags
Patch (2.26 KB, patch)
2020-07-15 10:06 PDT, Chris Dumez
no flags
Brady Eidson
Comment 1 2020-07-14 15:12:32 PDT
Brady Eidson
Comment 2 2020-07-14 15:27:08 PDT
Brady Eidson
Comment 3 2020-07-14 15:49:48 PDT
Brady Eidson
Comment 4 2020-07-14 17:13:29 PDT
(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 5 2020-07-14 21:59:09 PDT
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
Comment 6 2020-07-14 22:45:42 PDT
(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
Comment 7 2020-07-14 23:26:14 PDT
Created attachment 404320 [details] Those changes, plus some "make it build on Big Sur" changes
EWS
Comment 8 2020-07-15 01:00:17 PDT
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
Comment 9 2020-07-15 09:01:21 PDT
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
Comment 10 2020-07-15 09:19:23 PDT
Created attachment 404349 [details] Other configuration build fixes
Brady Eidson
Comment 11 2020-07-15 09:51:55 PDT
EWS isn't doing anything here... So I doubt the CQ will either.
Chris Dumez
Comment 12 2020-07-15 10:06:47 PDT
Reopening to attach new patch.
Chris Dumez
Comment 13 2020-07-15 10:06:48 PDT
Brady Eidson
Comment 14 2020-07-15 10:10:52 PDT
Note You need to log in before you can comment on or make changes to this bug.