From 3758741719ee8f84cf664cb3f34cd01e516b1fbb Mon Sep 17 00:00:00 2001 From: "brendan%mozilla.org" Date: Fri, 8 Mar 2002 20:11:49 +0000 Subject: [PATCH] Try to fix design flaw where a timer's destructor races with TimerThread::Run -- really want a better design, but this patches the problem in the context of the current design (118004, r=dbradley/pavlov, sr=shaver/jband, a=asa). git-svn-id: svn://10.0.0.236/trunk@116167 18797224-902f-48f8-a5cc-f745e15eee43 --- mozilla/xpcom/macbuild/XPCOMIDL.xml | 10 +- mozilla/xpcom/threads/MANIFEST_IDL | 2 +- mozilla/xpcom/threads/Makefile.in | 2 +- mozilla/xpcom/threads/TimerThread.cpp | 50 +++++++- mozilla/xpcom/threads/TimerThread.h | 6 +- mozilla/xpcom/threads/makefile.win | 2 +- ...rScriptable.idl => nsIScriptableTimer.idl} | 4 +- mozilla/xpcom/threads/nsTimerImpl.cpp | 116 +++++++++++++----- mozilla/xpcom/threads/nsTimerImpl.h | 44 ++++--- .../updates/src/nsUpdateNotifier.js | 8 +- 10 files changed, 171 insertions(+), 73 deletions(-) rename mozilla/xpcom/threads/{nsITimerScriptable.idl => nsIScriptableTimer.idl} (96%) diff --git a/mozilla/xpcom/macbuild/XPCOMIDL.xml b/mozilla/xpcom/macbuild/XPCOMIDL.xml index f8e378f7c69..58f3e72b302 100644 --- a/mozilla/xpcom/macbuild/XPCOMIDL.xml +++ b/mozilla/xpcom/macbuild/XPCOMIDL.xml @@ -943,7 +943,7 @@ Name - nsITimerScriptable.idl + nsIScriptableTimer.idl MacOS Text @@ -1410,7 +1410,7 @@ Name - nsITimerScriptable.idl + nsIScriptableTimer.idl MacOS @@ -2550,7 +2550,7 @@ Name - nsITimerScriptable.idl + nsIScriptableTimer.idl MacOS Text @@ -3017,7 +3017,7 @@ Name - nsITimerScriptable.idl + nsIScriptableTimer.idl MacOS @@ -3590,7 +3590,7 @@ headers Name - nsITimerScriptable.idl + nsIScriptableTimer.idl MacOS diff --git a/mozilla/xpcom/threads/MANIFEST_IDL b/mozilla/xpcom/threads/MANIFEST_IDL index ccf8d0a5610..81bc391b453 100644 --- a/mozilla/xpcom/threads/MANIFEST_IDL +++ b/mozilla/xpcom/threads/MANIFEST_IDL @@ -3,4 +3,4 @@ nsIThread.idl nsIThreadPool.idl nsIEventQueue.idl nsIEventQueueService.idl -nsITimerScriptable.idl +nsIScriptableTimer.idl diff --git a/mozilla/xpcom/threads/Makefile.in b/mozilla/xpcom/threads/Makefile.in index 3344538c344..c3d2147f42e 100644 --- a/mozilla/xpcom/threads/Makefile.in +++ b/mozilla/xpcom/threads/Makefile.in @@ -66,7 +66,7 @@ XPIDLSRCS = \ nsIEventQueue.idl \ nsIEventQueueService.idl \ nsIProcess.idl \ - nsITimerScriptable.idl \ + nsIScriptableTimer.idl \ $(NULL) EXPORTS := $(addprefix $(srcdir)/, $(EXPORTS)) diff --git a/mozilla/xpcom/threads/TimerThread.cpp b/mozilla/xpcom/threads/TimerThread.cpp index a87395b18d6..c15f458df3b 100644 --- a/mozilla/xpcom/threads/TimerThread.cpp +++ b/mozilla/xpcom/threads/TimerThread.cpp @@ -63,6 +63,12 @@ TimerThread::~TimerThread() PR_DestroyLock(mLock); mThread = nsnull; + + PRInt32 n = mTimers.Count(); + while (--n >= 0) { + nsTimerImpl *timer = NS_STATIC_CAST(nsTimerImpl *, mTimers[n]); + NS_RELEASE(timer); + } } nsresult TimerThread::Init() @@ -189,8 +195,14 @@ NS_IMETHODIMP TimerThread::Run() if (now >= timer->mTimeout + mTimeoutAdjustment) { next: - RemoveTimerInternal(timer); + // NB: AddRef before the Release under RemoveTimerInternal to avoid + // mRefCnt passing through zero, in case all other refs than the one + // from mTimers have gone away (the last non-mTimers[i]-ref's Release + // must be racing with us, blocked in gThread->RemoveTimer waiting + // for TimerThread::mLock, under nsTimerImpl::Release. + NS_ADDREF(timer); + RemoveTimerInternal(timer); // We release mLock around the Fire call, of course, to avoid deadlock. lock.unlock(); @@ -261,6 +273,8 @@ nsresult TimerThread::AddTimer(nsTimerImpl *aTimer) /* add the timer from our list */ PRInt32 i = AddTimerInternal(aTimer); + if (i < 0) + return NS_ERROR_OUT_OF_MEMORY; if (mCondVar && mWaiting && i == 0) PR_NotifyCondVar(mCondVar); @@ -272,8 +286,12 @@ nsresult TimerThread::TimerDelayChanged(nsTimerImpl *aTimer) { nsAutoLock lock(mLock); + // Our caller has a strong ref to aTimer, so it can't go away here under + // ReleaseTimerInternal. + RemoveTimerInternal(aTimer); - AddTimerInternal(aTimer); + if (AddTimerInternal(aTimer) < 0) + return NS_ERROR_OUT_OF_MEMORY; return NS_OK; } @@ -282,8 +300,15 @@ nsresult TimerThread::RemoveTimer(nsTimerImpl *aTimer) { nsAutoLock lock(mLock); - /* remove the timer from our list */ - RemoveTimerInternal(aTimer); + // Remove the timer from our array. Tell callers that aTimer was not found + // by returning NS_ERROR_NOT_AVAILABLE. Unlike the TimerDelayChanged case + // immediately above, our caller may be passing a (now-)weak ref in via the + // aTimer param, specifically when nsTimerImpl::Release loses a race with + // TimerThread::Run, must wait for the mLock auto-lock here, and during the + // wait Run drops the only remaining ref to aTimer via RemoveTimerInternal. + + if (!RemoveTimerInternal(aTimer)) + return NS_ERROR_NOT_AVAILABLE; return NS_OK; } @@ -300,7 +325,22 @@ PRInt32 TimerThread::AddTimerInternal(nsTimerImpl *aTimer) break; } } - mTimers.InsertElementAt(aTimer, i); + if (!mTimers.InsertElementAt(aTimer, i)) + return -1; + + aTimer->mArmed = PR_TRUE; + NS_ADDREF(aTimer); return i; } + +PRBool TimerThread::RemoveTimerInternal(nsTimerImpl *aTimer) +{ + if (!mTimers.RemoveElement(aTimer)) + return PR_FALSE; + + // Order is crucial here -- see nsTimerImpl::Release. + aTimer->mArmed = PR_FALSE; + NS_RELEASE(aTimer); + return PR_TRUE; +} diff --git a/mozilla/xpcom/threads/TimerThread.h b/mozilla/xpcom/threads/TimerThread.h index 52b22d64528..01e1e14e924 100644 --- a/mozilla/xpcom/threads/TimerThread.h +++ b/mozilla/xpcom/threads/TimerThread.h @@ -76,10 +76,8 @@ public: private: // These two internal functions must be called from within a lock - inline PRInt32 AddTimerInternal(nsTimerImpl *aTimer); - inline void RemoveTimerInternal(nsTimerImpl *aTimer) { - mTimers.RemoveElement(aTimer); - } + PRInt32 AddTimerInternal(nsTimerImpl *aTimer); + PRBool RemoveTimerInternal(nsTimerImpl *aTimer); nsCOMPtr mThread; PRLock *mLock; diff --git a/mozilla/xpcom/threads/makefile.win b/mozilla/xpcom/threads/makefile.win index 6a549d43676..aa49c3ddd48 100644 --- a/mozilla/xpcom/threads/makefile.win +++ b/mozilla/xpcom/threads/makefile.win @@ -47,7 +47,7 @@ XPIDLSRCS = \ .\nsIThreadPool.idl \ .\nsIRunnable.idl \ .\nsIProcess.idl \ - .\nsITimerScriptable.idl \ + .\nsIScriptableTimer.idl \ $(NULL) ################################################################################ diff --git a/mozilla/xpcom/threads/nsITimerScriptable.idl b/mozilla/xpcom/threads/nsIScriptableTimer.idl similarity index 96% rename from mozilla/xpcom/threads/nsITimerScriptable.idl rename to mozilla/xpcom/threads/nsIScriptableTimer.idl index fcd60739e12..1e8431ebe10 100644 --- a/mozilla/xpcom/threads/nsITimerScriptable.idl +++ b/mozilla/xpcom/threads/nsIScriptableTimer.idl @@ -48,7 +48,7 @@ */ [scriptable,uuid(84271f22-c023-4b01-8050-d71c0c6a6235)] -interface nsITimerScriptable : nsISupports +interface nsIScriptableTimer : nsISupports { /* Timer priorities */ const short PRIORITY_HIGHEST = 10; @@ -71,7 +71,7 @@ interface nsITimerScriptable : nsISupports * ``timer-callback'' topic with the subject being * the timer itself when the timer fires: * - * observe(nsISupports aSubject, => nsITimerScriptable + * observe(nsISupports aSubject, => nsIScriptableTimer * string aTopic, => ``timer-callback'' * wstring data => null * diff --git a/mozilla/xpcom/threads/nsTimerImpl.cpp b/mozilla/xpcom/threads/nsTimerImpl.cpp index 3a3f3eaa465..131d69d7bd3 100644 --- a/mozilla/xpcom/threads/nsTimerImpl.cpp +++ b/mozilla/xpcom/threads/nsTimerImpl.cpp @@ -48,7 +48,7 @@ static TimerThread *gThread = nsnull; double nsTimerImpl::sDeltaSumSquared = 0; double nsTimerImpl::sDeltaSum = 0; -double nsTimerImpl::sNum = 0; +double nsTimerImpl::sDeltaNum = 0; static void myNS_MeanAndStdDev(double n, double sumOfValues, double sumOfSquaredValues, @@ -70,8 +70,63 @@ myNS_MeanAndStdDev(double n, double sumOfValues, double sumOfSquaredValues, } #endif -NS_IMPL_THREADSAFE_ISUPPORTS2(nsTimerImpl, nsITimer, nsITimerScriptable) +NS_IMPL_THREADSAFE_QUERY_INTERFACE2(nsTimerImpl, nsITimer, nsIScriptableTimer) +NS_IMPL_THREADSAFE_ADDREF(nsTimerImpl) +NS_IMETHODIMP_(nsrefcnt) nsTimerImpl::Release(void) +{ + nsrefcnt count; + + NS_PRECONDITION(0 != mRefCnt, "dup release"); + count = PR_AtomicDecrement((PRInt32 *)&mRefCnt); + NS_LOG_RELEASE(this, count, "nsTimerImpl"); + if (count == 0) { + mRefCnt = 1; /* stabilize */ + + /* enable this to find non-threadsafe destructors: */ + /* NS_ASSERT_OWNINGTHREAD(nsTimerImpl); */ + NS_DELETEXPCOM(this); + return 0; + } + + // If only one reference remains, and mArmed is set, then the ref must be + // from the TimerThread::mTimers array, so we Cancel this timer to remove + // the mTimers element, and return 0 if Cancel in fact disarmed the timer. + // + // We use an inlined version of nsTimerImpl::Cancel here to check for the + // NS_ERROR_NOT_AVAILABLE code returned by gThread->RemoveTimer when this + // timer is not found in the mTimers array -- i.e., when the timer was not + // in fact armed once we acquired TimerThread::mLock, in spite of mArmed + // being true here. That can happen if the armed timer is being fired by + // TimerThread::Run as we race and test mArmed just before it is cleared by + // the timer thread. If the RemoveTimer call below doesn't find this timer + // in the mTimers array, then the last ref to this timer is held manually + // and temporarily by the TimerThread, so we should fall through to the + // final return and return 1, not 0. + // + // The original version of this thread-based timer code kept weak refs from + // TimerThread::mTimers, removing this timer's weak ref in the destructor, + // but that leads to double-destructions in the race described above, and + // adding mArmed doesn't help, because destructors can't be deferred, once + // begun. But by combining reference-counting and a specialized Release + // method with "is this timer still in the mTimers array once we acquire + // the TimerThread's lock" testing, we defer destruction until we're sure + // that only one thread has its hot little hands on this timer. + // + // Note that both approaches preclude a timer creator, and everyone else + // except the TimerThread who might have a strong ref, from dropping all + // their strong refs without implicitly canceling the timer. Timers need + // non-mTimers-element strong refs to stay alive. + + if (count == 1 && mArmed) { + mCanceled = PR_TRUE; + + if (NS_SUCCEEDED(gThread->RemoveTimer(this))) + return 0; + } + + return count; +} PR_STATIC_CALLBACK(PRStatus) InitThread(void) { @@ -94,7 +149,8 @@ nsTimerImpl::nsTimerImpl() : mClosure(nsnull), mCallbackType(CALLBACK_TYPE_UNKNOWN), mFiring(PR_FALSE), - mCancelled(PR_FALSE), + mArmed(PR_FALSE), + mCanceled(PR_FALSE), mTimeout(0) { NS_INIT_REFCNT(); @@ -117,9 +173,6 @@ nsTimerImpl::~nsTimerImpl() NS_RELEASE(mCallback.i); else if (mCallbackType == CALLBACK_TYPE_OBSERVER) NS_RELEASE(mCallback.o); - - if (gThread) - gThread->RemoveTimer(this); } @@ -128,9 +181,9 @@ void nsTimerImpl::Shutdown() #ifdef DEBUG_TIMERS if (PR_LOG_TEST(gTimerLog, PR_LOG_DEBUG)) { double mean = 0, stddev = 0; - myNS_MeanAndStdDev(sNum, sDeltaSum, sDeltaSumSquared, &mean, &stddev); + myNS_MeanAndStdDev(sDeltaNum, sDeltaSum, sDeltaSumSquared, &mean, &stddev); - PR_LOG(gTimerLog, PR_LOG_DEBUG, ("sNum = %f, sDeltaSum = %f, sDeltaSumSquared = %f\n", sNum, sDeltaSum, sDeltaSumSquared)); + PR_LOG(gTimerLog, PR_LOG_DEBUG, ("sDeltaNum = %f, sDeltaSum = %f, sDeltaSumSquared = %f\n", sDeltaNum, sDeltaSum, sDeltaSumSquared)); PR_LOG(gTimerLog, PR_LOG_DEBUG, ("mean: %fms, stddev: %fms\n", mean, stddev)); } #endif @@ -162,9 +215,7 @@ NS_IMETHODIMP nsTimerImpl::Init(nsTimerCallbackFunc aFunc, SetDelayInternal(aDelay); - gThread->AddTimer(this); - - return NS_OK; + return gThread->AddTimer(this); } NS_IMETHODIMP nsTimerImpl::Init(nsITimerCallback *aCallback, @@ -184,9 +235,7 @@ NS_IMETHODIMP nsTimerImpl::Init(nsITimerCallback *aCallback, SetDelayInternal(aDelay); - gThread->AddTimer(this); - - return NS_OK; + return gThread->AddTimer(this); } NS_IMETHODIMP nsTimerImpl::Init(nsIObserver *aObserver, @@ -194,6 +243,9 @@ NS_IMETHODIMP nsTimerImpl::Init(nsIObserver *aObserver, PRUint32 aPriority, PRUint32 aType) { + if (!gThread) + return NS_ERROR_FAILURE; + SetDelayInternal(aDelay); mCallback.o = aObserver; @@ -203,18 +255,12 @@ NS_IMETHODIMP nsTimerImpl::Init(nsIObserver *aObserver, mPriority = (PRUint8)aPriority; mType = (PRUint8)aType; - if (!gThread) - return NS_ERROR_FAILURE; - - gThread->AddTimer(this); - - return NS_OK; + return gThread->AddTimer(this); } NS_IMETHODIMP nsTimerImpl::Cancel() { - mCancelled = PR_TRUE; - mClosure = nsnull; + mCanceled = PR_TRUE; if (gThread) gThread->RemoveTimer(this); @@ -245,7 +291,7 @@ NS_IMETHODIMP_(void) nsTimerImpl::SetType(PRUint32 aType) void nsTimerImpl::Process() { - if (mCancelled) + if (mCanceled) return; PRIntervalTime now = PR_IntervalNow(); @@ -256,7 +302,7 @@ void nsTimerImpl::Process() PRUint32 d = PR_IntervalToMilliseconds((a > b) ? a - b : b - a); // delta in ms sDeltaSum += d; sDeltaSumSquared += double(d) * double(d); - sNum++; + sDeltaNum++; PR_LOG(gTimerLog, PR_LOG_DEBUG, ("[this=%p] expected delay time %4dms\n", this, mDelay)); PR_LOG(gTimerLog, PR_LOG_DEBUG, ("[this=%p] actual delay time %4dms\n", this, PR_IntervalToMilliseconds(a))); @@ -278,15 +324,19 @@ void nsTimerImpl::Process() mFiring = PR_TRUE; - if (mCallback.c) { - if (mCallbackType == CALLBACK_TYPE_FUNC) - (*mCallback.c)(this, mClosure); - else if (mCallbackType == CALLBACK_TYPE_INTERFACE) + switch (mCallbackType) { + case CALLBACK_TYPE_FUNC: + mCallback.c(this, mClosure); + break; + case CALLBACK_TYPE_INTERFACE: mCallback.i->Notify(this); - else if (mCallbackType == CALLBACK_TYPE_OBSERVER) - mCallback.o->Observe((nsITimerScriptable *) this, - NS_TIMER_CALLBACK_TOPIC, nsnull); - /* else the timer has been canceled, and we shouldn't do anything */ + break; + case CALLBACK_TYPE_OBSERVER: + mCallback.o->Observe(NS_STATIC_CAST(nsIScriptableTimer *, this), + NS_TIMER_CALLBACK_TOPIC, + nsnull); + break; + default:; } mFiring = PR_FALSE; @@ -393,7 +443,7 @@ void nsTimerImpl::SetDelayInternal(PRUint32 aDelay) if (mTimeout < now) { // we overflowed mTimeout = PRIntervalTime(-1); } - + #ifdef DEBUG_TIMERS if (PR_LOG_TEST(gTimerLog, PR_LOG_DEBUG)) { if (mStart == 0) diff --git a/mozilla/xpcom/threads/nsTimerImpl.h b/mozilla/xpcom/threads/nsTimerImpl.h index ac84a847d3f..63060f98b1e 100644 --- a/mozilla/xpcom/threads/nsTimerImpl.h +++ b/mozilla/xpcom/threads/nsTimerImpl.h @@ -42,7 +42,7 @@ #include "nsITimer.h" #include "nsITimerCallback.h" -#include "nsITimerScriptable.h" +#include "nsIScriptableTimer.h" #include "nsIThread.h" @@ -73,7 +73,7 @@ enum { CALLBACK_TYPE_OBSERVER = 3 }; -class nsTimerImpl : public nsITimer, public nsITimerScriptable +class nsTimerImpl : public nsITimer, public nsIScriptableTimer { public: @@ -100,12 +100,12 @@ public: PRUint32 aType); NS_DECL_ISUPPORTS - NS_DECL_NSITIMERSCRIPTABLE + NS_DECL_NSISCRIPTABLETIMER NS_IMETHOD_(PRUint32) GetDelay() { return mDelay; } NS_IMETHOD_(void) SetDelay(PRUint32 aDelay); - NS_IMETHOD_(PRUint32) GetPriority() { return (PRUint32)mPriority; } + NS_IMETHOD_(PRUint32) GetPriority() { return mPriority; } NS_IMETHOD_(void) SetPriority(PRUint32 aPriority); NS_IMETHOD_(PRUint32) GetType() { return (PRUint32)mType; } @@ -114,9 +114,9 @@ public: NS_IMETHOD_(void*) GetClosure() { return mClosure; } private: - nsCOMPtr mCallingThread; + nsCOMPtr mCallingThread; - void * mClosure; + void * mClosure; union { nsTimerCallbackFunc c; @@ -124,21 +124,31 @@ private: nsIObserver * o; } mCallback; - PRUint8 mCallbackType; - PRUint8 mPriority; - PRUint8 mType; - PRUint8 mFiring; + // These members are set by Init (called from NS_NewTimer) and never reset. + PRUint8 mCallbackType; + PRUint8 mPriority; - PRBool mCancelled; + // These members are set by the initiating thread, when the timer's type is + // changed and during the period where it fires on that thread. + PRUint8 mType; + PRPackedBool mFiring; - PRUint32 mDelay; - PRIntervalTime mTimeout; + + // Use a PRBool (int) here to isolate loads and stores of these two members + // done on various threads under the protection of TimerThread::mLock, from + // loads and stores done on the initiating/type-changing/timer-firing thread + // to the above PRUint8/PRPackedBool members. + PRBool mArmed; + PRBool mCanceled; + + PRUint32 mDelay; + PRIntervalTime mTimeout; #ifdef DEBUG_TIMERS - PRIntervalTime mStart, mStart2; - static double sDeltaSum; - static double sDeltaSumSquared; - static double sNum; + PRIntervalTime mStart, mStart2; + static double sDeltaSum; + static double sDeltaSumSquared; + static double sDeltaNum; #endif }; diff --git a/mozilla/xpfe/components/updates/src/nsUpdateNotifier.js b/mozilla/xpfe/components/updates/src/nsUpdateNotifier.js index b206a17b722..3b81b3c34a5 100644 --- a/mozilla/xpfe/components/updates/src/nsUpdateNotifier.js +++ b/mozilla/xpfe/components/updates/src/nsUpdateNotifier.js @@ -77,11 +77,11 @@ var nsUpdateNotifier = { try { - const kITimerScriptable = Components.interfaces.nsITimerScriptable; + const kIScriptableTimer = Components.interfaces.nsIScriptableTimer; mTimer = Components.classes["@mozilla.org/timer;1"]. - createInstance(kITimerScriptable); - mTimer.init(this, kUpdateCheckDelay, kITimerScriptable.PRIORITY_NORMAL, - kITimerScriptable.TYPE_ONE_SHOT); + createInstance(kIScriptableTimer); + mTimer.init(this, kUpdateCheckDelay, kIScriptableTimer.PRIORITY_NORMAL, + kIScriptableTimer.TYPE_ONE_SHOT); // we are no longer interested in the ``domwindowopened'' topic var observerService = Components.