comphelper,vcl: let DeInitVCL() join some AsyncEventNotifier threads

comphelper::AsyncEventNotifier is an amazing class that dispatches
events in separate threads, no doubt implemented during times of
exuberant optimism about the tractability of shared-state
multi-threading.

Unfortunately the authors forgot to think about how all those awesome
threads will be joined, so if they are somehow blocked, then it may well
happen that the events are dispatched when the main thread is already in
DeInitVCL, and the objects required for the dispatching already smell
somewhat funny.

This happens quite reproducibly when changing dbaccess' ModelMethodGuard
to lock the SolarMutex too, then CppunitTest_dbaccess_RowSetClones
crashes in DeInitVCL() because one AsyncEventNotifier thread was blocked
until then by SolarMutexGuard, and this test never Yields once its
document is loaded.

Try to fix this by joining the "DocumentEventNotifier" threads from
DeInitVCL() itself.

Since there's no rtl::WeakReference to go with rtl::Reference, refactor
the AsyncEventNotifier and create a new AsyncEventNotifierAutoJoin
that has to be used with std::shared_ptr and std::weak_ptr.

Change-Id: I50a0749795acb04b0776e543f7125767b697ea35
Reviewed-on: https://gerrit.libreoffice.org/27581
Tested-by: Jenkins <ci@libreoffice.org>
Reviewed-by: Michael Stahl <mstahl@redhat.com>

4 files changed, 202 insertions, 27 deletions

diff --git a/comphelper/source/misc/asyncnotification.cxx b/comphelper/source/misc/asyncnotification.cxx
index 85643e2b3cf4..e13be286a5dd 100644
--- a/comphelper/source/misc/asyncnotification.cxx
+++ b/comphelper/source/misc/asyncnotification.cxx
@@ -21,10 +21,12 @@
 #include <osl/diagnose.h>
 #include <osl/mutex.hxx>
 #include <osl/conditn.hxx>
+#include <rtl/instance.hxx>
 #include <comphelper/guarding.hxx>
 
 #include <cassert>
 #include <deque>
+#include <vector>
 #include <functional>
 #include <algorithm>
 
@@ -76,6 +78,9 @@ namespace comphelper
         ::osl::Condition    aPendingActions;
         EventQueue          aEvents;
         bool                bTerminate;
+        // only used for AsyncEventNotifierAutoJoin
+        char const*         name;
+        std::shared_ptr<AsyncEventNotifierAutoJoin> pKeepThisAlive;
 
         EventNotifierImpl()
             :bTerminate( false )
@@ -83,18 +88,18 @@ namespace comphelper
         }
     };
 
-    AsyncEventNotifier::AsyncEventNotifier(char const * name):
-        Thread(name), m_xImpl(new EventNotifierImpl)
+    AsyncEventNotifierBase::AsyncEventNotifierBase()
+        : m_xImpl(new EventNotifierImpl)
     {
     }
 
 
-    AsyncEventNotifier::~AsyncEventNotifier()
+    AsyncEventNotifierBase::~AsyncEventNotifierBase()
     {
     }
 
 
-    void AsyncEventNotifier::removeEventsForProcessor( const ::rtl::Reference< IEventProcessor >& _xProcessor )
+    void AsyncEventNotifierBase::removeEventsForProcessor( const ::rtl::Reference< IEventProcessor >& _xProcessor )
     {
         ::osl::MutexGuard aGuard( m_xImpl->aMutex );
 
@@ -103,7 +108,7 @@ namespace comphelper
     }
 
 
-    void SAL_CALL AsyncEventNotifier::terminate()
+    void SAL_CALL AsyncEventNotifierBase::terminate()
     {
         ::osl::MutexGuard aGuard( m_xImpl->aMutex );
 
@@ -115,7 +120,7 @@ namespace comphelper
     }
 
 
-    void AsyncEventNotifier::addEvent( const AnyEventRef& _rEvent, const ::rtl::Reference< IEventProcessor >& _xProcessor )
+    void AsyncEventNotifierBase::addEvent( const AnyEventRef& _rEvent, const ::rtl::Reference< IEventProcessor >& _xProcessor )
     {
         ::osl::MutexGuard aGuard( m_xImpl->aMutex );
 
@@ -128,7 +133,7 @@ namespace comphelper
     }
 
 
