Bug 1383019: Move more logic under the protection of nsTimerImpl::mMutex, and simplify. r=froydnj, a=abillings
authorByron Campen [:bwc] <docfaraday@gmail.com>
Thu, 14 Sep 2017 12:19:24 -0500
changeset 431552 2c443551b3cc3d280a89a300954abb7374b72309
parent 431466 c041f734d59f73ad77d0efc511e3b16b3860ac6c
child 431553 789e21a08030b00acd832a58268680dede2e7a1b
push id7784
push userryanvm@gmail.com
push dateThu, 21 Sep 2017 00:40:13 +0000
treeherdermozilla-beta@efff4f307675 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersfroydnj, abillings
bugs1383019
milestone57.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1383019: Move more logic under the protection of nsTimerImpl::mMutex, and simplify. r=froydnj, a=abillings MozReview-Commit-ID: JjYScKwyika
xpcom/threads/TimerThread.cpp
xpcom/threads/TimerThread.h
xpcom/threads/nsTimerImpl.cpp
xpcom/threads/nsTimerImpl.h
--- a/xpcom/threads/TimerThread.cpp
+++ b/xpcom/threads/TimerThread.cpp
@@ -326,17 +326,17 @@ nsresult
 TimerThread::Shutdown()
 {
   MOZ_LOG(GetTimerLog(), LogLevel::Debug, ("TimerThread::Shutdown begin\n"));
 
   if (!mThread) {
     return NS_ERROR_NOT_INITIALIZED;
   }
 
-  nsTArray<UniquePtr<Entry>> timers;
+  nsTArray<RefPtr<nsTimerImpl>> timers;
   {
     // lock scope
     MonitorAutoLock lock(mMonitor);
 
     mShutdown = true;
 
     // notify the cond var so that Run() can return
     if (mWaiting) {
@@ -345,22 +345,24 @@ TimerThread::Shutdown()
     }
 
     // Need to copy content of mTimers array to a local array
     // because call to timers' Cancel() (and release its self)
     // must not be done under the lock. Destructor of a callback
     // might potentially call some code reentering the same lock
     // that leads to unexpected behavior or deadlock.
     // See bug 422472.
-    mTimers.SwapElements(timers);
+    for (const UniquePtr<Entry>& entry : mTimers) {
+      timers.AppendElement(entry->Take());
+    }
+
+    mTimers.Clear();
   }
 
-  uint32_t timersCount = timers.Length();
-  for (uint32_t i = 0; i < timersCount; i++) {
-    RefPtr<nsTimerImpl> timer = timers[i]->Take();
+  for (const RefPtr<nsTimerImpl>& timer : timers) {
     if (timer) {
       timer->Cancel();
     }
   }
 
   mThread->Shutdown();    // wait for the thread to die
 
   nsTimerEvent::Shutdown();
--- a/xpcom/threads/TimerThread.h
+++ b/xpcom/threads/TimerThread.h
@@ -124,41 +124,9 @@ private:
       return mTimeout;
     }
   };
 
   nsTArray<mozilla::UniquePtr<Entry>> mTimers;
   uint32_t mAllowedEarlyFiringMicroseconds;
 };
 
-struct TimerAdditionComparator
-{
-  TimerAdditionComparator(const mozilla::TimeStamp& aNow,
-                          nsTimerImpl* aTimerToInsert) :
-    now(aNow)
-#ifdef DEBUG
-    , timerToInsert(aTimerToInsert)
-#endif
-  {
-  }
-
-  bool LessThan(nsTimerImpl* aFromArray, nsTimerImpl* aNewTimer) const
-  {
-    MOZ_ASSERT(aNewTimer == timerToInsert, "Unexpected timer ordering");
-
-    // Skip any overdue timers.
-    return aFromArray->mTimeout <= now ||
-           aFromArray->mTimeout <= aNewTimer->mTimeout;
-  }
-
-  bool Equals(nsTimerImpl* aFromArray, nsTimerImpl* aNewTimer) const
-  {
-    return false;
-  }
-
-private:
-  const mozilla::TimeStamp& now;
-#ifdef DEBUG
-  const nsTimerImpl* const timerToInsert;
-#endif
-};
-
 #endif /* TimerThread_h___ */
--- a/xpcom/threads/nsTimerImpl.cpp
+++ b/xpcom/threads/nsTimerImpl.cpp
@@ -128,26 +128,18 @@ NS_IMPL_ADDREF(nsTimer)
 
 NS_IMETHODIMP_(MozExternalRefCountType)
 nsTimer::Release(void)
 {
   nsrefcnt count = --mRefCnt;
   NS_LOG_RELEASE(this, count, "nsTimer");
 
   if (count == 1) {
-    if (!mImpl->CancelCheckIfFiring()) {
-      // Last ref, in nsTimerImpl::mITimer. Make sure the cycle is broken.
-      // (when Cancel fails, nsTimerImpl::Fire is in progress, which has grabbed
-      // another ref to the nsITimer since we checked the value of mRefCnt
-      // above)
-      // If there is a nsTimerEvent in a queue for this timer, the nsTimer will
-      // live until that event pops, otherwise the nsTimerImpl will go away and
-      // the nsTimer along with it.
-      mImpl = nullptr;
-    }
+    // Last ref, in nsTimerImpl::mITimer. Make sure the cycle is broken.
+    mImpl->CancelImpl(true);
   } else if (count == 0) {
     delete this;
   }
 
   return count;
 }
 
 nsTimerImpl::nsTimerImpl(nsITimer* aTimer) :
