MacOS 15.3.2 on Apple Silicon M1: Crash when game controller is disconnected

[uniemu]

Whenever - while a Bluetooth game controller is connected to a running SDL application on a current Mac M1 - the controller is disconnected or Bluetooth is disabled, the application will crash with an exception "assertion failure: Device has already been activated/cancelled" during the next call to SDL_PollEvent() or SDL_WaitEvent().

This exception is raised by IOHIDDeviceRegisterInputReportCallback() or (if the former is avoided) IOHIDDeviceClose() in hid_close() in "hidapi/mac/hid.c". These function calls are both subject to the following conditions:

if (is_macos_10_10_or_greater || !dev->disconnected) {

The flag dev->disconnected gets set correctly beforehand, so the culprit seems to be the switch is_macos_10_10_or_greater. There is a comment in the code saying the two functions crashed before MacOS 10.15 when unplugging a controller, but later it was the other way round (crash if they were not called). It appears that this issue still exists on MacOS 15.3.2, at least on the M1 platform.

Can anyone check if it happens on the Intel platform or on other MacOS versions later than 10.15?

[uniemu]

I forgot to report that the application becomes stable once I remove the is_macos_10_10_or_greater condition.

[slouken]

Can you look and see why is_macos_10_10_or_greater is not set to true on 10.15?

[uniemu]

Oh, it is set to true, that's the reason why the crashing IOHID functions are called at all (when they obviously shouldn't be, because dev->disconnected is true, and the controller's Event Loop in read_thread() has already finished).

Perhaps I was a bit ambigous. The present code in hid_close() looks like this:

void HID_API_EXPORT hid_close(hid_device *dev)
{
...
if (is_macos_10_10_or_greater || !dev->disconnected) {
IOHIDDeviceRegisterInputReportCallback(dev->device_handle, dev->input_report_buf, dev->max_input_report_len,NULL, dev); <<<==== THIS ONE CRASHES
IOHIDDeviceUnscheduleFromRunLoop(dev->device_handle, dev->run_loop, dev->run_loop_mode);
IOHIDDeviceScheduleWithRunLoop(dev->device_handle, CFRunLoopGetMain(), kCFRunLoopDefaultMode);
}

/* Cause read_thread() to stop. */
dev->shutdown_thread = 1;
...

Obviously, read_thread() is intended to finish only after this (triggered by the latter flag), but in fact it finishes first, setting dev->disconnected to true (and thus is already terminated when hid_close() is called):

static void *read_thread(void *param)
{
...
code = CFRunLoopRunInMode(dev->run_loop_mode, 1000/*sec*/, FALSE);
/* Return if the device has been disconnected */
if (code == kCFRunLoopRunFinished || code == kCFRunLoopRunStopped) {
dev->disconnected = 1;
break;
}
...

And, a new insight: I found an old Intel Mac Mini running MacOS 12.7.6. It behaves the same way as my M1 with 15.3.2. The application crashes with said exception in SDL_PollEvent() once I disable Bluetooth (with a game controller connected and working up to this point).

In my opinion, the additional condition is_macos_10_10_or_greater in hid_close() is simply wrong and should be deleted everywhere, the wording of the exception allows no other conclusion than that. As I wrote above, if dev->disconnected is already true, the read_thread() is already finished, because CFRunLoopRunInMode() returned kCFRunLoopRunFinished or kCFRunLoopRunStopped. The exception clearly tells us: Don't call IOHIDDeviceRegisterInputReportCallback() or IOHIDDeviceClose() after CFRunLoopRunInMode() returned kCFRunLoopRunFinished or kCFRunLoopRunStopped. The comment update in hid_close() ("UPD: The crash part was true in/until some version of macOS. ...") is obviously not correct.

If the condition is_macos_10_10_or_greater is removed, those functions won't be called after such a return code. This is what I described above, and it worked. The application continues to run even when Bluetooth is disabled.

[deveee]

Seems that I created duplicated bug report
#12812

So I just copy-paste here.

It's a regression since 8b0c9e1

I just disconnect the bluetooth device when game is started to reproduce it. I tested it on macOS 15.1.1 with Sony DualSense controller and it crashes every time.

We get a lot of crash reports related to this change recently and tbh. I didn't notice any crash in this code before 8b0c9e1.

[uniemu]

It's a regression since 8b0c9e1

That's exactly where is_macos_10_10_or_greater came into the code.

I found the following discussions on libusb/hidapi, their code looks familiar:

libusb/hidapi#144
libusb/hidapi#186
https://github.com/libusb/hidapi/blob/master/mac/hid.c

They tested up to MacOS 11 only. I wonder why there are no corresponding bug reports there.

Perhaps we should rename the flag to is_macos_10_10_or_greater_and_less_than_12 (the new code crashes on MacOS 12, see my report above) and set it accordingly. I will try to find a Mac with 10.10<MacOS<12 to check if code without the flag crashes there. If not, the flag should be deleted completely.

[deveee]

I just checked crash reports and the oldest version where current SDL crashes is 10.15.7. I see macOS 11, 12, 13 etc. either. And I didn't see any similar crash before this patch.

So maybe there is other reason of the issue that the is_macos_10_10_or_greater was meant to solve.

Also I didn't look at the details but I saw some differences between libusb hidapi code and SDL hidapi code.

[uniemu]

I reactivated my oldest Mac (Intel), running MacOS 10.15.7. The application, in the moment Bluetooth is switched off, crashes in IOHIDDeviceRegisterInputReportCallback() during the next SDL_PollEvent(), too, also with "assertion failure: Device has already been activated/cancelled".

The only significant difference that I see between SDL hidapi and libusb/hidapi code is that libusb/hidapi does a CFRelease(dev->device_handle) at the end of hid_close(), but that's after the crashing call to IOHIDDeviceRegisterInputReportCallback(), so doesn't matter for our issue.

I don't know how those guys at libusb tested, but is_macos_10_10_or_greater does not work, not in 10.15.7, not in 12.7.6 and not in 15.3.2.

I still suggest to undo 8b0c9e1.