[PATCH v2 0/4] MR1554: win32u: Reset focused item when removing menu item

[Vladislav Timonin (＠vt)]

Fixes an out of bounds access in `show_popup`.

```c
if (menu->FocusedItem != NO_SELECTED_ITEM)
{
    menu->items[menu->FocusedItem].fState &= ~(MF_HILITE|MF_MOUSESELECT); // <- can crash here
    menu->FocusedItem = NO_SELECTED_ITEM;
}
```

Menu resets focused item on show, not on close.
If items were later deleted, next time user opens the menu, menu can crash on out of bounds access and won't show up again, as menu thinks it's still on screen.

In other words:
- open split button dropdown
- click on any item
- clear dropdown items
- open dropdown again

Menu borked and won't open again.

Source for the testing app for reproduction.
[test.c](/uploads/2f703f63891b33e1a0eb0fcd27c912b7/test.c)

I guess, the alternative is to reset focused item when menu is closed, but not sure where's the best place to do that, haven't dug deep enough.

--
  v2: win32u: Reset focused item when removing menu item
      win32u: Allow (de)select_item to work with off-screen menus
      win32u: Add deselect to HiliteMenuItem
      win32u: Return FALSE in HiliteMenuItem when window handle is NULL

https://gitlab.winehq.org/wine/wine/-/merge_requests/1554

[PATCH v2 1/4] win32u: Return FALSE in HiliteMenuItem when window handle is NULL

[Vladislav Timonin]

From: Vladislav Timonin

---
 dlls/user32/tests/menu.c | 3 ---
 dlls/win32u/menu.c       | 6 ++++++
 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/dlls/user32/tests/menu.c b/dlls/user32/tests/menu.c
index 6862f13c6b1..b00c403940f 100644
--- a/dlls/user32/tests/menu.c
+++ b/dlls/user32/tests/menu.c
@@ -2531,11 +2531,8 @@ static void test_menu_hilitemenuitem( void )
       "HiliteMenuItem: Item 2 is hilited\n");

SetLastError(0xdeadbeef);
-    todo_wine
-    {
     ok(!HiliteMenuItem(NULL, hPopupMenu, 1, MF_HILITE | MF_BYPOSITION),
       "HiliteMenuItem: call should have failed.\n");
-    }
     ok(GetLastError() == 0xdeadbeef || /* 9x */
        GetLastError() == ERROR_INVALID_WINDOW_HANDLE /* NT */,
       "HiliteMenuItem: expected error ERROR_INVALID_WINDOW_HANDLE, got: %ld\n", GetLastError());
diff --git a/dlls/win32u/menu.c b/dlls/win32u/menu.c
index 98456446e11..3e42547650a 100644
--- a/dlls/win32u/menu.c
+++ b/dlls/win32u/menu.c
@@ -4545,6 +4545,12 @@ BOOL WINAPI NtUserHiliteMenuItem( HWND hwnd, HMENU handle, UINT item, UINT hilit

TRACE( "(%p, %p, %04x, %04x);\n", hwnd, handle, item, hilite );

+    if (!hwnd)
+    {
+        RtlSetLastWin32Error(ERROR_INVALID_WINDOW_HANDLE);
+        return FALSE;
+    }
+
     if (!(menu = find_menu_item(handle, item, hilite, &pos))) return FALSE;
     handle_menu = menu->obj.handle;
     focused_item = menu->FocusedItem;

[PATCH v2 2/4] win32u: Add deselect to HiliteMenuItem

[Vladislav Timonin]

From: Vladislav Timonin

---
 dlls/win32u/menu.c | 47 +++++++++++++++++++++++++++++++++++++---------
 1 file changed, 38 insertions(+), 9 deletions(-)

diff --git a/dlls/win32u/menu.c b/dlls/win32u/menu.c
index 3e42547650a..397fda2420d 100644
--- a/dlls/win32u/menu.c
+++ b/dlls/win32u/menu.c
@@ -3152,6 +3152,35 @@ static void ensure_menu_item_visible( struct menu *menu, UINT index, HDC hdc )
     }
 }

