tdf#113413 dbaccess: only use SolarMutex in ODatabaseDocument

The classes ODatabaseDocument, ODatabaseModelImpl and
ODatabaseSource share a single mutex declared as
ModelDependentComponent::m_aMutex.

Commit 403eefe81b8a0afe888c60452c17d6b2c5d8343f introduced
a new deadlock here: in case Yield is called, such as
when the user doesn't have a JRE and a dialog requesting
to install one is displayed, the SolarMutex is released
but the ModelDependentComponent mutex is still locked;
now another thread (such as the AsyncEventNotifier)
can lock the SolarMutex but not the other mutex,
while the main thread can't lock the SolarMutex again.

So the SolarMutex must be on the one hand be
locked before the other mutex, to ensure the behaviour
is the same whether the thread already holds it or not,
and locked after the other mutex, to prevent the Yield deadlock.

One option would be to revisit fca62934f492125ea6728fd6d09f0c66c9e4fa69
and add back the SfxLibraryContainer mutex, but the code
there is a mess, naively assuming that locking the mutex on
UNO entry points is sufficient, taking no care to temporarily
release it while calling other UNO components or event listeners;
since there's about 5kLOC in all the related classes it
would take some time to rewrite all that.

An easier fix is to remove the ModelDependentComponent::m_aMutex.

This patch removes all locking of that m_aMutex, but it still
exists as a non-shared Mutex because it's needed for the
WeakComponentImplHelper base classes, which shouldn't cause issues.

Reviewed-on: https://gerrit.libreoffice.org/45914
Tested-by: Jenkins <ci@libreoffice.org>
Reviewed-by: Michael Stahl <mstahl@redhat.com>
(cherry picked from commit a36b3f371343c4fb3708cbe0cd50270dda272385)

Change-Id: I94aaa0ae8698188c98633c638100a309b20976cc
Reviewed-on: https://gerrit.libreoffice.org/45925
Reviewed-by: Noel Grandin <noel.grandin@collabora.co.uk>
Tested-by: Jenkins <ci@libreoffice.org>
Reviewed-by: Michael Stahl <mstahl@redhat.com>

4 files changed, 11 insertions, 41 deletions

diff --git a/dbaccess/source/core/dataaccess/ModelImpl.cxx b/dbaccess/source/core/dataaccess/ModelImpl.cxx
index 48643a9c4892..8f3c47dadf45 100644
--- a/dbaccess/source/core/dataaccess/ModelImpl.cxx
+++ b/dbaccess/source/core/dataaccess/ModelImpl.cxx
@@ -402,8 +402,6 @@ void SAL_CALL DocumentStorageAccess::disposing( const css::lang::EventObject& So
 ODatabaseModelImpl::ODatabaseModelImpl( const Reference< XComponentContext >& _rxContext, ODatabaseContext& _rDBContext )
             :m_xModel()
             ,m_xDataSource()
-            ,m_aMutex()
-            ,m_aMutexFacade( m_aMutex )
             ,m_aContainer(4)
             ,m_aMacroMode( *this )
             ,m_nImposedMacroExecMode( MacroExecMode::NEVER_EXECUTE )
@@ -436,8 +434,6 @@ ODatabaseModelImpl::ODatabaseModelImpl(
                     )
             :m_xModel()
             ,m_xDataSource()
-            ,m_aMutex()
-            ,m_aMutexFacade( m_aMutex )
             ,m_aContainer(4)
             ,m_aMacroMode( *this )
             ,m_nImposedMacroExecMode( MacroExecMode::NEVER_EXECUTE )
@@ -1207,13 +1203,13 @@ namespace
 Reference< XStorage > ODatabaseModelImpl::impl_switchToStorage_throw( const Reference< XStorage >& _rxNewRootStorage )
 {
     // stop listening for modifications at the old storage
-    lcl_modifyListening( *this, m_xDocumentStorage.getTyped(), m_pStorageModifyListener, m_aMutexFacade, false );
+    lcl_modifyListening( *this, m_xDocumentStorage.getTyped(), m_pStorageModifyListener, Application::GetSolarMutex(), false );
 
     // set new storage
     m_xDocumentStorage.reset( _rxNewRootStorage, SharedStorage::TakeOwnership );
 
     // start listening for modifications
-    lcl_modifyListening( *this, m_xDocumentStorage.getTyped(), m_pStorageModifyListener, m_aMutexFacade, true );
+    lcl_modifyListening( *this, m_xDocumentStorage.getTyped(), m_pStorageModifyListener, Application::GetSolarMutex(), true );
 
     // forward new storage to Basic and Dialog library containers
     lcl_rebaseScriptStorage_throw( m_xBasicLibraries, m_xDocumentStorage.getTyped() );
@@ -1345,7 +1341,6 @@ void ODatabaseModelImpl::storageIsModified()
 
 ModelDependentComponent::ModelDependentComponent( const ::rtl::Reference< ODatabaseModelImpl >& _model )
     :m_pImpl( _model )
-    ,m_aMutex( _model->getSharedMutex() )
 {
 }
 
diff --git a/dbaccess/source/core/dataaccess/databasedocument.cxx b/dbaccess/source/core/dataaccess/databasedocument.cxx
index bfffaa9d7633..aa0c22f1d87c 100644
--- a/dbaccess/source/core/dataaccess/databasedocument.cxx
+++ b/dbaccess/source/core/dataaccess/databasedocument.cxx
@@ -1504,13 +1504,13 @@ void SAL_CALL ODatabaseDocument::close(sal_Bool bDeliverOwnership)
     }
     catch ( const Exception& )
     {
-        ::osl::MutexGuard aGuard( m_aMutex );
+        SolarMutexGuard g;
         m_bClosing = false;
         throw;
     }
 
     // SYNCHRONIZED ->
-    ::osl::MutexGuard aGuard( m_aMutex );
+    SolarMutexGuard g;
     m_bClosing = false;
     // <- SYNCHRONIZED
 }
