include/comphelper/interfacecontainer4.hxx | 25 ++++++++++++++++++------ include/comphelper/multiinterfacecontainer4.hxx | 22 ++++++++++++++++++--- 2 files changed, 38 insertions(+), 9 deletions(-)
New commits: commit 414172d7fafc2d2c6b1e698aeb67fdb5797b118d Author: Noel Grandin <[email protected]> AuthorDate: Sun Feb 19 15:28:06 2023 +0200 Commit: Noel Grandin <[email protected]> CommitDate: Sun Feb 19 18:28:57 2023 +0000 fix locking in OMultiTypeInterfaceContainerHelperVar4::diposeAndCloear where I forgot to relock at the end of the method And sprinkle some asserts around to catch mistakes like this in future Change-Id: I4908ac0ffdfe33b1b5cf1b02e6765f20973af841 Reviewed-on: https://gerrit.libreoffice.org/c/core/+/147296 Tested-by: Jenkins Reviewed-by: Noel Grandin <[email protected]> diff --git a/include/comphelper/interfacecontainer4.hxx b/include/comphelper/interfacecontainer4.hxx index 3a6696fda8c5..bfa3a3059212 100644 --- a/include/comphelper/interfacecontainer4.hxx +++ b/include/comphelper/interfacecontainer4.hxx @@ -60,13 +60,15 @@ public: @param rGuard this parameter only here to make that this container is accessed while locked */ - OInterfaceIteratorHelper4(std::unique_lock<std::mutex>& /*rGuard*/, + OInterfaceIteratorHelper4(std::unique_lock<std::mutex>& rGuard, OInterfaceContainerHelper4<ListenerT>& rCont_) : rCont(rCont_) , maData(rCont.maData) // const_cast so we don't trigger make_unique via o3tl::cow_wrapper::operator-> , nRemain(std::as_const(maData)->size()) { + assert(rGuard.owns_lock()); + (void)rGuard; } /** Return true, if there are more elements in the iterator. */ @@ -281,6 +283,7 @@ template <typename FuncT> inline void OInterfaceContainerHelper4<T>::forEach(std::unique_lock<std::mutex>& rGuard, FuncT const& func) const { + assert(rGuard.owns_lock()); if (std::as_const(maData)->size() == 0) { return; @@ -321,23 +324,29 @@ inline void OInterfaceContainerHelper4<ListenerT>::notifyEach( template <class ListenerT> sal_Int32 -OInterfaceContainerHelper4<ListenerT>::getLength(std::unique_lock<std::mutex>& /*rGuard*/) const +OInterfaceContainerHelper4<ListenerT>::getLength(std::unique_lock<std::mutex>& rGuard) const { + assert(rGuard.owns_lock()); + (void)rGuard; return maData->size(); } template <class ListenerT> std::vector<css::uno::Reference<ListenerT>> -OInterfaceContainerHelper4<ListenerT>::getElements(std::unique_lock<std::mutex>& /*rGuard*/) const +OInterfaceContainerHelper4<ListenerT>::getElements(std::unique_lock<std::mutex>& rGuard) const { + assert(rGuard.owns_lock()); + (void)rGuard; return *maData; } template <class ListenerT> sal_Int32 -OInterfaceContainerHelper4<ListenerT>::addInterface(std::unique_lock<std::mutex>& /*rGuard*/, +OInterfaceContainerHelper4<ListenerT>::addInterface(std::unique_lock<std::mutex>& rGuard, const css::uno::Reference<ListenerT>& rListener) { + assert(rGuard.owns_lock()); + (void)rGuard; assert(rListener.is()); maData->push_back(rListener); return maData->size(); @@ -345,8 +354,10 @@ OInterfaceContainerHelper4<ListenerT>::addInterface(std::unique_lock<std::mutex> template <class ListenerT> sal_Int32 OInterfaceContainerHelper4<ListenerT>::removeInterface( - std::unique_lock<std::mutex>& /*rGuard*/, const css::uno::Reference<ListenerT>& rListener) + std::unique_lock<std::mutex>& rGuard, const css::uno::Reference<ListenerT>& rListener) { + assert(rGuard.owns_lock()); + (void)rGuard; assert(rListener.is()); // It is not valid to compare the pointer directly, but it's faster. @@ -395,8 +406,10 @@ void OInterfaceContainerHelper4<ListenerT>::disposeAndClear(std::unique_lock<std } template <class ListenerT> -void OInterfaceContainerHelper4<ListenerT>::clear(::std::unique_lock<::std::mutex>& /*rGuard*/) +void OInterfaceContainerHelper4<ListenerT>::clear(::std::unique_lock<::std::mutex>& rGuard) { + assert(rGuard.owns_lock()); + (void)rGuard; maData->clear(); } } diff --git a/include/comphelper/multiinterfacecontainer4.hxx b/include/comphelper/multiinterfacecontainer4.hxx index 1241951f5505..2212d638410d 100644 --- a/include/comphelper/multiinterfacecontainer4.hxx +++ b/include/comphelper/multiinterfacecontainer4.hxx @@ -45,6 +45,7 @@ public: */ inline std::vector<key> getContainedTypes(std::unique_lock<std::mutex>& rGuard) const { + assert(rGuard.owns_lock()); std::vector<key> aInterfaceTypes; aInterfaceTypes.reserve(m_aMap.size()); for (const auto& rPair : m_aMap) @@ -56,6 +57,7 @@ public: } inline bool hasContainedTypes(std::unique_lock<std::mutex>& rGuard) const { + assert(rGuard.owns_lock()); for (const auto& rPair : m_aMap) // are interfaces added to this container? if (rPair.second->getLength(rGuard)) @@ -133,15 +135,24 @@ public: inline void disposeAndClear(std::unique_lock<std::mutex>& rGuard, const css::lang::EventObject& rEvt) { + assert(rGuard.owns_lock()); // create a copy, because do not fire event in a guarded section InterfaceMap tempMap; { tempMap = std::move(m_aMap); } rGuard.unlock(); + // So... we don't want to hold the normal mutex while we fire + // the events, but the calling convention here wants a mutex, so + // just create a temporary/fake one. Since the listeners we + // are working with are now function-local, we don't really need + // a mutex at all, but it's easier to create a fake one than + // create a bunch of special-case code for this situation. + std::mutex tempMutex; + std::unique_lock tempGuard(tempMutex); for (auto& rPair : tempMap) { - OInterfaceIteratorHelper4<listener> aIt(rGuard, *rPair.second); + OInterfaceIteratorHelper4<listener> aIt(tempGuard, *rPair.second); while (aIt.hasMoreElements()) { try @@ -155,12 +166,15 @@ public: } } } + rGuard.lock(); // return with lock in same state as entry } /** Remove all elements of all containers. Does not delete the container. */ - inline void clear(std::unique_lock<std::mutex>& /*rGuard*/) + inline void clear(std::unique_lock<std::mutex>& rGuard) { + assert(rGuard.owns_lock()); + (void)rGuard; for (const auto& rPair : m_aMap) rPair.second->clear(); } @@ -170,9 +184,11 @@ private: typedef ::std::vector<std::pair<key, std::unique_ptr<OInterfaceContainerHelper4<listener>>>> InterfaceMap; InterfaceMap m_aMap; - typename InterfaceMap::const_iterator find(std::unique_lock<std::mutex>& /*rGuard*/, + typename InterfaceMap::const_iterator find(std::unique_lock<std::mutex>& rGuard, const key& rKey) const { + assert(rGuard.owns_lock()); + (void)rGuard; auto iter = m_aMap.begin(); auto end = m_aMap.end(); while (iter != end)
