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>

3 files changed, 87 insertions, 41 deletions

diff --git a/include/comphelper/interfacecontainer4.hxx b/include/comphelper/interfacecontainer4.hxx
index e58b958629b9..b8e2079a9201 100644
--- a/include/comphelper/interfacecontainer4.hxx
+++ b/include/comphelper/interfacecontainer4.hxx
@@ -56,8 +56,11 @@ public:
        change the contents...
 
        @param rCont the container of the elements.
+       @param rGuard
+            this parameter only here to make that this container is accessed while locked
      */
-    OInterfaceIteratorHelper4(OInterfaceContainerHelper4<ListenerT>& rCont_)
+    OInterfaceIteratorHelper4(std::unique_lock<std::mutex>& /*rGuard*/,
+                              OInterfaceContainerHelper4<ListenerT>& rCont_)
         : rCont(rCont_)
         , maData(rCont.maData)
         , nRemain(maData->size())
@@ -75,8 +78,10 @@ public:
         from the underlying container. Calling this method before
         next() has been called or calling it twice with no next()
         in between is an error.
+        @param rGuard
+            this parameter only here to make that this container is accessed while locked
     */
-    void remove();
+    void remove(::std::unique_lock<::std::mutex>& rGuard);
 
 private:
     OInterfaceContainerHelper4<ListenerT>& rCont;
@@ -94,9 +99,10 @@ const css::uno::Reference<ListenerT>& OInterfaceIteratorHelper4<ListenerT>::next
     return (*maData)[nRemain];
 }
 
-template <class ListenerT> void OInterfaceIteratorHelper4<ListenerT>::remove()
+template <class ListenerT>
+void OInterfaceIteratorHelper4<ListenerT>::remove(::std::unique_lock<::std::mutex>& rGuard)
 {
-    rCont.removeInterface((*maData)[nRemain]);
+    rCont.removeInterface(rGuard, (*maData)[nRemain]);
 }
 
 /**
@@ -119,13 +125,18 @@ public:
     /**
       Return the number of Elements in the container. Only useful if you have acquired
       the mutex.
+      @param rGuard
+        this parameter only here to make that this container is accessed while locked
      */
-    sal_Int32 getLength() const;
+    sal_Int32 getLength(std::unique_lock<std::mutex>& rGuard) const;
 
     /**
       Return all interfaces added to this container.
+      @param rGuard
+          this parameter only here to make that this container is accessed while locked
      **/
-    std::vector<css::uno::Reference<ListenerT>> getElements() const;
+    std::vector<css::uno::Reference<ListenerT>>
+    getElements(std::unique_lock<std::mutex>& rGuard) const;
 
     /** Inserts an element into the container.  The position is not specified, thus it is not
         specified in which order events are fired.
@@ -140,18 +151,24 @@ public:
         @param rxIFace
                interface to be added; it is allowed to insert
                the same interface more than once
+        @param rGuard
+            this parameter only here to make that this container is accessed while locked
         @return
                 the new count of elements in the container
     */
-    sal_Int32 addInterface(const css::uno::Reference<ListenerT>& rxIFace);
+    sal_Int32 addInterface(std::unique_lock<std::mutex>& rGuard,
+                           const css::uno::Reference<ListenerT>& rxIFace);
     /** Removes an element from the container.  It uses interface equality to remove the interface.
 
         @param rxIFace
                interface to be removed
+        @param rGuard
+               this parameter only here to make that this container is accessed while locked
         @return
                 the new count of elements in the container
     */
-    sal_Int32 removeInterface(const css::uno::Reference<ListenerT>& rxIFace);
+    sal_Int32 removeInterface(std::unique_lock<std::mutex>& rGuard,
+                              const css::uno::Reference<ListenerT>& rxIFace);
     /**
       Call disposing on all object in the container that
       support XEventListener. Then clear the container.
@@ -161,8 +178,10 @@ public:
                          const css::lang::EventObject& rEvt);
     /**
       Clears the container without calling disposing().
+        @param rGuard
+            this parameter only here to make that this container is accessed while locked
      */
