On thread destroy, a WM_WINE_DESTROYWINDOW is sent to the child windows living in other threads.
There's then a race condition between these threads peeking for messages and the current thread detaching its child windows from their threads and clearing their message queues, and the message may never be received from these threads and the windows kept alive.
Signed-off-by: Rémi Bernon rbernon@codeweavers.com --- dlls/user32/tests/win.c | 240 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 240 insertions(+)
diff --git a/dlls/user32/tests/win.c b/dlls/user32/tests/win.c index 6b13d93be14..00e20719ba7 100644 --- a/dlls/user32/tests/win.c +++ b/dlls/user32/tests/win.c @@ -719,6 +719,245 @@ static void test_enum_thread_windows(void) CloseHandle( handle ); }
+struct test_thread_exit_parent_params +{ + HWND hwnd; + HANDLE created_event; + HANDLE stop_event; +}; + +static DWORD CALLBACK test_thread_exit_parent_thread( void *args ) +{ + struct test_thread_exit_parent_params *params = args; + DWORD ret; + MSG msg; + + params->hwnd = CreateWindowW( L"static", L"parent", WS_OVERLAPPEDWINDOW|WS_VISIBLE, + 100, 100, 200, 200, 0, 0, 0, NULL ); + ok( params->hwnd != 0, "CreateWindowExW failed, error %u\n", GetLastError() ); + flush_events( TRUE ); + SetEvent( params->created_event ); + + do + { + while (PeekMessageW( &msg, 0, 0, 0, PM_REMOVE )) DispatchMessageW( &msg ); + ret = MsgWaitForMultipleObjects( 1, ¶ms->stop_event, FALSE, INFINITE, QS_ALLINPUT ); + } + while (ret != WAIT_OBJECT_0); + + return 0; +} + +static LRESULT CALLBACK test_thread_exit_wnd_proc( HWND hwnd, UINT msg, WPARAM wp, LPARAM lp ) +{ + if (msg == WM_USER) return 0xdeadbeef; + if (msg == WM_USER + 1) return 0xfeedcafe; + return DefWindowProcW( hwnd, msg, wp, lp ); +} + +static void test_thread_exit_destroy(void) +{ + struct test_thread_exit_parent_params params; + HWND adopter, child1, child2, child3; + WNDPROC old_wndproc, wndproc; + WCHAR buffer[MAX_PATH]; + HANDLE thread; + DWORD ret; + HRGN rgn; + HWND tmp; + MSG msg; + + params.created_event = CreateEventW( NULL, FALSE, FALSE, NULL ); + params.stop_event = CreateEventW( NULL, FALSE, FALSE, NULL ); + + adopter = CreateWindowW( L"static", L"adopter", WS_OVERLAPPEDWINDOW|WS_VISIBLE, + 300, 100, 200, 200, 0, 0, 0, NULL ); + ok( adopter != 0, "CreateWindowExW failed, error %u\n", GetLastError() ); + flush_events( TRUE ); + + thread = CreateThread( NULL, 0, test_thread_exit_parent_thread, ¶ms, 0, NULL ); + ok( thread != 0, "CreateThread failed, error %u\n", GetLastError() ); + WaitForSingleObject( params.created_event, INFINITE ); + + child1 = CreateWindowW( L"static", L"child1", WS_CHILD|WS_VISIBLE, + 50, 50, 50, 50, params.hwnd, 0, 0, NULL ); + ok( child1 != 0, "CreateWindowExW failed, error %u\n", GetLastError() ); + child2 = CreateWindowW( L"static", L"child2", WS_CHILD|WS_VISIBLE, + 100, 50, 50, 50, params.hwnd, 0, 0, NULL ); + ok( child2 != 0, "CreateWindowExW failed, error %u\n", GetLastError() ); + child3 = CreateWindowW( L"static", L"child3", WS_CHILD|WS_VISIBLE, + 50, 100, 50, 50, params.hwnd, 0, 0, NULL ); + ok( child3 != 0, "CreateWindowExW failed, error %u\n", GetLastError() ); + flush_events( TRUE ); + + trace("parent %p adopter %p child1 %p child2 %p child3 %p\n", params.hwnd, adopter, child1, child2, child3); + + SetActiveWindow( child1 ); + SetFocus( child1 ); + SetCapture( child1 ); + + ok( GetActiveWindow() == params.hwnd, "GetActiveWindow %p, expected %p\n", GetActiveWindow(), params.hwnd ); + ok( GetFocus() == child1, "GetFocus %p, expected %p\n", GetFocus(), child1 ); + ok( GetCapture() == child1, "GetCapture %p, expected %p\n", GetCapture(), child1 ); + + ret = SetPropW( child1, L"myprop", UlongToHandle(0xdeadbeef) ); + ok( ret, "SetPropW failed, error %u\n", GetLastError() ); + ret = SetPropW( child2, L"myprop", UlongToHandle(0xdeadbeef) ); + ok( ret, "SetPropW failed, error %u\n", GetLastError() ); + + old_wndproc = (WNDPROC)GetWindowLongPtrW( child1, GWLP_WNDPROC ); + ok( old_wndproc != NULL, "GetWindowLongPtrW GWLP_WNDPROC failed, error %u\n", GetLastError() ); + + ret = GetWindowLongW( child1, GWL_STYLE ); + ok( ret == (WS_CHILD|WS_VISIBLE), "GetWindowLongW returned %#x\n", ret ); + + SetEvent( params.stop_event ); + ret = WaitForSingleObject( thread, INFINITE ); + ok( ret == WAIT_OBJECT_0, "WaitForSingleObject returned %#x\n", ret ); + CloseHandle( thread ); + + /* child windows should all still be alive but hidden */ + ret = IsWindow( child1 ); + ok( ret, "IsWindow returned %u\n", ret ); + ret = IsWindow( child2 ); + ok( ret, "IsWindow returned %u\n", ret ); + ret = IsWindow( child3 ); + ok( ret, "IsWindow returned %u\n", ret ); + + todo_wine + ok( GetActiveWindow() == adopter, "GetActiveWindow %p, expected %p\n", GetActiveWindow(), adopter ); + todo_wine + ok( GetFocus() == adopter, "GetFocus %p, expected %p\n", GetFocus(), adopter ); + todo_wine + ok( GetCapture() == child1, "GetCapture %p, expected %p\n", GetCapture(), child1 ); + + SetActiveWindow( child1 ); + SetFocus( child1 ); + SetCapture( child1 ); + + todo_wine + ok( GetActiveWindow() == adopter, "GetActiveWindow %p, expected %p\n", GetActiveWindow(), adopter ); + todo_wine + ok( GetFocus() == adopter, "GetFocus %p, expected %p\n", GetFocus(), adopter ); + todo_wine + ok( GetCapture() == child1, "GetCapture %p, expected %p\n", GetCapture(), child1 ); + + SetLastError( 0xdeadbeef ); + ret = GetWindowLongW( child1, GWL_STYLE ); + todo_wine + ok( ret == WS_CHILD, "GetWindowLongW returned %#x\n", ret ); + ok( GetLastError() == 0xdeadbeef, "GetWindowLongW error %u\n", GetLastError() ); + ret = SetWindowLongW( child1, GWL_STYLE, WS_CHILD|WS_VISIBLE ); + todo_wine + ok( ret, "SetWindowLongW failed, error %u\n", GetLastError() ); + ret = GetWindowLongW( child1, GWL_STYLE ); + ok( ret == (WS_CHILD|WS_VISIBLE), "GetWindowLongW returned %#x\n", ret ); + + /* and cannot be adopted */ + SetLastError( 0xdeadbeef ); + tmp = GetParent( child1 ); + ok( tmp == params.hwnd, "GetParent returned %p, error %u\n", tmp, GetLastError() ); + ok( GetLastError() == 0xdeadbeef, "GetWindowLongW error %u\n", GetLastError() ); + SetLastError( 0xdeadbeef ); + tmp = SetParent( child1, adopter ); + ok( tmp == 0, "SetParent returned %p\n", tmp ); + todo_wine + ok( GetLastError() == ERROR_INVALID_PARAMETER, "got error %u\n", GetLastError() ); + SetLastError( 0xdeadbeef ); + tmp = SetParent( child3, adopter ); + ok( tmp == 0, "SetParent returned %p\n", tmp ); + todo_wine + ok( GetLastError() == ERROR_INVALID_PARAMETER, "got error %u\n", GetLastError() ); + SetLastError( 0xdeadbeef ); + tmp = GetParent( child1 ); + ok( tmp == params.hwnd, "GetParent returned %p, error %u\n", tmp, GetLastError() ); + ok( GetLastError() == 0xdeadbeef, "GetWindowLongW error %u\n", GetLastError() ); + + SetLastError( 0xdeadbeef ); + ret = GetWindowLongW( params.hwnd, GWL_STYLE ); + ok( ret == 0, "GetWindowLongW returned %#x\n", ret ); + ok( GetLastError() == ERROR_INVALID_WINDOW_HANDLE, "GetWindowLongW error %u\n", GetLastError() ); + + wndproc = (WNDPROC)GetWindowLongPtrW( child1, GWLP_WNDPROC ); + ok( wndproc != NULL, "GetWindowLongPtrW GWLP_WNDPROC failed, error %u\n", GetLastError() ); + ok( wndproc == old_wndproc, "GetWindowLongPtrW GWLP_WNDPROC returned %p\n", wndproc ); + + tmp = GetPropW( child1, L"myprop" ); + todo_wine + ok( HandleToULong(tmp) == 0xdeadbeef, "GetPropW returned %p\n", tmp ); + tmp = GetPropW( child2, L"myprop" ); + todo_wine + ok( HandleToULong(tmp) == 0xdeadbeef, "GetPropW returned %p\n", tmp ); + + /* destroying child1 ourselves succeeds */ + ret = DestroyWindow( child1 ); + ok( ret, "DestroyWindow returned %u\n", ret ); + ret = DestroyWindow( child1 ); + ok( !ret, "DestroyWindow returned %u\n", ret ); + ret = IsWindow( child1 ); + ok( !ret, "IsWindow returned %u\n", ret ); + + tmp = GetPropW( child1, L"myprop" ); + ok( HandleToULong(tmp) == 0, "GetPropW returned %p\n", tmp ); + + /* child2 is still alive, for now */ + ret = IsWindow( child2 ); + ok( ret, "IsWindow returned %u\n", ret ); + + SetLastError( 0xdeadbeef ); + ret = SetWindowPos( child2, HWND_TOPMOST, 0, 0, 100, 100, SWP_NOSIZE|SWP_NOMOVE ); + ok( !ret, "SetWindowPos succeeded\n" ); + todo_wine + ok( GetLastError() == ERROR_INVALID_PARAMETER, "SetWindowPos returned error %u\n", GetLastError() ); + SetLastError( 0xdeadbeef ); + ret = SetWindowPos( child2, 0, 10, 10, 200, 200, SWP_NOZORDER | SWP_NOACTIVATE ); + ok( !ret, "SetWindowPos succeeded\n" ); + todo_wine + ok( GetLastError() == ERROR_INVALID_PARAMETER, "SetWindowPos returned error %u\n", GetLastError() ); + + rgn = CreateRectRgn( 5, 5, 15, 15 ); + SetLastError( 0xdeadbeef ); + ret = SetWindowRgn( child2, rgn, TRUE ); + todo_wine + ok( ret, "SetWindowRgn failed, error %u\n", GetLastError() ); + DeleteObject( rgn ); + + wndproc = (WNDPROC)SetWindowLongPtrW( child2, GWLP_WNDPROC, (LONG_PTR)test_thread_exit_wnd_proc ); + ret = SendMessageW( child2, WM_USER, 0, 0 ); + ok( ret == 0xdeadbeef, "SendMessageW returned %u, error %u\n", ret, GetLastError() ); + ret = SendMessageW( child2, WM_USER + 1, 0, 0 ); + ok( ret == 0xfeedcafe, "SendMessageW returned %u, error %u\n", ret, GetLastError() ); + ret = SendMessageW( child2, WM_USER + 2, 0, 0 ); + ok( ret == 0, "SendMessageW returned %u, error %u\n", ret, GetLastError() ); + + ret = GetWindowTextW( child2, buffer, ARRAY_SIZE(buffer) ); + ok( ret == 6, "GetWindowTextW returned %u\n", ret ); + ok( !wcscmp( buffer, L"child2" ), "GetWindowTextW returned %s\n", debugstr_w( buffer ) ); + ret = IsWindow( child2 ); + ok( ret, "IsWindow returned %u\n", ret ); + + /* but peeking any message should reap them all */ + PeekMessageW( &msg, child2, 0, 0, PM_REMOVE ); + + tmp = GetPropW( child2, L"myprop" ); + ok( HandleToULong(tmp) == 0, "GetPropW returned %p\n", tmp ); + + ret = IsWindow( child2 ); + todo_wine + ok( !ret, "IsWindow returned %u\n", ret ); + ret = IsWindow( child3 ); + todo_wine + ok( !ret, "IsWindow returned %u\n", ret ); + ret = DestroyWindow( child2 ); + todo_wine + ok( !ret, "DestroyWindow returned %u\n", ret ); + + DestroyWindow( adopter ); + + CloseHandle( params.created_event ); + CloseHandle( params.stop_event ); +} + static struct wm_gettext_override_data { BOOL enabled; /* when 1 bypasses default procedure */ @@ -12396,6 +12635,7 @@ START_TEST(win) test_CreateWindow(); test_parent_owner(); test_enum_thread_windows(); + test_thread_exit_destroy();
test_mdi(); test_icons();
This test is failing from time to time. Making sure the parent thread has terminated, before continuing, triggers the underlying race shown in previous patch, and makes the test to always fail.
Signed-off-by: Rémi Bernon rbernon@codeweavers.com --- dlls/user32/tests/msg.c | 52 ++++++++++++++++++++++++----------------- 1 file changed, 31 insertions(+), 21 deletions(-)
diff --git a/dlls/user32/tests/msg.c b/dlls/user32/tests/msg.c index 43253953a9e..cc362623cf1 100644 --- a/dlls/user32/tests/msg.c +++ b/dlls/user32/tests/msg.c @@ -8982,7 +8982,10 @@ static void test_paint_messages(void) struct wnd_event { HWND hwnd; + HWND child_hwnd; + HANDLE child; HANDLE grand_child; + HANDLE ready_event; HANDLE start_event; HANDLE stop_event; }; @@ -9013,42 +9016,48 @@ static DWORD CALLBACK create_grand_child_thread( void *param ) { struct wnd_event *wnd_event = param; HWND hchild; - MSG msg; + DWORD ret;
hchild = CreateWindowExA(0, "TestWindowClass", "Test child", - WS_CHILD | WS_VISIBLE, 0, 0, 10, 10, wnd_event->hwnd, 0, 0, NULL); + WS_CHILD | WS_VISIBLE, 0, 0, 10, 10, wnd_event->child_hwnd, 0, 0, NULL); ok (hchild != 0, "Failed to create child window\n"); flush_events(); flush_sequence(); SetEvent( wnd_event->start_event );
- for (;;) - { - MsgWaitForMultipleObjects(0, NULL, FALSE, 1000, QS_ALLINPUT); - if (!IsWindow( hchild )) break; /* will be destroyed when parent thread exits */ - while (PeekMessageA(&msg, 0, 0, 0, PM_REMOVE)) DispatchMessageA(&msg); - } + /* wait for wnd_event->child to be set */ + ret = WaitForSingleObject( wnd_event->ready_event, 1000 ); + ok( !ret, "WaitForSingleObject failed %x\n", ret ); + + /* wait for parent window thread to exit */ + ret = WaitForSingleObject( wnd_event->child, 1000 ); + ok( !ret, "WaitForSingleObject returned %x, error: %u\n", ret, GetLastError() ); + ok( IsWindow( hchild ), "Child window already destroyed\n" ); + flush_events(); + todo_wine ok( !IsWindow( hchild ), "Child window not destroyed\n" ); + return 0; }
static DWORD CALLBACK create_child_thread( void *param ) { struct wnd_event *wnd_event = param; - struct wnd_event child_event; DWORD ret, tid; MSG msg;
- child_event.hwnd = CreateWindowExA(0, "TestWindowClass", "Test child", - WS_CHILD | WS_VISIBLE, 0, 0, 10, 10, wnd_event->hwnd, 0, 0, NULL); - ok (child_event.hwnd != 0, "Failed to create child window\n"); - SetFocus( child_event.hwnd ); + wnd_event->child_hwnd = CreateWindowExA( 0, "TestWindowClass", "Test child", WS_CHILD | WS_VISIBLE, + 0, 0, 10, 10, wnd_event->hwnd, 0, 0, NULL ); + ok( wnd_event->child_hwnd != 0, "Failed to create child windows\n" ); + SetFocus( wnd_event->child_hwnd ); + + wnd_event->grand_child = CreateThread( NULL, 0, create_grand_child_thread, wnd_event, 0, &tid ); + ok( wnd_event->grand_child != 0, "CreateThread failed, error %u\n", GetLastError() ); + flush_events(); flush_sequence(); - child_event.start_event = wnd_event->start_event; - wnd_event->grand_child = CreateThread(NULL, 0, create_grand_child_thread, &child_event, 0, &tid); for (;;) { - DWORD ret = MsgWaitForMultipleObjects(1, &child_event.start_event, FALSE, 1000, QS_SENDMESSAGE); + DWORD ret = MsgWaitForMultipleObjects(1, &wnd_event->start_event, FALSE, 1000, QS_SENDMESSAGE); if (ret != 1) break; while (PeekMessageA(&msg, 0, 0, 0, PM_REMOVE)) DispatchMessageA(&msg); } @@ -9188,9 +9197,11 @@ static void test_interthread_messages(void) flush_events(); flush_sequence(); log_all_parent_messages++; + wnd_event.ready_event = CreateEventA( NULL, TRUE, FALSE, NULL ); wnd_event.start_event = CreateEventA( NULL, TRUE, FALSE, NULL ); wnd_event.stop_event = CreateEventA( NULL, TRUE, FALSE, NULL ); - hThread = CreateThread( NULL, 0, create_child_thread, &wnd_event, 0, &tid ); + wnd_event.child = CreateThread( NULL, 0, create_child_thread, &wnd_event, 0, &tid ); + SetEvent( wnd_event.ready_event ); for (;;) { ret = MsgWaitForMultipleObjects(1, &wnd_event.start_event, FALSE, 1000, QS_SENDMESSAGE); @@ -9200,18 +9211,17 @@ static void test_interthread_messages(void) ok( !ret, "MsgWaitForMultipleObjects failed %x\n", ret ); /* now wait for the thread without processing messages; this shouldn't deadlock */ SetEvent( wnd_event.stop_event ); - ret = WaitForSingleObject( hThread, 5000 ); - ok( !ret, "WaitForSingleObject failed %x\n", ret ); - CloseHandle( hThread );
ret = WaitForSingleObject( wnd_event.grand_child, 5000 ); ok( !ret, "WaitForSingleObject failed %x\n", ret ); CloseHandle( wnd_event.grand_child ); + CloseHandle( wnd_event.child );
CloseHandle( wnd_event.start_event ); CloseHandle( wnd_event.stop_event ); + CloseHandle( wnd_event.ready_event ); flush_events(); - ok_sequence(WmExitThreadSeq, "destroy child on thread exit", FALSE); + ok_sequence( WmExitThreadSeq, "destroy child on thread exit", TRUE ); log_all_parent_messages--; DestroyWindow( wnd_event.hwnd );
Hi,
While running your changed tests, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check?
Full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=100081
Your paranoid android.
=== w1064_tsign (32 bit report) ===
user32: msg.c:12834: Test failed: message time not advanced: 16462 16462 msg.c:12835: Test failed: coords not changed: (105 105) (105 105)
=== w10pro64_he (64 bit report) ===
user32: msg.c:12816: Test failed: message time not advanced: f915 f915 msg.c:12817: Test failed: coords not changed: (101 101) (101 101) msg.c:12834: Test failed: message time not advanced: f915 f915 msg.c:12835: Test failed: coords not changed: (101 101) (101 101)
Instead or using the parent pointer only.
This also checks if the pointer itself is NULL, as the function is sometimes called with win->parent parameter, which could be NULL for orphan windows.
Signed-off-by: Rémi Bernon rbernon@codeweavers.com --- server/window.c | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-)
diff --git a/server/window.c b/server/window.c index 7ea91d2a7dc..2ed4aa57d1a 100644 --- a/server/window.c +++ b/server/window.c @@ -80,6 +80,7 @@ struct window unsigned int is_unicode : 1; /* ANSI or unicode */ unsigned int is_linked : 1; /* is it linked into the parent z-order list? */ unsigned int is_layered : 1; /* has layered info been set? */ + unsigned int is_desktop : 1; /* is it a desktop window? */ unsigned int color_key; /* color key for a layered window */ unsigned int alpha; /* alpha value for a layered window */ unsigned int layered_flags; /* flags for a layered window */ @@ -140,7 +141,7 @@ static inline struct window *get_window( user_handle_t handle ) /* check if window is the desktop */ static inline int is_desktop_window( const struct window *win ) { - return !win->parent; /* only desktop windows have no parent */ + return win && !win->parent && win->is_desktop; }
/* get next window in Z-order list */ @@ -183,7 +184,7 @@ static inline void update_pixel_format_flags( struct window *win ) static unsigned int get_monitor_dpi( struct window *win ) { /* FIXME: we return the desktop window DPI for now */ - while (!is_desktop_window( win )) win = win->parent; + while (win->parent) win = win->parent; return win->dpi ? win->dpi : USER_DEFAULT_SCREEN_DPI; }
@@ -502,6 +503,7 @@ static struct window *create_window( struct window *parent, struct window *owner win->is_unicode = 1; win->is_linked = 0; win->is_layered = 0; + win->is_desktop = parent ? 0 : 1; win->dpi_awareness = DPI_AWARENESS_PER_MONITOR_AWARE; win->dpi = 0; win->user_data = 0; @@ -662,7 +664,7 @@ static void map_dpi_region( struct window *win, struct region *region, unsigned /* convert coordinates from client to screen coords */ static inline void client_to_screen( struct window *win, int *x, int *y ) { - for ( ; win && !is_desktop_window(win); win = win->parent) + for ( ; win && win->parent; win = win->parent) { *x += win->client_rect.left; *y += win->client_rect.top; @@ -833,7 +835,8 @@ struct thread *window_thread_from_point( user_handle_t scope, int x, int y ) static int all_windows_from_point( struct window *top, int x, int y, unsigned int dpi, struct user_handle_array *array ) { - if (!is_desktop_window( top ) && !is_desktop_window( top->parent )) + assert( top != NULL ); + if (top->parent && !is_desktop_window( top->parent )) { screen_to_client( top->parent, &x, &y, dpi ); dpi = top->parent->dpi; @@ -941,13 +944,14 @@ static struct region *intersect_window_region( struct region *region, struct win /* convert coordinates from client to screen coords */ static inline void client_to_screen_rect( struct window *win, rectangle_t *rect ) { - for ( ; win && !is_desktop_window(win); win = win->parent) + for ( ; win && win->parent; win = win->parent) offset_rect( rect, win->client_rect.left, win->client_rect.top ); }
/* map the region from window to screen coordinates */ static inline void map_win_region_to_screen( struct window *win, struct region *region ) { + assert( win != NULL ); if (!is_desktop_window(win)) { int x = win->window_rect.left; @@ -2287,7 +2291,7 @@ DECL_HANDLER(set_window_pos) unsigned int flags = req->swp_flags;
if (!win) return; - if (!win->parent) flags |= SWP_NOZORDER; /* no Z order for the desktop */ + if (is_desktop_window(win)) flags |= SWP_NOZORDER; /* no Z order for the desktop */
if (!(flags & SWP_NOZORDER)) {
Signed-off-by: Rémi Bernon rbernon@codeweavers.com --- dlls/user32/tests/msg.c | 4 ++-- dlls/user32/tests/win.c | 7 ------ server/window.c | 52 +++++++++++++++++++++++++++++++++++++---- 3 files changed, 50 insertions(+), 13 deletions(-)
diff --git a/dlls/user32/tests/msg.c b/dlls/user32/tests/msg.c index cc362623cf1..57cb9685254 100644 --- a/dlls/user32/tests/msg.c +++ b/dlls/user32/tests/msg.c @@ -9034,7 +9034,7 @@ static DWORD CALLBACK create_grand_child_thread( void *param ) ok( !ret, "WaitForSingleObject returned %x, error: %u\n", ret, GetLastError() ); ok( IsWindow( hchild ), "Child window already destroyed\n" ); flush_events(); - todo_wine ok( !IsWindow( hchild ), "Child window not destroyed\n" ); + ok( !IsWindow( hchild ), "Child window not destroyed\n" );
return 0; } @@ -9221,7 +9221,7 @@ static void test_interthread_messages(void) CloseHandle( wnd_event.stop_event ); CloseHandle( wnd_event.ready_event ); flush_events(); - ok_sequence( WmExitThreadSeq, "destroy child on thread exit", TRUE ); + ok_sequence( WmExitThreadSeq, "destroy child on thread exit", FALSE ); log_all_parent_messages--; DestroyWindow( wnd_event.hwnd );
diff --git a/dlls/user32/tests/win.c b/dlls/user32/tests/win.c index 00e20719ba7..b2f3f48a504 100644 --- a/dlls/user32/tests/win.c +++ b/dlls/user32/tests/win.c @@ -828,7 +828,6 @@ static void test_thread_exit_destroy(void) ok( GetActiveWindow() == adopter, "GetActiveWindow %p, expected %p\n", GetActiveWindow(), adopter ); todo_wine ok( GetFocus() == adopter, "GetFocus %p, expected %p\n", GetFocus(), adopter ); - todo_wine ok( GetCapture() == child1, "GetCapture %p, expected %p\n", GetCapture(), child1 );
SetActiveWindow( child1 ); @@ -839,7 +838,6 @@ static void test_thread_exit_destroy(void) ok( GetActiveWindow() == adopter, "GetActiveWindow %p, expected %p\n", GetActiveWindow(), adopter ); todo_wine ok( GetFocus() == adopter, "GetFocus %p, expected %p\n", GetFocus(), adopter ); - todo_wine ok( GetCapture() == child1, "GetCapture %p, expected %p\n", GetCapture(), child1 );
SetLastError( 0xdeadbeef ); @@ -883,10 +881,8 @@ static void test_thread_exit_destroy(void) ok( wndproc == old_wndproc, "GetWindowLongPtrW GWLP_WNDPROC returned %p\n", wndproc );
tmp = GetPropW( child1, L"myprop" ); - todo_wine ok( HandleToULong(tmp) == 0xdeadbeef, "GetPropW returned %p\n", tmp ); tmp = GetPropW( child2, L"myprop" ); - todo_wine ok( HandleToULong(tmp) == 0xdeadbeef, "GetPropW returned %p\n", tmp );
/* destroying child1 ourselves succeeds */ @@ -918,7 +914,6 @@ static void test_thread_exit_destroy(void) rgn = CreateRectRgn( 5, 5, 15, 15 ); SetLastError( 0xdeadbeef ); ret = SetWindowRgn( child2, rgn, TRUE ); - todo_wine ok( ret, "SetWindowRgn failed, error %u\n", GetLastError() ); DeleteObject( rgn );
@@ -943,13 +938,11 @@ static void test_thread_exit_destroy(void) ok( HandleToULong(tmp) == 0, "GetPropW returned %p\n", tmp );
ret = IsWindow( child2 ); - todo_wine ok( !ret, "IsWindow returned %u\n", ret ); ret = IsWindow( child3 ); todo_wine ok( !ret, "IsWindow returned %u\n", ret ); ret = DestroyWindow( child2 ); - todo_wine ok( !ret, "DestroyWindow returned %u\n", ret );
DestroyWindow( adopter ); diff --git a/server/window.c b/server/window.c index 2ed4aa57d1a..8fcc2fdde2c 100644 --- a/server/window.c +++ b/server/window.c @@ -144,6 +144,12 @@ static inline int is_desktop_window( const struct window *win ) return win && !win->parent && win->is_desktop; }
+/* check if window has lost its parent */ +static inline int is_orphan_window( const struct window *win ) +{ + return !win->parent && !win->is_desktop; +} + /* get next window in Z-order list */ static inline struct window *get_next_window( struct window *win ) { @@ -689,6 +695,7 @@ static int is_visible( const struct window *win ) { while (win) { + if (is_orphan_window( win )) return 0; if (!(win->style & WS_VISIBLE)) return 0; win = win->parent; /* if parent is minimized children are not visible */ @@ -1196,6 +1203,7 @@ static int get_window_visible_rect( struct window *win, rectangle_t *rect, int f *rect = frame ? win->window_rect : win->client_rect;
if (!(win->style & WS_VISIBLE)) return 0; + if (is_orphan_window( win )) return 0; if (is_desktop_window( win )) return 1;
while (!is_desktop_window( win->parent )) @@ -1893,9 +1901,25 @@ void destroy_window( struct window *win )
/* destroy all children */ while (!list_empty(&win->children)) - destroy_window( LIST_ENTRY( list_head(&win->children), struct window, entry )); + { + struct window *child = LIST_ENTRY( list_head( &win->children ), struct window, entry ); + if (!child->thread || child->thread == win->thread) destroy_window( child ); + else + { + list_remove( &child->entry ); + child->parent = NULL; + } + } while (!list_empty(&win->unlinked)) - destroy_window( LIST_ENTRY( list_head(&win->unlinked), struct window, entry )); + { + struct window *child = LIST_ENTRY( list_head( &win->unlinked ), struct window, entry ); + if (!child->thread || child->thread == win->thread) destroy_window( child ); + else + { + list_remove( &child->entry ); + child->parent = NULL; + } + }
/* reset global window pointers, if the corresponding window is destroyed */ if (win == shell_window) shell_window = NULL; @@ -1938,6 +1962,11 @@ DECL_HANDLER(create_window)
reply->handle = 0; if (req->parent && !(parent = get_window( req->parent ))) return; + if (parent && is_orphan_window( parent )) + { + set_error( STATUS_INVALID_PARAMETER ); + return; + }
if (req->owner) { @@ -1988,8 +2017,7 @@ DECL_HANDLER(set_parent)
if (!(win = get_window( req->handle ))) return; if (req->parent && !(parent = get_window( req->parent ))) return; - - if (is_desktop_window(win)) + if (!win->parent) { set_error( STATUS_INVALID_PARAMETER ); return; @@ -2110,6 +2138,12 @@ DECL_HANDLER(set_window_info) struct window *win = get_window( req->handle );
if (!win) return; + if (is_orphan_window( win )) + { + set_error( STATUS_INVALID_PARAMETER ); + return; + } + if (req->flags && is_desktop_window(win) && win->thread != current) { set_error( STATUS_ACCESS_DENIED ); @@ -2291,6 +2325,11 @@ DECL_HANDLER(set_window_pos) unsigned int flags = req->swp_flags;
if (!win) return; + if (is_orphan_window( win )) + { + set_error( STATUS_INVALID_PARAMETER ); + return; + } if (is_desktop_window(win)) flags |= SWP_NOZORDER; /* no Z order for the desktop */
if (!(flags & SWP_NOZORDER)) @@ -2479,6 +2518,11 @@ DECL_HANDLER(get_visible_region) struct window *top, *win = get_window( req->window );
if (!win) return; + if (is_orphan_window( win )) + { + set_error( STATUS_INVALID_PARAMETER ); + return; + }
top = get_top_clipping_window( win ); if ((region = get_visible_region( win, req->flags )))
Hi,
While running your changed tests, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check?
Full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=100083
Your paranoid android.
=== w1064v1809 (64 bit report) ===
user32: msg.c:15250: Test failed: unexpected message 31f msg.c:15251: Test failed: bad wparam 1 msg.c:15257: Test failed: unicode WM_CHAR: 0: the msg sequence is not complete: expected 0102 - actual 0000
=== w1064_tsign (32 bit report) ===
user32: win.c:4305: Test failed: hwnd 00020178/00020178 message 0200 win.c:4309: Test failed: hwnd 00020178/00020178 message 0201 win.c:4318: Test failed: hwnd 00860274/00860274 message 0202 win.c:4321: Test failed: hwnd 00860274/00860274 message 0201
=== w1064 (64 bit report) ===
user32: win.c:10512: Test failed: pos = 014a015e
=== debiant2 (32 bit Hebrew:Israel report) ===
user32: win.c:10652: Test failed: Expected foreground window 00030068, got 00AC00AC
Signed-off-by: Rémi Bernon rbernon@codeweavers.com --- dlls/user32/tests/win.c | 4 ---- dlls/user32/win.c | 2 +- dlls/user32/winpos.c | 2 +- 3 files changed, 2 insertions(+), 6 deletions(-)
diff --git a/dlls/user32/tests/win.c b/dlls/user32/tests/win.c index b2f3f48a504..f128d15419d 100644 --- a/dlls/user32/tests/win.c +++ b/dlls/user32/tests/win.c @@ -859,12 +859,10 @@ static void test_thread_exit_destroy(void) SetLastError( 0xdeadbeef ); tmp = SetParent( child1, adopter ); ok( tmp == 0, "SetParent returned %p\n", tmp ); - todo_wine ok( GetLastError() == ERROR_INVALID_PARAMETER, "got error %u\n", GetLastError() ); SetLastError( 0xdeadbeef ); tmp = SetParent( child3, adopter ); ok( tmp == 0, "SetParent returned %p\n", tmp ); - todo_wine ok( GetLastError() == ERROR_INVALID_PARAMETER, "got error %u\n", GetLastError() ); SetLastError( 0xdeadbeef ); tmp = GetParent( child1 ); @@ -903,12 +901,10 @@ static void test_thread_exit_destroy(void) SetLastError( 0xdeadbeef ); ret = SetWindowPos( child2, HWND_TOPMOST, 0, 0, 100, 100, SWP_NOSIZE|SWP_NOMOVE ); ok( !ret, "SetWindowPos succeeded\n" ); - todo_wine ok( GetLastError() == ERROR_INVALID_PARAMETER, "SetWindowPos returned error %u\n", GetLastError() ); SetLastError( 0xdeadbeef ); ret = SetWindowPos( child2, 0, 10, 10, 200, 200, SWP_NOZORDER | SWP_NOACTIVATE ); ok( !ret, "SetWindowPos succeeded\n" ); - todo_wine ok( GetLastError() == ERROR_INVALID_PARAMETER, "SetWindowPos returned error %u\n", GetLastError() );
rgn = CreateRectRgn( 5, 5, 15, 15 ); diff --git a/dlls/user32/win.c b/dlls/user32/win.c index 5e89f4c2c97..815c668d675 100644 --- a/dlls/user32/win.c +++ b/dlls/user32/win.c @@ -3357,7 +3357,7 @@ HWND WINAPI SetParent( HWND hwnd, HWND parent ) { req->handle = wine_server_user_handle( hwnd ); req->parent = wine_server_user_handle( parent ); - if ((ret = !wine_server_call( req ))) + if ((ret = !wine_server_call_err( req ))) { old_parent = wine_server_ptr_handle( reply->old_parent ); wndPtr->parent = parent = wine_server_ptr_handle( reply->full_parent ); diff --git a/dlls/user32/winpos.c b/dlls/user32/winpos.c index fa2f7e6fede..7253231b84a 100644 --- a/dlls/user32/winpos.c +++ b/dlls/user32/winpos.c @@ -2245,7 +2245,7 @@ BOOL set_window_pos( HWND hwnd, HWND insert_after, UINT swp_flags, if (new_surface) req->paint_flags |= SET_WINPOS_PAINT_SURFACE; if (win->pixel_format) req->paint_flags |= SET_WINPOS_PIXEL_FORMAT;
- if ((ret = !wine_server_call( req ))) + if ((ret = !wine_server_call_err( req ))) { win->dwStyle = reply->new_style; win->dwExStyle = reply->new_ex_style;
Hi,
While running your changed tests, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check?
Full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=100084
Your paranoid android.
=== debiant2 (32 bit WoW report) ===
user32: win.c:11312: Test failed: 0058007A: expected prev 005F0084, got 00000000 win.c:11326: Test failed: hwnd should NOT be topmost win.c:11328: Test failed: 0058007A: expected NOT topmost win.c:11272: Test failed: 1: hwnd 0058007A is still topmost
Signed-off-by: Rémi Bernon rbernon@codeweavers.com --- server/queue.c | 72 +++++++++++++++++--------------------------------- 1 file changed, 24 insertions(+), 48 deletions(-)
diff --git a/server/queue.c b/server/queue.c index f38c7e9fb17..fbbb230e6dc 100644 --- a/server/queue.c +++ b/server/queue.c @@ -753,16 +753,14 @@ static struct message_result *alloc_message_result( struct msg_queue *send_queue }
/* receive a message, removing it from the sent queue */ -static void receive_message( struct msg_queue *queue, struct message *msg, - struct get_message_reply *reply ) +static int receive_message( struct msg_queue *queue, struct message *msg, + struct get_message_reply *reply ) { - struct message_result *result = msg->result; - reply->total = msg->data_size; if (msg->data_size > get_reply_max_size()) { set_error( STATUS_BUFFER_OVERFLOW ); - return; + return 0; } reply->type = msg->type; reply->win = msg->win; @@ -774,17 +772,7 @@ static void receive_message( struct msg_queue *queue, struct message *msg, reply->time = msg->time;
if (msg->data) set_reply_data_ptr( msg->data, msg->data_size ); - - list_remove( &msg->entry ); - /* put the result on the receiver result stack */ - if (result) - { - result->msg = NULL; - result->recv_next = queue->recv_result; - queue->recv_result = result; - } - free( msg ); - if (list_empty( &queue->msg_list[SEND_MESSAGE] )) clear_queue_bits( queue, QS_SENDMESSAGE ); + return 1; }
/* set the result of the current received message */ @@ -818,17 +806,18 @@ static int match_window( user_handle_t win, user_handle_t msg_win ) return is_child_window( win, msg_win ); }
-/* retrieve a posted message */ -static int get_posted_message( struct msg_queue *queue, user_handle_t win, +/* retrieve a sent or posted message */ +static int get_queued_message( struct msg_queue *queue, enum message_kind kind, user_handle_t win, unsigned int first, unsigned int last, unsigned int flags, struct get_message_reply *reply ) { + struct message_result *result; struct message *msg;
/* check against the filters */ - LIST_FOR_EACH_ENTRY( msg, &queue->msg_list[POST_MESSAGE], struct message, entry ) + LIST_FOR_EACH_ENTRY( msg, &queue->msg_list[kind], struct message, entry ) { - if (!match_window( win, msg->win )) continue; + if (kind != SEND_MESSAGE && !match_window( win, msg->win )) continue; if (!check_msg_filter( msg->msg, first, last )) continue; goto found; /* found one */ } @@ -836,32 +825,24 @@ static int get_posted_message( struct msg_queue *queue, user_handle_t win,
/* return it to the app */ found: - reply->total = msg->data_size; - if (msg->data_size > get_reply_max_size()) - { - set_error( STATUS_BUFFER_OVERFLOW ); + if (!receive_message( queue, msg, reply )) return 1; + + /* put the result on the receiver result stack */ + if (kind == SEND_MESSAGE && (result = msg->result)) + { + msg->result = NULL; + result->msg = NULL; + result->recv_next = queue->recv_result; + queue->recv_result = result; } - reply->type = msg->type; - reply->win = msg->win; - reply->msg = msg->msg; - reply->wparam = msg->wparam; - reply->lparam = msg->lparam; - reply->x = msg->x; - reply->y = msg->y; - reply->time = msg->time;
if (flags & PM_REMOVE) { - if (msg->data) - { - set_reply_data_ptr( msg->data, msg->data_size ); - msg->data = NULL; - msg->data_size = 0; - } - remove_queue_message( queue, msg, POST_MESSAGE ); + msg->data = NULL; + msg->data_size = 0; + remove_queue_message( queue, msg, kind ); } - else if (msg->data) set_reply_data( msg->data, msg->data_size );
return 1; } @@ -2586,7 +2567,6 @@ DECL_HANDLER(post_quit_message) DECL_HANDLER(get_message) { struct timer *timer; - struct list *ptr; struct msg_queue *queue = get_current_queue(); user_handle_t get_win = get_user_full_handle( req->get_win ); unsigned int filter = req->flags >> 16; @@ -2604,12 +2584,8 @@ DECL_HANDLER(get_message) if (!filter) filter = QS_ALLINPUT;
/* first check for sent messages */ - if ((ptr = list_head( &queue->msg_list[SEND_MESSAGE] ))) - { - struct message *msg = LIST_ENTRY( ptr, struct message, entry ); - receive_message( queue, msg, reply ); + if (get_queued_message( queue, SEND_MESSAGE, 0, 0, 0xffffffff, PM_REMOVE, reply )) return; - }
/* clear changed bits so we can wait on them if we don't find a message */ if (filter & QS_POSTMESSAGE) @@ -2622,12 +2598,12 @@ DECL_HANDLER(get_message)
/* then check for posted messages */ if ((filter & QS_POSTMESSAGE) && - get_posted_message( queue, get_win, req->get_first, req->get_last, req->flags, reply )) + get_queued_message( queue, POST_MESSAGE, get_win, req->get_first, req->get_last, req->flags, reply )) return;
if ((filter & QS_HOTKEY) && queue->hotkey_count && req->get_first <= WM_HOTKEY && req->get_last >= WM_HOTKEY && - get_posted_message( queue, get_win, WM_HOTKEY, WM_HOTKEY, req->flags, reply )) + get_queued_message( queue, POST_MESSAGE, get_win, WM_HOTKEY, WM_HOTKEY, req->flags, reply )) return;
/* only check for quit messages if not posted messages pending */
Signed-off-by: Rémi Bernon rbernon@codeweavers.com --- dlls/user32/tests/win.c | 1 - server/queue.c | 6 +++++- 2 files changed, 5 insertions(+), 2 deletions(-)
diff --git a/dlls/user32/tests/win.c b/dlls/user32/tests/win.c index f128d15419d..b460416e89d 100644 --- a/dlls/user32/tests/win.c +++ b/dlls/user32/tests/win.c @@ -936,7 +936,6 @@ static void test_thread_exit_destroy(void) ret = IsWindow( child2 ); ok( !ret, "IsWindow returned %u\n", ret ); ret = IsWindow( child3 ); - todo_wine ok( !ret, "IsWindow returned %u\n", ret ); ret = DestroyWindow( child2 ); ok( !ret, "DestroyWindow returned %u\n", ret ); diff --git a/server/queue.c b/server/queue.c index fbbb230e6dc..5ef23996317 100644 --- a/server/queue.c +++ b/server/queue.c @@ -2573,6 +2573,10 @@ DECL_HANDLER(get_message)
reply->active_hooks = get_active_hooks();
+ /* first check for internal messages */ + if (queue && get_queued_message( queue, SEND_MESSAGE, 0, 0x80000000, 0xffffffff, PM_REMOVE, reply )) + return; + if (get_win && get_win != 1 && get_win != -1 && !get_user_object( get_win, USER_WINDOW )) { set_win32_error( ERROR_INVALID_WINDOW_HANDLE ); @@ -2584,7 +2588,7 @@ DECL_HANDLER(get_message) if (!filter) filter = QS_ALLINPUT;
/* first check for sent messages */ - if (get_queued_message( queue, SEND_MESSAGE, 0, 0, 0xffffffff, PM_REMOVE, reply )) + if (get_queued_message( queue, SEND_MESSAGE, 0, 0, 0x7fffffff, PM_REMOVE, reply )) return;
/* clear changed bits so we can wait on them if we don't find a message */
Hi,
While running your changed tests, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check?
Full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=100080
Your paranoid android.
=== w1064_tsign (32 bit report) ===
user32: win.c:4325: Test failed: hwnd 00230270/00230270 message 0200 win.c:4328: Test failed: hwnd 00230270/00230270 message 0201
=== w1064v1809 (64 bit report) ===
user32: win.c:4252: Test failed: message 0738 available
=== w1064_tsign (64 bit report) ===
user32: win.c:4291: Test failed: hwnd 00000000001C0210/00000000001C0210 message 0200 win.c:4296: Test failed: hwnd 00000000001C0210/00000000001C0210 message 0203 win.c:4300: Test failed: message 0202 available
=== w10pro64_ja (64 bit report) ===
user32: win.c:4374: Test failed: hwnd 00000000002A036E/00000000002A036E message 0200 win.c:4376: Test failed: wparam 0 win.c:4387: Test failed: hwnd 00000000002A036E/00000000002A036E message 0203 win.c:4391: Test failed: message 0202 available
-
Marvin -
Rémi Bernon