[SHELL32] CDefView: Rework context menu handling

Previously, we would share one object between a multitude of options.
Now, the only two options that need to store something for later use each have their own space for it.
The context menu always cleans up after itself, the File menu does not.
CORE-18345
CORE-18361
CORE-18366

dll/win32/shell32/CDefView.cpp

@@ -58,7 +58,7 @@ typedef struct
 static void
 ClientToListView(HWND hwndLV, POINT *ppt)
 {
-    POINT Origin;
+    POINT Origin = {};
 
     /* FIXME: LVM_GETORIGIN is broken. See CORE-17266 */
     if (!ListView_GetOrigin(hwndLV, &Origin))
@@ -68,6 +68,33 @@ ClientToListView(HWND hwndLV, POINT *ppt)
     ppt->y += Origin.y;
 }
 
+// Helper struct to automatically cleanup the IContextMenu
+// We want to explicitly reset the Site, so there are no circular references
+struct MenuCleanup
+{
+    CComPtr<IContextMenu> &m_pCM;
+    HMENU &m_hMenu;
+
+    MenuCleanup(CComPtr<IContextMenu> &pCM, HMENU& menu)
+        : m_pCM(pCM), m_hMenu(menu)
+    {
+    }
+    ~MenuCleanup()
+    {
+        if (m_hMenu)
+        {
+            DestroyMenu(m_hMenu);
+            m_hMenu = NULL;
+        }
+        if (m_pCM)
+        {
+            IUnknown_SetSite(m_pCM, NULL);
+            m_pCM.Release();
+        }
+    }
+};
+
+
 class CDefView :
     public CWindowImpl<CDefView, CWindow, CControlWinTraits>,
     public CComObjectRootEx<CComMultiThreadModelNoCS>,
@@ -116,6 +143,7 @@ class CDefView :
         DWORD                     m_grfKeyState;
         //
         CComPtr<IContextMenu>     m_pCM;
+        CComPtr<IContextMenu>     m_pFileMenu;
 
         BOOL                      m_isEditing;
         BOOL                      m_isParentFolderSpecial;
@@ -169,7 +197,7 @@ class CDefView :
         void OnDeactivate();
         void DoActivate(UINT uState);
         HRESULT drag_notify_subitem(DWORD grfKeyState, POINTL pt, DWORD *pdwEffect);
-        HRESULT InvokeContextMenuCommand(UINT uCommand);
+        HRESULT InvokeContextMenuCommand(CComPtr<IContextMenu> &pCM, UINT uCommand);
         LRESULT OnExplorerCommand(UINT uCommand, BOOL bUseSelection);
 
         // *** IOleWindow methods ***
@@ -1310,13 +1338,6 @@ HRESULT CDefView::FillFileMenu()
     if (!hFileMenu)
         return E_FAIL;
 
-    /* Release cached IContextMenu */
-    if (m_pCM)
-    {
-        IUnknown_SetSite(m_pCM, NULL);
-        m_pCM.Release();
-    }
-
     /* Cleanup the items added previously */
     for (int i = GetMenuItemCount(hFileMenu) - 1; i >= 0; i--)
     {
@@ -1327,15 +1348,20 @@ HRESULT CDefView::FillFileMenu()
 
     m_cidl = m_ListView.GetSelectedCount();
 
-    /* Store the context menu in m_pCM and keep it in order to invoke the selected command later on */
+    /* In case we still have this left over, clean it up! */
-    HRESULT hr = GetItemObject((m_cidl ? SVGIO_SELECTION : SVGIO_BACKGROUND),
+    if (m_pFileMenu)
-                               IID_PPV_ARG(IContextMenu, &m_pCM));
+    {
+        IUnknown_SetSite(m_pFileMenu, NULL);
+        m_pFileMenu.Release();
+    }
+    /* Store the context menu in m_pFileMenu and keep it in order to invoke the selected command later on */
+    HRESULT hr = GetItemObject((m_cidl ? SVGIO_SELECTION : SVGIO_BACKGROUND), IID_PPV_ARG(IContextMenu, &m_pFileMenu));
     if (FAILED_UNEXPECTEDLY(hr))
         return hr;
 
     HMENU hmenu = CreatePopupMenu();
 
-    hr = m_pCM->QueryContextMenu(hmenu, 0, FCIDM_SHVIEWFIRST, FCIDM_SHVIEWLAST, 0);
+    hr = m_pFileMenu->QueryContextMenu(hmenu, 0, FCIDM_SHVIEWFIRST, FCIDM_SHVIEWLAST, 0);
     if (FAILED_UNEXPECTEDLY(hr))
         return hr;
 
@@ -1470,7 +1496,7 @@ UINT CDefView::GetSelections()
     return m_cidl;
 }
 
