tdf#113413 dbaccess: only use SolarMutex in ODatabaseDocument

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

Commit 403eefe8 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 fca62934
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/45914Tested-by: Jenkins <ci@libreoffice.org>
Reviewed-by: Michael Stahl <mstahl@redhat.com>
(cherry picked from commit a36b3f37)

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

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() )
 {
 }
 

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

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&)

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();
     }
 };