[PATCH v10 0/2] MR8258: comctl32/treeview: Return from TREEVIEW_LButtonDown when the treeview handle is invalid.
comctl32/treeview: Return from TREEVIEW_LButtonDown when the treeview handle is invalid. LButtonDown should return when the treeview handle is invalid (e.g. destroyed) after NM_CLICK to prevent further message processing. Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=58211 -- v10: comctl32/treeview: Return from TREEVIEW_LButtonDown when the treeview handle is invalid. https://gitlab.winehq.org/wine/wine/-/merge_requests/8258
From: Jacob Czekalla <jczekalla(a)codeweavers.com> --- dlls/comctl32/tests/treeview.c | 72 ++++++++++++++++++++++++++++++++++ 1 file changed, 72 insertions(+) diff --git a/dlls/comctl32/tests/treeview.c b/dlls/comctl32/tests/treeview.c index 5b53a32f51b..1b743a0a23c 100644 --- a/dlls/comctl32/tests/treeview.c +++ b/dlls/comctl32/tests/treeview.c @@ -52,6 +52,7 @@ static char *g_endedit_overwrite_ptr; static HFONT g_customdraw_font; static BOOL g_v6; static int g_reject_tvn_itemexpanding = 0; +static int g_click_delete_test = 0; #define NUM_MSG_SEQUENCES 3 #define TREEVIEW_SEQ_INDEX 0 @@ -243,6 +244,20 @@ static const struct message test_right_click_seq[] = { { 0 } }; +static const struct message test_click_delete_seq[] = { + { WM_LBUTTONDOWN, sent|wparam, MK_LBUTTON }, + { WM_CAPTURECHANGED, sent|defwinproc }, + { 0x90, sent|defwinproc|optional }, + { WM_SHOWWINDOW, sent|defwinproc|wparam|lparam, 0, 0 }, + { WM_WINDOWPOSCHANGING, sent|defwinproc }, + { WM_WINDOWPOSCHANGED, sent|defwinproc }, + { WM_KILLFOCUS, sent|defwinproc }, + { WM_IME_SETCONTEXT, sent|defwinproc|wparam, 0 }, + { WM_DESTROY, sent|defwinproc }, + { WM_NCDESTROY, sent|defwinproc }, + { 0 } +}; + static const struct message parent_expand_seq[] = { { WM_NOTIFY, sent|id, 0, 0, TVN_ITEMEXPANDINGA }, { WM_NOTIFY, sent|id, 0, 0, TVN_ITEMEXPANDEDA }, @@ -1478,6 +1493,15 @@ static LRESULT CALLBACK parent_wnd_proc(HWND hWnd, UINT message, WPARAM wParam, ok(selected == hChild, "child item should still be selected\n"); break; } + case NM_CLICK: + { + if (g_click_delete_test) + { + DestroyWindow(pHdr->hwndFrom); + return FALSE; + } + break; + } } } break; @@ -3123,6 +3147,53 @@ static void test_right_click(void) DestroyWindow(hTree); } +static void test_treeview_delete_midclick(void) +{ + HWND treeview; + RECT rc; + POINT pt, orig_pos; + MSG msg; + + g_click_delete_test = 1; + treeview = create_treeview_control(0); + fill_tree(treeview); + ShowWindow(hMainWnd, SW_SHOW); + UpdateWindow(hMainWnd); + + *(HTREEITEM *)&rc = hRoot; + SendMessageA(treeview, TVM_GETITEMRECT, TRUE, (LPARAM)&rc); + + pt.x = (rc.left + rc.right) / 2; + pt.y = (rc.top + rc.bottom) / 2; + ClientToScreen(hMainWnd, &pt); + GetCursorPos(&orig_pos); + SetCursorPos(pt.x, pt.y); + ScreenToClient(treeview, &pt); + + flush_events(); + flush_sequences(sequences, NUM_MSG_SEQUENCES); + + PostMessageA(treeview, WM_LBUTTONDOWN, MK_LBUTTON, MAKELPARAM(pt.x, pt.y)); + PostMessageA(treeview, WM_LBUTTONUP, 0, MAKELPARAM(pt.x, pt.y)); + while (GetMessageA(&msg, 0, 0, 0)) + { + TranslateMessage(&msg); + DispatchMessageA(&msg); + if((msg.hwnd == treeview) && (msg.message == WM_LBUTTONDOWN)) + break; + } + + flush_events(); + ok_sequence(sequences, TREEVIEW_SEQ_INDEX, test_click_delete_seq, "treeview click and destroy sequence", TRUE); + + flush_sequences(sequences, NUM_MSG_SEQUENCES); + SetCursorPos(orig_pos.x, orig_pos.y); + g_click_delete_test = 0; + + if (!IsWindow(treeview)) + DestroyWindow(treeview); +} + static void init_functions(void) { HMODULE hComCtl32 = LoadLibraryA("comctl32.dll"); @@ -3206,6 +3277,7 @@ START_TEST(treeview) test_TVS_FULLROWSELECT(); test_TVM_SORTCHILDREN(); test_right_click(); + test_treeview_delete_midclick(); if (!load_v6_module(&ctx_cookie, &hCtx)) { -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/8258
From: Jacob Czekalla <jczekalla(a)codeweavers.com> LButtonDown should return when the treeview handle is invalid (e.g. destroyed) after NM_CLICK to prevent further message processing. Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=58211 --- dlls/comctl32/tests/treeview.c | 2 +- dlls/comctl32/treeview.c | 45 +++++++++++++++++++++++++--------- 2 files changed, 34 insertions(+), 13 deletions(-) diff --git a/dlls/comctl32/tests/treeview.c b/dlls/comctl32/tests/treeview.c index 1b743a0a23c..d7591b568e9 100644 --- a/dlls/comctl32/tests/treeview.c +++ b/dlls/comctl32/tests/treeview.c @@ -3184,7 +3184,7 @@ static void test_treeview_delete_midclick(void) } flush_events(); - ok_sequence(sequences, TREEVIEW_SEQ_INDEX, test_click_delete_seq, "treeview click and destroy sequence", TRUE); + ok_sequence(sequences, TREEVIEW_SEQ_INDEX, test_click_delete_seq, "treeview click and destroy sequence", FALSE); flush_sequences(sequences, NUM_MSG_SEQUENCES); SetCursorPos(orig_pos.x, orig_pos.y); diff --git a/dlls/comctl32/treeview.c b/dlls/comctl32/treeview.c index 073f62c66a2..0e232169105 100644 --- a/dlls/comctl32/treeview.c +++ b/dlls/comctl32/treeview.c @@ -523,10 +523,15 @@ TREEVIEW_SendRealNotify(const TREEVIEW_INFO *infoPtr, UINT code, NMHDR *hdr) } static BOOL -TREEVIEW_SendSimpleNotify(const TREEVIEW_INFO *infoPtr, UINT code) +TREEVIEW_SendSimpleNotify(const TREEVIEW_INFO *infoPtr, UINT code, BOOL *result) { NMHDR hdr; - return TREEVIEW_SendRealNotify(infoPtr, code, &hdr); + HWND hwnd; + + hwnd = infoPtr->hwnd; /* save hwnd before after TREEVIEW_SendRealNotify(), the window could be already destroyed after sending notifications */ + if (result) + *result = TREEVIEW_SendRealNotify(infoPtr, code, &hdr); + return IsWindow(hwnd); } static VOID @@ -4193,6 +4198,7 @@ TREEVIEW_LButtonDoubleClick(TREEVIEW_INFO *infoPtr, LPARAM lParam) { TREEVIEW_ITEM *item; TVHITTESTINFO hit; + BOOL notify_result; TRACE("\n"); SetFocus(infoPtr->hwnd); @@ -4211,7 +4217,9 @@ TREEVIEW_LButtonDoubleClick(TREEVIEW_INFO *infoPtr, LPARAM lParam) return 0; TRACE("item %d\n", TREEVIEW_GetItemIndex(infoPtr, item)); - if (TREEVIEW_SendSimpleNotify(infoPtr, NM_DBLCLK) == FALSE) + if (TREEVIEW_SendSimpleNotify(infoPtr, NM_DBLCLK, ¬ify_result) == FALSE) + return 0; + if (notify_result == FALSE) { /* FIXME! */ switch (hit.flags) { @@ -4258,7 +4266,7 @@ TREEVIEW_LButtonDoubleClick(TREEVIEW_INFO *infoPtr, LPARAM lParam) static LRESULT TREEVIEW_LButtonDown(TREEVIEW_INFO *infoPtr, LPARAM lParam) { - BOOL do_track, do_select, bDoLabelEdit; + BOOL do_track, do_select, bDoLabelEdit, notify_result; HWND hwnd = infoPtr->hwnd; TVHITTESTINFO ht; @@ -4315,8 +4323,13 @@ TREEVIEW_LButtonDown(TREEVIEW_INFO *infoPtr, LPARAM lParam) (ht.flags & TVHT_ONITEMLABEL) && (infoPtr->selectedItem == ht.hItem); /* Send NM_CLICK right away */ - if (!do_track && TREEVIEW_SendSimpleNotify(infoPtr, NM_CLICK)) - goto setfocus; + if (!do_track) + { + if (!TREEVIEW_SendSimpleNotify(infoPtr, NM_CLICK, ¬ify_result)) + return 0; + if (notify_result) + goto setfocus; + } if (ht.flags & TVHT_ONITEMBUTTON) { @@ -4345,8 +4358,13 @@ TREEVIEW_LButtonDown(TREEVIEW_INFO *infoPtr, LPARAM lParam) } } - if (do_track && TREEVIEW_SendSimpleNotify(infoPtr, NM_CLICK)) - goto setfocus; + if (do_track) + { + if (!TREEVIEW_SendSimpleNotify(infoPtr, NM_CLICK, ¬ify_result)) + return 0; + if (notify_result) + goto setfocus; + } if (TREEVIEW_IsFullRowSelect(infoPtr)) do_select = ht.flags & (TVHT_ONITEMINDENT | TVHT_ONITEMICON | TVHT_ONITEMLABEL | TVHT_ONITEMRIGHT); @@ -4385,6 +4403,7 @@ TREEVIEW_LButtonDown(TREEVIEW_INFO *infoPtr, LPARAM lParam) static LRESULT TREEVIEW_RButtonDown(TREEVIEW_INFO *infoPtr, LPARAM lParam) { + BOOL notify_result; TVHITTESTINFO ht; if (infoPtr->hwndEdit) @@ -4414,7 +4433,9 @@ TREEVIEW_RButtonDown(TREEVIEW_INFO *infoPtr, LPARAM lParam) else { SetFocus(infoPtr->hwnd); - if(!TREEVIEW_SendSimpleNotify(infoPtr, NM_RCLICK)) + if (!TREEVIEW_SendSimpleNotify(infoPtr, NM_RCLICK, ¬ify_result)) + return 0; + if (!notify_result) { /* Send a WM_CONTEXTMENU message in response to the RBUTTONUP */ SendMessageW(infoPtr->hwndNotify, WM_CONTEXTMENU, @@ -5340,7 +5361,7 @@ TREEVIEW_KeyDown(TREEVIEW_INFO *infoPtr, WPARAM wParam) break; case VK_RETURN: - TREEVIEW_SendSimpleNotify(infoPtr, NM_RETURN); + TREEVIEW_SendSimpleNotify(infoPtr, NM_RETURN, NULL); break; case VK_HOME: @@ -5666,7 +5687,7 @@ TREEVIEW_SetFocus(TREEVIEW_INFO *infoPtr) } TREEVIEW_Invalidate(infoPtr, infoPtr->selectedItem); - TREEVIEW_SendSimpleNotify(infoPtr, NM_SETFOCUS); + TREEVIEW_SendSimpleNotify(infoPtr, NM_SETFOCUS, NULL); return 0; } @@ -5677,7 +5698,7 @@ TREEVIEW_KillFocus(const TREEVIEW_INFO *infoPtr) TREEVIEW_Invalidate(infoPtr, infoPtr->selectedItem); UpdateWindow(infoPtr->hwnd); - TREEVIEW_SendSimpleNotify(infoPtr, NM_KILLFOCUS); + TREEVIEW_SendSimpleNotify(infoPtr, NM_KILLFOCUS, NULL); return 0; } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/8258
On Thu Jul 10 16:16:56 2025 +0000, Jacob Czekalla wrote:
changed this line in [version 9 of the diff](/wine/wine/-/merge_requests/8258/diffs?diff_id=192271&start_sha=41debb624e8f52772580ae04fd16553ee22929cb#ea6661805bd26d4797d9aebb9c2d0e45a9d9acc7_536_531) I think I know what you mean now. I didn't read that as actually changing the other places. I modified each spot it's called.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/8258#note_109478
Zhiyi Zhang (@zhiyi) commented about dlls/comctl32/treeview.c:
}
static BOOL -TREEVIEW_SendSimpleNotify(const TREEVIEW_INFO *infoPtr, UINT code) +TREEVIEW_SendSimpleNotify(const TREEVIEW_INFO *infoPtr, UINT code, BOOL *result) Let's add a comment clarifying the meaning of the return value and `result`. For example, "Return TRUE if TREEVIEW_SendSimpleNotify() succeeded. The return code for the notification is in `result`"
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/8258#note_110281
Zhiyi Zhang (@zhiyi) commented about dlls/comctl32/treeview.c:
}
static BOOL -TREEVIEW_SendSimpleNotify(const TREEVIEW_INFO *infoPtr, UINT code) +TREEVIEW_SendSimpleNotify(const TREEVIEW_INFO *infoPtr, UINT code, BOOL *result) { NMHDR hdr; - return TREEVIEW_SendRealNotify(infoPtr, code, &hdr); + HWND hwnd; + + hwnd = infoPtr->hwnd; /* save hwnd before after TREEVIEW_SendRealNotify(), the window could be already destroyed after sending notifications */ + if (result) + *result = TREEVIEW_SendRealNotify(infoPtr, code, &hdr); This is wrong. TREEVIEW_SendRealNotify() only gets called when `result` is not NULL. So things like `TREEVIEW_SendSimpleNotify(infoPtr, NM_RETURN, NULL);` basically do nothing. You should store the result in an intermediate variable, then assign it to `result` if it's not NULL.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/8258#note_110280
This merge request was closed by Jacob Czekalla. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/8258
participants (3)
-
Jacob Czekalla -
Jacob Czekalla (＠JacobCzekalla)
-
Zhiyi Zhang (＠zhiyi)