+static void deselect_item(HWND owner, HMENU hmenu)
+{
+    struct menu *menu;
+    HDC hdc;
+
+    TRACE( "owner %p menu %p\n", owner, hmenu);
+
+    menu = unsafe_menu_ptr( hmenu );
+    if (!menu || !menu->nItems || !menu->hWnd)
+        return;
+
+    if (menu->wFlags & MF_POPUP)
+        hdc = NtUserGetDCEx( menu->hWnd, 0, DCX_USESTYLE );
+    else
+        hdc = NtUserGetDCEx( menu->hWnd, 0, DCX_CACHE | DCX_WINDOW );
+
+    NtGdiSelectFont( hdc, get_menu_font( FALSE ) );
+
+    if (menu->FocusedItem != NO_SELECTED_ITEM)
+    {
+        menu->items[menu->FocusedItem].fState &= ~(MF_HILITE | MF_MOUSESELECT);
+        draw_menu_item( menu->hWnd, menu, owner, hdc, &menu->items[menu->FocusedItem],
+                        !(menu->wFlags & MF_POPUP), ODA_SELECT );
+        menu->FocusedItem = NO_SELECTED_ITEM;
+    }
+
+    NtUserReleaseDC( menu->hWnd, hdc );
+}
+
 static void select_item( HWND owner, HMENU hmenu, UINT index, BOOL send_select, HMENU topmenu )
 {
     struct menu *menu;
@@ -3174,12 +3203,7 @@ static void select_item( HWND owner, HMENU hmenu, UINT index, BOOL send_select,
     NtGdiSelectFont( hdc, get_menu_font( FALSE ));

/* Clear previous highlighted item */
-    if (menu->FocusedItem != NO_SELECTED_ITEM)
-    {
-        menu->items[menu->FocusedItem].fState &= ~(MF_HILITE|MF_MOUSESELECT);
-        draw_menu_item( menu->hWnd, menu, owner, hdc, &menu->items[menu->FocusedItem],
-                        !(menu->wFlags & MF_POPUP), ODA_SELECT );
-    }
+    deselect_item(owner, hmenu);

/* Highlight new item (if any) */
     menu->FocusedItem = index;
@@ -4556,11 +4580,16 @@ BOOL WINAPI NtUserHiliteMenuItem( HWND hwnd, HMENU handle, UINT item, UINT hilit
     focused_item = menu->FocusedItem;
     release_menu_ptr(menu);

-    if (focused_item != pos)
+    if ((hilite & MF_HILITE))
     {
-        hide_sub_popups( hwnd, handle_menu, FALSE, 0 );
-        select_item( hwnd, handle_menu, pos, TRUE, 0 );
+        if (focused_item != pos)
+        {
+            hide_sub_popups( hwnd, handle_menu, FALSE, 0 );
+            select_item( hwnd, handle_menu, pos, TRUE, 0 );
+        }
     }
+    else /* MF_UNHILITE */
+        deselect_item( hwnd, handle_menu );

return TRUE;
 }

[PATCH v2 3/4] win32u: Allow (de)select_item to work with off-screen menus

[Vladislav Timonin]

From: Vladislav Timonin

---
 dlls/user32/tests/menu.c |  6 -----
 dlls/win32u/menu.c       | 58 +++++++++++++++++++++++++---------------
 2 files changed, 37 insertions(+), 27 deletions(-)

diff --git a/dlls/user32/tests/menu.c b/dlls/user32/tests/menu.c
index b00c403940f..9cd24141ad5 100644
--- a/dlls/user32/tests/menu.c
+++ b/dlls/user32/tests/menu.c
@@ -2578,11 +2578,8 @@ static void test_menu_hilitemenuitem( void )
     ok(GetLastError() == 0xdeadbeef,
       "HiliteMenuItem: expected error 0xdeadbeef, got: %ld\n", GetLastError());

-    todo_wine
-    {
     ok(GetMenuState(hPopupMenu, 1, MF_BYPOSITION) & MF_HILITE,
       "HiliteMenuItem: Item 2 is not hilited\n");
-    }

/* unhilite a menu item (by position) */

@@ -2603,11 +2600,8 @@ static void test_menu_hilitemenuitem( void )
     ok(GetLastError() == 0xdeadbeef,
       "HiliteMenuItem: expected error 0xdeadbeef, got: %ld\n", GetLastError());

-    todo_wine
-    {
     ok(GetMenuState(hPopupMenu, 2, MF_BYPOSITION) & MF_HILITE,
       "HiliteMenuItem: Item 3 is not hilited\n");
-    }

/* unhilite a menu item (by command) */

diff --git a/dlls/win32u/menu.c b/dlls/win32u/menu.c
index 397fda2420d..a0fb851bb87 100644
--- a/dlls/win32u/menu.c
+++ b/dlls/win32u/menu.c
@@ -3155,66 +3155,80 @@ static void ensure_menu_item_visible( struct menu *menu, UINT index, HDC hdc )
 static void deselect_item(HWND owner, HMENU hmenu)
 {
     struct menu *menu;
-    HDC hdc;
+    HDC hdc = NULL;

TRACE( "owner %p menu %p\n", owner, hmenu);

menu = unsafe_menu_ptr( hmenu );
-    if (!menu || !menu->nItems || !menu->hWnd)
+    if (!menu || !menu->nItems)
         return;

-    if (menu->wFlags & MF_POPUP)
-        hdc = NtUserGetDCEx( menu->hWnd, 0, DCX_USESTYLE );
-    else
-        hdc = NtUserGetDCEx( menu->hWnd, 0, DCX_CACHE | DCX_WINDOW );
+    if (menu->hWnd)
+    {
+        if (menu->wFlags & MF_POPUP)
+            hdc = NtUserGetDCEx( menu->hWnd, 0, DCX_USESTYLE );
+        else
+            hdc = NtUserGetDCEx( menu->hWnd, 0, DCX_CACHE | DCX_WINDOW );

-    NtGdiSelectFont( hdc, get_menu_font( FALSE ) );
+        NtGdiSelectFont( hdc, get_menu_font( FALSE ) );
+    }

if (menu->FocusedItem != NO_SELECTED_ITEM)
     {
         menu->items[menu->FocusedItem].fState &= ~(MF_HILITE | MF_MOUSESELECT);
-        draw_menu_item( menu->hWnd, menu, owner, hdc, &menu->items[menu->FocusedItem],
-                        !(menu->wFlags & MF_POPUP), ODA_SELECT );
+        if (menu->hWnd)
+            draw_menu_item( menu->hWnd, menu, owner, hdc, &menu->items[menu->FocusedItem],
+                            !(menu->wFlags & MF_POPUP), ODA_SELECT );
         menu->FocusedItem = NO_SELECTED_ITEM;
     }

-    NtUserReleaseDC( menu->hWnd, hdc );
+    if (hdc)
+        NtUserReleaseDC( menu->hWnd, hdc );
 }

static void select_item( HWND owner, HMENU hmenu, UINT index, BOOL send_select, HMENU topmenu )
 {
     struct menu *menu;
-    HDC hdc;
+    HDC hdc = NULL;

TRACE( "owner %p menu %p index 0x%04x select 0x%04x\n", owner, hmenu, index, send_select );

menu = unsafe_menu_ptr( hmenu );
-    if (!menu || !menu->nItems || !menu->hWnd) return;
+    if (!menu || !menu->nItems) return;

if (menu->FocusedItem == index) return;
-    if (menu->wFlags & MF_POPUP) hdc = NtUserGetDCEx( menu->hWnd, 0, DCX_USESTYLE );
-    else hdc = NtUserGetDCEx( menu->hWnd, 0, DCX_CACHE | DCX_WINDOW);
-    if (!top_popup)
+    if (menu->hWnd)
+    {
+        if (menu->wFlags & MF_POPUP)
+            hdc = NtUserGetDCEx( menu->hWnd, 0, DCX_USESTYLE );
+        else
+            hdc = NtUserGetDCEx( menu->hWnd, 0, DCX_CACHE | DCX_WINDOW);
+
+        NtGdiSelectFont( hdc, get_menu_font( FALSE ));
+    }
+    if (!top_popup && menu->hWnd)
     {
         top_popup = menu->hWnd;
         top_popup_hmenu = hmenu;
     }

-    NtGdiSelectFont( hdc, get_menu_font( FALSE ));
-
     /* Clear previous highlighted item */
     deselect_item(owner, hmenu);

/* Highlight new item (if any) */
     menu->FocusedItem = index;
+    send_select = send_select && menu->hWnd;
     if (menu->FocusedItem != NO_SELECTED_ITEM)
     {
         if (!(menu->items[index].fType & MF_SEPARATOR))
         {
             menu->items[index].fState |= MF_HILITE;
-            ensure_menu_item_visible( menu, index, hdc );
-            draw_menu_item( menu->hWnd, menu, owner, hdc, &menu->items[index],
-                            !(menu->wFlags & MF_POPUP), ODA_SELECT );
+            if (menu->hWnd)
+            {
+                ensure_menu_item_visible( menu, index, hdc );
+                draw_menu_item( menu->hWnd, menu, owner, hdc, &menu->items[index],
+                                !(menu->wFlags & MF_POPUP), ODA_SELECT );
+            }
         }
         if (send_select)
         {
@@ -3240,7 +3254,9 @@ static void select_item( HWND owner, HMENU hmenu, UINT index, BOOL send_select,
             }
         }
     }
-    NtUserReleaseDC( menu->hWnd, hdc );
+
+    if (hdc)
+        NtUserReleaseDC( menu->hWnd, hdc );
 }

/***********************************************************************

[PATCH v2 4/4] win32u: Reset focused item when removing menu item

[Vladislav Timonin]

From: Vladislav Timonin

---
 dlls/user32/tests/menu.c | 16 ++++++++++++++++
 dlls/win32u/menu.c       |  5 +++++
 2 files changed, 21 insertions(+)

diff --git a/dlls/user32/tests/menu.c b/dlls/user32/tests/menu.c
index 9cd24141ad5..53bff17e1e6 100644
--- a/dlls/user32/tests/menu.c
+++ b/dlls/user32/tests/menu.c
@@ -2614,6 +2614,22 @@ static void test_menu_hilitemenuitem( void )
     ok(!(GetMenuState(hPopupMenu, 2, MF_BYPOSITION) & MF_HILITE),
       "HiliteMenuItem: Item 3 is hilited\n");

+    /* deleting an item resets hilite */
+
+    SetLastError(0xdeadbeef);
+    ok(HiliteMenuItem(hWnd, hPopupMenu, 0, MF_HILITE | MF_BYPOSITION),
+      "HiliteMenuItem: call should not have failed.\n");
+    ok(GetLastError() == 0xdeadbeef,
+      "HiliteMenuItem: expected error 0xdeadbeef, got: %ld\n", GetLastError());
+    ok(GetMenuState(hPopupMenu, 0, MF_BYPOSITION) & MF_HILITE,
+      "HiliteMenuItem: Item 1 is not hilited\n");
+    ok(DeleteMenu(hPopupMenu, 2, MF_BYPOSITION),
+      "DeleteMenu: call should have succeeded.\n");
+    ok(!(GetMenuState(hPopupMenu, 0, MF_BYPOSITION) & MF_HILITE),
+      "HiliteMenuItem: Item 1 is hilited\n");
+    ok(!(GetMenuState(hPopupMenu, 1, MF_BYPOSITION) & MF_HILITE),
+      "HiliteMenuItem: Item 2 is hilited\n");
+
     DestroyWindow(hWnd);
 }

diff --git a/dlls/win32u/menu.c b/dlls/win32u/menu.c
index a0fb851bb87..ac9f31fe545 100644
--- a/dlls/win32u/menu.c
+++ b/dlls/win32u/menu.c
@@ -1375,6 +1375,9 @@ BOOL WINAPI NtUserRemoveMenu( HMENU handle, UINT id, UINT flags )
     {
         struct menu_item *new_items, *item = &menu->items[pos];

+        if (menu->FocusedItem != NO_SELECTED_ITEM)
+            menu->items[menu->FocusedItem].fState &= ~(MF_HILITE | MF_MOUSESELECT);
+
         while (pos < menu->nItems)
         {
             *item = item[1];
@@ -1385,6 +1388,8 @@ BOOL WINAPI NtUserRemoveMenu( HMENU handle, UINT id, UINT flags )
         if (new_items) menu->items = new_items;
     }

+    menu->FocusedItem = NO_SELECTED_ITEM;
+
     release_menu_ptr(menu);
     return TRUE;
 }

[PATCH v2 4/4] win32u: Reset focused item when removing menu item

[Marvin]

Hi,

It looks like your patch introduced the new failures shown below.
Please investigate and fix them before resubmitting your patch.
If they are not new, fixing them anyway would help a lot. Otherwise
please ask for the known failures list to be updated.

The tests also ran into some preexisting test failures. If you know how
to fix them that would be helpful. See the TestBot job for the details:

The full results can be found at:
https://testbot.winehq.org/JobDetails.pl?Key=126831

Your paranoid android.

=== w7u_2qxl (32 bit report) ===

user32:
menu.c:2628: Test failed: HiliteMenuItem: Item 1 is hilited

=== w7u_adm (32 bit report) ===

user32:
menu.c:2628: Test failed: HiliteMenuItem: Item 1 is hilited

=== w7u_el (32 bit report) ===

user32:
menu.c:2628: Test failed: HiliteMenuItem: Item 1 is hilited

=== w8 (32 bit report) ===

user32:
menu.c:2628: Test failed: HiliteMenuItem: Item 1 is hilited

=== w8adm (32 bit report) ===

user32:
menu.c:2628: Test failed: HiliteMenuItem: Item 1 is hilited

=== w864 (32 bit report) ===

user32:
menu.c:2628: Test failed: HiliteMenuItem: Item 1 is hilited

=== w1064v1507 (32 bit report) ===

user32:
menu.c:2628: Test failed: HiliteMenuItem: Item 1 is hilited

=== w1064v1809 (32 bit report) ===

user32:
menu.c:2628: Test failed: HiliteMenuItem: Item 1 is hilited

=== w1064_tsign (32 bit report) ===

user32:
menu.c:2628: Test failed: HiliteMenuItem: Item 1 is hilited

=== w10pro64 (32 bit report) ===

user32:
menu.c:2628: Test failed: HiliteMenuItem: Item 1 is hilited

=== w864 (64 bit report) ===

user32:
menu.c:2628: Test failed: HiliteMenuItem: Item 1 is hilited

=== w1064v1507 (64 bit report) ===

user32:
menu.c:2628: Test failed: HiliteMenuItem: Item 1 is hilited

=== w1064v1809 (64 bit report) ===

user32:
menu.c:2628: Test failed: HiliteMenuItem: Item 1 is hilited

=== w1064_2qxl (64 bit report) ===

user32:
menu.c:2628: Test failed: HiliteMenuItem: Item 1 is hilited

=== w1064_adm (64 bit report) ===

user32:
menu.c:2628: Test failed: HiliteMenuItem: Item 1 is hilited

=== w1064_tsign (64 bit report) ===

user32:
menu.c:2628: Test failed: HiliteMenuItem: Item 1 is hilited

=== w10pro64 (64 bit report) ===

user32:
menu.c:2628: Test failed: HiliteMenuItem: Item 1 is hilited

=== w10pro64_en_AE_u8 (64 bit report) ===

user32:
menu.c:2628: Test failed: HiliteMenuItem: Item 1 is hilited

=== w10pro64_ar (64 bit report) ===

user32:
menu.c:2628: Test failed: HiliteMenuItem: Item 1 is hilited

=== w10pro64_ja (64 bit report) ===

user32:
menu.c:2628: Test failed: HiliteMenuItem: Item 1 is hilited

=== w10pro64_zh_CN (64 bit report) ===

user32:
menu.c:2628: Test failed: HiliteMenuItem: Item 1 is hilited

[Vladislav Timonin (＠vt)]

Added a test, but for it be actually useful it required changing `HiliteMenuItem` to be able to work with off-screen menus.

[Vladislav Timonin (＠vt)]

On Sun Nov 27 16:58:55 2022 +0000, **** wrote:
> Marvin replied on the mailing list:
> Hi,
> It looks like your patch introduced the new failures shown below.
> Please investigate and fix them before resubmitting your patch.
> If they are not new, fixing them anyway would help a lot. Otherwise
> please ask for the known failures list to be updated.
> The tests also ran into some preexisting test failures. If you know how
> to fix them that would be helpful. See the TestBot job for the details:
> The full results can be found at:
> https://testbot.winehq.org/JobDetails.pl?Key=126831
> Your paranoid android.
> === w7u_2qxl (32 bit report) ===
> user32:
> menu.c:2628: Test failed: HiliteMenuItem: Item 1 is hilited
> === w7u_adm (32 bit report) ===
> user32:
> menu.c:2628: Test failed: HiliteMenuItem: Item 1 is hilited
> === w7u_el (32 bit report) ===
> user32:
> menu.c:2628: Test failed: HiliteMenuItem: Item 1 is hilited
> === w8 (32 bit report) ===
> user32:
> menu.c:2628: Test failed: HiliteMenuItem: Item 1 is hilited
> === w8adm (32 bit report) ===
> user32:
> menu.c:2628: Test failed: HiliteMenuItem: Item 1 is hilited
> === w864 (32 bit report) ===
> user32:
> menu.c:2628: Test failed: HiliteMenuItem: Item 1 is hilited
> === w1064v1507 (32 bit report) ===
> user32:
> menu.c:2628: Test failed: HiliteMenuItem: Item 1 is hilited
> === w1064v1809 (32 bit report) ===
> user32:
> menu.c:2628: Test failed: HiliteMenuItem: Item 1 is hilited
> === w1064_tsign (32 bit report) ===
> user32:
> menu.c:2628: Test failed: HiliteMenuItem: Item 1 is hilited
> === w10pro64 (32 bit report) ===
> user32:
> menu.c:2628: Test failed: HiliteMenuItem: Item 1 is hilited
> === w864 (64 bit report) ===
> user32:
> menu.c:2628: Test failed: HiliteMenuItem: Item 1 is hilited
> === w1064v1507 (64 bit report) ===
> user32:
> menu.c:2628: Test failed: HiliteMenuItem: Item 1 is hilited
> === w1064v1809 (64 bit report) ===
> user32:
> menu.c:2628: Test failed: HiliteMenuItem: Item 1 is hilited
> === w1064_2qxl (64 bit report) ===
> user32:
> menu.c:2628: Test failed: HiliteMenuItem: Item 1 is hilited
> === w1064_adm (64 bit report) ===
> user32:
> menu.c:2628: Test failed: HiliteMenuItem: Item 1 is hilited
> === w1064_tsign (64 bit report) ===
> user32:
> menu.c:2628: Test failed: HiliteMenuItem: Item 1 is hilited
> === w10pro64 (64 bit report) ===
> user32:
> menu.c:2628: Test failed: HiliteMenuItem: Item 1 is hilited
> === w10pro64_en_AE_u8 (64 bit report) ===
> user32:
> menu.c:2628: Test failed: HiliteMenuItem: Item 1 is hilited
> === w10pro64_ar (64 bit report) ===
> user32:
> menu.c:2628: Test failed: HiliteMenuItem: Item 1 is hilited
> === w10pro64_ja (64 bit report) ===
> user32:
> menu.c:2628: Test failed: HiliteMenuItem: Item 1 is hilited
> === w10pro64_zh_CN (64 bit report) ===
> user32:
> menu.c:2628: Test failed: HiliteMenuItem: Item 1 is hilited

Awkward, thought I checked this on Windows before submitting.

Then I'm guessing the reset happens when popup is opened or closed, seeing how the test GUI app refreshes the menu after an item was deleted. 
Wine currently doesn't do that so the item will be stuck on screen, non-interactable.