-    void AsyncEventNotifier::execute()
+    void AsyncEventNotifierBase::execute()
     {
         for (;;)
         {
@@ -160,6 +165,115 @@ namespace comphelper
         }
     }
 
+    AsyncEventNotifier::AsyncEventNotifier(char const* name)
+        : salhelper::Thread(name)
+    {
+    }
+
+    AsyncEventNotifier::~AsyncEventNotifier()
+    {
+    }
+
+    void AsyncEventNotifier::execute()
+    {
+        return AsyncEventNotifierBase::execute();
+    }
+
+    void AsyncEventNotifier::terminate()
+    {
+        return AsyncEventNotifierBase::terminate();
+    }
+
+    struct theNotifiersMutex : public rtl::Static<osl::Mutex, theNotifiersMutex> {};
+    static std::vector<std::weak_ptr<AsyncEventNotifierAutoJoin>> g_Notifiers;
+
+    void JoinAsyncEventNotifiers()
+    {
+        std::vector<std::weak_ptr<AsyncEventNotifierAutoJoin>> notifiers;
+        {
+            ::osl::MutexGuard g(theNotifiersMutex::get());
+            notifiers = g_Notifiers;
+        }
+        for (std::weak_ptr<AsyncEventNotifierAutoJoin> const& wNotifier : notifiers)
+        {
+            std::shared_ptr<AsyncEventNotifierAutoJoin> const pNotifier(
+                    wNotifier.lock());
+            if (pNotifier)
+            {
+                pNotifier->terminate();
+                pNotifier->join();
+            }
+        }
+        // note it's possible that g_Notifiers isn't empty now in case of leaks,
+        // particularly since the UNO service manager isn't disposed yet
+    }
+
+    AsyncEventNotifierAutoJoin::AsyncEventNotifierAutoJoin(char const* name)
+    {
+        m_xImpl->name = name;
+    }
+
+    AsyncEventNotifierAutoJoin::~AsyncEventNotifierAutoJoin()
+    {
+        ::osl::MutexGuard g(theNotifiersMutex::get());
+        // note: this doesn't happen atomically with the refcount
+        // hence it's possible this deletes > 1 or 0 elements
+        g_Notifiers.erase(
+            std::remove_if(g_Notifiers.begin(), g_Notifiers.end(),
+                [](std::weak_ptr<AsyncEventNotifierAutoJoin> const& w) {
+                    return w.expired();
+                } ),
+            g_Notifiers.end());
+    }
+
+    std::shared_ptr<AsyncEventNotifierAutoJoin>
+    AsyncEventNotifierAutoJoin::newAsyncEventNotifierAutoJoin(char const* name)
+    {
+        std::shared_ptr<AsyncEventNotifierAutoJoin> const ret(
+                new AsyncEventNotifierAutoJoin(name));
+        ::osl::MutexGuard g(theNotifiersMutex::get());
+        g_Notifiers.push_back(ret);
+        return ret;
+    }
+
+    void AsyncEventNotifierAutoJoin::terminate()
+    {
+        return AsyncEventNotifierBase::terminate();
+    }
+
+    void AsyncEventNotifierAutoJoin::launch(std::shared_ptr<AsyncEventNotifierAutoJoin> const& xThis)
+    {
+        // see salhelper::Thread::launch
+        xThis->m_xImpl->pKeepThisAlive = xThis;
+        try {
+            if (!xThis->create()) {
+                throw std::runtime_error("osl::Thread::create failed");
+            }
+        } catch (...) {
+            xThis->m_xImpl->pKeepThisAlive.reset();
+            throw;
+        }
+    }
+
+    void AsyncEventNotifierAutoJoin::run()
+    {
+        // see salhelper::Thread::run
+        try {
+            setName(m_xImpl->name);
+            execute();
+        } catch (...) {
+            onTerminated();
+            throw;
+        }
+    }
+
+    void AsyncEventNotifierAutoJoin::onTerminated()
+    {
+        // try to delete "this"
+        m_xImpl->pKeepThisAlive.reset();
+        return osl::Thread::onTerminated();
+    }
+
 } // namespace comphelper
 
 
