fix shutdown crash in basic

another change I am working on slightly tweaks the shutdown ordering
and exposes this problem where two classes both think they own
the same object.

Change-Id: I7477cf7eda5b5729ee3861cb4a1be43bb34d9ea6
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/99724
Tested-by: Jenkins
Reviewed-by: Noel Grandin <noel.grandin@collabora.co.uk>

5 files changed, 64 insertions, 53 deletions

diff --git a/basic/inc/sbxbase.hxx b/basic/inc/sbxbase.hxx
index 269f6029a55a..361dd52bd691 100644
--- a/basic/inc/sbxbase.hxx
+++ b/basic/inc/sbxbase.hxx
@@ -38,9 +38,8 @@ struct SbxAppData
 {
     ErrCode             eErrCode;  // Error code
     SbxVariableRef      m_aGlobErr; // Global error object
-    std::vector<std::unique_ptr<SbxFactory>>
-                        m_Factories;
-    tools::SvRef<SvRefBase> mrImplRepository;
+    std::vector<SbxFactory*> m_Factories; // these are owned by
+    tools::SvRef<SvRefBase>  mrImplRepository;
 
     // Pointer to Format()-Command helper class
     std::unique_ptr<SbxBasicFormater>   pBasicFormater;
@@ -55,6 +54,8 @@ struct SbxAppData
 };
 
 SbxAppData& GetSbxData_Impl();
+/** returns true if the SbxAppData is still valid, used to check if we are in shutdown. */
+bool IsSbxData_Impl();
 
 #endif // INCLUDED_BASIC_INC_SBXBASE_HXX
 
diff --git a/basic/source/classes/sb.cxx b/basic/source/classes/sb.cxx
index 1de6c8203426..c9f34e90ae16 100644
--- a/basic/source/classes/sb.cxx
+++ b/basic/source/classes/sb.cxx
@@ -463,14 +463,6 @@ SbxObject* SbiFactory::CreateObject( const OUString& rClass )
 }
 
 
