make comphelper::OInterfaceContainerHelper4 more threadsafe

(*) make all the methods that require an external mutex take a
std::unique_lock as a parameter, so that call sites cannot forget

(*) make the forEach method drop the lock when firing listener methods,
to reduce the odds of deadlock

Change-Id: I0a80e3b3d1c1c03b7de4a658d31fcc2847690903
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/128415
Tested-by: Jenkins
Reviewed-by: Noel Grandin <noel.grandin@collabora.co.uk>

5 files changed, 45 insertions, 66 deletions

diff --git a/sfx2/inc/unoctitm.hxx b/sfx2/inc/unoctitm.hxx
index 1a53091b2b74..a021c5379840 100644
--- a/sfx2/inc/unoctitm.hxx
+++ b/sfx2/inc/unoctitm.hxx
@@ -63,7 +63,7 @@ public:
     // Something else
     void                ReleaseAll();
     void                sendStatusChanged(const OUString& rURL, const css::frame::FeatureStateEvent& rEvent);
-    std::vector<OUString> getContainedTypes() { return maListeners.getContainedTypes(); };
+    std::vector<OUString> getContainedTypes() { std::unique_lock g(maMutex); return maListeners.getContainedTypes(g); };
 };
 
 class SfxSlotServer;
diff --git a/sfx2/source/control/unoctitm.cxx b/sfx2/source/control/unoctitm.cxx
index 5ae4e87f77e4..87d04d3524a8 100644
--- a/sfx2/source/control/unoctitm.cxx
+++ b/sfx2/source/control/unoctitm.cxx
@@ -134,19 +134,12 @@ void SfxStatusDispatcher::sendStatusChanged(const OUString& rURL, const css::fra
     ::comphelper::OInterfaceContainerHelper4<css::frame::XStatusListener>* pContnr = maListeners.getContainer(rURL);
     if (!pContnr)
         return;
-    ::comphelper::OInterfaceIteratorHelper4 aIt(*pContnr);
-    aGuard.unlock();
-    while (aIt.hasMoreElements())
-    {
-        try
-        {
-            aIt.next()->statusChanged(rEvent);
-        }
-        catch (const css::uno::RuntimeException&)
+    pContnr->forEach(aGuard,
+        [&rEvent](const css::uno::Reference<css::frame::XStatusListener>& xListener)
         {
-            aIt.remove();
+            xListener->statusChanged(rEvent);
         }
-    }
+    );
 }
 
 void SAL_CALL SfxStatusDispatcher::dispatch( const css::util::URL&, const css::uno::Sequence< css::beans::PropertyValue >& )
