[PATCH 0/2] MR6093: user32/tests: Test repeated AttachThreadInput() calls.
For example, attaching thread A and B twice but detach them only once, then these two threads should still be attached. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6093
From: Zhiyi Zhang <zzhang(a)codeweavers.com> For example, attaching thread A and B twice but detach them only once, then these two threads should still be attached. --- dlls/user32/tests/input.c | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/dlls/user32/tests/input.c b/dlls/user32/tests/input.c index 9196e67c15b..47c26293dec 100644 --- a/dlls/user32/tests/input.c +++ b/dlls/user32/tests/input.c @@ -4516,7 +4516,9 @@ static LRESULT WINAPI MsgCheckProcA(HWND hwnd, UINT message, WPARAM wParam, LPAR if (message == WM_USER+1) { HWND hwnd = (HWND)lParam; + todo_wine_if(wParam) ok(GetFocus() == hwnd, "thread expected focus %p, got %p\n", hwnd, GetFocus()); + todo_wine_if(wParam) ok(GetActiveWindow() == hwnd, "thread expected active %p, got %p\n", hwnd, GetActiveWindow()); } return DefWindowProcA(hwnd, message, wParam, lParam); @@ -4662,6 +4664,27 @@ static void test_attach_input(void) SendMessageA(wnd_event.hwnd, WM_USER+1, 0, 0); + /* Test repeated AttachThreadInput() calls */ + SetFocus(ourWnd); + SetActiveWindow(ourWnd); + + ret = AttachThreadInput(GetCurrentThreadId(), tid, TRUE); + ok(ret, "AttachThreadInput error %ld\n", GetLastError()); + SendMessageA(wnd_event.hwnd, WM_USER+1, 0, (LPARAM)ourWnd); + + ret = AttachThreadInput(GetCurrentThreadId(), tid, TRUE); + ok(ret, "AttachThreadInput error %ld\n", GetLastError()); + SendMessageA(wnd_event.hwnd, WM_USER+1, 0, (LPARAM)ourWnd); + + ret = AttachThreadInput(GetCurrentThreadId(), tid, FALSE); + ok(ret, "AttachThreadInput error %ld\n", GetLastError()); + SendMessageA(wnd_event.hwnd, WM_USER+1, TRUE, (LPARAM)ourWnd); + + ret = AttachThreadInput(GetCurrentThreadId(), tid, FALSE); + todo_wine + ok(ret, "AttachThreadInput error %ld\n", GetLastError()); + SendMessageA(wnd_event.hwnd, WM_USER+1, 0, 0); + ret = PostMessageA(wnd_event.hwnd, WM_QUIT, 0, 0); ok(ret, "PostMessageA(WM_QUIT) error %ld\n", GetLastError()); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/6093
From: Zhiyi Zhang <zzhang(a)codeweavers.com> Fix an application failing to receive keyboard input after changing focus. --- dlls/user32/tests/input.c | 3 +-- server/queue.c | 24 +++++++++++++++++++++--- server/user.h | 2 +- server/winstation.c | 2 +- 4 files changed, 24 insertions(+), 7 deletions(-) diff --git a/dlls/user32/tests/input.c b/dlls/user32/tests/input.c index 47c26293dec..ede787f1096 100644 --- a/dlls/user32/tests/input.c +++ b/dlls/user32/tests/input.c @@ -4678,10 +4678,9 @@ static void test_attach_input(void) ret = AttachThreadInput(GetCurrentThreadId(), tid, FALSE); ok(ret, "AttachThreadInput error %ld\n", GetLastError()); - SendMessageA(wnd_event.hwnd, WM_USER+1, TRUE, (LPARAM)ourWnd); + SendMessageA(wnd_event.hwnd, WM_USER+1, 0, (LPARAM)ourWnd); ret = AttachThreadInput(GetCurrentThreadId(), tid, FALSE); - todo_wine ok(ret, "AttachThreadInput error %ld\n", GetLastError()); SendMessageA(wnd_event.hwnd, WM_USER+1, 0, 0); diff --git a/server/queue.c b/server/queue.c index ee03ceb55c8..d00b6a2e0c9 100644 --- a/server/queue.c +++ b/server/queue.c @@ -131,6 +131,7 @@ struct msg_queue lparam_t next_timer_id; /* id for the next timer with a 0 window */ struct timeout_user *timeout; /* timeout for next timer to expire */ struct thread_input *input; /* thread input descriptor */ + int attach_input_count; /* how many times the thread input has attached to the other thread */ struct hook_table *hooks; /* hook table */ timeout_t last_get_msg; /* time of last get message call */ int keystate_lock; /* owns an input keystate lock */ @@ -311,6 +312,7 @@ static struct msg_queue *create_msg_queue( struct thread *thread, struct thread_ queue->next_timer_id = 0x7fff; queue->timeout = NULL; queue->input = (struct thread_input *)grab_object( input ); + queue->attach_input_count = 0; queue->hooks = NULL; queue->last_get_msg = current_time; queue->keystate_lock = 0; @@ -423,6 +425,7 @@ static int assign_thread_input( struct thread *thread, struct thread_input *new_ } SHARED_WRITE_END; + queue->attach_input_count = 0; return 1; } @@ -1408,6 +1411,13 @@ int attach_thread_input( struct thread *thread_from, struct thread *thread_to ) struct thread_input *input, *old_input; int ret; + if (thread_from->queue && thread_to->queue + && thread_from->queue->input == thread_to->queue->input) + { + thread_from->queue->attach_input_count++; + return 1; + } + if (!thread_to->queue && !(thread_to->queue = create_msg_queue( thread_to, NULL ))) return 0; if (!(desktop = get_thread_desktop( thread_from, 0 ))) return 0; input = (struct thread_input *)grab_object( thread_to->queue->input ); @@ -1436,17 +1446,25 @@ int attach_thread_input( struct thread *thread_from, struct thread *thread_to ) } ret = assign_thread_input( thread_from, input ); - if (ret) memset( input->keystate, 0, sizeof(input->keystate) ); + if (ret) + { + if (thread_from->queue) + thread_from->queue->attach_input_count = 1; + memset( input->keystate, 0, sizeof(input->keystate) ); + } release_object( input ); return ret; } /* detach two thread input data structures */ -void detach_thread_input( struct thread *thread_from ) +void detach_thread_input( struct thread *thread_from, int force ) { struct thread *thread; struct thread_input *input, *old_input = thread_from->queue->input; + if (!force && --thread_from->queue->attach_input_count > 0) + return; + if ((input = create_thread_input( thread_from ))) { const input_shm_t *old_input_shm, *input_shm; @@ -3660,7 +3678,7 @@ DECL_HANDLER(attach_thread_input) { if (thread_from->queue && thread_to->queue && thread_from->queue->input == thread_to->queue->input) - detach_thread_input( thread_from ); + detach_thread_input( thread_from, 0 ); else set_error( STATUS_ACCESS_DENIED ); } diff --git a/server/user.h b/server/user.h index 6f10f296200..2d572d20e9b 100644 --- a/server/user.h +++ b/server/user.h @@ -120,7 +120,7 @@ extern void queue_cleanup_window( struct thread *thread, user_handle_t win ); extern int init_thread_queue( struct thread *thread ); extern void check_thread_queue_idle( struct thread *thread ); extern int attach_thread_input( struct thread *thread_from, struct thread *thread_to ); -extern void detach_thread_input( struct thread *thread_from ); +extern void detach_thread_input( struct thread *thread_from, int force ); extern void set_clip_rectangle( struct desktop *desktop, const rectangle_t *rect, unsigned int flags, int reset ); extern void post_message( user_handle_t win, unsigned int message, diff --git a/server/winstation.c b/server/winstation.c index 76a23b197a4..f0a1158d164 100644 --- a/server/winstation.c +++ b/server/winstation.c @@ -823,7 +823,7 @@ DECL_HANDLER(set_thread_desktop) if (!current->process->desktop) set_process_default_desktop( current->process, new_desktop, req->handle ); - if (old_desktop != new_desktop && current->queue) detach_thread_input( current ); + if (old_desktop != new_desktop && current->queue) detach_thread_input( current, 1 ); if (old_desktop) release_object( old_desktop ); release_object( new_desktop ); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/6093
Hi, It looks like your patch introduced the new failures shown below. Please investigate and fix them before resubmitting your patch. If they are not new, fixing them anyway would help a lot. Otherwise please ask for the known failures list to be updated. The tests also ran into some preexisting test failures. If you know how to fix them that would be helpful. See the TestBot job for the details: The full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=147140 Your paranoid android. === w11pro64 (32 bit report) === user32: input.c:4309: Test failed: button_up_hwnd 0: got MSG_TEST_WIN hwnd 00050040, msg WM_LBUTTONUP, wparam 0, lparam 0x320032 input.c:4309: Test failed: button_up_hwnd 1 (missing): MSG_TEST_WIN hwnd 00050040, WM_LBUTTONUP, wparam 0, lparam 0x320032 input.c:4417: Test failed: layered 0: button_up_hwnd_todo 0: got MSG_TEST_WIN hwnd 00050040, msg WM_LBUTTONUP, wparam 0, lparam 0x320032 input.c:4417: Test failed: layered 0: button_up_hwnd_todo 1 (missing): MSG_TEST_WIN hwnd 00050040, WM_LBUTTONUP, wparam 0, lparam 0x320032 input.c:4417: Test failed: layered 3: button_up_hwnd_todo 0: got MSG_TEST_WIN hwnd 000E00F0, msg WM_LBUTTONUP, wparam 0, lparam 0x320032 input.c:4417: Test failed: layered 3: button_up_hwnd_todo 1 (missing): MSG_TEST_WIN hwnd 000E00F0, WM_LBUTTONUP, wparam 0, lparam 0x320032 input.c:4437: Test failed: layered 3: button_up_hwnd_todo 0: got MSG_TEST_WIN hwnd 000E00F0, msg WM_LBUTTONUP, wparam 0, lparam 0x320032 input.c:4437: Test failed: layered 3: button_up_hwnd_todo 1 (missing): MSG_TEST_WIN hwnd 000E00F0, WM_LBUTTONUP, wparam 0, lparam 0x320032 === debian11 (32 bit zh:CN report) === user32: input.c:4305: Test succeeded inside todo block: button_down_hwnd_todo 1: got MSG_TEST_WIN hwnd 0004009A, msg WM_LBUTTONDOWN, wparam 0x1, lparam 0x320032 === debian11b (64 bit WoW report) === ddraw: ddraw1.c:3645: Test failed: Expected (0,0)-(640,480), got (-32000,-32000)-(-31840,-31969). Report validation errors: mshtml:script crashed (c0000005)
Rémi Bernon (@rbernon) commented about server/queue.c:
struct thread_input *input, *old_input; int ret;
+ if (thread_from->queue && thread_to->queue + && thread_from->queue->input == thread_to->queue->input) + { + thread_from->queue->attach_input_count++;
This assumes that the tid are always in the same order, what if the order is reversed? What if a third thread gets attached? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6093#note_76321
This assumes that the tid are always in the same order, what if the order is reversed? What if a third thread gets attached?
Yes, this would not work correctly when the TIDs are reversed or more than two threads are involved. But so was the case before this patch. The complete fix is more complicated. For example, say thread A attaches to thread B, and thread C attaches to thread D, then thread B attaches to thread C. Now all four threads should have the same input according to my local tests. Then if you detach thread B from thread C, thread C and thread D are still attached, and thread A and thread B are still attached as well. So this would require a rewrite to use trees or graphs with refcount. So I added the refcount first to fix the application. I think it would be an improvement on top of what we have without major changes. If this is not enough, then I guess I will try to implement it properly when I have time. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6093#note_76324
On Tue Jul 16 14:40:08 2024 +0000, Zhiyi Zhang wrote:
This assumes that the tid are always in the same order, what if the order is reversed? What if a third thread gets attached? Yes, this would not work correctly when the TIDs are reversed or more than two threads are involved. But so was the case before this patch. The complete fix is more complicated. For example, say thread A attaches to thread B, and thread C attaches to thread D, then thread B attaches to thread C. Now all four threads should have the same input according to my local tests. Then if you detach thread B from thread C, thread C and thread D are still attached, and thread A and thread B are still attached as well. So this would require a rewrite to use trees or graphs with refcount. So I added the refcount first to fix the application. I think it would be an improvement on top of what we have without major changes. If this is not enough, then I guess I will try to implement it properly when I have time. I don't know, this still looks confusing to me, reading the now code it isn't obvious why this is breaking symmetry and that looks wrong.
Yes, the current code is broken already but we didn't know that it was supposed to be a graph. Now that we do, we shouldn't shy away from implementing it correctly. Anyway, I will be away for some time so I don't want to block this unnecessarily. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6093#note_76532
On Thu Jul 18 16:39:01 2024 +0000, Rémi Bernon wrote:
I don't know, this still looks confusing to me, reading the now code it isn't obvious why this is breaking symmetry and that looks wrong. Yes, the current code is broken already but we didn't know that it was supposed to be a graph. Now that we do, we shouldn't shy away from implementing it correctly. Anyway, I will be away for some time so I don't want to block this unnecessarily. Okay, I will revisit this when I have time. Closing this for now.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6093#note_76947
This merge request was closed by Zhiyi Zhang. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6093
participants (4)
-
Marvin -
Rémi Bernon -
Zhiyi Zhang
-
Zhiyi Zhang (＠zhiyi)