Re: [PATCH] user32: Don't depend on HMENU being valid for popup menus

[Vincent Povirk]

if (menu->FocusedItem != NO_SELECTED_ITEM)
     {
+        ERR("focused: %u, nitms: 0x%x\n",
+                menu->FocusedItem,
+                menu->nItems);
  menu->items[menu->FocusedItem].fState &= ~(MF_HILITE|MF_MOUSESELECT);
  menu->FocusedItem = NO_SELECTED_ITEM;
     }

Is this stray debugging code or a real error condition?

- * Return the handle of the submenu, or hmenu if no submenu to display.
+ * Return the handle of the submenu, or menu if no submenu to display.

Still slightly inaccurate as the returned submenu is no longer a handle.

@@ -4787,24 +4770,29 @@ static BOOL SetMenuItemInfo_common(MENUITEM * menu,
  menu->wID = lpmii->wID;

     if (lpmii->fMask & MIIM_SUBMENU) {
- menu->hSubMenu = lpmii->hSubMenu;
- if (menu->hSubMenu) {
-    POPUPMENU *subMenu = MENU_GetMenu(menu->hSubMenu);
-    if (subMenu) {
-                if( MENU_depth( subMenu, 0) > MAXMENUDEPTH) {
+        if(menu->fType & MF_POPUP){
+            if(menu->submenu)
+                InterlockedDecrement(&menu->submenu->refcount);
+        }

How do we know this isn't the last reference?

I'm not sure this frees submenus correctly. If I create a popup menu
and add a submenu to it, that submenu has two references (one because
it has an HMENU and one because it is a submenu). DestroyMenu should
recursively destroy submenus. So it seems to me, destroying a menu
needs to destroy the handles to submenus (if they have one) and
release its references to them.

[PATCH] user32: Don't depend on HMENU being valid for popup menus

[Andrew Eikum]

On Tue, May 06, 2014 at 04:34:21PM -0500, Vincent Povirk wrote:
> if (menu->FocusedItem != NO_SELECTED_ITEM)
>      {
> +        ERR("focused: %u, nitms: 0x%x\n",
> +                menu->FocusedItem,
> +                menu->nItems);
>   menu->items[menu->FocusedItem].fState &= ~(MF_HILITE|MF_MOUSESELECT);
>   menu->FocusedItem = NO_SELECTED_ITEM;
>      }
>
> Is this stray debugging code or a real error condition?

Yes, it's debugging code. The dangers of sending really large
patches...
> I'm not sure this frees submenus correctly. If I create a popup menu
> and add a submenu to it, that submenu has two references (one because
> it has an HMENU and one because it is a submenu). DestroyMenu should
> recursively destroy submenus. So it seems to me, destroying a menu
> needs to destroy the handles to submenus (if they have one) and
> release its references to them.

We don't have any tests for this, though MSDN agrees with you. I'm
surprised that adding a submenu transfers the HMENU ownership to the
owning menu. I'll add a DestroyMenu call for submenus in
MENU_ReleaseMenu, which should close the handle and release the final
reference.

I fixed the other errors you pointed out. Thanks for reviewing!

Andrew