From bc3de19411e0966bbcdc9e247b2af96bff4333b2 Mon Sep 17 00:00:00 2001 From: Michael Stahl Date: Wed, 27 Jul 2016 14:51:08 +0200 Subject: 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 Reviewed-by: Michael Stahl --- ucb/source/ucp/tdoc/tdoc_docmgr.cxx | 177 ++++++++++++++++++++---------------- ucb/source/ucp/tdoc/tdoc_docmgr.hxx | 4 +- 2 files changed, 101 insertions(+), 80 deletions(-) diff --git a/ucb/source/ucp/tdoc/tdoc_docmgr.cxx b/ucb/source/ucp/tdoc/tdoc_docmgr.cxx index f6efac77d46c..6994349dfdbe 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 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 const xDoc( + Event.Source, uno::UNO_QUERY ); + OSL_ENSURE( xDoc.is(), + "Got no document::XStorageBasedDocument!" ); + uno::Reference 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 const xDoc( + Event.Source, uno::UNO_QUERY ); + OSL_ENSURE( xDoc.is(), + "Got no document::XStorageBasedDocument!" ); + uno::Reference 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 const xDoc( + Event.Source, uno::UNO_QUERY ); + OSL_ENSURE( xDoc.is(), + "Got no document::XStorageBasedDocument!" ); + uno::Reference 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 a98411cf25ab..2568c2a8953e 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 m_xDocCloseListener; + ContentProvider * const m_pDocEventListener; + ::rtl::Reference const m_xDocCloseListener; }; } // namespace tdoc_ucp -- cgit v1.2.3