[PATCH 0/1] 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.

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

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

[Vladislav Timonin]

From: Vladislav Timonin <timoninvlad(a)yandex.ru>

---
 dlls/win32u/menu.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/dlls/win32u/menu.c b/dlls/win32u/menu.c
index 98456446e11..43c548b6461 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;
 }
-- 
GitLab

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

[PATCH 1/1] 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=126823

Your paranoid android.


=== debian11 (32 bit report) ===

shell32:
shelllink.c:847: Test failed: SHCreateStreamOnFileW failed 80070020
shelllink.c:849: Test failed: IPersistFile_Save failed (0x00000000)
shelllink.c:851: Test failed: IPersistFile_Load failed (0x00000000)

[Vladislav Timonin (＠vt)]

I was suggested to clear the menu while it was open to see if selection is reset on Windows.
With the help of a timer, ~~and re-running the test app 13 times because it keeps randomly crashing when clicking on any button~~, I conclude that it resets the selection.

When opening the menu with 3 items, and then pressing the down arrow to select the 1st item, when 1st item is removed, selection disappears and no other item is selected. Pressing the down arrow again then selects the 1st item.

With this patch, I get the similar behavior in Wine. Without, after the item is removed, pressing the down arrow selects the 2nd item.

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

[Vladislav Timonin (＠vt)]

On Sat Nov 26 18:44:57 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=126823
> Your paranoid android.
> === debian11 (32 bit report) ===
> shell32:
> shelllink.c:847: Test failed: SHCreateStreamOnFileW failed 80070020
> shelllink.c:849: Test failed: IPersistFile_Save failed (0x00000000)
> shelllink.c:851: Test failed: IPersistFile_Load failed (0x00000000)
> ```
> This and pipeline failure (winhttp) seem unrelated?

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