Merge pull request #10014 from OatmealDome/wiimote-disconnect-crash

WiimoteReal: Fix Wiimote disconnection causing Dolphin to crash on macOS

4 files changed, 139 insertions, 14 deletions

diff --git a/Externals/hidapi/applied_patches/0003-hidapi-Stop-current-run-loop-in-removal-callback-ins.patch b/Externals/hidapi/applied_patches/0003-hidapi-Stop-current-run-loop-in-removal-callback-ins.patch
new file mode 100644
index 0000000000..c0c29f9131
--- /dev/null
+++ b/Externals/hidapi/applied_patches/0003-hidapi-Stop-current-run-loop-in-removal-callback-ins.patch
@@ -0,0 +1,42 @@
+From b24599cf5bf07f0f4d12e197e61c8f107a042dd4 Mon Sep 17 00:00:00 2001
+From: OatmealDome <julian@oatmealdome.me>
+Date: Mon, 9 Aug 2021 21:15:17 -0400
+Subject: [PATCH] hidapi: Stop current run loop in removal callback instead of
+ fetching from context
+
+---
+ Externals/hidapi/mac/hid.c | 11 +++++------
+ 1 file changed, 5 insertions(+), 6 deletions(-)
+
+diff --git a/Externals/hidapi/mac/hid.c b/Externals/hidapi/mac/hid.c
+index 70b615d40d..d8055f372c 100644
+--- a/Externals/hidapi/mac/hid.c
++++ b/Externals/hidapi/mac/hid.c
+@@ -562,11 +562,10 @@ hid_device * HID_API_EXPORT hid_open(unsigned short vendor_id, unsigned short pr
+ static void hid_device_removal_callback(void *context, IOReturn result,
+                                         void *sender)
+ {
+-	/* Stop the Run Loop for this device. */
+-	hid_device *d = context;
+-
+-	d->disconnected = 1;
+-	CFRunLoopStop(d->run_loop);
++	/* Stop the Run Loop for this device. This callback will always
++	   be called on the device's registered run loop, so we can just
++	   get the current loop. */		
++	CFRunLoopStop(CFRunLoopGetCurrent());
+ }
+ 
+ /* The Run Loop calls this function for each input report received.
+@@ -658,7 +657,7 @@ static void *read_thread(void *param)
+ 	while (!dev->shutdown_thread && !dev->disconnected) {
+ 		code = CFRunLoopRunInMode(dev->run_loop_mode, 1000/*sec*/, FALSE);
+ 		/* Return if the device has been disconnected */
+-		if (code == kCFRunLoopRunFinished) {
++		if (code == kCFRunLoopRunFinished || code == kCFRunLoopRunStopped) {
+ 			dev->disconnected = 1;
+ 			break;
+ 		}
+-- 
+2.30.1 (Apple Git-130)
+
diff --git a/Externals/hidapi/applied_patches/0004-hidapi-Don-t-leak-device-handle-in-macOS-10.10-or-ne.patch b/Externals/hidapi/applied_patches/0004-hidapi-Don-t-leak-device-handle-in-macOS-10.10-or-ne.patch
new file mode 100644
index 0000000000..8ee722bfeb
--- /dev/null
+++ b/Externals/hidapi/applied_patches/0004-hidapi-Don-t-leak-device-handle-in-macOS-10.10-or-ne.patch
@@ -0,0 +1,70 @@
+From 25c85d827aed098d2536f09a68c23780346cd4e1 Mon Sep 17 00:00:00 2001
+From: OatmealDome <julian@oatmealdome.me>
+Date: Mon, 9 Aug 2021 21:24:10 -0400
+Subject: [PATCH] hidapi: Don't leak device handle in macOS 10.10 or newer
+ Ported from libusb's hidapi fork. Original patch by Youw
+ (cdc473dfe43f6432dda7ad53d7656b8ae8ff968b).
+
+---
+ Externals/hidapi/mac/hid.c | 17 ++++++++++++++---
+ 1 file changed, 14 insertions(+), 3 deletions(-)
+
+diff --git a/Externals/hidapi/mac/hid.c b/Externals/hidapi/mac/hid.c
+index d8055f372c..ea945d4dec 100644
+--- a/Externals/hidapi/mac/hid.c
++++ b/Externals/hidapi/mac/hid.c
+@@ -33,6 +33,9 @@
+ 
+ #include "hidapi.h"
+ 
++/* As defined in AppKit.h, but we don't need the entire AppKit for a single constant. */
++extern const double NSAppKitVersionNumber;
++
+ /* Barrier implementation because Mac OSX doesn't have pthread_barrier.
+    It also doesn't have clock_gettime(). So much for POSIX and SUSv2.
+    This implementation came from Brent Priddy and was posted on
+@@ -177,6 +180,7 @@ static void free_hid_device(hid_device *dev)
+ }
+ 
+ static	IOHIDManagerRef hid_mgr = 0x0;
++static	int is_macos_10_10_or_greater = 0;
+ 
+ 
+ #if 0
+@@ -390,6 +394,7 @@ static int init_hid_manager(void)
+ int HID_API_EXPORT hid_init(void)
+ {
+ 	if (!hid_mgr) {
++		is_macos_10_10_or_greater = (NSAppKitVersionNumber >= 1343); /* NSAppKitVersionNumber10_10 */
+ 		return init_hid_manager();
+ 	}
+ 
+@@ -989,7 +994,7 @@ void HID_API_EXPORT hid_close(hid_device *dev)
+ 		return;
+ 
+ 	/* Disconnect the report callback before close. */
+-	if (!dev->disconnected) {
++	if (is_macos_10_10_or_greater || !dev->disconnected) {
+ 		IOHIDDeviceRegisterInputReportCallback(
+ 			dev->device_handle, dev->input_report_buf, dev->max_input_report_len,
+ 			NULL, dev);
+@@ -1013,8 +1018,14 @@ void HID_API_EXPORT hid_close(hid_device *dev)
+ 
+ 	/* Close the OS handle to the device, but only if it's not
+ 	   been unplugged. If it's been unplugged, then calling
+-	   IOHIDDeviceClose() will crash. */
+-	if (!dev->disconnected) {
++	   IOHIDDeviceClose() will crash.
++
++	   UPD: The crash part was true in/until some version of macOS.
++	   Starting with macOS 10.15, there is an opposite effect in some environments:
++	   crash happenes if IOHIDDeviceClose() is not called.
++	   Not leaking a resource in all tested environments.
++	*/
++	if (is_macos_10_10_or_greater || !dev->disconnected) {
+ 		IOHIDDeviceClose(dev->device_handle, kIOHIDOptionsTypeSeizeDevice);
+ 	}
+ 
+-- 
+2.30.1 (Apple Git-130)
+
diff --git a/Externals/hidapi/mac/hid.c b/Externals/hidapi/mac/hid.c
index 70b615d40d..ea945d4dec 100644
--- a/Externals/hidapi/mac/hid.c
+++ b/Externals/hidapi/mac/hid.c
@@ -33,6 +33,9 @@
 
 #include "hidapi.h"
 
+/* As defined in AppKit.h, but we don't need the entire AppKit for a single constant. */
+extern const double NSAppKitVersionNumber;
+
 /* Barrier implementation because Mac OSX doesn't have pthread_barrier.
    It also doesn't have clock_gettime(). So much for POSIX and SUSv2.
    This implementation came from Brent Priddy and was posted on
@@ -177,6 +180,7 @@ static void free_hid_device(hid_device *dev)
 }
 
 static	IOHIDManagerRef hid_mgr = 0x0;
+static	int is_macos_10_10_or_greater = 0;
 
 
 #if 0
@@ -390,6 +394,7 @@ static int init_hid_manager(void)
 int HID_API_EXPORT hid_init(void)
 {
 	if (!hid_mgr) {
+		is_macos_10_10_or_greater = (NSAppKitVersionNumber >= 1343); /* NSAppKitVersionNumber10_10 */
 		return init_hid_manager();
 	}
 
@@ -562,11 +567,10 @@ hid_device * HID_API_EXPORT hid_open(unsigned short vendor_id, unsigned short pr
 static void hid_device_removal_callback(void *context, IOReturn result,
                                         void *sender)
 {
-	/* Stop the Run Loop for this device. */
-	hid_device *d = context;
-
-	d->disconnected = 1;
-	CFRunLoopStop(d->run_loop);
+	/* Stop the Run Loop for this device. This callback will always
+	   be called on the device's registered run loop, so we can just
+	   get the current loop. */		
+	CFRunLoopStop(CFRunLoopGetCurrent());
 }
 
 /* The Run Loop calls this function for each input report received.
@@ -658,7 +662,7 @@ static void *read_thread(void *param)
 	while (!dev->shutdown_thread && !dev->disconnected) {
 		code = CFRunLoopRunInMode(dev->run_loop_mode, 1000/*sec*/, FALSE);
 		/* Return if the device has been disconnected */
-		if (code == kCFRunLoopRunFinished) {
+		if (code == kCFRunLoopRunFinished || code == kCFRunLoopRunStopped) {
 			dev->disconnected = 1;
 			break;
 		}
@@ -990,7 +994,7 @@ void HID_API_EXPORT hid_close(hid_device *dev)
 		return;
 
 	/* Disconnect the report callback before close. */
-	if (!dev->disconnected) {
+	if (is_macos_10_10_or_greater || !dev->disconnected) {
 		IOHIDDeviceRegisterInputReportCallback(
 			dev->device_handle, dev->input_report_buf, dev->max_input_report_len,
 			NULL, dev);
@@ -1014,8 +1018,14 @@ void HID_API_EXPORT hid_close(hid_device *dev)
 
 	/* Close the OS handle to the device, but only if it's not
 	   been unplugged. If it's been unplugged, then calling
-	   IOHIDDeviceClose() will crash. */
-	if (!dev->disconnected) {
+	   IOHIDDeviceClose() will crash.
+
+	   UPD: The crash part was true in/until some version of macOS.
+	   Starting with macOS 10.15, there is an opposite effect in some environments:
+	   crash happenes if IOHIDDeviceClose() is not called.
+	   Not leaking a resource in all tested environments.
+	*/
+	if (is_macos_10_10_or_greater || !dev->disconnected) {
 		IOHIDDeviceClose(dev->device_handle, kIOHIDOptionsTypeSeizeDevice);
 	}
 
diff --git a/Source/Core/Core/HW/WiimoteReal/WiimoteReal.cpp b/Source/Core/Core/HW/WiimoteReal/WiimoteReal.cpp
index b0b03f81ab..b3c5b1f203 100644
--- a/Source/Core/Core/HW/WiimoteReal/WiimoteReal.cpp
+++ b/Source/Core/Core/HW/WiimoteReal/WiimoteReal.cpp
@@ -280,6 +280,14 @@ void Wiimote::Read()
   Report rpt(MAX_PAYLOAD);
   auto const result = IORead(rpt.data());
 
+  if (0 == result)
+  {
+    ERROR_LOG_FMT(WIIMOTE, "Wiimote::IORead failed. Disconnecting Wii Remote {}.", m_index + 1);
+    DisconnectInternal();
+
+    return;
+  }
+
   // Drop the report if not connected.
   if (!m_is_linked)
     return;
@@ -297,11 +305,6 @@ void Wiimote::Read()
     rpt.resize(result);
     m_read_reports.Push(std::move(rpt));
   }
-  else if (0 == result)
-  {
-    ERROR_LOG_FMT(WIIMOTE, "Wiimote::IORead failed. Disconnecting Wii Remote {}.", m_index + 1);
-    DisconnectInternal();
-  }
 }
 
 bool Wiimote::Write()