[xpcom] Timer cleanup, assertions and comments.

4 files changed, 24 insertions, 7 deletions

diff --git a/xpcom/threads/TimerThread.cpp b/xpcom/threads/TimerThread.cpp
index 36168e373..a2e40c388 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<nsTimerImpl> 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 4d4b7be77..c82f3ec7f 100644
--- a/xpcom/threads/TimerThread.h
+++ b/xpcom/threads/TimerThread.h
@@ -69,6 +69,10 @@ private:
   already_AddRefed<nsTimerImpl> PostTimerEvent(already_AddRefed<nsTimerImpl> aTimerRef);
 
   nsCOMPtr<nsIThread> 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 3ca54fd61..1a200a6b3 100644
--- a/xpcom/threads/nsTimerImpl.cpp
+++ b/xpcom/threads/nsTimerImpl.cpp
@@ -443,6 +443,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 9d59b4e67..039294995 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