-HRESULT CDefView::InvokeContextMenuCommand(UINT uCommand)
+HRESULT CDefView::InvokeContextMenuCommand(CComPtr<IContextMenu> &pCM, UINT uCommand)
 {
     CMINVOKECOMMANDINFO cmi;
 
@@ -1485,7 +1511,11 @@ HRESULT CDefView::InvokeContextMenuCommand(UINT uCommand)
     if (GetKeyState(VK_CONTROL) & 0x8000)
         cmi.fMask |= CMIC_MASK_CONTROL_DOWN;
 
-    HRESULT hr = m_pCM->InvokeCommand(&cmi);
+    HRESULT hr = pCM->InvokeCommand(&cmi);
+    // Most of our callers will do this, but in case they don't do that (File menu!)
+    IUnknown_SetSite(pCM, NULL);
+    pCM.Release();
+
     if (FAILED_UNEXPECTEDLY(hr))
         return hr;
 
@@ -1513,33 +1543,24 @@ HRESULT CDefView::OpenSelectedItems()
     if (!hMenu)
         return E_FAIL;
 
-    hResult = GetItemObject(SVGIO_SELECTION, IID_PPV_ARG(IContextMenu, &m_pCM));
+    CComPtr<IContextMenu> pCM;
+    hResult = GetItemObject(SVGIO_SELECTION, IID_PPV_ARG(IContextMenu, &pCM));
+    MenuCleanup _(pCM, hMenu);
     if (FAILED_UNEXPECTEDLY(hResult))
-        goto cleanup;
+        return hResult;
 
-    hResult = m_pCM->QueryContextMenu(hMenu, 0, FCIDM_SHVIEWFIRST, FCIDM_SHVIEWLAST, CMF_DEFAULTONLY);
+    hResult = pCM->QueryContextMenu(hMenu, 0, FCIDM_SHVIEWFIRST, FCIDM_SHVIEWLAST, CMF_DEFAULTONLY);
     if (FAILED_UNEXPECTEDLY(hResult))
-        goto cleanup;
+        return hResult;
 
     uCommand = GetMenuDefaultItem(hMenu, FALSE, 0);
     if (uCommand == (UINT)-1)
     {
-        hResult = E_FAIL;
+        ERR("GetMenuDefaultItem returned -1\n");
-        goto cleanup;
+        return E_FAIL;
     }
 
-    InvokeContextMenuCommand(uCommand);
+    InvokeContextMenuCommand(pCM, uCommand);
-
-cleanup:
-
-    if (hMenu)
-        DestroyMenu(hMenu);
-
-    if (m_pCM)
-    {
-        IUnknown_SetSite(m_pCM, NULL);
-        m_pCM.Release();
-    }
 
     return hResult;
 }
@@ -1555,6 +1576,12 @@ LRESULT CDefView::OnContextMenu(UINT uMsg, WPARAM wParam, LPARAM lParam, BOOL &b
 
     TRACE("(%p)\n", this);
 
+    if (m_hContextMenu != NULL)
+    {
+        ERR("HACK: Aborting context menu in nested call!\n");
+        return 0;
+    }
+
     m_hContextMenu = CreatePopupMenu();
     if (!m_hContextMenu)
         return E_FAIL;
@@ -1578,15 +1605,16 @@ LRESULT CDefView::OnContextMenu(UINT uMsg, WPARAM wParam, LPARAM lParam, BOOL &b
     }
 
     m_cidl = m_ListView.GetSelectedCount();
-
+    /* In case we still have this left over, clean it up! */
     hResult = GetItemObject( m_cidl ? SVGIO_SELECTION : SVGIO_BACKGROUND, IID_PPV_ARG(IContextMenu, &m_pCM));
+    MenuCleanup _(m_pCM, m_hContextMenu);
     if (FAILED_UNEXPECTEDLY(hResult))
-        goto cleanup;
+        return 0;
 
     /* Use 1 as the first id as we want 0 the mean that the user canceled the menu */
     hResult = m_pCM->QueryContextMenu(m_hContextMenu, 0, CONTEXT_MENU_BASE_ID, FCIDM_SHVIEWLAST, CMF_NORMAL);
     if (FAILED_UNEXPECTEDLY(hResult))
-        goto cleanup;
+        return 0;
 
     /* There is no position requested, so try to find one */
     if (lParam == ~0)
@@ -1624,29 +1652,17 @@ LRESULT CDefView::OnContextMenu(UINT uMsg, WPARAM wParam, LPARAM lParam, BOOL &b
         y = pt.y;
     }
 
+    // This runs the message loop, calling back to us with f.e. WM_INITPOPUP (hence why m_hContextMenu and m_pCM exist) 
     uCommand = TrackPopupMenu(m_hContextMenu,
                               TPM_LEFTALIGN | TPM_RETURNCMD | TPM_LEFTBUTTON | TPM_RIGHTBUTTON,
                               x, y, 0, m_hWnd, NULL);
     if (uCommand == 0)
-        goto cleanup;
+        return 0;
 
     if (uCommand == FCIDM_SHVIEW_OPEN && OnDefaultCommand() == S_OK)
-        goto cleanup;
+        return 0;
 
-    InvokeContextMenuCommand(uCommand - CONTEXT_MENU_BASE_ID);
+    InvokeContextMenuCommand(m_pCM, uCommand - CONTEXT_MENU_BASE_ID);
-
-cleanup:
-    if (m_pCM)
-    {
-        IUnknown_SetSite(m_pCM, NULL);
-        m_pCM.Release();
-    }
-
-    if (m_hContextMenu)
-    {
-        DestroyMenu(m_hContextMenu);
-        m_hContextMenu = NULL;
-    }
 
     return 0;
 }
