Fix races in AsyncEventNotifier::execute

* m_aDeadProcessors was useless; for one, removeEventsForProcessor could never
have run (and set m_aDeadProcessors) between execute's reading from aEvents
and checking m_aDeadProcessors (because of the use of aMutex in both
functions), and if that were addressed, there would always be a race that
execute would run a processor that has just been removed.  Clients have to be
aware that a call to removeEventsForProcessor is just an optimization hint,
but does not guarantee that the given processor is not called from the execute
thread after removeEventsForProcessor returns.

* The sequence

aGuard.clear(); aPendingActions.reset(); aPanedingActions.wait();

could cause calls to aPendingActions.set() to get lost, both for signalling
new content in the queue and for signalling termination.

Change-Id: I43293e3d5090c2d46db8bc8ed6fb9c71e049d55c

1 files changed, 28 insertions, 68 deletions

diff --git a/comphelper/source/misc/asyncnotification.cxx b/comphelper/source/misc/asyncnotification.cxx
index d419580cf0e5..a8c018cd3ce1 100644
--- a/comphelper/source/misc/asyncnotification.cxx
+++ b/comphelper/source/misc/asyncnotification.cxx
@@ -23,8 +23,8 @@
 #include <osl/conditn.hxx>
 #include <comphelper/guarding.hxx>
 
+#include <cassert>
 #include <deque>
-#include <set>
 #include <functional>
 #include <algorithm>
 
@@ -72,23 +72,14 @@ namespace comphelper
         AnyEventRef                         aEvent;
         ::rtl::Reference< IEventProcessor > xProcessor;
 
-        ProcessableEvent( const AnyEventRef& _rEvent, const ::rtl::Reference< IEventProcessor >& _xProcessor )
-            :aEvent( _rEvent )
-            ,xProcessor( _xProcessor )
+        ProcessableEvent()
         {
         }
 
-        ProcessableEvent( const ProcessableEvent& _rRHS )
-            :aEvent( _rRHS.aEvent )
-            ,xProcessor( _rRHS.xProcessor )
-        {
-        }
-
-        ProcessableEvent& operator=( const ProcessableEvent& _rRHS )
+        ProcessableEvent( const AnyEventRef& _rEvent, const ::rtl::Reference< IEventProcessor >& _xProcessor )
+            :aEvent( _rEvent )
+            ,xProcessor( _xProcessor )
         {
-            aEvent = _rRHS.aEvent;
-            xProcessor = _rRHS.xProcessor;
-            return *this;
         }
     };
 
@@ -113,20 +104,14 @@ namespace comphelper
     struct EventNotifierImpl
     {
         ::osl::Mutex        aMutex;
-        oslInterlockedCount m_refCount;
         ::osl::Condition    aPendingActions;
         EventQueue          aEvents;
-        ::std::set< ::rtl::Reference< IEventProcessor > >
-                            m_aDeadProcessors;
+        bool                bTerminate;
 
         EventNotifierImpl()
-            :m_refCount( 0 )
+            :bTerminate( false )
         {
         }
-
-    private:
-        EventNotifierImpl( const EventNotifierImpl& );              // never implemented
-        EventNotifierImpl& operator=( const EventNotifierImpl& );   // never implemented
     };
 
 
@@ -150,10 +135,6 @@ namespace comphelper
 
         // remove all events for this processor
         ::std::remove_if( m_pImpl->aEvents.begin(), m_pImpl->aEvents.end(), EqualProcessor( _xProcessor ) );
-
-        // and just in case that an event for exactly this processor has just been
-        // popped from the queue, but not yet processed: remember it:
-        m_pImpl->m_aDeadProcessors.insert( _xProcessor );
     }
 
 
@@ -162,7 +143,7 @@ namespace comphelper
         ::osl::MutexGuard aGuard( m_pImpl->aMutex );
 
         // remember the termination request
-        Thread::terminate();
+        m_pImpl->bTerminate = true;
 
         // awake the thread
         m_pImpl->aPendingActions.set();
@@ -184,57 +165,36 @@ namespace comphelper
 
     void AsyncEventNotifier::execute()
     {
-        do
+        for (;;)
         {
-            AnyEventRef aNextEvent;
-            ::rtl::Reference< IEventProcessor > xNextProcessor;
-
-            ::osl::ClearableMutexGuard aGuard( m_pImpl->aMutex );
-            while ( m_pImpl->aEvents.size() > 0 )
+            m_pImpl->aPendingActions.wait();
+            ProcessableEvent aEvent;
             {
-                ProcessableEvent aEvent( m_pImpl->aEvents.front() );
-                aNextEvent = aEvent.aEvent;
-                xNextProcessor = aEvent.xProcessor;
-                m_pImpl->aEvents.pop_front();
-
-                OSL_TRACE( "AsyncEventNotifier(%p): popping %p", this, aNextEvent.get() );
-
-                if ( !aNextEvent.get() )
-                    continue;
-
-                // process the event, but only if it's processor did not die inbetween
-                ::std::set< ::rtl::Reference< IEventProcessor > >::iterator deadPos = m_pImpl->m_aDeadProcessors.find( xNextProcessor );
-                if ( deadPos != m_pImpl->m_aDeadProcessors.end() )
+                osl::MutexGuard aGuard(m_pImpl->aMutex);
+                if (m_pImpl->bTerminate)
                 {
-                    m_pImpl->m_aDeadProcessors.erase( xNextProcessor );
-                    xNextProcessor.clear();
-                    OSL_TRACE( "AsyncEventNotifier(%p): removing %p", this, aNextEvent.get() );
+                    break;
                 }
-
-                // if there was a termination request (->terminate), respect it
-                if ( !schedule() )
-                    return;
-
+                if (!m_pImpl->aEvents.empty())
+                {
+                    aEvent = m_pImpl->aEvents.front();
+                    m_pImpl->aEvents.pop_front();
+                    OSL_TRACE(
+                        "AsyncEventNotifier(%p): popping %p", this,
+                        aEvent.aEvent.get());
+                }
+                if (m_pImpl->aEvents.empty())
                 {
-                    ::comphelper::MutexRelease aReleaseOnce( m_pImpl->aMutex );
-                    if ( xNextProcessor.get() )
-                        xNextProcessor->processEvent( *aNextEvent.get() );
+                    m_pImpl->aPendingActions.reset();
                 }
             }
-
-            // if there was a termination request (->terminate), respect it
-            if ( !schedule() )
-                return;
-
-            // wait for new events to process
-            aGuard.clear();
-            m_pImpl->aPendingActions.reset();
-            m_pImpl->aPendingActions.wait();
+            if (aEvent.aEvent.is()) {
+                assert(aEvent.xProcessor.is());
+                aEvent.xProcessor->processEvent(*aEvent.aEvent);
+            }
         }
-        while ( true );
     }
 
-
 } // namespace comphelper