[PATCH v10 0/2] MR8258: comctl32/treeview: Return from TREEVIEW_LButtonDown when the treeview handle is invalid.

[Jacob Czekalla (＠JacobCzekalla)]

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

[PATCH v10 1/2] comctl32/tests: Add a test for treeview deletion during NM_CLICK in LBUTTONDOWN.

[Jacob Czekalla]

From: Jacob Czekalla

---
 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))
     {

[PATCH v10 2/2] comctl32/treeview: Return from TREEVIEW_LButtonDown when the treeview handle is invalid.

[Jacob Czekalla]

From: Jacob Czekalla

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, &notify_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, &notify_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, &notify_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, &notify_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;
 }

[Jacob Czekalla (＠JacobCzekalla)]

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.

[Zhiyi Zhang (＠zhiyi)]

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.

[Zhiyi Zhang (＠zhiyi)]

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`"