Bug 404870. Don't allow nsThread::PutEvent to succeed when the thread is no longer able to process the event. This stops us leaking the event and anything hanging off it. r+sr=bsmedberg
authorroc+@cs.cmu.edu
Tue, 04 Dec 2007 18:17:15 -0800
changeset 8724 d89113a3284e042ef4e5d666bf05d524d7dc7f7a
parent 8723 799d4149719988ae3391cf2aada2b5a6660c2173
child 8725 c7a12cacc09bfc0c551d43bba75dfcd263282c91
push idunknown
push userunknown
push dateunknown
bugs404870
milestone1.9b2pre
Bug 404870. Don't allow nsThread::PutEvent to succeed when the thread is no longer able to process the event. This stops us leaking the event and anything hanging off it. r+sr=bsmedberg
xpcom/threads/nsIEventTarget.idl
xpcom/threads/nsThread.cpp
xpcom/threads/nsThread.h
--- a/xpcom/threads/nsIEventTarget.idl
+++ b/xpcom/threads/nsIEventTarget.idl
@@ -50,16 +50,19 @@ interface nsIEventTarget : nsISupports
    * @param event
    *   The event to dispatch.
    * @param flags
    *   The flags modifying event dispatch.  The flags are described in detail
    *   below.
    * 
    * @throws NS_ERROR_INVALID_ARG
    *   Indicates that event is null.
+   * @throws NS_ERROR_UNEXPECTED
+   *   Indicates that the thread is shutting down and has finished processing
+   * events, so this event would never run and has not been dispatched. 
    */
   void dispatch(in nsIRunnable event, in unsigned long flags);
 
   /**
    * This flag specifies the default mode of event dispatch, whereby the event
    * is simply queued for later processing.  When this flag is specified,
    * dispatch returns immediately after the event is queued.
    */
--- a/xpcom/threads/nsThread.cpp
+++ b/xpcom/threads/nsThread.cpp
@@ -248,17 +248,35 @@ nsThread::ThreadFunc(void *arg)
   }
   event->Run();  // unblocks nsThread::Init
   event = nsnull;
 
   // Now, process incoming events...
   while (!self->ShuttingDown())
     NS_ProcessNextEvent(self);
 
-  NS_ProcessPendingEvents(self);
+  // Do NS_ProcessPendingEvents but with special handling to set
+  // mEventsAreDoomed atomically with the removal of the last event. The key
+  // invariant here is that we will never permit PutEvent to succeed if the
+  // event would be left in the queue after our final call to
+  // NS_ProcessPendingEvents.
+  while (PR_TRUE) {
+    {
+      nsAutoLock lock(self->mLock);
+      if (!self->mEvents->HasPendingEvent()) {
+        // No events in the queue, so we will stop now. Don't let any more
+        // events be added, since they won't be processed. It is critical
+        // that no PutEvent can occur between testing that the event queue is
+        // empty and setting mEventsAreDoomed!
+        self->mEventsAreDoomed = PR_TRUE;
+        break;
+      }
+    }
+    NS_ProcessPendingEvents(self);
+  }
 
   // Inform the threadmanager that this thread is going away
   nsThreadManager::get()->UnregisterCurrentThread(self);
 
   // Dispatch shutdown ACK
   event = new nsThreadShutdownAckEvent(self->mShutdownContext);
   self->mShutdownContext->joiningThread->Dispatch(event, NS_DISPATCH_NORMAL);
 
@@ -270,16 +288,17 @@ nsThread::ThreadFunc(void *arg)
 nsThread::nsThread()
   : mLock(PR_NewLock())
   , mEvents(&mEventsRoot)
   , mPriority(PRIORITY_NORMAL)
   , mThread(nsnull)
   , mRunningEvent(0)
   , mShutdownContext(nsnull)
   , mShutdownRequired(PR_FALSE)
+  , mEventsAreDoomed(PR_FALSE)
 {
 }
 
 nsThread::~nsThread()
 {
   if (mLock)
     PR_DestroyLock(mLock);
 }
@@ -326,70 +345,70 @@ nsThread::InitCurrentThread()
   NS_ENSURE_TRUE(mLock, NS_ERROR_OUT_OF_MEMORY);
 
   mThread = PR_GetCurrentThread();
 
   nsThreadManager::get()->RegisterCurrentThread(this);
   return NS_OK;
 }
 
