tdf#104686: do not crash if Menu has been somehow disposed

The rare crashes in MenuFloatingWindow::ImplGetStartY() and
MenuFloatingWindow::ImplScroll(bool) likely happen because
of a disposed Menu.

Let's guard against invalid accesses.

Change-Id: Ie31240abbc48c06edd40d0a95f319725cdb3db16
Reviewed-on: https://gerrit.libreoffice.org/36026
Tested-by: Jenkins <ci@libreoffice.org>
Reviewed-by: Michael Meeks <michael.meeks@collabora.com>

5 files changed, 58 insertions, 32 deletions

diff --git a/include/vcl/menu.hxx b/include/vcl/menu.hxx
index 3249a3bac25f..4e11b3d2e0da 100644
--- a/include/vcl/menu.hxx
+++ b/include/vcl/menu.hxx
@@ -133,7 +133,7 @@ class VCL_DLLPUBLIC Menu : public Resource, public VclReferenceBase
     friend struct ImplMenuDelData;
 private:
     ImplMenuDelData* mpFirstDel;
-    MenuItemList* pItemList; // list with MenuItems
+    std::unique_ptr<MenuItemList> pItemList; // list with MenuItems
     MenuLogo* pLogo;
     VclPtr<Menu> pStartedFrom;
     VclPtr<vcl::Window> pWindow;
@@ -355,7 +355,7 @@ public:
     // Fuer Menu-'Funktionen'
     MenuItemList* GetItemList() const
     {
-        return pItemList;
+        return pItemList.get();
     }
 
     // returns the system's menu handle if native menus are supported
diff --git a/vcl/source/window/menu.cxx b/vcl/source/window/menu.cxx
index cda664076440..866c0f03ca6c 100644
--- a/vcl/source/window/menu.cxx
+++ b/vcl/source/window/menu.cxx
@@ -188,9 +188,11 @@ void Menu::dispose()
 
     bKilled = true;
 
-    delete pItemList;
+    pItemList->Clear();
     delete pLogo;
+    pLogo = nullptr;
     delete mpLayoutData;
+    mpLayoutData = nullptr;
 
     // Native-support: destroy SalMenu
     ImplSetSalMenu( nullptr );
