[Libreoffice-commits] core.git: Branch 'libreoffice-6-0' - dbaccess/source
Michael Stahl
mstahl at redhat.com
Wed Dec 6 12:02:10 UTC 2017
dbaccess/source/core/dataaccess/ModelImpl.cxx | 9 +----
dbaccess/source/core/dataaccess/databasedocument.cxx | 6 +--
dbaccess/source/core/dataaccess/datasource.cxx | 4 --
dbaccess/source/core/inc/ModelImpl.hxx | 33 ++-----------------
4 files changed, 11 insertions(+), 41 deletions(-)
New commits:
commit 83566e416c40c89656714e77d5101a4dbfc2751f
Author: Michael Stahl <mstahl at redhat.com>
Date: Tue Dec 5 22:51:41 2017 +0100
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 at libreoffice.org>
Reviewed-by: Michael Stahl <mstahl at redhat.com>
(cherry picked from commit a36b3f371343c4fb3708cbe0cd50270dda272385)
Change-Id: I94aaa0ae8698188c98633c638100a309b20976cc
Reviewed-on: https://gerrit.libreoffice.org/45925
Reviewed-by: Noel Grandin <noel.grandin at collabora.co.uk>
Tested-by: Jenkins <ci at libreoffice.org>
Reviewed-by: Michael Stahl <mstahl at redhat.com>
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();
}
};
More information about the Libreoffice-commits
mailing list