[PATCH v5 0/2] MR8258: Draft: 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
-- v5: comctl32/treeview: Return from TREEVIEW_LButtonDown when the treeview handle is invalid. comctl32/tests: Add a test for treeview deletion during NM_CLICK in LBUTTONDOWN.
From: Jacob Czekalla jczekalla@codeweavers.com
--- dlls/comctl32/tests/treeview.c | 69 ++++++++++++++++++++++++++++++++++ 1 file changed, 69 insertions(+)
diff --git a/dlls/comctl32/tests/treeview.c b/dlls/comctl32/tests/treeview.c index 5b53a32f51b..e7142a54cdf 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,50 @@ 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; +} + static void init_functions(void) { HMODULE hComCtl32 = LoadLibraryA("comctl32.dll"); @@ -3206,6 +3274,7 @@ START_TEST(treeview) test_TVS_FULLROWSELECT(); test_TVM_SORTCHILDREN(); test_right_click(); + test_treeview_delete_midclick();
if (!load_v6_module(&ctx_cookie, &hCtx)) {
From: Jacob Czekalla jczekalla@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 | 21 +++++++++++++++++---- 2 files changed, 18 insertions(+), 5 deletions(-)
diff --git a/dlls/comctl32/tests/treeview.c b/dlls/comctl32/tests/treeview.c index e7142a54cdf..eb5ffab788d 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..e426d666230 100644 --- a/dlls/comctl32/treeview.c +++ b/dlls/comctl32/treeview.c @@ -4261,6 +4261,7 @@ TREEVIEW_LButtonDown(TREEVIEW_INFO *infoPtr, LPARAM lParam) BOOL do_track, do_select, bDoLabelEdit; HWND hwnd = infoPtr->hwnd; TVHITTESTINFO ht; + BOOL res;
/* If Edit control is active - kill it and return. * The best way to do it is to set focus to itself. @@ -4315,8 +4316,14 @@ 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) + { + res = TREEVIEW_SendSimpleNotify(infoPtr, NM_CLICK); + if (!IsWindow(hwnd)) + return 0; + if (res) + goto setfocus; + }
if (ht.flags & TVHT_ONITEMBUTTON) { @@ -4345,8 +4352,14 @@ TREEVIEW_LButtonDown(TREEVIEW_INFO *infoPtr, LPARAM lParam) } }
- if (do_track && TREEVIEW_SendSimpleNotify(infoPtr, NM_CLICK)) - goto setfocus; + if (do_track) + { + res = TREEVIEW_SendSimpleNotify(infoPtr, NM_CLICK); + if (!IsWindow(hwnd)) + return 0; + if (res) + goto setfocus; + }
if (TREEVIEW_IsFullRowSelect(infoPtr)) do_select = ht.flags & (TVHT_ONITEMINDENT | TVHT_ONITEMICON | TVHT_ONITEMLABEL | TVHT_ONITEMRIGHT);
This merge request was approved by Zhiyi Zhang.
The MR looks fine. I wonder if we should apply the same treatment to other notification codes as well.
On Tue Jun 24 11:47:01 2025 +0000, Zhiyi Zhang wrote:
The MR looks fine. I wonder if we should apply the same treatment to other notification codes as well.
I don't think we need this IsWindow check duplicated everywhere it's needed. It needs to happen after every callback message, so it's better to have it in existing helpers somewhere.
On Tue Jun 24 11:47:00 2025 +0000, Nikolay Sivov wrote:
I don't think we need this IsWindow check duplicated everywhere it's needed. It needs to happen after every callback message, so it's better to have it in existing helpers somewhere.
I can do something similar to what I had at the beginning. Maybe make another version of TREEVIEW_SendSimpleNotify that checks if the listview is still valid afterwards.
-
Jacob Czekalla -
Jacob Czekalla (＠JacobCzekalla)
-
Nikolay Sivov (＠nsivov)
-
Zhiyi Zhang (＠zhiyi)