diff --git a/vcl/source/window/menufloatingwindow.cxx b/vcl/source/window/menufloatingwindow.cxx
index 2c27e57ec775..5852b1980505 100644
--- a/vcl/source/window/menufloatingwindow.cxx
+++ b/vcl/source/window/menufloatingwindow.cxx
@@ -157,6 +157,12 @@ long MenuFloatingWindow::ImplGetStartY() const
     long nY = 0;
     if( pMenu )
     {
+        // avoid crash if somehow menu got disposed, and MenuItemList is empty (workaround for tdf#104686)
+        if ( nFirstEntry > 0 && !pMenu->GetItemList()->GetDataFromPos(nFirstEntry - 1) )
+        {
+            return 0;
+        }
+
         for ( sal_uInt16 n = 0; n < nFirstEntry; n++ )
             nY += pMenu->GetItemList()->GetDataFromPos( n )->aSz.Height();
         nY -= pMenu->GetTitleHeight();
@@ -611,45 +617,55 @@ void MenuFloatingWindow::ImplScroll( bool bUp )
         nFirstEntry = pMenu->ImplGetPrevVisible( nFirstEntry );
         SAL_WARN_IF( nFirstEntry == ITEMPOS_INVALID, "vcl", "Scroll?!" );
 
-        long nScrollEntryHeight = pMenu->GetItemList()->GetDataFromPos( nFirstEntry )->aSz.Height();
-
-        if ( !bScrollDown )
+        // avoid crash if somehow menu got disposed, and MenuItemList is empty (workaround for tdf#104686)
+        const auto pItemData = pMenu->GetItemList()->GetDataFromPos( nFirstEntry );
+        if ( pItemData )
         {
-            bScrollDown = true;
-            Invalidate();
-        }
+            long nScrollEntryHeight = pItemData->aSz.Height();
 
-        if ( pMenu->ImplGetPrevVisible( nFirstEntry ) == ITEMPOS_INVALID )
-        {
-            bScrollUp = false;
-            Invalidate();
-        }
+            if ( !bScrollDown )
+            {
+                bScrollDown = true;
+                Invalidate();
+            }
+
+            if ( pMenu->ImplGetPrevVisible( nFirstEntry ) == ITEMPOS_INVALID )
+            {
+                bScrollUp = false;
+                Invalidate();
+            }
 
-        Scroll( 0, nScrollEntryHeight, ImplCalcClipRegion( false ).GetBoundRect(), ScrollFlags::Clip );
+            Scroll( 0, nScrollEntryHeight, ImplCalcClipRegion( false ).GetBoundRect(), ScrollFlags::Clip );
+        }
     }
     else if ( bScrollDown && !bUp )
     {
-        long nScrollEntryHeight = pMenu->GetItemList()->GetDataFromPos( nFirstEntry )->aSz.Height();
+        // avoid crash if somehow menu got disposed, and MenuItemList is empty (workaround for tdf#104686)
+        const auto pItemData = pMenu->GetItemList()->GetDataFromPos( nFirstEntry );
+        if ( pItemData )
+        {
+            long nScrollEntryHeight = pItemData->aSz.Height();
 
-        nFirstEntry = pMenu->ImplGetNextVisible( nFirstEntry );
-        SAL_WARN_IF( nFirstEntry == ITEMPOS_INVALID, "vcl", "Scroll?!" );
+            nFirstEntry = pMenu->ImplGetNextVisible( nFirstEntry );
+            SAL_WARN_IF( nFirstEntry == ITEMPOS_INVALID, "vcl", "Scroll?!" );
 
-        if ( !bScrollUp )
-        {
-            bScrollUp = true;
-            Invalidate();
-        }
+            if ( !bScrollUp )
+            {
+                bScrollUp = true;
+                Invalidate();
+            }
 
-        long nHeight = GetOutputSizePixel().Height();
-        sal_uInt16 nLastVisible;
-        static_cast<PopupMenu*>(pMenu.get())->ImplCalcVisEntries( nHeight, nFirstEntry, &nLastVisible );
-        if ( pMenu->ImplGetNextVisible( nLastVisible ) == ITEMPOS_INVALID )
-        {
-            bScrollDown = false;
-            Invalidate();
-        }
+            long nHeight = GetOutputSizePixel().Height();
+            sal_uInt16 nLastVisible;
+            static_cast<PopupMenu*>(pMenu.get())->ImplCalcVisEntries( nHeight, nFirstEntry, &nLastVisible );
+            if ( pMenu->ImplGetNextVisible( nLastVisible ) == ITEMPOS_INVALID )
+            {
+                bScrollDown = false;
+                Invalidate();
+            }
 
-        Scroll( 0, -nScrollEntryHeight, ImplCalcClipRegion( false ).GetBoundRect(), ScrollFlags::Clip );
+            Scroll( 0, -nScrollEntryHeight, ImplCalcClipRegion( false ).GetBoundRect(), ScrollFlags::Clip );
+        }
     }
 
     Invalidate();
diff --git a/vcl/source/window/menuitemlist.cxx b/vcl/source/window/menuitemlist.cxx
index 7316ed98355f..67cdb6af2cce 100644
--- a/vcl/source/window/menuitemlist.cxx
+++ b/vcl/source/window/menuitemlist.cxx
@@ -135,6 +135,13 @@ void MenuItemList::Remove( size_t nPos )
     }
 }
 
+void MenuItemList::Clear()
+{
+    for (MenuItemData* i : maItemList)
+        delete i;
+    maItemList.resize(0);
+}
+
 MenuItemData* MenuItemList::GetData( sal_uInt16 nSVId, size_t& rPos ) const
 {
     for( size_t i = 0, n = maItemList.size(); i < n; ++i )
diff --git a/vcl/source/window/menuitemlist.hxx b/vcl/source/window/menuitemlist.hxx
index b06a61446de4..dbcc4c805dfb 100644
--- a/vcl/source/window/menuitemlist.hxx
+++ b/vcl/source/window/menuitemlist.hxx
@@ -116,6 +116,7 @@ public:
                     );
     void            InsertSeparator(const OString &rIdent, size_t nPos);
     void            Remove( size_t nPos );
+    void            Clear();
 
     MenuItemData*   GetData( sal_uInt16 nSVId, size_t& rPos ) const;
     MenuItemData*   GetData( sal_uInt16 nSVId ) const