@@ -168,7 +161,7 @@ void SAL_CALL SfxStatusDispatcher::addStatusListener(const css::uno::Reference<
 {
     {
         std::unique_lock aGuard(maMutex);
-        maListeners.addInterface( aURL.Complete, aListener );
+        maListeners.addInterface( aGuard, aURL.Complete, aListener );
     }
     if ( aURL.Complete == ".uno:LifeTime" )
     {
@@ -184,7 +177,7 @@ void SAL_CALL SfxStatusDispatcher::addStatusListener(const css::uno::Reference<
 void SAL_CALL SfxStatusDispatcher::removeStatusListener( const css::uno::Reference< css::frame::XStatusListener > & aListener, const css::util::URL& aURL )
 {
     std::unique_lock aGuard(maMutex);
-    maListeners.removeInterface( aURL.Complete, aListener );
+    maListeners.removeInterface( aGuard, aURL.Complete, aListener );
 }
 
 
@@ -300,7 +293,7 @@ void SAL_CALL SfxOfficeDispatch::addStatusListener(const css::uno::Reference< cs
 {
     {
         std::unique_lock aGuard(maMutex);
-        maListeners.addInterface( aURL.Complete, aListener );
+        maListeners.addInterface( aGuard, aURL.Complete, aListener );
     }
     if ( pImpl )
     {
diff --git a/sfx2/source/doc/printhelper.cxx b/sfx2/source/doc/printhelper.cxx
index 5cbd8e258c72..0ccdb1b1f2b0 100644
--- a/sfx2/source/doc/printhelper.cxx
+++ b/sfx2/source/doc/printhelper.cxx
@@ -782,13 +782,13 @@ void IMPL_PrintListener_DataContainer::Notify( SfxBroadcaster& rBC, const SfxHin
     }
 
     std::unique_lock aGuard(m_aMutex);
-    if (!m_aJobListeners.getLength())
+    if (!m_aJobListeners.getLength(aGuard))
         return;
     view::PrintJobEvent aEvent;
     aEvent.Source = m_xPrintJob;
     aEvent.State = pPrintHint->GetWhich();
 
-    comphelper::OInterfaceIteratorHelper4 pIterator(m_aJobListeners);
+    comphelper::OInterfaceIteratorHelper4 pIterator(aGuard, m_aJobListeners);
     aGuard.unlock();
     while (pIterator.hasMoreElements())
         pIterator.next()->printJobEvent( aEvent );
@@ -797,13 +797,13 @@ void IMPL_PrintListener_DataContainer::Notify( SfxBroadcaster& rBC, const SfxHin
 void SAL_CALL SfxPrintHelper::addPrintJobListener( const css::uno::Reference< css::view::XPrintJobListener >& xListener )
 {
     std::unique_lock aGuard(m_pData->m_aMutex);
-    m_pData->m_aJobListeners.addInterface( xListener );
+    m_pData->m_aJobListeners.addInterface( aGuard, xListener );
 }
 
 void SAL_CALL SfxPrintHelper::removePrintJobListener( const css::uno::Reference< css::view::XPrintJobListener >& xListener )
 {
     std::unique_lock aGuard(m_pData->m_aMutex);
-    m_pData->m_aJobListeners.removeInterface( xListener );
+    m_pData->m_aJobListeners.removeInterface( aGuard, xListener );
 }
 
 
diff --git a/sfx2/source/notify/globalevents.cxx b/sfx2/source/notify/globalevents.cxx
index 0208a5f412a5..817e8a38448b 100644
--- a/sfx2/source/notify/globalevents.cxx
+++ b/sfx2/source/notify/globalevents.cxx
@@ -168,35 +168,35 @@ uno::Reference< container::XNameReplace > SAL_CALL SfxGlobalEvents_Impl::getEven
 
 void SAL_CALL SfxGlobalEvents_Impl::addEventListener(const uno::Reference< document::XEventListener >& xListener)
 {
-    std::scoped_lock g(m_aLock);
+    std::unique_lock g(m_aLock);
     if (m_disposed)
         throw css::lang::DisposedException();
-    m_aLegacyListeners.addInterface(xListener);
+    m_aLegacyListeners.addInterface(g, xListener);
 }
 
 
 void SAL_CALL SfxGlobalEvents_Impl::removeEventListener(const uno::Reference< document::XEventListener >& xListener)
 {
-    std::scoped_lock g(m_aLock);
+    std::unique_lock g(m_aLock);
     // The below removeInterface will silently do nothing when m_disposed
-    m_aLegacyListeners.removeInterface(xListener);
+    m_aLegacyListeners.removeInterface(g, xListener);
 }
 
 
 void SAL_CALL SfxGlobalEvents_Impl::addDocumentEventListener( const uno::Reference< document::XDocumentEventListener >& Listener )
 {
-    std::scoped_lock g(m_aLock);
+    std::unique_lock g(m_aLock);
     if (m_disposed)
         throw css::lang::DisposedException();
-    m_aDocumentListeners.addInterface( Listener );
+    m_aDocumentListeners.addInterface( g, Listener );
 }
 
 
 void SAL_CALL SfxGlobalEvents_Impl::removeDocumentEventListener( const uno::Reference< document::XDocumentEventListener >& Listener )
 {
-    std::scoped_lock g(m_aLock);
+    std::unique_lock g(m_aLock);
     // The below removeInterface will silently do nothing when m_disposed:
-    m_aDocumentListeners.removeInterface( Listener );
+    m_aDocumentListeners.removeInterface( g, Listener );
 }
 
 
@@ -478,43 +478,21 @@ void SfxGlobalEvents_Impl::implts_checkAndExecuteEventBindings(const document::D
 
 void SfxGlobalEvents_Impl::implts_notifyListener(const document::DocumentEvent& aEvent)
 {
-    std::optional<comphelper::OInterfaceIteratorHelper4<document::XEventListener>> aIt1;
-    std::optional<comphelper::OInterfaceIteratorHelper4<document::XDocumentEventListener>> aIt2;
-
-    {
-        std::scoped_lock g(m_aLock);
-        aIt1.emplace(m_aLegacyListeners);
-        aIt2.emplace(m_aDocumentListeners);
-    }
+    std::unique_lock g(m_aLock);
 
     document::EventObject aLegacyEvent(aEvent.Source, aEvent.EventName);
-    while (aIt1->hasMoreElements())
-    {
-        auto xListener = aIt1->next();
-        try
+    m_aLegacyListeners.forEach(g,
+        [&aLegacyEvent](const css::uno::Reference<document::XEventListener>& xListener)
         {
             xListener->notifyEvent(aLegacyEvent);
         }
-        catch (css::lang::DisposedException const& exc)
-        {
-            if (exc.Context == xListener)
-                aIt1->remove();
-        }
-    }
-
-    while (aIt2->hasMoreElements())
-    {
-        auto xListener = aIt2->next();
-        try
+    );
+    m_aDocumentListeners.forEach(g,
+        [&aEvent](const css::uno::Reference<document::XDocumentEventListener>& xListener)
         {
             xListener->documentEventOccured(aEvent);
         }
-        catch (css::lang::DisposedException const& exc)
-        {
-            if (exc.Context == xListener)
-                aIt2->remove();
-        }
-    }
+    );
 }
 
 
diff --git a/sfx2/source/view/viewsh.cxx b/sfx2/source/view/viewsh.cxx
index 8fdf33487a56..298f7a722ed2 100644
--- a/sfx2/source/view/viewsh.cxx
+++ b/sfx2/source/view/viewsh.cxx
@@ -1841,12 +1841,14 @@ SfxBaseController* SfxViewShell::GetBaseController_Impl() const
 
 void SfxViewShell::AddContextMenuInterceptor_Impl( const uno::Reference< ui::XContextMenuInterceptor >& xInterceptor )
 {
-    pImpl->aInterceptorContainer.addInterface( xInterceptor );
+    std::unique_lock g(pImpl->aMutex);
+    pImpl->aInterceptorContainer.addInterface( g, xInterceptor );
 }
 
 void SfxViewShell::RemoveContextMenuInterceptor_Impl( const uno::Reference< ui::XContextMenuInterceptor >& xInterceptor )
 {
-    pImpl->aInterceptorContainer.removeInterface( xInterceptor );
+    std::unique_lock g(pImpl->aMutex);
+    pImpl->aInterceptorContainer.removeInterface( g, xInterceptor );
 }
 
 bool SfxViewShell::TryContextMenuInterception(const css::uno::Reference<css::awt::XPopupMenu>& rIn,
@@ -1866,16 +1868,17 @@ bool SfxViewShell::TryContextMenuInterception(const css::uno::Reference<css::awt
 
     // call interceptors
     std::unique_lock g(pImpl->aMutex);
-    ::comphelper::OInterfaceIteratorHelper4 aIt( pImpl->aInterceptorContainer );
+    std::vector<uno::Reference< ui::XContextMenuInterceptor>> aInterceptors =
+        pImpl->aInterceptorContainer.getElements(g);
     g.unlock();
-    while( aIt.hasMoreElements() )
+    for (const auto & rListener : aInterceptors )
     {
         try
         {
             ui::ContextMenuInterceptorAction eAction;
             {
                 SolarMutexReleaser rel;
-                eAction = aIt.next()->notifyContextMenuExecute( aEvent );
+                eAction = rListener->notifyContextMenuExecute( aEvent );
             }
             switch ( eAction )
             {
@@ -1900,7 +1903,9 @@ bool SfxViewShell::TryContextMenuInterception(const css::uno::Reference<css::awt
         }
         catch (...)
         {
-            aIt.remove();
+            g.lock();
+            pImpl->aInterceptorContainer.removeInterface(g, rListener);
+            g.unlock();
         }
 
         break;
@@ -1931,16 +1936,17 @@ bool SfxViewShell::TryContextMenuInterception(const css::uno::Reference<css::awt
 
     // call interceptors
     std::unique_lock g(pImpl->aMutex);
-    ::comphelper::OInterfaceIteratorHelper4 aIt( pImpl->aInterceptorContainer );
+    std::vector<uno::Reference< ui::XContextMenuInterceptor>> aInterceptors =
+        pImpl->aInterceptorContainer.getElements(g);
     g.unlock();
-    while( aIt.hasMoreElements() )
+    for (const auto & rListener : aInterceptors )
     {
         try
         {
             css::ui::ContextMenuInterceptorAction eAction;
             {
                 SolarMutexReleaser rel;
-                eAction = aIt.next()->notifyContextMenuExecute( aEvent );
+                eAction = rListener->notifyContextMenuExecute( aEvent );
             }
             switch ( eAction )
             {
@@ -1965,7 +1971,9 @@ bool SfxViewShell::TryContextMenuInterception(const css::uno::Reference<css::awt
         }
         catch (...)
         {
-            aIt.remove();
+            g.lock();
+            pImpl->aInterceptorContainer.removeInterface(g, rListener);
+            g.unlock();
         }
 
         break;