@@ -317,43 +309,49 @@ nsTimerImpl::Init(nsIObserver* aObserver
   cb.mType = Callback::Type::Observer;
   cb.mCallback.o = aObserver;
   NS_ADDREF(cb.mCallback.o);
 
   MutexAutoLock lock(mMutex);
   return InitCommon(aDelay, aType, mozilla::Move(cb));
 }
 
-bool
-nsTimerImpl::CancelCheckIfFiring()
-{
-  Callback cb;
-
-  MutexAutoLock lock(mMutex);
-
-  if (gThread) {
-    gThread->RemoveTimer(this);
-  }
-
-  cb.swap(mCallback);
-  ++mGeneration;
-
-  if (mCallbackDuringFire.mType != Callback::Type::Unknown) {
-    return true;
-  }
-  return false;
-}
-
 nsresult
 nsTimerImpl::Cancel()
 {
-  (void)CancelCheckIfFiring();
+  CancelImpl(false);
   return NS_OK;
 }
 
+void
+nsTimerImpl::CancelImpl(bool aClearITimer)
+{
+  Callback cbTrash;
+  RefPtr<nsITimer> timerTrash;
+
+  {
+    MutexAutoLock lock(mMutex);
+    if (gThread) {
+      gThread->RemoveTimer(this);
+    }
+
+    cbTrash.swap(mCallback);
+    ++mGeneration;
+
+    // Don't clear this if we're firing; once Fire returns, we'll get this call
+    // again.
+    if (aClearITimer &&
+        (mCallbackDuringFire.mType == Callback::Type::Unknown)) {
+      MOZ_RELEASE_ASSERT(mITimer, "mITimer was nulled already! "
+          "This indicates that someone has messed up the refcount on nsTimer!");
+      timerTrash.swap(mITimer);
+    }
+  }
+}
+
 nsresult
 nsTimerImpl::SetDelay(uint32_t aDelay)
 {
   MutexAutoLock lock(mMutex);
   if (GetCallback().mType == Callback::Type::Unknown && !IsRepeating()) {
     // This may happen if someone tries to re-use a one-shot timer
     // by re-setting delay instead of reinitializing the timer.
     NS_ERROR("nsITimer->SetDelay() called when the "
--- a/xpcom/threads/nsTimerImpl.h
+++ b/xpcom/threads/nsTimerImpl.h
@@ -51,17 +51,17 @@ public:
   explicit nsTimerImpl(nsITimer* aTimer);
   NS_INLINE_DECL_THREADSAFE_REFCOUNTING(nsTimerImpl)
   NS_DECL_NON_VIRTUAL_NSITIMER
 
   static nsresult Startup();
   static void Shutdown();
 
   void SetDelayInternal(uint32_t aDelay, TimeStamp aBase = TimeStamp::Now());
-  bool CancelCheckIfFiring();
+  void CancelImpl(bool aClearITimer);
 
   void Fire(int32_t aGeneration);
 
 #ifdef MOZ_TASK_TRACER
   void GetTLSTraceInfo();
   mozilla::tasktracer::TracedTaskCommon GetTracedTask();
 #endif
 
@@ -206,31 +206,30 @@ public:
 
 #ifdef MOZ_TASK_TRACER
   mozilla::tasktracer::TracedTaskCommon mTracedTask;
 #endif
 
   static double         sDeltaSum;
   static double         sDeltaSumSquared;
   static double         sDeltaNum;
-  const RefPtr<nsITimer>      mITimer;
+  RefPtr<nsITimer>      mITimer;
   mozilla::Mutex mMutex;
   Callback              mCallback;
   Callback              mCallbackDuringFire;
 };
 
 class nsTimer final : public nsITimer
 {
   virtual ~nsTimer();
 public:
   nsTimer() : mImpl(new nsTimerImpl(this)) {}
 
   friend class TimerThread;
   friend class nsTimerEvent;
-  friend struct TimerAdditionComparator;
 
   NS_DECL_THREADSAFE_ISUPPORTS
   NS_FORWARD_SAFE_NSITIMER(mImpl);
 
   virtual size_t SizeOfIncludingThis(mozilla::MallocSizeOf aMallocSizeOf) const override;
 
 private:
   // nsTimerImpl holds a strong ref to us. When our refcount goes to 1, we will