diff --git a/dbaccess/source/core/dataaccess/documenteventnotifier.cxx b/dbaccess/source/core/dataaccess/documenteventnotifier.cxx
index 0f4d3f21130c..aca027e159d9 100644
--- a/dbaccess/source/core/dataaccess/documenteventnotifier.cxx
+++ b/dbaccess/source/core/dataaccess/documenteventnotifier.cxx
@@ -50,7 +50,7 @@ namespace dbaccess
         ::osl::Mutex&                                           m_rMutex;
         bool                                                    m_bInitialized;
         bool                                                    m_bDisposed;
-        ::rtl::Reference< ::comphelper::AsyncEventNotifier >    m_pEventBroadcaster;
+        ::std::shared_ptr<::comphelper::AsyncEventNotifierAutoJoin> m_pEventBroadcaster;
         ::comphelper::OInterfaceContainerHelper2                      m_aLegacyEventListeners;
         ::comphelper::OInterfaceContainerHelper2                      m_aDocumentEventListeners;
 
@@ -137,7 +137,7 @@ namespace dbaccess
         // SYNCHRONIZED ->
         // cancel any pending asynchronous events
         ::osl::ResettableMutexGuard aGuard( m_rMutex );
-        if ( m_pEventBroadcaster.is() )
+        if (m_pEventBroadcaster)
         {
             m_pEventBroadcaster->removeEventsForProcessor( this );
             m_pEventBroadcaster->terminate();
@@ -147,7 +147,9 @@ namespace dbaccess
                 // in atexit handlers; simply calling join here leads to
                 // deadlock, as this thread holds the solar mutex while the
                 // other thread is typically blocked waiting for the solar mutex
-            m_pEventBroadcaster.clear();
+                // For now, use newAutoJoinAsyncEventNotifier which is
+                // better than nothing.
+            m_pEventBroadcaster.reset();
         }
 
         lang::EventObject aEvent( m_rDocument );
@@ -169,9 +171,11 @@ namespace dbaccess
             throw DoubleInitializationException();
 
         m_bInitialized = true;
-        if ( m_pEventBroadcaster.is() )
+        if (m_pEventBroadcaster)
+        {
             // there are already pending asynchronous events
-            m_pEventBroadcaster->launch();
+            ::comphelper::AsyncEventNotifierAutoJoin::launch(m_pEventBroadcaster);
+        }
     }
 
     void DocumentEventNotifier_Impl::impl_notifyEvent_nothrow( const DocumentEvent& _rEvent )
@@ -199,14 +203,16 @@ namespace dbaccess
 
     void DocumentEventNotifier_Impl::impl_notifyEventAsync_nothrow( const DocumentEvent& _rEvent )
     {
-        if ( !m_pEventBroadcaster.is() )
+        if (!m_pEventBroadcaster)
         {
-            m_pEventBroadcaster.set(
-                new ::comphelper::AsyncEventNotifier("DocumentEventNotifier"));
+            m_pEventBroadcaster = ::comphelper::AsyncEventNotifierAutoJoin
+                ::newAsyncEventNotifierAutoJoin("DocumentEventNotifier");
             if ( m_bInitialized )
+            {
                 // start processing the events if and only if we (our document, respectively) are
                 // already initialized
-                m_pEventBroadcaster->launch();
+                ::comphelper::AsyncEventNotifierAutoJoin::launch(m_pEventBroadcaster);
+            }
         }
         m_pEventBroadcaster->addEvent( new DocumentEventHolder( _rEvent ), this );
     }
diff --git a/include/comphelper/asyncnotification.hxx b/include/comphelper/asyncnotification.hxx
index 2f33c236283c..a8439f8d36e7 100644
--- a/include/comphelper/asyncnotification.hxx
+++ b/include/comphelper/asyncnotification.hxx
@@ -95,25 +95,20 @@ namespace comphelper
         events in the queue. As soon as you add an event, the thread is woken up, processes the event,
         and sleeps again.
     */
