diff --git a/xpcom/threads/TimerThread.cpp b/xpcom/threads/TimerThread.cpp index 36168e3734..a2e40c3880 100644 --- a/xpcom/threads/TimerThread.cpp +++ b/xpcom/threads/TimerThread.cpp @@ -623,10 +623,13 @@ TimerThread::AddTimerInternal(nsTimerImpl* aTimer) return insertSlot - mTimers.Elements(); } +// This function must be called from within a lock. +// Also: we hold the mutex for the nsTimerImpl. bool TimerThread::RemoveTimerInternal(nsTimerImpl* aTimer) { mMonitor.AssertCurrentThreadOwns(); + aTimer->mMutex.AssertCurrentThreadOwns(); if (!mTimers.RemoveElement(aTimer)) { return false; } @@ -690,12 +693,17 @@ TimerThread::PostTimerEvent(already_AddRefed aTimerRef) // at the TimerThread we'll deadlock. MonitorAutoUnlock unlock(mMonitor); rv = target->Dispatch(event, NS_DISPATCH_NORMAL); - } - - if (NS_FAILED(rv)) { - timer = event->ForgetTimer(); - RemoveTimerInternal(timer); - return timer.forget(); + if (NS_FAILED(rv)) { + timer = event->ForgetTimer(); + // We do this to avoid possible deadlock by taking the two locks in a + // different order than is used in RemoveTimer(). RemoveTimer() has + // aTimer->mMutex first. We use timer.get() to keep static analysis + // happy. + MutexAutoLock lock1(timer.get()->mMutex); + MonitorAutoLock lock2(mMonitor); + RemoveTimerInternal(timer.get()); + return timer.forget(); + } } return nullptr; diff --git a/xpcom/threads/TimerThread.h b/xpcom/threads/TimerThread.h index 4d4b7be77d..c82f3ec7f5 100644 --- a/xpcom/threads/TimerThread.h +++ b/xpcom/threads/TimerThread.h @@ -69,6 +69,10 @@ private: already_AddRefed PostTimerEvent(already_AddRefed aTimerRef); nsCOMPtr mThread; + // Lock ordering requirements: + // (optional) ThreadWrapper::sMutex -> + // (optional) nsTimerImpl::mMutex -> + // TimerThread::mMonitor Monitor mMonitor; bool mShutdown; diff --git a/xpcom/threads/nsTimerImpl.cpp b/xpcom/threads/nsTimerImpl.cpp index 9e58ed0232..2a3531a858 100644 --- a/xpcom/threads/nsTimerImpl.cpp +++ b/xpcom/threads/nsTimerImpl.cpp @@ -441,6 +441,8 @@ nsTimerImpl::Fire(int32_t aGeneration) // Don't fire callbacks or fiddle with refcounts when the mutex is locked. // If some other thread Cancels/Inits after this, they're just too late. if (aGeneration != mGeneration) { + // This timer got rescheduled or cancelled before we fired, so ignore this + // firing return; } diff --git a/xpcom/threads/nsTimerImpl.h b/xpcom/threads/nsTimerImpl.h index 9d59b4e672..039294995d 100644 --- a/xpcom/threads/nsTimerImpl.h +++ b/xpcom/threads/nsTimerImpl.h @@ -166,7 +166,10 @@ public: int32_t mGeneration; uint32_t mDelay; - // Updated only after this timer has been removed from the timer thread. + // Never updated while in the TimerThread's timer list. Only updated + // before adding to that list or during nsTimerImpl::Fire(), when it has + // been removed from the TimerThread's list. TimerThread can safely access + // mTimeout of any timer in the list. TimeStamp mTimeout; #ifdef MOZ_TASK_TRACER