[PATCH 0/2] MR8258: comctl32/treeview: Prevent default processing in TREEVIEW_SendSimpleNotify...
comctl32/treeview: Prevent default processing in TREEVIEW_SendSimpleNotify when window handle is invalid. SendSimpleNotify should return TRUE when the window handle receiving the message is invalid (e.g. destroyed) to prevent further default message processing. Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=58211 -- https://gitlab.winehq.org/wine/wine/-/merge_requests/8258
From: Jacob Czekalla <jczekalla(a)codeweavers.com> --- dlls/comctl32/tests/treeview.c | 68 ++++++++++++++++++++++++++++++++++ 1 file changed, 68 insertions(+) diff --git a/dlls/comctl32/tests/treeview.c b/dlls/comctl32/tests/treeview.c index 5b53a32f51b..e6f8ca648f4 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,49 @@ 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); + + SetCursorPos(orig_pos.x, orig_pos.y); + g_click_delete_test = 0; +} + static void init_functions(void) { HMODULE hComCtl32 = LoadLibraryA("comctl32.dll"); @@ -3206,6 +3273,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> SendSimpleNotify should return TRUE when the window handle receiving the message is invalid (e.g. destroyed) to prevent further default message processing. Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=58211 --- dlls/comctl32/tests/treeview.c | 2 +- dlls/comctl32/treeview.c | 6 +++++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/dlls/comctl32/tests/treeview.c b/dlls/comctl32/tests/treeview.c index e6f8ca648f4..2708bffb669 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); SetCursorPos(orig_pos.x, orig_pos.y); g_click_delete_test = 0; diff --git a/dlls/comctl32/treeview.c b/dlls/comctl32/treeview.c index 073f62c66a2..5848c8c1932 100644 --- a/dlls/comctl32/treeview.c +++ b/dlls/comctl32/treeview.c @@ -526,7 +526,11 @@ static BOOL TREEVIEW_SendSimpleNotify(const TREEVIEW_INFO *infoPtr, UINT code) { NMHDR hdr; - return TREEVIEW_SendRealNotify(infoPtr, code, &hdr); + BOOL result = TREEVIEW_SendRealNotify(infoPtr, code, &hdr); + + if(!IsWindow(infoPtr->hwnd)) + result = TRUE; + return result; } static VOID -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/8258
Nikolay Sivov (@nsivov) commented about dlls/comctl32/treeview.c:
TREEVIEW_SendSimpleNotify(const TREEVIEW_INFO *infoPtr, UINT code) { NMHDR hdr; - return TREEVIEW_SendRealNotify(infoPtr, code, &hdr); + BOOL result = TREEVIEW_SendRealNotify(infoPtr, code, &hdr); + + if(!IsWindow(infoPtr->hwnd)) + result = TRUE; + return result; } As unfortunate as it looks, it might be close to what we need. However there are two problems with it. First, easy to fix, you're accessing potentially freed "infoPtr". Second is a bit worse - all notifications go through this helper, so we can't really use control data after returning from it because it could have been destroyed. Returning TRUE from TVN_ENDLABELEDITW for example will go through "save text" path, with newly added check too.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/8258#note_105930
On Mon Jun 9 20:46:15 2025 +0000, Nikolay Sivov wrote:
As unfortunate as it looks, it might be close to what we need. However there are two problems with it. First, easy to fix, you're accessing potentially freed "infoPtr". Second is a bit worse - all notifications go through this helper, so we can't really use control data after returning from it because it could have been destroyed. Returning TRUE from TVN_ENDLABELEDITW for example will go through "save text" path, with newly added check too. I definitely need to fix the bad infoPtr, which should be easy. But for the other problem, I could be wrong on this, but the TVN_ENDLABELEDITW goes through SendRealNotify and not SendSimpleNotify. Doing this in SendSimpleNotify should be OK and prevent similar problems in other notificatins like NM_DBLCLK. Thanks for the feedback.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/8258#note_105977
On Mon Jun 9 22:42:08 2025 +0000, Jacob Czekalla wrote:
I definitely need to fix the bad infoPtr, which should be easy. But for the other problem, I could be wrong on this, but the TVN_ENDLABELEDITW goes through SendRealNotify and not SendSimpleNotify. Doing this in SendSimpleNotify should be OK and prevent similar problems in other notificatins like NM_DBLCLK. Thanks for the feedback. The point is that right now return value means application return value from WM_NOTIFY handling. After this change it diverges between two helpers that are basically the same. Making same change to SendRealNotify does not work for ENDLABELEDIT, because it means the opposite thing for it.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/8258#note_105978
On Mon Jun 9 22:47:14 2025 +0000, Nikolay Sivov wrote:
The point is that right now return value means application return value from WM_NOTIFY handling. After this change it diverges between two helpers that are basically the same. Making same change to SendRealNotify does not work for ENDLABELEDIT, because it means the opposite thing for it. Fair enough. I can fix the bad infoPtr access and I will move the the check inside WM_LBUTTONDOWN.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/8258#note_105985
participants (3)
-
Jacob Czekalla -
Jacob Czekalla (＠JacobCzekalla)
-
Nikolay Sivov (＠nsivov)