@@ -1656,18 +1672,22 @@ LRESULT CDefView::OnExplorerCommand(UINT uCommand, BOOL bUseSelection)
     HRESULT hResult;
     HMENU hMenu = NULL;
 
-    hResult = GetItemObject( bUseSelection ? SVGIO_SELECTION : SVGIO_BACKGROUND, IID_PPV_ARG(IContextMenu, &m_pCM));
+    CComPtr<IContextMenu> pCM;
-    if (FAILED_UNEXPECTEDLY( hResult))
+    hResult = GetItemObject( bUseSelection ? SVGIO_SELECTION : SVGIO_BACKGROUND, IID_PPV_ARG(IContextMenu, &pCM));
-        goto cleanup;
+    if (FAILED_UNEXPECTEDLY(hResult))
+        return 0;
+
+    MenuCleanup _(pCM, hMenu);
+
     if ((uCommand != FCIDM_SHVIEW_DELETE) && (uCommand != FCIDM_SHVIEW_RENAME))
     {
         hMenu = CreatePopupMenu();
         if (!hMenu)
             return 0;
 
-        hResult = m_pCM->QueryContextMenu(hMenu, 0, FCIDM_SHVIEWFIRST, FCIDM_SHVIEWLAST, CMF_NORMAL);
+        hResult = pCM->QueryContextMenu(hMenu, 0, FCIDM_SHVIEWFIRST, FCIDM_SHVIEWLAST, CMF_NORMAL);
         if (FAILED_UNEXPECTEDLY(hResult))
-            goto cleanup;
+            return 0;
     }
 
     if (bUseSelection)
@@ -1690,18 +1710,7 @@ LRESULT CDefView::OnExplorerCommand(UINT uCommand, BOOL bUseSelection)
             return 0;
     }
 
-    InvokeContextMenuCommand(uCommand);
+    InvokeContextMenuCommand(pCM, uCommand);
-
-cleanup:
-    if (m_pCM)
-    {
-        IUnknown_SetSite(m_pCM, NULL);
-        m_pCM.Release();
-    }
-
-    if (hMenu)
-        DestroyMenu(hMenu);
-
     return 0;
 }
 
@@ -1936,10 +1945,12 @@ LRESULT CDefView::OnCommand(UINT uMsg, WPARAM wParam, LPARAM lParam, BOOL &bHand
         case FCIDM_SHVIEW_NEWFOLDER:
             return OnExplorerCommand(dwCmdID, FALSE);
         default:
-            /* WM_COMMAND messages from the file menu are routed to the CDefView so as to let m_pCM handle the command */
+            /* WM_COMMAND messages from the file menu are routed to the CDefView so as to let m_pFileMenu handle the command */
-            if (m_pCM && dwCmd == 0)
+            if (m_pFileMenu && dwCmd == 0)
             {
-                InvokeContextMenuCommand(dwCmdID);
+                HMENU Dummy = NULL;
+                MenuCleanup _(m_pFileMenu, Dummy);
+                InvokeContextMenuCommand(m_pFileMenu, dwCmdID);
             }
     }
 
@@ -2370,10 +2381,10 @@ HRESULT SHSetMenuIdInMenuMsg(UINT uMsg, LPARAM lParam, UINT CmdId);
 */
 LRESULT CDefView::OnCustomItem(UINT uMsg, WPARAM wParam, LPARAM lParam, BOOL &bHandled)
 {
-    if (!m_pCM.p)
+    if (!m_pCM)
     {
         /* no menu */
-        ERR("no menu!!!\n");
+        ERR("no context menu!!!\n");
         return FALSE;
     }
 
@@ -2409,7 +2420,10 @@ LRESULT CDefView::OnInitMenuPopup(UINT uMsg, WPARAM wParam, LPARAM lParam, BOOL
     int nPos = LOWORD(lParam);
     UINT  menuItemId;
 
-    OnCustomItem(uMsg, wParam, lParam, bHandled);
+    if (m_pCM)
+    {
+        OnCustomItem(uMsg, wParam, lParam, bHandled);
+    }
 
     HMENU hViewMenu = GetSubmenuByID(m_hMenu, FCIDM_MENU_VIEW);