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.
From: Arkadiusz Hiler ahiler@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; }
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()?
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.
This merge request was approved by Rémi Bernon.
-
Arek Hiler (＠ivyl) -
Arkadiusz Hiler
-
Rémi Bernon (＠rbernon)
-
Zebediah Figura (＠zfigura)