cppuhelper: WeakReference isn't thread-safe

... but its documentation claims that it is, which is partially
misleading, so fix both the documentation and the data race in
WeakReferenceHelper::clear().

This actually crashed in clear() in the multi-threaded ZipPackage code
on exporting the bugdoc from tdf#94212, presumably because clear()
races against OWeakRefListener::dispose().

Change-Id: I85665c11b8157e90d15e8263758e24e66efeb86c
(cherry picked from commit debe788bcf3ec258b6b95df3db1f7bfeba881be1)
Reviewed-on: https://gerrit.libreoffice.org/26424
Reviewed-by: Stephan Bergmann <sbergman@redhat.com>
Tested-by: Stephan Bergmann <sbergman@redhat.com>

2 files changed, 20 insertions, 11 deletions

diff --git a/cppuhelper/source/weak.cxx b/cppuhelper/source/weak.cxx
index b6abef5ebe56..ed1f77208249 100644
--- a/cppuhelper/source/weak.cxx
+++ b/cppuhelper/source/weak.cxx
@@ -34,6 +34,7 @@ namespace cppu
 {
 
 // due to static Reflection destruction from usr, there must be a mutex leak (#73272#)
+// this is used to lock all instances of OWeakConnectionPoint and OWeakRefListener as well as OWeakObject::m_pWeakConnectionPoint
 inline static Mutex & getWeakMutex()
 {
     static Mutex * s_pMutex = nullptr;
@@ -358,7 +359,7 @@ public:
 
     /// The reference counter.
     oslInterlockedCount         m_aRefCount;
-    /// The connection point of the weak object
+    /// The connection point of the weak object, guarded by getWeakMutex()
     Reference< XAdapter >       m_XWeakConnectionPoint;
 };
 
@@ -463,12 +464,7 @@ void WeakReferenceHelper::clear()
     {
         if (m_pImpl)
         {
-            if (m_pImpl->m_XWeakConnectionPoint.is())
-            {
-                m_pImpl->m_XWeakConnectionPoint->removeReference(
-                        static_cast<XReference*>(m_pImpl));
-                m_pImpl->m_XWeakConnectionPoint.clear();
-            }
+            m_pImpl->dispose();
             m_pImpl->release();
             m_pImpl = nullptr;
         }
@@ -513,6 +509,7 @@ Reference< XInterface > WeakReferenceHelper::get() const
     {
         Reference< XAdapter > xAdp;
         {
+            // must lock to access m_XWeakConnectionPoint
             MutexGuard guard(cppu::getWeakMutex());
             if( m_pImpl && m_pImpl->m_XWeakConnectionPoint.is() )
                 xAdp = m_pImpl->m_XWeakConnectionPoint;
diff --git a/include/cppuhelper/weakref.hxx b/include/cppuhelper/weakref.hxx
index 783649418594..9cd422b8aab6 100644
--- a/include/cppuhelper/weakref.hxx
+++ b/include/cppuhelper/weakref.hxx
@@ -39,8 +39,14 @@ namespace uno
 
 class OWeakRefListener;
 
-/** The WeakReferenceHelper holds a weak reference to an object. This object must implement
-    the css::uno::XWeak interface.  The implementation is thread safe.
+/** The WeakReferenceHelper holds a weak reference to an object.
+
+    This object must implement the css::uno::XWeak interface.
+
+    The WeakReferenceHelper itself is *not* thread safe, just as
+    Reference itself isn't, but the implementation of the listeners etc.
+    behind it *is* thread-safe, so multiple threads can have their own
+    WeakReferences to the same XWeak object.
 */
 class CPPUHELPER_DLLPUBLIC WeakReferenceHelper
 {
@@ -116,8 +122,14 @@ protected:
     /// @endcond
 };
 
-/** The WeakReference<> holds a weak reference to an object. This object must implement
-    the css::uno::XWeak interface.  The implementation is thread safe.
+/** The WeakReference<> holds a weak reference to an object.
+
+    This object must implement the css::uno::XWeak interface.
+
+    The WeakReference itself is *not* thread safe, just as
+    Reference itself isn't, but the implementation of the listeners etc.
+    behind it *is* thread-safe, so multiple threads can have their own
+    WeakReferences to the same XWeak object.
 
     @tparam interface_type type of interface
 */