@@ -1789,7 +1789,7 @@ void ODatabaseDocument::disposing()
     std::vector< Reference< XInterface > > aKeepAlive;
 
     // SYNCHRONIZED ->
-    ::osl::ClearableMutexGuard aGuard( m_aMutex );
+    SolarMutexClearableGuard aGuard;
 
     OSL_ENSURE( m_aControllers.empty(), "ODatabaseDocument::disposing: there still are controllers!" );
     // normally, nobody should explicitly dispose, but only XCloseable::close
diff --git a/dbaccess/source/core/dataaccess/datasource.cxx b/dbaccess/source/core/dataaccess/datasource.cxx
index 0bad4dd8b594..d37c5f3325c3 100644
--- a/dbaccess/source/core/dataaccess/datasource.cxx
+++ b/dbaccess/source/core/dataaccess/datasource.cxx
@@ -493,7 +493,7 @@ void ODatabaseSource::setName( const Reference< XDocumentDataSource >& _rxDocume
 {
     ODatabaseSource& rModelImpl = dynamic_cast< ODatabaseSource& >( *_rxDocument.get() );
 
-    ::osl::MutexGuard aGuard( rModelImpl.m_aMutex );
+    SolarMutexGuard g;
     if ( rModelImpl.m_pImpl.is() )
         rModelImpl.m_pImpl->m_sName = _rNewName;
 }
@@ -1065,8 +1065,6 @@ Reference< XConnection > SAL_CALL ODatabaseSource::connectWithCompletion( const
         // handle the request
         try
         {
-            MutexRelease aRelease( getMutex() );
-                // release the mutex when calling the handler, it may need to lock the SolarMutex
             _rxHandler->handle(xRequest);
         }
         catch(Exception&)
diff --git a/dbaccess/source/core/inc/ModelImpl.hxx b/dbaccess/source/core/inc/ModelImpl.hxx
index 7a4931557bab..0d694c64b10d 100644
--- a/dbaccess/source/core/inc/ModelImpl.hxx
+++ b/dbaccess/source/core/inc/ModelImpl.hxx
@@ -172,8 +172,6 @@ private:
     css::uno::WeakReference< css::sdbc::XDataSource >                 m_xDataSource;
 
     rtl::Reference<DocumentStorageAccess>                             m_pStorageAccess;
-    ::comphelper::SharedMutex                                         m_aMutex;
-    VosMutexFacade                                                    m_aMutexFacade;
     std::vector< TContentPtr >                                      m_aContainer;   // one for each ObjectType
     ::sfx2::DocumentMacroMode                                         m_aMacroMode;
     sal_Int16                                                         m_nImposedMacroExecMode;
@@ -367,8 +365,6 @@ public:
     css::uno::Reference< css::document::XDocumentSubStorageSupplier >
             getDocumentSubStorageSupplier();
 
-    const ::comphelper::SharedMutex& getSharedMutex() const { return m_aMutex; }
-
     void SAL_CALL acquire();
 
     void SAL_CALL release();
@@ -496,7 +492,7 @@ class ModelDependentComponent
 {
 protected:
     ::rtl::Reference< ODatabaseModelImpl >  m_pImpl;
-    mutable ::comphelper::SharedMutex       m_aMutex;
+    ::osl::Mutex m_aMutex; // only use this to init WeakComponentImplHelper
 
 protected:
     explicit ModelDependentComponent( const ::rtl::Reference< ODatabaseModelImpl >& _model );
@@ -506,25 +502,12 @@ protected:
     */
     virtual css::uno::Reference< css::uno::XInterface > getThis() const = 0;
 
-    ::osl::Mutex& getMutex() const
+    ::osl::Mutex& getMutex()
     {
         return m_aMutex;
     }
 
 public:
-    struct GuardAccess { friend class ModelMethodGuard; private: GuardAccess() { } };
-
-    /** returns the mutex used for thread safety
-
-        @throws css::lang::DisposedException
-            if m_pImpl is <NULL/>. Usually, you will set this member in your derived
-            component's <code>dispose</code> method to <NULL/>.
-    */
-    ::osl::Mutex& getMutex( GuardAccess ) const
-    {
-        return getMutex();
-    }
-
     /// checks whether the component is already disposed, throws a DisposedException if so
     void checkDisposed() const
     {
@@ -569,11 +552,8 @@ private:
 class ModelMethodGuard
 {
 private:
-    // to avoid deadlocks, lock SolarMutex too, and before the own osl::Mutex
+    // to avoid deadlocks, lock SolarMutex
     SolarMutexResettableGuard m_SolarGuard;
-    ::osl::ResettableMutexGuard m_OslGuard;
-
-    typedef ::osl::ResettableMutexGuard             BaseMutexGuard;
 
 public:
     /** constructs the guard
@@ -585,22 +565,19 @@ public:
             If the given component is already disposed
     */
     explicit ModelMethodGuard( const ModelDependentComponent& _component )
-        : m_OslGuard(_component.getMutex(ModelDependentComponent::GuardAccess()))
     {
         _component.checkDisposed();
     }
 
     void clear()
     {
-        m_OslGuard.clear();
         // note: this only releases *once* so may still be locked
-        m_SolarGuard.clear(); // SolarMutex last
+        m_SolarGuard.clear();
     }
 
     void reset()
     {
-        m_SolarGuard.reset(); // SolarMutex first
-        m_OslGuard.reset();
+        m_SolarGuard.reset();
     }
 };