[xpcom] Timer cleanup, assertions and comments.

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;

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;

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;
     }
 

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