-// Factory class to create OLE objects
-class SbOLEFactory : public SbxFactory
-{
-public:
-    virtual SbxBase* Create( sal_uInt16 nSbxId, sal_uInt32 ) override;
-    virtual SbxObject* CreateObject( const OUString& ) override;
-};
-
 SbxBase* SbOLEFactory::Create( sal_uInt16, sal_uInt32 )
 {
     // Not supported
@@ -486,13 +478,6 @@ SbxObject* SbOLEFactory::CreateObject( const OUString& rClassName )
 
 // SbFormFactory, show user forms by: dim as new <user form name>
 
-class SbFormFactory : public SbxFactory
-{
-public:
-    virtual SbxBase* Create( sal_uInt16 nSbxId, sal_uInt32 ) override;
-    virtual SbxObject* CreateObject( const OUString& ) override;
-};
-
 SbxBase* SbFormFactory::Create( sal_uInt16, sal_uInt32 )
 {
     // Not supported
@@ -587,14 +572,6 @@ SbxObject* cloneTypeObjectImpl( const SbxObject& rTypeObj )
     return pRet;
 }
 
-// Factory class to create user defined objects (type command)
-class SbTypeFactory : public SbxFactory
-{
-public:
-    virtual SbxBase* Create( sal_uInt16 nSbxId, sal_uInt32 ) override;
-    virtual SbxObject* CreateObject( const OUString& ) override;
-};
-
 SbxBase* SbTypeFactory::Create( sal_uInt16, sal_uInt32 )
 {
     // Not supported
@@ -922,14 +899,14 @@ StarBASIC::StarBASIC( StarBASIC* p, bool bIsDocBasic  )
     {
         GetSbData()->pSbFac.reset( new SbiFactory );
         AddFactory( GetSbData()->pSbFac.get() );
-        GetSbData()->pTypeFac = new SbTypeFactory;
-        AddFactory( GetSbData()->pTypeFac );
-        GetSbData()->pClassFac = new SbClassFactory;
-        AddFactory( GetSbData()->pClassFac );
-        GetSbData()->pOLEFac = new SbOLEFactory;
-        AddFactory( GetSbData()->pOLEFac );
-        GetSbData()->pFormFac = new SbFormFactory;
-        AddFactory( GetSbData()->pFormFac );
+        GetSbData()->pTypeFac.reset(new SbTypeFactory);
+        AddFactory( GetSbData()->pTypeFac.get() );
+        GetSbData()->pClassFac.reset(new SbClassFactory);
+        AddFactory( GetSbData()->pClassFac.get() );
+        GetSbData()->pOLEFac.reset(new SbOLEFactory);
+        AddFactory( GetSbData()->pOLEFac.get() );
+        GetSbData()->pFormFac.reset(new SbFormFactory);
+        AddFactory( GetSbData()->pFormFac.get() );
         GetSbData()->pUnoFac.reset( new SbUnoFactory );
         AddFactory( GetSbData()->pUnoFac.get() );
     }
@@ -963,14 +940,14 @@ StarBASIC::~StarBASIC()
         GetSbData()->pSbFac.reset();
         RemoveFactory( GetSbData()->pUnoFac.get() );
         GetSbData()->pUnoFac.reset();
-        RemoveFactory( GetSbData()->pTypeFac );
-        delete GetSbData()->pTypeFac; GetSbData()->pTypeFac = nullptr;
-        RemoveFactory( GetSbData()->pClassFac );
-        delete GetSbData()->pClassFac; GetSbData()->pClassFac = nullptr;
-        RemoveFactory( GetSbData()->pOLEFac );
-        delete GetSbData()->pOLEFac; GetSbData()->pOLEFac = nullptr;
-        RemoveFactory( GetSbData()->pFormFac );
-        delete GetSbData()->pFormFac; GetSbData()->pFormFac = nullptr;
+        RemoveFactory( GetSbData()->pTypeFac.get() );
+        GetSbData()->pTypeFac.reset();
+        RemoveFactory( GetSbData()->pClassFac.get() );
+        GetSbData()->pClassFac.reset();
+        RemoveFactory( GetSbData()->pOLEFac.get() );
+        GetSbData()->pOLEFac.reset();
+        RemoveFactory( GetSbData()->pFormFac.get() );
+        GetSbData()->pFormFac.reset();
 
         if( SbiGlobals::pGlobals )
         {
diff --git a/basic/source/inc/sbintern.hxx b/basic/source/inc/sbintern.hxx
index 75e3ede9a6bb..9a0436813f42 100644
--- a/basic/source/inc/sbintern.hxx
+++ b/basic/source/inc/sbintern.hxx
@@ -78,16 +78,45 @@ public:
     SbModule* FindClass( const OUString& rClassName );
 };
 
+// Factory class to create user defined objects (type command)
+class SbTypeFactory : public SbxFactory
+{
+public:
+    virtual SbxBase* Create( sal_uInt16 nSbxId, sal_uInt32 ) override;
+    virtual SbxObject* CreateObject( const OUString& ) override;
+};
+
+class SbFormFactory : public SbxFactory
+{
+public:
+    virtual SbxBase* Create( sal_uInt16 nSbxId, sal_uInt32 ) override;
+    virtual SbxObject* CreateObject( const OUString& ) override;
+};
+
+// Factory class to create OLE objects
+class SbOLEFactory : public SbxFactory
+{
+public:
+    virtual SbxBase* Create( sal_uInt16 nSbxId, sal_uInt32 ) override;
+    virtual SbxObject* CreateObject( const OUString& ) override;
+};
+
+
+
 struct SbiGlobals
 {
     static SbiGlobals* pGlobals;
     SbiInstance*    pInst;          // all active runtime instances
     std::unique_ptr<SbiFactory>   pSbFac;    // StarBASIC-Factory
     std::unique_ptr<SbUnoFactory> pUnoFac;   // Factory for Uno-Structs at DIM AS NEW
-    SbTypeFactory*  pTypeFac;       // Factory for user defined types
-    SbClassFactory* pClassFac;      // Factory for user defined classes (based on class modules)
-    SbOLEFactory*   pOLEFac;        // Factory for OLE types
-    SbFormFactory*  pFormFac;       // Factory for user forms
+    std::unique_ptr<SbTypeFactory>
+                    pTypeFac;       // Factory for user defined types
+    std::unique_ptr<SbClassFactory>
+                    pClassFac;      // Factory for user defined classes (based on class modules)
+    std::unique_ptr<SbOLEFactory>
+                    pOLEFac;        // Factory for OLE types
+    std::unique_ptr<SbFormFactory>
+                    pFormFac;       // Factory for user forms
     SbModule*       pMod;           // currently active module
     SbModule*       pCompMod;       // currently compiled module
     short           nInst;          // number of BASICs
diff --git a/basic/source/runtime/basrdll.cxx b/basic/source/runtime/basrdll.cxx
index 6da6ed9e2e2d..29cd292e2bdf 100644
--- a/basic/source/runtime/basrdll.cxx
+++ b/basic/source/runtime/basrdll.cxx
@@ -123,4 +123,9 @@ SbxAppData& GetSbxData_Impl()
     return *BasicDLLImpl::BASIC_DLL->xSbxAppData;
 }
 
+bool IsSbxData_Impl()
+{
+    return BasicDLLImpl::BASIC_DLL && BasicDLLImpl::BASIC_DLL->xSbxAppData;
+}
+
 /* vim:set shiftwidth=4 softtabstop=4 expandtab: */
diff --git a/basic/source/sbx/sbxbase.cxx b/basic/source/sbx/sbxbase.cxx
index c80b681c644d..f62949ada7ec 100644
--- a/basic/source/sbx/sbxbase.cxx
+++ b/basic/source/sbx/sbxbase.cxx
@@ -48,7 +48,9 @@ SbxAppData::~SbxAppData()
     pBasicFormater.reset();
     // basic manager repository must be destroyed before factories
     mrImplRepository.clear();
-    m_Factories.clear();
+    // we need to move stuff out otherwise the destruction of the factories
+    // calls back into SbxBase::RemoveFactory and sees partially destructed data
+    std::move(m_Factories);
 }
 
 SbxBase::SbxBase()
@@ -121,15 +123,12 @@ void SbxBase::AddFactory( SbxFactory* pFac )
 
 void SbxBase::RemoveFactory( SbxFactory const * pFac )
 {
+    if (!IsSbxData_Impl())
+        return;
     SbxAppData& r = GetSbxData_Impl();
-    auto it = std::find_if(r.m_Factories.begin(), r.m_Factories.end(),
-        [&pFac](const std::unique_ptr<SbxFactory>& rxFactory) { return rxFactory.get() == pFac; });
+    auto it = std::find(r.m_Factories.begin(), r.m_Factories.end(), pFac);
     if (it != r.m_Factories.end())
-    {
-        std::unique_ptr<SbxFactory> tmp(std::move(*it));
         r.m_Factories.erase( it );
-        (void)tmp.release();
-    }
 }