From f1a0afa78ecf5a2a203c5ba2a8347cdea40996ec Mon Sep 17 00:00:00 2001 From: "roc+%cs.cmu.edu" Date: Wed, 5 Dec 2007 02:17:15 +0000 Subject: [PATCH] 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 git-svn-id: svn://10.0.0.236/trunk@240482 18797224-902f-48f8-a5cc-f745e15eee43 --- mozilla/xpcom/threads/nsIEventTarget.idl | 3 ++ mozilla/xpcom/threads/nsThread.cpp | 52 ++++++++++++++++-------- mozilla/xpcom/threads/nsThread.h | 18 +++++--- 3 files changed, 51 insertions(+), 22 deletions(-) diff --git a/mozilla/xpcom/threads/nsIEventTarget.idl b/mozilla/xpcom/threads/nsIEventTarget.idl index 37999ac7228..a42edc63d43 100644 --- a/mozilla/xpcom/threads/nsIEventTarget.idl +++ b/mozilla/xpcom/threads/nsIEventTarget.idl @@ -55,6 +55,9 @@ interface nsIEventTarget : nsISupports * * @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); diff --git a/mozilla/xpcom/threads/nsThread.cpp b/mozilla/xpcom/threads/nsThread.cpp index 13e51583b0b..9631c6be6dd 100644 --- a/mozilla/xpcom/threads/nsThread.cpp +++ b/mozilla/xpcom/threads/nsThread.cpp @@ -253,7 +253,25 @@ nsThread::ThreadFunc(void *arg) 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); @@ -275,6 +293,7 @@ nsThread::nsThread() , mRunningEvent(0) , mShutdownContext(nsnull) , mShutdownRequired(PR_FALSE) + , mEventsAreDoomed(PR_FALSE) { } @@ -331,22 +350,24 @@ nsThread::InitCurrentThread() 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 obs = GetObserver(); if (obs) obs->OnDispatchedEvent(this); - return PR_TRUE; + return NS_OK; } //----------------------------------------------------------------------------- @@ -359,7 +380,6 @@ nsThread::Dispatch(nsIRunnable *event, PRUint32 flags) NS_ENSURE_ARG_POINTER(event); - PRBool dispatched; if (flags & DISPATCH_SYNC) { nsThread *thread = nsThreadManager::get()->GetCurrentThread(); NS_ENSURE_STATE(thread); @@ -372,19 +392,18 @@ nsThread::Dispatch(nsIRunnable *event, PRUint32 flags) 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 @@ -434,6 +453,7 @@ nsThread::Shutdown() nsCOMPtr 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 diff --git a/mozilla/xpcom/threads/nsThread.h b/mozilla/xpcom/threads/nsThread.h index 40d719b6fa6..69764c3baa9 100644 --- a/mozilla/xpcom/threads/nsThread.h +++ b/mozilla/xpcom/threads/nsThread.h @@ -95,7 +95,7 @@ private: 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 { @@ -113,6 +113,10 @@ private: } PRBool PutEvent(nsIRunnable *event); + + PRBool HasPendingEvent() { + return mQueue.HasPendingEvent(); + } class nsChainedEventQueue *mNext; private: @@ -120,11 +124,11 @@ private: 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 mObserver; @@ -140,6 +144,8 @@ private: PRPackedBool mShutdownRequired; PRPackedBool mShutdownPending; + // Set to true when events posted to this thread will never run. + PRPackedBool mEventsAreDoomed; }; //-----------------------------------------------------------------------------