-    class COMPHELPER_DLLPUBLIC AsyncEventNotifier: public salhelper::Thread
+    class COMPHELPER_DLLPUBLIC AsyncEventNotifierBase
     {
         friend struct EventNotifierImpl;
 
-    private:
+    protected:
         std::unique_ptr<EventNotifierImpl>        m_xImpl;
 
-        SAL_DLLPRIVATE virtual ~AsyncEventNotifier();
+        SAL_DLLPRIVATE virtual ~AsyncEventNotifierBase();
 
         // Thread
-        SAL_DLLPRIVATE virtual void execute() override;
+        SAL_DLLPRIVATE virtual void execute();
 
     public:
-        /** constructs a notifier thread
-
-            @param name the thread name, see ::osl_setThreadName; must not be
-            null
-        */
-        AsyncEventNotifier(char const * name);
+        AsyncEventNotifierBase();
 
         /** terminates the thread
 
@@ -123,7 +118,7 @@ namespace comphelper
             itself, it will return immediately, and the thread will be terminated as soon as
             the current notification is finished.
         */
-        virtual void SAL_CALL terminate() override;
+        virtual void SAL_CALL terminate();
 
         /** adds an event to the queue, together with the instance which is responsible for
             processing it
@@ -143,6 +138,60 @@ namespace comphelper
         void removeEventsForProcessor( const ::rtl::Reference< IEventProcessor >& _xProcessor );
     };
 
+    /** This class is usable with rtl::Reference.
+        As always, the thread must be joined somewhere.
+     */
+    class COMPHELPER_DLLPUBLIC AsyncEventNotifier
+        : public AsyncEventNotifierBase
+        , public salhelper::Thread
+    {
+
+    private:
+        SAL_DLLPRIVATE virtual ~AsyncEventNotifier();
+
+        SAL_DLLPRIVATE virtual void execute() override;
+
+    public:
+        /** constructs a notifier thread
+
+            @param name the thread name, see ::osl_setThreadName; must not be
+            null
+        */
+        AsyncEventNotifier(char const* name);
+
+        virtual void SAL_CALL terminate() override;
+    };
+
+    /** This is a hack (when proper joining is not possible), use of which
+        should be avoided by good design.
+     */
+    class COMPHELPER_DLLPUBLIC AsyncEventNotifierAutoJoin
+        : public AsyncEventNotifierBase
+        , private osl::Thread
+    {
+
+    private:
+        SAL_DLLPRIVATE AsyncEventNotifierAutoJoin(char const* name);
+
+        SAL_DLLPRIVATE virtual void SAL_CALL run() override;
+        SAL_DLLPRIVATE virtual void SAL_CALL onTerminated() override;
+
+    public:
+        // only public so shared_ptr finds it
+        SAL_DLLPRIVATE virtual ~AsyncEventNotifierAutoJoin();
+
+        static std::shared_ptr<AsyncEventNotifierAutoJoin>
+            newAsyncEventNotifierAutoJoin(char const* name);
+
+        virtual void SAL_CALL terminate() override;
+
+        using osl::Thread::join;
+        using osl::Thread::operator new;
+        using osl::Thread::operator delete; // clang really wants this?
+
+        static void launch(std::shared_ptr<AsyncEventNotifierAutoJoin> const&);
+    };
+
 
     //= EventHolder
 
@@ -166,6 +215,7 @@ namespace comphelper
         inline const EventObjectType& getEventObject() const { return m_aEvent; }
     };
 
+    COMPHELPER_DLLPUBLIC void JoinAsyncEventNotifiers();
 
 } // namespace comphelper
 
diff --git a/vcl/source/app/svmain.cxx b/vcl/source/app/svmain.cxx
index b3f4ed688d1e..aba97b58482a 100644
--- a/vcl/source/app/svmain.cxx
+++ b/vcl/source/app/svmain.cxx
@@ -28,6 +28,7 @@
 #include <tools/resmgr.hxx>
 
 #include <comphelper/processfactory.hxx>
+#include <comphelper/asyncnotification.hxx>
 
 #include <unotools/syslocaleoptions.hxx>
 #include <vcl/svapp.hxx>
@@ -352,6 +353,10 @@ VCLUnoWrapperDeleter::disposing(lang::EventObject const& /* rSource */)
 
 void DeInitVCL()
 {
+    {
+        SolarMutexReleaser r; // unblock threads blocked on that so we can join
+        ::comphelper::JoinAsyncEventNotifiers();
+    }
     ImplSVData* pSVData = ImplGetSVData();
     // lp#1560328: clear cache before disposing rest of VCL
     if(pSVData->mpBlendFrameCache)