[Libreoffice-commits] core.git: ucb/source
Michael Stahl
mstahl at redhat.com
Thu Jul 28 10:23:39 UTC 2016
ucb/source/ucp/tdoc/tdoc_docmgr.cxx | 177 ++++++++++++++++++++----------------
ucb/source/ucp/tdoc/tdoc_docmgr.hxx | 4
2 files changed, 101 insertions(+), 80 deletions(-)
New commits:
commit bc3de19411e0966bbcdc9e247b2af96bff4333b2
Author: Michael Stahl <mstahl at redhat.com>
Date: Wed Jul 27 14:51:08 2016 +0200
ucb: fix deadlocks in tdoc UCP OfficeDocumentsManager
Reduce scope of MutexGuards so that UNO methods are called without the
mutex locked.
This avoids deadlocks in CppunitTest_sw_mailmerge after using SolarMutex
in dbaccess' ModelMethodGuard.
Change-Id: I0229ebc2983c85e2003d51053a6bd130240274c7
Reviewed-on: https://gerrit.libreoffice.org/27582
Tested-by: Jenkins <ci at libreoffice.org>
Reviewed-by: Michael Stahl <mstahl at redhat.com>
diff --git a/ucb/source/ucp/tdoc/tdoc_docmgr.cxx b/ucb/source/ucp/tdoc/tdoc_docmgr.cxx
index f6efac7..6994349 100644
--- a/ucb/source/ucp/tdoc/tdoc_docmgr.cxx
+++ b/ucb/source/ucp/tdoc/tdoc_docmgr.cxx
@@ -183,25 +183,33 @@ void SAL_CALL OfficeDocumentsManager::documentEventOccured(
{
if ( isOfficeDocument( Event.Source ) )
{
- osl::MutexGuard aGuard( m_aMtx );
-
- uno::Reference< frame::XModel >
- xModel( Event.Source, uno::UNO_QUERY );
+ uno::Reference<frame::XModel> const xModel(
+ Event.Source, uno::UNO_QUERY );
OSL_ENSURE( xModel.is(), "Got no frame::XModel!" );
- DocumentList::const_iterator it = m_aDocs.begin();
- while ( it != m_aDocs.end() )
+ bool found(false);
+
{
- if ( (*it).second.xModel == xModel )
+ osl::MutexGuard aGuard( m_aMtx );
+
+ DocumentList::const_iterator it = m_aDocs.begin();
+ while ( it != m_aDocs.end() )
{
- // already known.
- break;
+ if ( (*it).second.xModel == xModel )
+ {
+ // already known.
+ found = true;
+ break;
+ }
+ ++it;
}
- ++it;
}
- if ( it == m_aDocs.end() )
+ if (!found)
{
+ // no mutex to avoid deadlocks!
+ // need no lock to access const members, ContentProvider is safe
+
// new document
uno::Reference< document::XStorageBasedDocument >
@@ -216,7 +224,10 @@ void SAL_CALL OfficeDocumentsManager::documentEventOccured(
rtl:: OUString aTitle = comphelper::DocumentInfo::getDocumentTitle(
uno::Reference< frame::XModel >( Event.Source, uno::UNO_QUERY ) );
- m_aDocs[ aDocId ] = StorageInfo( aTitle, xStorage, xModel );
+ {
+ osl::MutexGuard g(m_aMtx);
+ m_aDocs[ aDocId ] = StorageInfo( aTitle, xStorage, xModel );
+ }
uno::Reference< util::XCloseBroadcaster > xCloseBroadcaster(
Event.Source, uno::UNO_QUERY );
@@ -247,45 +258,46 @@ void SAL_CALL OfficeDocumentsManager::documentEventOccured(
// document from TDOC docs list on XCloseListener::notifyClosing.
// See OfficeDocumentsManager::OfficeDocumentsListener::notifyClosing.
- osl::MutexGuard aGuard( m_aMtx );
-
uno::Reference< frame::XModel >
xModel( Event.Source, uno::UNO_QUERY );
OSL_ENSURE( xModel.is(), "Got no frame::XModel!" );
- DocumentList::iterator it = m_aDocs.begin();
- while ( it != m_aDocs.end() )
+ bool found(false);
+ OUString aDocId;
+
{
- if ( (*it).second.xModel == xModel )
- {
- // Propagate document closure.
- OSL_ENSURE( m_pDocEventListener,
- "OnUnload event: no owner for close event propagation!" );
+ osl::MutexGuard aGuard( m_aMtx );
- if ( m_pDocEventListener )
+ for (auto it = m_aDocs.begin(); it != m_aDocs.end(); ++it)
+ {
+ if ( (*it).second.xModel == xModel )
{
- OUString aDocId( (*it).first );
- m_pDocEventListener->notifyDocumentClosed( aDocId );
+ aDocId = (*it).first;
+ found = true;
+ m_aDocs.erase( it );
+ break;
}
- break;
}
- ++it;
}
- OSL_ENSURE( it != m_aDocs.end(),
+ OSL_ENSURE( found,
"OnUnload event notified for unknown document!" );
- if ( it != m_aDocs.end() )
+ if (found)
{
+ // Propagate document closure.
+ OSL_ENSURE( m_pDocEventListener,
+ "OnUnload event: no owner for close event propagation!" );
+ if (m_pDocEventListener)
+ {
+ m_pDocEventListener->notifyDocumentClosed(aDocId);
+ }
uno::Reference< util::XCloseBroadcaster > xCloseBroadcaster(
Event.Source, uno::UNO_QUERY );
OSL_ENSURE( xCloseBroadcaster.is(),
"OnUnload event: got no XCloseBroadcaster from XModel" );
-
if ( xCloseBroadcaster.is() )
xCloseBroadcaster->removeCloseListener(m_xDocCloseListener.get());
-
- m_aDocs.erase( it );
}
}
}
@@ -293,27 +305,26 @@ void SAL_CALL OfficeDocumentsManager::documentEventOccured(
{
if ( isOfficeDocument( Event.Source ) )
{
- osl::MutexGuard aGuard( m_aMtx );
+ // Storage gets exchanged while saving.
+ uno::Reference<document::XStorageBasedDocument> const xDoc(
+ Event.Source, uno::UNO_QUERY );
+ OSL_ENSURE( xDoc.is(),
+ "Got no document::XStorageBasedDocument!" );
+ uno::Reference<embed::XStorage> const xStorage(
+ xDoc->getDocumentStorage());
+ OSL_ENSURE( xStorage.is(), "Got no document storage!" );
uno::Reference< frame::XModel >
xModel( Event.Source, uno::UNO_QUERY );
OSL_ENSURE( xModel.is(), "Got no frame::XModel!" );
+ osl::MutexGuard aGuard( m_aMtx );
+
DocumentList::iterator it = m_aDocs.begin();
while ( it != m_aDocs.end() )
{
if ( (*it).second.xModel == xModel )
{
- // Storage gets exchanged while saving.
- uno::Reference< document::XStorageBasedDocument >
- xDoc( Event.Source, uno::UNO_QUERY );
- OSL_ENSURE( xDoc.is(),
- "Got no document::XStorageBasedDocument!" );
-
- uno::Reference< embed::XStorage > xStorage
- = xDoc->getDocumentStorage();
- OSL_ENSURE( xStorage.is(), "Got no document storage!" );
-
(*it).second.xStorage = xStorage;
break;
}
@@ -328,31 +339,32 @@ void SAL_CALL OfficeDocumentsManager::documentEventOccured(
{
if ( isOfficeDocument( Event.Source ) )
{
- osl::MutexGuard aGuard( m_aMtx );
+ // Storage gets exchanged while saving.
+ uno::Reference<document::XStorageBasedDocument> const xDoc(
+ Event.Source, uno::UNO_QUERY );
+ OSL_ENSURE( xDoc.is(),
+ "Got no document::XStorageBasedDocument!" );
+ uno::Reference<embed::XStorage> const xStorage(
+ xDoc->getDocumentStorage());
+ OSL_ENSURE( xStorage.is(), "Got no document storage!" );
uno::Reference< frame::XModel >
xModel( Event.Source, uno::UNO_QUERY );
OSL_ENSURE( xModel.is(), "Got no frame::XModel!" );
+ OUString const title(comphelper::DocumentInfo::getDocumentTitle(xModel));
+
+ osl::MutexGuard aGuard( m_aMtx );
+
DocumentList::iterator it = m_aDocs.begin();
while ( it != m_aDocs.end() )
{
if ( (*it).second.xModel == xModel )
{
- // Storage gets exchanged while saving.
- uno::Reference< document::XStorageBasedDocument >
- xDoc( Event.Source, uno::UNO_QUERY );
- OSL_ENSURE( xDoc.is(),
- "Got no document::XStorageBasedDocument!" );
-
- uno::Reference< embed::XStorage > xStorage
- = xDoc->getDocumentStorage();
- OSL_ENSURE( xStorage.is(), "Got no document storage!" );
-
(*it).second.xStorage = xStorage;
// Adjust title.
- (*it).second.aTitle = comphelper::DocumentInfo::getDocumentTitle( xModel );
+ (*it).second.aTitle = title;
break;
}
++it;
@@ -367,32 +379,33 @@ void SAL_CALL OfficeDocumentsManager::documentEventOccured(
{
if ( isOfficeDocument( Event.Source ) )
{
- osl::MutexGuard aGuard( m_aMtx );
+ // Storage gets exchanged while saving.
+ uno::Reference<document::XStorageBasedDocument> const xDoc(
+ Event.Source, uno::UNO_QUERY );
+ OSL_ENSURE( xDoc.is(),
+ "Got no document::XStorageBasedDocument!" );
+ uno::Reference<embed::XStorage> const xStorage(
+ xDoc->getDocumentStorage());
+ OSL_ENSURE( xStorage.is(), "Got no document storage!" );
uno::Reference< frame::XModel >
xModel( Event.Source, uno::UNO_QUERY );
OSL_ENSURE( xModel.is(), "Got no frame::XModel!" );
+ OUString const aTitle(comphelper::DocumentInfo::getDocumentTitle(xModel));
+
+ OUString const aDocId(getDocumentId(Event.Source));
+
+ osl::MutexGuard aGuard( m_aMtx );
+
DocumentList::iterator it = m_aDocs.begin();
while ( it != m_aDocs.end() )
{
if ( (*it).second.xModel == xModel )
{
// Adjust title.
- rtl:: OUString aTitle = comphelper::DocumentInfo::getDocumentTitle( xModel );
(*it).second.aTitle = aTitle;
- // Adjust storage.
- uno::Reference< document::XStorageBasedDocument >
- xDoc( Event.Source, uno::UNO_QUERY );
- OSL_ENSURE( xDoc.is(), "Got no document::XStorageBasedDocument!" );
-
- uno::Reference< embed::XStorage > xStorage
- = xDoc->getDocumentStorage();
- OSL_ENSURE( xDoc.is(), "Got no document storage!" );
-
- rtl:: OUString aDocId = getDocumentId( Event.Source );
-
m_aDocs[ aDocId ] = StorageInfo( aTitle, xStorage, xModel );
break;
}
@@ -431,8 +444,6 @@ void OfficeDocumentsManager::buildDocumentsList()
uno::Reference< container::XEnumeration > xEnum
= m_xDocEvtNotifier->createEnumeration();
- osl::MutexGuard aGuard( m_aMtx );
-
while ( xEnum->hasMoreElements() )
{
uno::Any aValue = xEnum->nextElement();
@@ -448,18 +459,25 @@ void OfficeDocumentsManager::buildDocumentsList()
{
if ( isOfficeDocument( xModel ) )
{
- DocumentList::const_iterator it = m_aDocs.begin();
- while ( it != m_aDocs.end() )
+ bool found(false);
+
{
- if ( (*it).second.xModel == xModel )
+ osl::MutexGuard aGuard( m_aMtx );
+
+ DocumentList::const_iterator it = m_aDocs.begin();
+ while ( it != m_aDocs.end() )
{
- // already known.
- break;
+ if ( (*it).second.xModel == xModel )
+ {
+ // already known.
+ found = true;
+ break;
+ }
+ ++it;
}
- ++it;
}
- if ( it == m_aDocs.end() )
+ if (!found)
{
// new document
OUString aDocId = getDocumentId( xModel );
@@ -474,8 +492,11 @@ void OfficeDocumentsManager::buildDocumentsList()
= xDoc->getDocumentStorage();
OSL_ENSURE( xDoc.is(), "Got no document storage!" );
- m_aDocs[ aDocId ]
- = StorageInfo( aTitle, xStorage, xModel );
+ {
+ osl::MutexGuard aGuard( m_aMtx );
+ m_aDocs[ aDocId ]
+ = StorageInfo( aTitle, xStorage, xModel );
+ }
uno::Reference< util::XCloseBroadcaster > xCloseBroadcaster(
xModel, uno::UNO_QUERY );
diff --git a/ucb/source/ucp/tdoc/tdoc_docmgr.hxx b/ucb/source/ucp/tdoc/tdoc_docmgr.hxx
index a98411c..2568c2a 100644
--- a/ucb/source/ucp/tdoc/tdoc_docmgr.hxx
+++ b/ucb/source/ucp/tdoc/tdoc_docmgr.hxx
@@ -157,8 +157,8 @@ namespace tdoc_ucp {
css::uno::Reference< css::frame::XGlobalEventBroadcaster > m_xDocEvtNotifier;
css::uno::Reference< css::frame::XModuleManager2 > m_xModuleMgr;
DocumentList m_aDocs;
- ContentProvider * m_pDocEventListener;
- ::rtl::Reference<OfficeDocumentsCloseListener> m_xDocCloseListener;
+ ContentProvider * const m_pDocEventListener;
+ ::rtl::Reference<OfficeDocumentsCloseListener> const m_xDocCloseListener;
};
} // namespace tdoc_ucp
More information about the Libreoffice-commits
mailing list