Always unlink children before sending WM_NCDESTROY message
Dmitry Timoshkov
dmitry at baikal.ru
Fri Feb 18 09:56:40 CST 2005
Hello,
this patch fixes a crash in the app I'm working on. The app creates
a complex window hierarchy by creating a lot of windows and then
reparenting them to each other, so that some WS_CHILD windows have
children with children. On WM_CLOSE message of the main window its
handler does DestroyWindow(main_window). The crash happens at the point
when one of the WS_CHILD windows with children receives WM_NCDESTROY
message and does: hwnd = GetParent(); DestroyWindow(hwnd). A parent
of the window already have received WM_NCDESTROY message but didn't
unlink its children because current Wine code unlinks children only from
the window passed directly to DestroyWindow, but not for the children's
recursion.
Changelog:
Dmitry Timoshkov <dmitry at codeweavers.com>
- Always unlink children before sending WM_NCDESTROY message.
- Fix order of WM_DESTROY messages for the children's recursion.
- DestroyWindow should hide only visible windows.
- Add a test case for the above fixes.
diff -up cvs/hq/wine/dlls/user/tests/msg.c wine/dlls/user/tests/msg.c
--- cvs/hq/wine/dlls/user/tests/msg.c 2005-02-10 13:36:31.000000000 +0800
+++ wine/dlls/user/tests/msg.c 2005-02-18 23:33:31.000000000 +0800
@@ -39,6 +39,11 @@
#define SWP_NOCLIENTSIZE 0x0800
#define SWP_NOCLIENTMOVE 0x1000
+#define WND_PARENT_ID 1
+#define WND_POPUP_ID 2
+#define WND_CHILD_ID 3
+
+static BOOL test_DestroyWindow_flag;
static HWINEVENTHOOK hEvent_hook;
/*
@@ -234,10 +239,6 @@ static const struct message WmHideOverla
{ WM_IME_NOTIFY, sent|optional|defwinproc },
{ 0 }
};
-/* ShowWindow(SW_HIDE) for an invisible overlapped window */
-static const struct message WmHideInvisibleOverlappedSeq[] = {
- { 0 }
-};
/* DestroyWindow for a visible overlapped window */
static const struct message WmDestroyOverlappedSeq[] = {
{ HCBT_DESTROYWND, hook },
@@ -1018,7 +1019,7 @@ static const struct message WmSHOWNATopI
{ WM_NCPAINT, sent|wparam, 1 },
{ WM_GETTEXT, sent|defwinproc|optional },
{ WM_ERASEBKGND, sent|optional },
- { WM_WINDOWPOSCHANGED, sent|wparam, SWP_SHOWWINDOW|SWP_NOSIZE|SWP_NOMOVE|SWP_NOCLIENTSIZE|SWP_NOACTIVATE|SWP_NOZORDER|SWP_NOCLIENTMOVE },
+ { WM_WINDOWPOSCHANGED, sent|wparam, SWP_SHOWWINDOW|SWP_NOACTIVATE|SWP_NOSIZE|SWP_NOMOVE|SWP_NOCLIENTSIZE|SWP_NOCLIENTMOVE },
{ WM_SIZE, sent },
{ WM_MOVE, sent },
{ 0 }
@@ -1946,7 +1947,8 @@ static LRESULT WINAPI mdi_client_hook_pr
message != WM_NCPAINT &&
message != WM_NCHITTEST &&
message != WM_GETTEXT &&
- message != WM_MDIGETACTIVE)
+ message != WM_MDIGETACTIVE &&
+ message != WM_DEVICECHANGE)
{
trace("mdi client: %p, %04x, %08x, %08lx\n", hwnd, message, wParam, lParam);
@@ -1991,7 +1993,8 @@ static LRESULT WINAPI mdi_child_wnd_proc
message != WM_ERASEBKGND &&
message != WM_NCPAINT &&
message != WM_NCHITTEST &&
- message != WM_GETTEXT)
+ message != WM_GETTEXT &&
+ message != WM_DEVICECHANGE)
{
trace("mdi child: %p, %04x, %08x, %08lx\n", hwnd, message, wParam, lParam);
@@ -2054,7 +2057,8 @@ static LRESULT WINAPI mdi_frame_wnd_proc
message != WM_ERASEBKGND &&
message != WM_NCPAINT &&
message != WM_NCHITTEST &&
- message != WM_GETTEXT)
+ message != WM_GETTEXT &&
+ message != WM_DEVICECHANGE)
{
trace("mdi frame: %p, %04x, %08x, %08lx\n", hwnd, message, wParam, lParam);
@@ -2819,7 +2823,7 @@ static void test_messages(void)
/* test ShowWindow(SW_HIDE) on a newly created invisible window */
ok( ShowWindow(hwnd, SW_HIDE) == FALSE, "ShowWindow: window was visible\n" );
- ok_sequence(WmHideInvisibleOverlappedSeq, "ShowWindow(SW_HIDE):overlapped, invisible", FALSE);
+ ok_sequence(WmEmptySeq, "ShowWindow(SW_HIDE):overlapped, invisible", FALSE);
/* test WM_SETREDRAW on a not visible top level window */
test_WM_SETREDRAW(hwnd);
@@ -3840,6 +3844,7 @@ static void test_paint_messages(void)
log_all_parent_messages--;
DestroyWindow( hparent );
+ ok(!IsWindow(hchild), "child must be destroyed with its parent\n");
DeleteObject( hrgn );
DeleteObject( hrgn2 );
@@ -4090,7 +4095,8 @@ static void pump_msg_loop(HWND hwnd, HAC
trace("accel: %p, %04x, %08x, %08lx\n", msg.hwnd, msg.message, msg.wParam, msg.lParam);
/* ignore some unwanted messages */
- if (msg.message == WM_MOUSEMOVE)
+ if (msg.message == WM_MOUSEMOVE ||
+ msg.message == WM_DEVICECHANGE)
continue;
log_msg.message = msg.message;
@@ -4274,6 +4280,23 @@ static LRESULT WINAPI MsgCheckProcA(HWND
switch (message)
{
+ case WM_NCDESTROY:
+ ok(!GetWindow(hwnd, GW_CHILD), "children should be unlinked at this point\n");
+ /* fall through */
+ case WM_DESTROY:
+ ok(GetAncestor(hwnd, GA_PARENT) != 0, "parent should NOT be unlinked at this point\n");
+ if (test_DestroyWindow_flag)
+ {
+ DWORD style = GetWindowLongA(hwnd, GWL_STYLE);
+ if (style & WS_CHILD)
+ lParam = GetWindowLongA(hwnd, GWL_ID);
+ else if (style & WS_POPUP)
+ lParam = WND_POPUP_ID;
+ else
+ lParam = WND_PARENT_ID;
+ }
+ break;
+
/* test_accelerators() depends on this */
case WM_NCHITTEST:
return HTCLIENT;
@@ -4281,6 +4304,7 @@ static LRESULT WINAPI MsgCheckProcA(HWND
/* ignore */
case WM_MOUSEMOVE:
case WM_SETCURSOR:
+ case WM_DEVICECHANGE:
return 0;
case WM_WINDOWPOSCHANGING:
@@ -4533,6 +4557,20 @@ static LRESULT CALLBACK cbt_hook_proc(in
return CallNextHookEx(hCBT_hook, nCode, wParam, lParam);
}
+ if (nCode == HCBT_DESTROYWND)
+ {
+ if (test_DestroyWindow_flag)
+ {
+ DWORD style = GetWindowLongA((HWND)wParam, GWL_STYLE);
+ if (style & WS_CHILD)
+ lParam = GetWindowLongA((HWND)wParam, GWL_ID);
+ else if (style & WS_POPUP)
+ lParam = WND_POPUP_ID;
+ else
+ lParam = WND_PARENT_ID;
+ }
+ }
+
/* Log also SetFocus(0) calls */
if (!wParam) wParam = lParam;
@@ -5292,6 +5330,111 @@ static void test_scrollwindowex(void)
flush_sequence();
}
+static const struct message destroy_window_with_children[] = {
+ { HCBT_DESTROYWND, hook|lparam, 0, WND_PARENT_ID }, /* parent */
+ { HCBT_DESTROYWND, hook|lparam, 0, WND_POPUP_ID }, /* popup */
+ { WM_DESTROY, sent|wparam|lparam, 0, WND_POPUP_ID }, /* popup */
+ { WM_NCDESTROY, sent|wparam|lparam, 0, WND_POPUP_ID }, /* popup */
+ { WM_DESTROY, sent|wparam|lparam, 0, WND_PARENT_ID }, /* parent */
+ { WM_DESTROY, sent|wparam|lparam, 0, WND_CHILD_ID + 2 }, /* child2 */
+ { WM_DESTROY, sent|wparam|lparam, 0, WND_CHILD_ID + 1 }, /* child1 */
+ { WM_DESTROY, sent|wparam|lparam, 0, WND_CHILD_ID + 3 }, /* child3 */
+ { WM_NCDESTROY, sent|wparam|lparam, 0, WND_CHILD_ID + 2 }, /* child2 */
+ { WM_NCDESTROY, sent|wparam|lparam, 0, WND_CHILD_ID + 3 }, /* child3 */
+ { WM_NCDESTROY, sent|wparam|lparam, 0, WND_CHILD_ID + 1 }, /* child1 */
+ { WM_NCDESTROY, sent|wparam|lparam, 0, WND_PARENT_ID }, /* parent */
+ { 0 }
+};
+
+static void test_DestroyWindow(void)
+{
+ HWND parent, child1, child2, child3, child4, test;
+ UINT child_id = WND_CHILD_ID + 1;
+
+ parent = CreateWindowExA(0, "TestWindowClass", NULL, WS_OVERLAPPEDWINDOW,
+ 100, 100, 200, 200, 0, 0, 0, NULL);
+ assert(parent != 0);
+ child1 = CreateWindowExA(0, "TestWindowClass", NULL, WS_CHILD,
+ 0, 0, 50, 50, parent, (HMENU)child_id++, 0, NULL);
+ assert(child1 != 0);
+ child2 = CreateWindowExA(0, "TestWindowClass", NULL, WS_CHILD,
+ 0, 0, 50, 50, GetDesktopWindow(), (HMENU)child_id++, 0, NULL);
+ assert(child2 != 0);
+ child3 = CreateWindowExA(0, "TestWindowClass", NULL, WS_CHILD,
+ 0, 0, 50, 50, child1, (HMENU)child_id++, 0, NULL);
+ assert(child3 != 0);
+ child4 = CreateWindowExA(0, "TestWindowClass", NULL, WS_POPUP,
+ 0, 0, 50, 50, parent, 0, 0, NULL);
+ assert(child4 != 0);
+
+ /* test owner/parent of child2 */
+ test = GetParent(child2);
+ ok(test == GetDesktopWindow(), "wrong parent %p\n", test);
+ test = GetAncestor(child2, GA_PARENT);
+ ok(test == GetDesktopWindow(), "wrong parent %p\n", test);
+ test = GetWindow(child2, GW_OWNER);
+ ok(!test, "wrong owner %p\n", test);
+
+ test = SetParent(child2, parent);
+ ok(test == GetDesktopWindow(), "wrong old parent %p\n", test);
+
+ /* test owner/parent of the parent */
+ test = GetParent(parent);
+ ok(!test, "wrong parent %p\n", test);
+ test = GetAncestor(parent, GA_PARENT);
+ ok(test == GetDesktopWindow(), "wrong parent %p\n", test);
+ test = GetWindow(parent, GW_OWNER);
+ ok(!test, "wrong owner %p\n", test);
+
+ /* test owner/parent of child1 */
+ test = GetParent(child1);
+ ok(test == parent, "wrong parent %p\n", test);
+ test = GetAncestor(child1, GA_PARENT);
+ ok(test == parent, "wrong parent %p\n", test);
+ test = GetWindow(child1, GW_OWNER);
+ ok(!test, "wrong owner %p\n", test);
+
+ /* test owner/parent of child2 */
+ test = GetParent(child2);
+ ok(test == parent, "wrong parent %p\n", test);
+ test = GetAncestor(child2, GA_PARENT);
+ ok(test == parent, "wrong parent %p\n", test);
+ test = GetWindow(child2, GW_OWNER);
+ ok(!test, "wrong owner %p\n", test);
+
+ /* test owner/parent of child3 */
+ test = GetParent(child3);
+ ok(test == child1, "wrong parent %p\n", test);
+ test = GetAncestor(child3, GA_PARENT);
+ ok(test == child1, "wrong parent %p\n", test);
+ test = GetWindow(child3, GW_OWNER);
+ ok(!test, "wrong owner %p\n", test);
+
+ /* test owner/parent of child4 */
+ test = GetParent(child4);
+ ok(test == parent, "wrong parent %p\n", test);
+ test = GetAncestor(child4, GA_PARENT);
+ ok(test == GetDesktopWindow(), "wrong parent %p\n", test);
+ test = GetWindow(child4, GW_OWNER);
+ ok(test == parent, "wrong owner %p\n", test);
+
+ flush_sequence();
+
+ trace("parent %p, child1 %p, child2 %p, child3 %p, child4 %p\n",
+ parent, child1, child2, child3, child4);
+
+ test_DestroyWindow_flag = TRUE;
+ ok(DestroyWindow(parent), "DestroyWindow() error %ld\n", GetLastError());
+ test_DestroyWindow_flag = FALSE;
+ ok_sequence(destroy_window_with_children, "destroy window with children\n", 0);
+
+ ok(!IsWindow(parent), "parent still exists");
+ ok(!IsWindow(child1), "child1 still exists");
+ ok(!IsWindow(child2), "child2 still exists");
+ ok(!IsWindow(child3), "child3 still exists");
+ ok(!IsWindow(child4), "child4 still exists");
+}
+
START_TEST(msg)
{
HMODULE user32 = GetModuleHandleA("user32.dll");
@@ -5340,6 +5483,7 @@ START_TEST(msg)
test_accelerators();
test_timers();
test_set_hook();
+ test_DestroyWindow();
UnhookWindowsHookEx(hCBT_hook);
if (pUnhookWinEvent)
diff -up cvs/hq/wine/windows/win.c wine/windows/win.c
--- cvs/hq/wine/windows/win.c 2005-02-17 15:32:40.000000000 +0800
+++ wine/windows/win.c 2005-02-18 22:36:26.000000000 +0800
@@ -650,10 +650,13 @@ LRESULT WIN_DestroyWindow( HWND hwnd )
RedrawWindow( hwnd, NULL, 0,
RDW_VALIDATE | RDW_NOFRAME | RDW_NOERASE | RDW_NOINTERNALPAINT | RDW_NOCHILDREN);
+ /* Unlink now so we won't bother with the children later on */
+ WIN_UnlinkWindow( hwnd );
+
/*
* Send the WM_NCDESTROY to the window being destroyed.
*/
- SendMessageA( hwnd, WM_NCDESTROY, 0, 0);
+ SendMessageW( hwnd, WM_NCDESTROY, 0, 0 );
/* FIXME: do we need to fake QS_MOUSEMOVE wakebit? */
@@ -812,9 +815,9 @@ static void WIN_FixCoordinates( CREATEST
}
else /* overlapped window */
{
- STARTUPINFOA info;
+ STARTUPINFOW info;
- GetStartupInfoA( &info );
+ GetStartupInfoW( &info );
if (cs->x == CW_USEDEFAULT || cs->x == CW_USEDEFAULT16)
{
@@ -849,7 +852,7 @@ static void WIN_FixCoordinates( CREATEST
else /* if no other hint from the app, pick 3/4 of the screen real estate */
{
RECT r;
- SystemParametersInfoA( SPI_GETWORKAREA, 0, &r, 0);
+ SystemParametersInfoW( SPI_GETWORKAREA, 0, &r, 0);
cs->cx = (((r.right - r.left) * 3) / 4) - cs->x;
cs->cy = (((r.bottom - r.top) * 3) / 4) - cs->y;
}
@@ -858,7 +861,7 @@ static void WIN_FixCoordinates( CREATEST
else if (cs->cy == CW_USEDEFAULT || cs->cy == CW_USEDEFAULT16)
{
RECT r;
- SystemParametersInfoA( SPI_GETWORKAREA, 0, &r, 0);
+ SystemParametersInfoW( SPI_GETWORKAREA, 0, &r, 0);
cs->cy = (((r.bottom - r.top) * 3) / 4) - cs->y;
}
}
@@ -872,7 +875,7 @@ static void WIN_FixCoordinates( CREATEST
if (cs->cy == CW_USEDEFAULT || cs->cy == CW_USEDEFAULT16) {
RECT r;
FIXME("Strange use of CW_USEDEFAULT in nHeight\n");
- SystemParametersInfoA( SPI_GETWORKAREA, 0, &r, 0);
+ SystemParametersInfoW( SPI_GETWORKAREA, 0, &r, 0);
cs->cy = (((r.bottom - r.top) * 3) / 4) - cs->y;
}
}
@@ -1431,7 +1434,7 @@ static void WIN_SendDestroyMsg( HWND hwn
/*
* Send the WM_DESTROY to the window.
*/
- SendMessageA( hwnd, WM_DESTROY, 0, 0);
+ SendMessageW( hwnd, WM_DESTROY, 0, 0);
/*
* This WM_DESTROY message can trigger re-entrant calls to DestroyWindow
@@ -1444,10 +1447,7 @@ static void WIN_SendDestroyMsg( HWND hwn
if (!(pWndArray = WIN_ListChildren( hwnd ))) return;
- /* start from the end (FIXME: is this needed?) */
- for (i = 0; pWndArray[i]; i++) ;
-
- while (--i >= 0)
+ for (i = 0; pWndArray[i]; i++)
{
if (IsWindow( pWndArray[i] )) WIN_SendDestroyMsg( pWndArray[i] );
}
@@ -1499,13 +1499,15 @@ BOOL WINAPI DestroyWindow( HWND hwnd )
USER_Driver.pResetSelectionOwner( hwnd, FALSE ); /* before the window is unmapped */
/* Hide the window */
-
- /* Only child windows receive WM_SHOWWINDOW in DestroyWindow() */
- if (is_child)
- ShowWindow( hwnd, SW_HIDE );
- else
- SetWindowPos( hwnd, 0, 0, 0, 0, 0, SWP_NOMOVE | SWP_NOSIZE |
- SWP_NOZORDER | SWP_NOACTIVATE | SWP_HIDEWINDOW );
+ if (GetWindowLongW( hwnd, GWL_STYLE ) & WS_VISIBLE)
+ {
+ /* Only child windows receive WM_SHOWWINDOW in DestroyWindow() */
+ if (is_child)
+ ShowWindow( hwnd, SW_HIDE );
+ else
+ SetWindowPos( hwnd, 0, 0, 0, 0, 0, SWP_NOMOVE | SWP_NOSIZE |
+ SWP_NOZORDER | SWP_NOACTIVATE | SWP_HIDEWINDOW );
+ }
if (!IsWindow(hwnd)) return TRUE;
@@ -1544,10 +1546,6 @@ BOOL WINAPI DestroyWindow( HWND hwnd )
if (GetClipboardOwner() == hwnd)
CLIPBOARD_ReleaseOwner();
- /* Unlink now so we won't bother with the children later on */
-
- WIN_UnlinkWindow( hwnd );
-
/* Destroy the window storage */
WIN_DestroyWindow( hwnd );
@@ -1736,13 +1734,13 @@ BOOL WINAPI EnableWindow( HWND hwnd, BOO
if (enable && retvalue)
{
WIN_SetStyle( hwnd, 0, WS_DISABLED );
- SendMessageA( hwnd, WM_ENABLE, TRUE, 0 );
+ SendMessageW( hwnd, WM_ENABLE, TRUE, 0 );
}
else if (!enable && !retvalue)
{
HWND capture_wnd;
- SendMessageA( hwnd, WM_CANCELMODE, 0, 0);
+ SendMessageW( hwnd, WM_CANCELMODE, 0, 0);
WIN_SetStyle( hwnd, WS_DISABLED, 0 );
@@ -1753,7 +1751,7 @@ BOOL WINAPI EnableWindow( HWND hwnd, BOO
if (hwnd == capture_wnd || IsChild(hwnd, capture_wnd))
ReleaseCapture(); /* A disabled window can't capture the mouse */
- SendMessageA( hwnd, WM_ENABLE, FALSE, 0 );
+ SendMessageW( hwnd, WM_ENABLE, FALSE, 0 );
}
return retvalue;
}
@@ -3122,7 +3120,7 @@ BOOL WINAPI DragDetect( HWND hWnd, POINT
while(1)
{
- while(PeekMessageA(&msg ,0 ,WM_MOUSEFIRST ,WM_MOUSELAST ,PM_REMOVE))
+ while (PeekMessageW( &msg, 0, WM_MOUSEFIRST, WM_MOUSELAST, PM_REMOVE ))
{
if( msg.message == WM_LBUTTONUP )
{
More information about the wine-patches
mailing list