[PATCH 0/1] MR4065: xinput1_3: Hold module reference while the hid thread is running.

[Arek Hiler (＠ivyl)]

We have no means of stopping the thread from DllMain on
DLL_PROCESS_DETACH without having a race condition - waiting on the
thread itself is not an option due to the loader lock.

The best we can do is just never unload the DLL as long as the thread is
active which is forever.

This fixes crashes in Mighty Switch Force! Collection while controllers
are unplugged - the game loads and frees xinput in a loop.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/4065

[PATCH 1/1] xinput1_3: Hold module reference while the hid thread is running.

[Arkadiusz Hiler]

From: Arkadiusz Hiler <ahiler(a)codeweavers.com>

We have no means of stopping the thread from DllMain on
DLL_PROCESS_DETACH without having a race condition - waiting on the
thread itself is not an option due to the loader lock.

The best we can do is just never unload the DLL as long as the thread is
active which is forever.

This fixes crashes in Mighty Switch Force! Collection while controllers
are unplugged - the game loads and frees xinput in a loop.
---
 dlls/xinput1_3/main.c | 50 +++++++++++--------------------------------
 1 file changed, 13 insertions(+), 37 deletions(-)

diff --git a/dlls/xinput1_3/main.c b/dlls/xinput1_3/main.c
index 2abf33a8d45..e69553fba16 100644
--- a/dlls/xinput1_3/main.c
+++ b/dlls/xinput1_3/main.c
@@ -121,8 +121,6 @@ static struct xinput_controller controllers[XUSER_MAX_COUNT] =
 
 static HMODULE xinput_instance;
 static HANDLE start_event;
-static HANDLE stop_event;
-static HANDLE done_event;
 static HANDLE update_event;
 
 static BOOL find_opened_device(const WCHAR *device_path, int *free_slot)
@@ -547,21 +545,6 @@ static void controller_destroy(struct xinput_controller *controller, BOOL alread
     LeaveCriticalSection(&controller->crit);
 }
 
-static void stop_update_thread(void)
-{
-    int i;
-
-    SetEvent(stop_event);
-    WaitForSingleObject(done_event, INFINITE);
-
-    CloseHandle(start_event);
-    CloseHandle(stop_event);
-    CloseHandle(done_event);
-    CloseHandle(update_event);
-
-    for (i = 0; i < XUSER_MAX_COUNT; i++) controller_destroy(&controllers[i], FALSE);
-}
-
 static LONG sign_extend(ULONG value, const HIDP_VALUE_CAPS *caps)
 {
     UINT sign = 1 << (caps->BitSize - 1);
@@ -687,9 +670,9 @@ static LRESULT CALLBACK xinput_devnotify_wndproc(HWND hwnd, UINT msg, WPARAM wpa
 
 static DWORD WINAPI hid_update_thread_proc(void *param)
 {
-    struct xinput_controller *devices[XUSER_MAX_COUNT + 2];
-    HANDLE events[XUSER_MAX_COUNT + 2];
-    DWORD i, count = 2, ret = WAIT_TIMEOUT;
+    struct xinput_controller *devices[XUSER_MAX_COUNT + 1];
+    HANDLE events[XUSER_MAX_COUNT + 1];
+    DWORD i, count = 1, ret = WAIT_TIMEOUT;
     DEV_BROADCAST_DEVICEINTERFACE_W filter =
     {
         .dbcc_size = sizeof(DEV_BROADCAST_DEVICEINTERFACE_W),
@@ -721,7 +704,7 @@ static DWORD WINAPI hid_update_thread_proc(void *param)
     {
         if (ret == count) while (PeekMessageW(&msg, hwnd, 0, 0, PM_REMOVE)) DispatchMessageW(&msg);
         if (ret == WAIT_TIMEOUT) update_controller_list();
-        if (ret < count - 2) read_controller_state(devices[ret]);
+        if (ret < count - 1) read_controller_state(devices[ret]);
 
         count = 0;
         for (i = 0; i < XUSER_MAX_COUNT; ++i)
@@ -737,33 +720,30 @@ static DWORD WINAPI hid_update_thread_proc(void *param)
             LeaveCriticalSection(&controllers[i].crit);
         }
         events[count++] = update_event;
-        events[count++] = stop_event;
     }
-    while ((ret = MsgWaitForMultipleObjectsEx(count, events, 2000, QS_ALLINPUT, MWMO_ALERTABLE)) < count - 1 ||
-            ret == count || ret == WAIT_TIMEOUT);
+    while ((ret = MsgWaitForMultipleObjectsEx(count, events, 2000, QS_ALLINPUT, MWMO_ALERTABLE)) <= count ||
+            ret == WAIT_TIMEOUT);
+
+    ERR("wait failed in the update thread, ret %lu, error %lu\n", ret, GetLastError());
 
     UnregisterDeviceNotification(notif);
     DestroyWindow(hwnd);
     UnregisterClassW(cls.lpszClassName, xinput_instance);
 
-    if (ret != count - 1) ERR("update thread exited unexpectedly, ret %lu\n", ret);
-    SetEvent(done_event);
-    return ret;
+    FreeLibraryAndExitThread(xinput_instance, ret);
 }
 
 static BOOL WINAPI start_update_thread_once( INIT_ONCE *once, void *param, void **context )
 {
     HANDLE thread;
+    HMODULE module;
+
+    if (!GetModuleHandleExW(GET_MODULE_HANDLE_EX_FLAG_FROM_ADDRESS, (void*)hid_update_thread_proc, &module))
+        WARN("Failed to increase module's reference count, error: %lu\n", GetLastError());
 
     start_event = CreateEventA(NULL, FALSE, FALSE, NULL);
     if (!start_event) ERR("failed to create start event, error %lu\n", GetLastError());
 
-    stop_event = CreateEventA(NULL, FALSE, FALSE, NULL);
-    if (!stop_event) ERR("failed to create stop event, error %lu\n", GetLastError());
-
-    done_event = CreateEventA(NULL, FALSE, FALSE, NULL);
-    if (!done_event) ERR("failed to create done event, error %lu\n", GetLastError());
-
     update_event = CreateEventA(NULL, FALSE, FALSE, NULL);
     if (!update_event) ERR("failed to create update event, error %lu\n", GetLastError());
 
@@ -811,10 +791,6 @@ BOOL WINAPI DllMain(HINSTANCE inst, DWORD reason, LPVOID reserved)
         xinput_instance = inst;
         DisableThreadLibraryCalls(inst);
         break;
-    case DLL_PROCESS_DETACH:
-        if (reserved) break;
-        stop_update_thread();
-        break;
     }
     return TRUE;
 }
-- 
GitLab

https://gitlab.winehq.org/wine/wine/-/merge_requests/4065

[Zebediah Figura (＠zfigura)]

Just a drive-by comment, but I have to wonder if we actually need a thread for this? Could we replace RegisterDeviceNotificationW() with I_ScRegisterDeviceNotification() and do an async read in XInputGetState()?

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/4065#note_48217

[Rémi Bernon]

Reading from Wine NT devices is currently quite slow and heavy and I don't think we want to add that overhead to XInputGetState. Also it's a much larger change, and I'm not sure to see what advantage it has.

Fwiw XInputSetState is also supposed to be fast and asynchronous. It currently isn't and we instead avoid writing output reports if nothing changed, but that's probably incorrect and we should delegate the I/O to the thread instead.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/4065#note_48232

[PATCH 0/1] MR4065: xinput1_3: Hold module reference while the hid thread is running. - approved

[Rémi Bernon]

This merge request was approved by Rémi Bernon.
-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/4065