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@libreoffice.org>
Reviewed-by: Michael Stahl <mstahl@redhat.com>

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<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 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<OfficeDocumentsCloseListener> m_xDocCloseListener;
+        ContentProvider * const                             m_pDocEventListener;
+        ::rtl::Reference<OfficeDocumentsCloseListener> const m_xDocCloseListener;
     };
 
 } // namespace tdoc_ucp