-    void clear();
+    void clear(::std::unique_lock<::std::mutex>& rGuard);
 
     /** Executes a functor for each contained listener of specified type, e.g.
         <code>forEach<awt::XPaintListener>(...</code>.
@@ -173,8 +192,11 @@ public:
         @tparam FuncT unary functor type, let your compiler deduce this for you
         @param func unary functor object expecting an argument of type
                     css::uno::Reference<ListenerT>
+        @param rGuard
+            this parameter only here to make that this container is accessed while locked
     */
-    template <typename FuncT> inline void forEach(FuncT const& func);
+    template <typename FuncT>
+    inline void forEach(std::unique_lock<std::mutex>& rGuard, FuncT const& func);
 
     /** Calls a UNO listener method for each contained listener.
 
@@ -189,6 +211,8 @@ public:
             Pointer to a method of a ListenerT interface.
         @param Event
             Event to notify to all contained listeners
+        @param rGuard
+            this parameter only here to make that this container is accessed while locked
 
         Example:
 @code
@@ -197,7 +221,8 @@ public:
 @endcode
     */
     template <typename EventT>
-    inline void notifyEach(void (SAL_CALL ListenerT::*NotificationMethod)(const EventT&),
+    inline void notifyEach(std::unique_lock<std::mutex>& rGuard,
+                           void (SAL_CALL ListenerT::*NotificationMethod)(const EventT&),
                            const EventT& Event);
 
 private:
@@ -242,9 +267,14 @@ inline OInterfaceContainerHelper4<T>::OInterfaceContainerHelper4()
 
 template <class T>
 template <typename FuncT>
-inline void OInterfaceContainerHelper4<T>::forEach(FuncT const& func)
+inline void OInterfaceContainerHelper4<T>::forEach(std::unique_lock<std::mutex>& rGuard,
+                                                   FuncT const& func)
 {
-    OInterfaceIteratorHelper4<T> iter(*this);
+    if (std::as_const(maData)->size() == 0)
+        return;
+    maData.make_unique(); // so we can iterate over the data without holding the lock
+    OInterfaceIteratorHelper4<T> iter(rGuard, *this);
+    rGuard.unlock();
     while (iter.hasMoreElements())
     {
         auto xListener = iter.next();
@@ -255,34 +285,44 @@ inline void OInterfaceContainerHelper4<T>::forEach(FuncT const& func)
         catch (css::lang::DisposedException const& exc)
         {
             if (exc.Context == xListener)
-                iter.remove();
+            {
+                rGuard.lock();
+                iter.remove(rGuard);
+                rGuard.unlock();
+            }
         }
     }
+    rGuard.lock();
 }
 
 template <class ListenerT>
 template <typename EventT>
 inline void OInterfaceContainerHelper4<ListenerT>::notifyEach(
+    std::unique_lock<std::mutex>& rGuard,
     void (SAL_CALL ListenerT::*NotificationMethod)(const EventT&), const EventT& Event)
 {
-    forEach<NotifySingleListener<EventT>>(NotifySingleListener<EventT>(NotificationMethod, Event));
+    forEach<NotifySingleListener<EventT>>(rGuard,
+                                          NotifySingleListener<EventT>(NotificationMethod, Event));
 }
 
-template <class ListenerT> sal_Int32 OInterfaceContainerHelper4<ListenerT>::getLength() const
+template <class ListenerT>
+sal_Int32
+OInterfaceContainerHelper4<ListenerT>::getLength(std::unique_lock<std::mutex>& /*rGuard*/) const
 {
     return maData->size();
 }
 
 template <class ListenerT>
 std::vector<css::uno::Reference<ListenerT>>
-OInterfaceContainerHelper4<ListenerT>::getElements() const
+OInterfaceContainerHelper4<ListenerT>::getElements(std::unique_lock<std::mutex>& /*rGuard*/) const
 {
     return *maData;
 }
 
 template <class ListenerT>
 sal_Int32
-OInterfaceContainerHelper4<ListenerT>::addInterface(const css::uno::Reference<ListenerT>& rListener)
+OInterfaceContainerHelper4<ListenerT>::addInterface(std::unique_lock<std::mutex>& /*rGuard*/,
+                                                    const css::uno::Reference<ListenerT>& rListener)
 {
     assert(rListener.is());
     maData->push_back(rListener);
@@ -291,7 +331,7 @@ OInterfaceContainerHelper4<ListenerT>::addInterface(const css::uno::Reference<Li
 
 template <class ListenerT>
 sal_Int32 OInterfaceContainerHelper4<ListenerT>::removeInterface(
-    const css::uno::Reference<ListenerT>& rListener)
+    std::unique_lock<std::mutex>& /*rGuard*/, const css::uno::Reference<ListenerT>& rListener)
 {
     assert(rListener.is());
 
@@ -315,9 +355,10 @@ template <class ListenerT>
 void OInterfaceContainerHelper4<ListenerT>::disposeAndClear(std::unique_lock<std::mutex>& rGuard,
                                                             const css::lang::EventObject& rEvt)
 {
-    OInterfaceIteratorHelper4<ListenerT> aIt(*this);
+    OInterfaceIteratorHelper4<ListenerT> aIt(rGuard, *this);
     maData->clear();
     rGuard.unlock();
+    // unlock followed by iterating is only safe because we are not going to call remove() on the iterator
     while (aIt.hasMoreElements())
     {
         try
@@ -332,7 +373,11 @@ void OInterfaceContainerHelper4<ListenerT>::disposeAndClear(std::unique_lock<std
     }
 }
 
-template <class ListenerT> void OInterfaceContainerHelper4<ListenerT>::clear() { maData->clear(); }
+template <class ListenerT>
+void OInterfaceContainerHelper4<ListenerT>::clear(::std::unique_lock<::std::mutex>& /*rGuard*/)
+{
+    maData->clear();
+}
 }
 
 /* vim:set shiftwidth=4 softtabstop=4 expandtab: */
diff --git a/include/comphelper/multiinterfacecontainer4.hxx b/include/comphelper/multiinterfacecontainer4.hxx
index 5d2457669f0d..eeccd20920ca 100644
--- a/include/comphelper/multiinterfacecontainer4.hxx
+++ b/include/comphelper/multiinterfacecontainer4.hxx
@@ -43,13 +43,13 @@ public:
     /**
       Return all id's under which at least one interface is added.
      */
-    inline std::vector<key> getContainedTypes() const
+    inline std::vector<key> getContainedTypes(std::unique_lock<std::mutex>& rGuard) const
     {
         std::vector<key> aInterfaceTypes;
         aInterfaceTypes.reserve(m_aMap.size());
         for (const auto& rPair : m_aMap)
             // are interfaces added to this container?
-            if (rPair.second->getLength())
+            if (rPair.second->getLength(rGuard))
                 // yes, put the type in the array
                 aInterfaceTypes.push_back(rPair.first);
         return aInterfaceTypes;
@@ -91,17 +91,18 @@ public:
         @return
                 the new count of elements in the container
     */
-    inline sal_Int32 addInterface(const key& rKey, const css::uno::Reference<listener>& rListener)
+    inline sal_Int32 addInterface(::std::unique_lock<::std::mutex>& rGuard, const key& rKey,
+                                  const css::uno::Reference<listener>& rListener)
     {
         auto iter = find(rKey);
         if (iter == m_aMap.end())
         {
             auto pLC = new OInterfaceContainerHelper4<listener>();
             m_aMap.emplace_back(rKey, pLC);
-            return pLC->addInterface(rListener);
+            return pLC->addInterface(rGuard, rListener);
         }
         else
-            return (*iter).second->addInterface(rListener);
+            return (*iter).second->addInterface(rGuard, rListener);
     }
     /** Removes an element from the container with the specified key.
         It uses interface equality to remove the interface.
@@ -112,14 +113,14 @@ public:
         @return
                 the new count of elements in the container
     */
-    inline sal_Int32 removeInterface(const key& rKey,
+    inline sal_Int32 removeInterface(::std::unique_lock<::std::mutex>& rGuard, const key& rKey,
                                      const css::uno::Reference<listener>& rListener)
     {
         // search container with id nUik
         auto iter = find(rKey);
         // container found?
         if (iter != m_aMap.end())
-            return (*iter).second->removeInterface(rListener);
+            return (*iter).second->removeInterface(rGuard, rListener);
         // no container with this id. Always return 0
         return 0;
     }
@@ -139,7 +140,7 @@ public:
         rGuard.unlock();
         for (auto& rPair : tempMap)
         {
-            OInterfaceIteratorHelper4<listener> aIt(*rPair.second);
+            OInterfaceIteratorHelper4<listener> aIt(rGuard, *rPair.second);
             while (aIt.hasMoreElements())
             {
                 try
diff --git a/include/vcl/weldutils.hxx b/include/vcl/weldutils.hxx
index a8c2e1eac11d..07fc5eb2d684 100644
--- a/include/vcl/weldutils.hxx
+++ b/include/vcl/weldutils.hxx
@@ -89,84 +89,84 @@ public:
     addWindowListener(const css::uno::Reference<css::awt::XWindowListener>& rListener) override
     {
         std::unique_lock g(m_aMutex);
-        m_aWindowListeners.addInterface(rListener);
+        m_aWindowListeners.addInterface(g, rListener);
     }
 
     void SAL_CALL
     removeWindowListener(const css::uno::Reference<css::awt::XWindowListener>& rListener) override
     {
         std::unique_lock g(m_aMutex);
-        m_aWindowListeners.removeInterface(rListener);
+        m_aWindowListeners.removeInterface(g, rListener);
     }
 
     void SAL_CALL
     addFocusListener(const css::uno::Reference<css::awt::XFocusListener>& rListener) override
     {
         std::unique_lock g(m_aMutex);
-        m_aFocusListeners.addInterface(rListener);
+        m_aFocusListeners.addInterface(g, rListener);
     }
 
     void SAL_CALL
     removeFocusListener(const css::uno::Reference<css::awt::XFocusListener>& rListener) override
     {
         std::unique_lock g(m_aMutex);
-        m_aFocusListeners.removeInterface(rListener);
+        m_aFocusListeners.removeInterface(g, rListener);
     }
 
     void SAL_CALL
     addKeyListener(const css::uno::Reference<css::awt::XKeyListener>& rListener) override
     {
         std::unique_lock g(m_aMutex);
-        m_aKeyListeners.addInterface(rListener);
+        m_aKeyListeners.addInterface(g, rListener);
     }
 
     void SAL_CALL
     removeKeyListener(const css::uno::Reference<css::awt::XKeyListener>& rListener) override
     {
         std::unique_lock g(m_aMutex);
-        m_aKeyListeners.removeInterface(rListener);
+        m_aKeyListeners.removeInterface(g, rListener);
     }
 
     void SAL_CALL
     addMouseListener(const css::uno::Reference<css::awt::XMouseListener>& rListener) override
     {
         std::unique_lock g(m_aMutex);
-        m_aMouseListeners.addInterface(rListener);
+        m_aMouseListeners.addInterface(g, rListener);
     }
 
     void SAL_CALL
     removeMouseListener(const css::uno::Reference<css::awt::XMouseListener>& rListener) override
     {
         std::unique_lock g(m_aMutex);
-        m_aMouseListeners.removeInterface(rListener);
+        m_aMouseListeners.removeInterface(g, rListener);
     }
 
     void SAL_CALL addMouseMotionListener(
         const css::uno::Reference<css::awt::XMouseMotionListener>& rListener) override
     {
         std::unique_lock g(m_aMutex);
-        m_aMotionListeners.addInterface(rListener);
+        m_aMotionListeners.addInterface(g, rListener);
     }
 
     void SAL_CALL removeMouseMotionListener(
         const css::uno::Reference<css::awt::XMouseMotionListener>& rListener) override
     {
         std::unique_lock g(m_aMutex);
-        m_aMotionListeners.removeInterface(rListener);
+        m_aMotionListeners.removeInterface(g, rListener);
     }
 
     void SAL_CALL
     addPaintListener(const css::uno::Reference<css::awt::XPaintListener>& rListener) override
     {
         std::unique_lock g(m_aMutex);
-        m_aPaintListeners.addInterface(rListener);
+        m_aPaintListeners.addInterface(g, rListener);
     }
 
     void SAL_CALL
     removePaintListener(const css::uno::Reference<css::awt::XPaintListener>& rListener) override
     {
         std::unique_lock g(m_aMutex);
-        m_aPaintListeners.removeInterface(rListener);
+        m_aPaintListeners.removeInterface(g, rListener);
     }
 };