-PRBool
+nsresult
 nsThread::PutEvent(nsIRunnable *event)
 {
-  PRBool rv;
   {
     nsAutoLock lock(mLock);
-    rv = mEvents->PutEvent(event);
+    if (mEventsAreDoomed) {
+      NS_WARNING("An event was posted to a thread that will never run it (rejected)");
+      return NS_ERROR_UNEXPECTED;
+    }
+    if (!mEvents->PutEvent(event))
+      return NS_ERROR_OUT_OF_MEMORY;
   }
-  if (!rv)
-    return PR_FALSE;
 
   nsCOMPtr<nsIThreadObserver> obs = GetObserver();
   if (obs)
     obs->OnDispatchedEvent(this);
 
-  return PR_TRUE;
+  return NS_OK;
 }
 
 //-----------------------------------------------------------------------------
 // nsIEventTarget
 
 NS_IMETHODIMP
 nsThread::Dispatch(nsIRunnable *event, PRUint32 flags)
 {
   LOG(("THRD(%p) Dispatch [%p %x]\n", this, event, flags));
 
   NS_ENSURE_ARG_POINTER(event);
 
-  PRBool dispatched;
   if (flags & DISPATCH_SYNC) {
     nsThread *thread = nsThreadManager::get()->GetCurrentThread();
     NS_ENSURE_STATE(thread);
 
     // XXX we should be able to do something better here... we should
     //     be able to monitor the slot occupied by this event and use
     //     that to tell us when the event has been processed.
  
     nsRefPtr<nsThreadSyncDispatch> wrapper =
         new nsThreadSyncDispatch(thread, event);
     if (!wrapper)
       return NS_ERROR_OUT_OF_MEMORY;
-    dispatched = PutEvent(wrapper);
+    nsresult rv = PutEvent(wrapper);
+    // Don't wait for the event to finish if we didn't dispatch it...
+    if (NS_FAILED(rv))
+      return rv;
 
     while (wrapper->IsPending())
       NS_ProcessNextEvent(thread);
-  } else {
-    NS_ASSERTION(flags == NS_DISPATCH_NORMAL, "unexpected dispatch flags");
-    dispatched = PutEvent(event);
+    return rv;
   }
 
-  if (NS_UNLIKELY(!dispatched))
-    return NS_ERROR_OUT_OF_MEMORY;
-
-  return NS_OK;
+  NS_ASSERTION(flags == NS_DISPATCH_NORMAL, "unexpected dispatch flags");
+  return PutEvent(event);
 }
 
 NS_IMETHODIMP
 nsThread::IsOnCurrentThread(PRBool *result)
 {
   *result = (PR_GetCurrentThread() == mThread);
   return NS_OK;
 }
@@ -429,16 +448,17 @@ nsThread::Shutdown()
   context.joiningThread = nsThreadManager::get()->GetCurrentThread();
   context.shutdownAck = PR_FALSE;
 
   // Set mShutdownContext and wake up the thread in case it is waiting for
   // events to process.
   nsCOMPtr<nsIRunnable> event = new nsThreadShutdownEvent(this, &context);
   if (!event)
     return NS_ERROR_OUT_OF_MEMORY;
+  // XXXroc What if posting the event fails due to OOM?
   PutEvent(event);
 
   // We could still end up with other events being added after the shutdown
   // task, but that's okay because we process pending events in ThreadFunc
   // after setting mShutdownContext just before exiting.
   
   // Process events on the current thread until we receive a shutdown ACK.
   while (!context.shutdownAck)
--- a/xpcom/threads/nsThread.h
+++ b/xpcom/threads/nsThread.h
@@ -90,17 +90,17 @@ private:
     nsThread::GetObserver(&obs);
     return already_AddRefed<nsIThreadObserver>(obs);
   }
 
   // Wrappers for event queue methods:
   PRBool GetEvent(PRBool mayWait, nsIRunnable **event) {
     return mEvents->GetEvent(mayWait, event);
   }
-  PRBool PutEvent(nsIRunnable *event);
+  nsresult PutEvent(nsIRunnable *event);
 
   // Wrapper for nsEventQueue that supports chaining.
   class nsChainedEventQueue {
   public:
     nsChainedEventQueue(nsIThreadEventFilter *filter = nsnull)
       : mNext(nsnull), mFilter(filter) {
     }
 
@@ -108,43 +108,49 @@ private:
       return mQueue.IsInitialized();
     }
 
     PRBool GetEvent(PRBool mayWait, nsIRunnable **event) {
       return mQueue.GetEvent(mayWait, event);
     }
 
     PRBool PutEvent(nsIRunnable *event);
+    
+    PRBool HasPendingEvent() {
+      return mQueue.HasPendingEvent();
+    }
 
     class nsChainedEventQueue *mNext;
   private:
     nsCOMPtr<nsIThreadEventFilter> mFilter;
     nsEventQueue mQueue;
   };
 
-  // This lock protects access to mObserver and mEvents.  Both of those fields
-  // are only modified on the thread itself (never from another thread).  This
-  // means that we can avoid holding the lock while using mObserver and mEvents
-  // on the thread itself.  When calling PutEvent on mEvents, we have to hold
-  // the lock to synchronize with PopEventQueue.
+  // This lock protects access to mObserver, mEvents and mEventsAreDoomed.
+  // All of those fields are only modified on the thread itself (never from
+  // another thread).  This means that we can avoid holding the lock while
+  // using mObserver and mEvents on the thread itself.  When calling PutEvent
+  // on mEvents, we have to hold the lock to synchronize with PopEventQueue.
   PRLock *mLock;
 
   nsCOMPtr<nsIThreadObserver> mObserver;
 
   nsChainedEventQueue *mEvents;   // never null
   nsChainedEventQueue  mEventsRoot;
 
   PRInt32   mPriority;
   PRThread *mThread;
   PRUint32  mRunningEvent;  // counter
 
   struct nsThreadShutdownContext *mShutdownContext;
 
   PRPackedBool mShutdownRequired;
   PRPackedBool mShutdownPending;
+  // Set to true when events posted to this thread will never run.
+  PRPackedBool mEventsAreDoomed;
 };
 
 //-----------------------------------------------------------------------------
 
 class nsThreadSyncDispatch : public nsRunnable {
 public:
   nsThreadSyncDispatch(nsIThread *origin, nsIRunnable *task)
     : mOrigin(origin), mSyncTask(task) {