Bug 1328643 - Add some locking to prevent races caused by Cancel/Init from threads other than the target. (build fixed) r=froydnj, a=jcristau FIREFOX_52_0b5_BUILD1 FIREFOX_52_0b5_RELEASE
authorByron Campen [:bwc] <docfaraday@gmail.com>
Wed, 11 Jan 2017 13:59:19 -0600
changeset 482945 49b3ad9f467d48194dab8121f82e4c938b70b484
parent 482944 cbfc87ea5508b744abc2e2909cddd01840877236
child 482946 b6e8246f967b9d786e2b15a53616a90cdc791c93
push id45184
push userbmo:ralin@mozilla.com
push dateMon, 13 Feb 2017 16:26:39 +0000
reviewersfroydnj, jcristau
bugs1328643
milestone52.0
Bug 1328643 - Add some locking to prevent races caused by Cancel/Init from threads other than the target. (build fixed) r=froydnj, a=jcristau MozReview-Commit-ID: FdAPTGDNKup
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
@@ -270,28 +270,24 @@ nsTimerEvent::DeleteAllocatorIfNeeded()
 NS_IMETHODIMP
 nsTimerEvent::Run()
 {
   if (!mTimer) {
     MOZ_ASSERT(false);
     return NS_OK;
   }
 
-  if (mGeneration != mTimer->GetGeneration()) {
-    return NS_OK;
-  }
-
   if (MOZ_LOG_TEST(GetTimerLog(), LogLevel::Debug)) {
     TimeStamp now = TimeStamp::Now();
     MOZ_LOG(GetTimerLog(), LogLevel::Debug,
            ("[this=%p] time between PostTimerEvent() and Fire(): %fms\n",
             this, (now - mInitTime).ToMilliseconds()));
   }
 
-  mTimer->Fire();
+  mTimer->Fire(mGeneration);
 
   // We call Cancel() to correctly release mTimer.
   // Read more in the Cancel() implementation.
   return Cancel();
 }
 
 nsresult
 TimerThread::Init()
@@ -360,29 +356,29 @@ TimerThread::Shutdown()
 
     // notify the cond var so that Run() can return
     if (mWaiting) {
       mNotified = true;
       mMonitor.Notify();
     }
 
     // Need to copy content of mTimers array to a local array
-    // because call to timers' ReleaseCallback() (and release its self)
+    // 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.
     timers.AppendElements(mTimers);
     mTimers.Clear();
   }
 
   uint32_t timersCount = timers.Length();
   for (uint32_t i = 0; i < timersCount; i++) {
     nsTimerImpl* timer = timers[i];
-    timer->ReleaseCallback();
+    timer->Cancel();
     ReleaseTimerInternal(timer);
   }
 
   mThread->Shutdown();    // wait for the thread to die
 
   nsTimerEvent::Shutdown();
 
   MOZ_LOG(GetTimerLog(), LogLevel::Debug, ("TimerThread::Shutdown end\n"));
@@ -585,31 +581,27 @@ TimerThread::AddTimer(nsTimerImpl* aTime
     mNotified = true;
     mMonitor.Notify();
   }
 
   return NS_OK;
 }
 
 nsresult
-TimerThread::RemoveTimer(nsTimerImpl* aTimer, bool aDisable)
+TimerThread::RemoveTimer(nsTimerImpl* aTimer)
 {
   MonitorAutoLock lock(mMonitor);
 
   // Remove the timer from our array.  Tell callers that aTimer was not found
   // by returning NS_ERROR_NOT_AVAILABLE.
 
   if (!RemoveTimerInternal(aTimer)) {
     return NS_ERROR_NOT_AVAILABLE;
   }
 
-  if (aDisable) {
-    aTimer->mEventTarget = nullptr;
-  }
-
   // Awaken the timer thread.
   if (mWaiting) {
     mNotified = true;
     mMonitor.Notify();
   }
 
   return NS_OK;
 }
--- a/xpcom/threads/TimerThread.h
+++ b/xpcom/threads/TimerThread.h
@@ -39,17 +39,17 @@ public:
   NS_DECL_THREADSAFE_ISUPPORTS
   NS_DECL_NSIRUNNABLE
   NS_DECL_NSIOBSERVER
 
   nsresult Init();
   nsresult Shutdown();
 
   nsresult AddTimer(nsTimerImpl* aTimer);
-  nsresult RemoveTimer(nsTimerImpl* aTimer, bool aDisable=false);
+  nsresult RemoveTimer(nsTimerImpl* aTimer);
 
   void DoBeforeSleep();
   void DoAfterSleep();
 
   bool IsOnTimerThread() const
   {
     return mThread == NS_GetCurrentThread();
   }
--- a/xpcom/threads/nsTimerImpl.cpp
+++ b/xpcom/threads/nsTimerImpl.cpp
@@ -28,17 +28,16 @@ using namespace mozilla::tasktracer;
 #include <unistd.h>
 #endif
 
 using mozilla::Atomic;
 using mozilla::LogLevel;
 using mozilla::TimeDuration;
 using mozilla::TimeStamp;
 
-static Atomic<int32_t>  gGenerator;
 static TimerThread*     gThread = nullptr;
 
 // This module prints info about the precision of timers.
 static mozilla::LazyLogModule sTimerLog("nsTimerImpl");
 
 mozilla::LogModule*
 GetTimerLog()
 {
@@ -47,17 +46,17 @@ GetTimerLog()
 
 // This module prints info about which timers are firing, which is useful for
 // wakeups for the purposes of power profiling. Set the following environment
 // variable before starting the browser.
 //
 //   MOZ_LOG=TimerFirings:4
 //
 // Then a line will be printed for every timer that fires. The name used for a
-// |CallbackType::Function| timer depends on the circumstances.
+// |Callback::Type::Function| timer depends on the circumstances.
 //
 // - If it was explicitly named (e.g. it was initialized with
 //   InitWithNamedFuncCallback()) then that explicit name will be shown.
 //
 // - Otherwise, if we are on a platform that supports function name lookup
 //   (Mac or Linux) then the looked-up name will be shown with a
 //   "[from dladdr]" annotation. On Mac the looked-up name will be immediately
 //   useful. On Linux it'll need post-processing with
@@ -121,44 +120,34 @@ nsTimer::Release(void)
   nsrefcnt count = --mRefCnt;
   NS_LOG_RELEASE(this, count, "nsTimer");
 
   if (count == 1) {
     // Last ref, held by nsTimerImpl. Make sure the cycle is broken.
     // 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->Neuter();
+    mImpl->Cancel();
     mImpl = nullptr;
   } else if (count == 0) {
     delete this;
   }
 
   return count;
 }
 
 nsTimerImpl::nsTimerImpl(nsITimer* aTimer) :
-  mClosure(nullptr),
-  mName(nsTimerImpl::Nothing),
-  mCallbackType(CallbackType::Unknown),
   mGeneration(0),
   mDelay(0),
-  mITimer(aTimer)
+  mITimer(aTimer),
+  mMutex("nsTimerImpl::mMutex")
 {
   MOZ_COUNT_CTOR(nsTimerImpl);
   // XXXbsmedberg: shouldn't this be in Init()?
   mEventTarget = static_cast<nsIEventTarget*>(NS_GetCurrentThread());
-
-  mCallback.c = nullptr;
-}
-
-nsTimerImpl::~nsTimerImpl()
-{
-  MOZ_COUNT_DTOR(nsTimerImpl);
-  ReleaseCallback();
 }
 
 //static
 nsresult
 nsTimerImpl::Startup()
 {
   nsresult rv;
 
@@ -195,376 +184,385 @@ nsTimerImpl::Shutdown()
   gThread->Shutdown();
   NS_RELEASE(gThread);
 }
 
 
 nsresult
 nsTimerImpl::InitCommon(uint32_t aDelay, uint32_t aType)
 {
+  mMutex.AssertCurrentThreadOwns();
   nsresult rv;
 
   if (NS_WARN_IF(!gThread)) {
     return NS_ERROR_NOT_INITIALIZED;
   }
   if (!mEventTarget) {
     NS_ERROR("mEventTarget is NULL");
     return NS_ERROR_NOT_INITIALIZED;
   }
 
   rv = gThread->Init();
   if (NS_WARN_IF(NS_FAILED(rv))) {
     return rv;
   }
 
   gThread->RemoveTimer(this);
-  mTimeout = TimeStamp();
-  mGeneration = gGenerator++;
+  ++mGeneration;
 
   mType = (uint8_t)aType;
-  SetDelayInternal(aDelay);
+  mDelay = aDelay;
+  mTimeout = TimeStamp::Now() + TimeDuration::FromMilliseconds(mDelay);
 
   return gThread->AddTimer(this);
 }
 
 nsresult
 nsTimerImpl::InitWithFuncCallbackCommon(nsTimerCallbackFunc aFunc,
                                         void* aClosure,
                                         uint32_t aDelay,
                                         uint32_t aType,
-                                        Name aName)
+                                        Callback::Name aName)
 {
   if (NS_WARN_IF(!aFunc)) {
     return NS_ERROR_INVALID_ARG;
   }
 
-  ReleaseCallback();
-  mCallbackType = CallbackType::Function;
-  mCallback.c = aFunc;
-  mClosure = aClosure;
-  mName = aName;
+  Callback cb; // Goes out of scope after the unlock, prevents deadlock
+  cb.mType = Callback::Type::Function;
+  cb.mCallback.c = aFunc;
+  cb.mClosure = aClosure;
+  cb.mName = aName;
+
+  MutexAutoLock lock(mMutex);
+  cb.swap(mCallback);
 
   return InitCommon(aDelay, aType);
 }
 
 NS_IMETHODIMP
 nsTimerImpl::InitWithFuncCallback(nsTimerCallbackFunc aFunc,
                                   void* aClosure,
                                   uint32_t aDelay,
                                   uint32_t aType)
 {
-  Name name(nsTimerImpl::Nothing);
+  Callback::Name name(Callback::Nothing);
   return InitWithFuncCallbackCommon(aFunc, aClosure, aDelay, aType, name);
 }
 
 NS_IMETHODIMP
 nsTimerImpl::InitWithNamedFuncCallback(nsTimerCallbackFunc aFunc,
                                        void* aClosure,
                                        uint32_t aDelay,
                                        uint32_t aType,
                                        const char* aNameString)
 {
-  Name name(aNameString);
+  Callback::Name name(aNameString);
   return InitWithFuncCallbackCommon(aFunc, aClosure, aDelay, aType, name);
 }
 
 NS_IMETHODIMP
 nsTimerImpl::InitWithNameableFuncCallback(nsTimerCallbackFunc aFunc,
                                           void* aClosure,
                                           uint32_t aDelay,
                                           uint32_t aType,
                                           nsTimerNameCallbackFunc aNameFunc)
 {
-  Name name(aNameFunc);
+  Callback::Name name(aNameFunc);
   return InitWithFuncCallbackCommon(aFunc, aClosure, aDelay, aType, name);
 }
 
 NS_IMETHODIMP
 nsTimerImpl::InitWithCallback(nsITimerCallback* aCallback,
                               uint32_t aDelay,
                               uint32_t aType)
 {
   if (NS_WARN_IF(!aCallback)) {
     return NS_ERROR_INVALID_ARG;
   }
 
-  ReleaseCallback();
-  mCallbackType = CallbackType::Interface;
-  mCallback.i = aCallback;
-  NS_ADDREF(mCallback.i);
+  Callback cb; // Goes out of scope after the unlock, prevents deadlock
+  cb.mType = Callback::Type::Interface;
+  cb.mCallback.i = aCallback;
+  NS_ADDREF(cb.mCallback.i);
+
+  MutexAutoLock lock(mMutex);
+  cb.swap(mCallback);
 
   return InitCommon(aDelay, aType);
 }
 
 NS_IMETHODIMP
 nsTimerImpl::Init(nsIObserver* aObserver, uint32_t aDelay, uint32_t aType)
 {
   if (NS_WARN_IF(!aObserver)) {
     return NS_ERROR_INVALID_ARG;
   }
 
-  ReleaseCallback();
-  mCallbackType = CallbackType::Observer;
-  mCallback.o = aObserver;
-  NS_ADDREF(mCallback.o);
+  Callback cb; // Goes out of scope after the unlock, prevents deadlock
+  cb.mType = Callback::Type::Observer;
+  cb.mCallback.o = aObserver;
+  NS_ADDREF(cb.mCallback.o);
+
+  MutexAutoLock lock(mMutex);
+  cb.swap(mCallback);
 
   return InitCommon(aDelay, aType);
 }
 
 NS_IMETHODIMP
 nsTimerImpl::Cancel()
 {
+  Callback cb;
+
+  MutexAutoLock lock(mMutex);
 
   if (gThread) {
     gThread->RemoveTimer(this);
   }
 
-  ReleaseCallback();
+  cb.swap(mCallback);
+  ++mGeneration;
 
   return NS_OK;
 }
 
-void
-nsTimerImpl::Neuter()
-{
-  if (gThread) {
-    gThread->RemoveTimer(this, true);
-  }
-
-  // If invoked on the target thread, guarantees that the timer doesn't pop.
-  // If invoked anywhere else, it might prevent the timer from popping, but
-  // no guarantees. In any case, the above RemoveTimer call will prevent it
-  // from being rescheduled.
-  ++mGeneration;
-}
-
 NS_IMETHODIMP
 nsTimerImpl::SetDelay(uint32_t aDelay)
 {
-  if (mCallbackType == CallbackType::Unknown && mType == nsITimer::TYPE_ONE_SHOT) {
+  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 "
              "one-shot timer is not set up.");
     return NS_ERROR_NOT_INITIALIZED;
   }
 
   bool reAdd = false;
   if (gThread) {
     reAdd = NS_SUCCEEDED(gThread->RemoveTimer(this));
   }
 
-  SetDelayInternal(aDelay);
+  mDelay = aDelay;
+  mTimeout = TimeStamp::Now() + TimeDuration::FromMilliseconds(mDelay);
 
   if (reAdd) {
     gThread->AddTimer(this);
   }
 
   return NS_OK;
 }
 
 NS_IMETHODIMP
 nsTimerImpl::GetDelay(uint32_t* aDelay)
 {
+  MutexAutoLock lock(mMutex);
   *aDelay = mDelay;
   return NS_OK;
 }
 
 NS_IMETHODIMP
 nsTimerImpl::SetType(uint32_t aType)
 {
+  MutexAutoLock lock(mMutex);
   mType = (uint8_t)aType;
   // XXX if this is called, we should change the actual type.. this could effect
   // repeating timers.  we need to ensure in Fire() that if mType has changed
   // during the callback that we don't end up with the timer in the queue twice.
   return NS_OK;
 }
 
 NS_IMETHODIMP
 nsTimerImpl::GetType(uint32_t* aType)
 {
+  MutexAutoLock lock(mMutex);
   *aType = mType;
   return NS_OK;
 }
 
 
 NS_IMETHODIMP
 nsTimerImpl::GetClosure(void** aClosure)
 {
-  *aClosure = mClosure;
+  MutexAutoLock lock(mMutex);
+  *aClosure = GetCallback().mClosure;
   return NS_OK;
 }
 
 
 NS_IMETHODIMP
 nsTimerImpl::GetCallback(nsITimerCallback** aCallback)
 {
-  if (mCallbackType == CallbackType::Interface) {
-    NS_IF_ADDREF(*aCallback = mCallback.i);
+  MutexAutoLock lock(mMutex);
+  if (GetCallback().mType == Callback::Type::Interface) {
+    NS_IF_ADDREF(*aCallback = GetCallback().mCallback.i);
   } else {
     *aCallback = nullptr;
   }
 
   return NS_OK;
 }
 
 
 NS_IMETHODIMP
 nsTimerImpl::GetTarget(nsIEventTarget** aTarget)
 {
+  MutexAutoLock lock(mMutex);
   NS_IF_ADDREF(*aTarget = mEventTarget);
   return NS_OK;
 }
 
 
 NS_IMETHODIMP
 nsTimerImpl::SetTarget(nsIEventTarget* aTarget)
 {
-  if (NS_WARN_IF(mCallbackType != CallbackType::Unknown)) {
+  MutexAutoLock lock(mMutex);
+  if (NS_WARN_IF(mCallback.mType != Callback::Type::Unknown)) {
     return NS_ERROR_ALREADY_INITIALIZED;
   }
 
   if (aTarget) {
     mEventTarget = aTarget;
   } else {
     mEventTarget = static_cast<nsIEventTarget*>(NS_GetCurrentThread());
   }
   return NS_OK;
 }
 
 
 void
-nsTimerImpl::Fire()
+nsTimerImpl::Fire(int32_t aGeneration)
 {
-  if (mCallbackType == CallbackType::Unknown) {
-    return;
+  uint8_t oldType;
+  uint32_t oldDelay;
+  TimeStamp oldTimeout;
+
+  {
+    // 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.
+    MutexAutoLock lock(mMutex);
+    if (aGeneration != mGeneration) {
+      return;
+    }
+
+    mCallbackDuringFire.swap(mCallback);
+    oldType = mType;
+    oldDelay = mDelay;
+    oldTimeout = mTimeout;
   }
 
   PROFILER_LABEL("Timer", "Fire",
                  js::ProfileEntry::Category::OTHER);
 
   TimeStamp now = TimeStamp::Now();
   if (MOZ_LOG_TEST(GetTimerLog(), LogLevel::Debug)) {
-    TimeDuration   a = now - mStart; // actual delay in intervals
-    TimeDuration   b = TimeDuration::FromMilliseconds(mDelay); // expected delay in intervals
-    TimeDuration   delta = (a > b) ? a - b : b - a;
-    uint32_t       d = delta.ToMilliseconds(); // delta in ms
-    sDeltaSum += d;
+    TimeDuration   delta = now - oldTimeout;
+    int32_t       d = delta.ToMilliseconds(); // delta in ms
+    sDeltaSum += abs(d);
     sDeltaSumSquared += double(d) * double(d);
     sDeltaNum++;
 
     MOZ_LOG(GetTimerLog(), LogLevel::Debug,
-           ("[this=%p] expected delay time %4ums\n", this, mDelay));
+           ("[this=%p] expected delay time %4ums\n", this, oldDelay));
     MOZ_LOG(GetTimerLog(), LogLevel::Debug,
-           ("[this=%p] actual delay time   %fms\n", this,
-            a.ToMilliseconds()));
+           ("[this=%p] actual delay time   %4dms\n", this, oldDelay + d));
     MOZ_LOG(GetTimerLog(), LogLevel::Debug,
-           ("[this=%p] (mType is %d)       -------\n", this, mType));
+           ("[this=%p] (mType is %d)       -------\n", this, oldType));
     MOZ_LOG(GetTimerLog(), LogLevel::Debug,
-           ("[this=%p]     delta           %4dms\n",
-            this, (a > b) ? (int32_t)d : -(int32_t)d));
-
-    mStart = mStart2;
-    mStart2 = TimeStamp();
+           ("[this=%p]     delta           %4dms\n", this, d));
   }
 
   if (MOZ_LOG_TEST(GetTimerFiringsLog(), LogLevel::Debug)) {
-    LogFiring(mCallbackType, mCallback);
+    LogFiring(mCallbackDuringFire, oldType, oldDelay);
   }
 
-  int32_t oldGeneration = mGeneration;
-
-  switch (mCallbackType) {
-    case CallbackType::Function:
-      mCallback.c(mITimer, mClosure);
+  switch (mCallbackDuringFire.mType) {
+    case Callback::Type::Function:
+      mCallbackDuringFire.mCallback.c(mITimer, mCallbackDuringFire.mClosure);
       break;
-    case CallbackType::Interface:
-      {
-        nsCOMPtr<nsITimerCallback> keepalive(mCallback.i);
-        keepalive->Notify(mITimer);
-      }
+    case Callback::Type::Interface:
+      mCallbackDuringFire.mCallback.i->Notify(mITimer);
       break;
-    case CallbackType::Observer:
-      {
-        nsCOMPtr<nsIObserver> keepalive(mCallback.o);
-        keepalive->Observe(mITimer,
-                           NS_TIMER_CALLBACK_TOPIC,
-                           nullptr);
-      }
+    case Callback::Type::Observer:
+      mCallbackDuringFire.mCallback.o->Observe(mITimer, NS_TIMER_CALLBACK_TOPIC,
+                                               nullptr);
       break;
     default:
       ;
   }
 
-  if (oldGeneration == mGeneration && mCallbackType != CallbackType::Unknown) {
-    // Timer has not been re-init or canceled; clear out one-shot timers, and
-    // re-schedule repeating timers.
-    if (IsRepeating()) {
-      if (mType == nsITimer::TYPE_REPEATING_SLACK) {
-        SetDelayInternal(mDelay);
-      } else {
-        SetDelayInternal(mDelay, mTimeout);
-      }
-      if (gThread) {
-        gThread->AddTimer(this);
-      }
+  Callback trash; // Swap into here to dispose of callback after the unlock
+  MutexAutoLock lock(mMutex);
+  if (aGeneration == mGeneration && IsRepeating()) {
+    // Repeating timer has not been re-init or canceled; reschedule
+    mCallbackDuringFire.swap(mCallback);
+    TimeDuration delay = TimeDuration::FromMilliseconds(mDelay);
+    if (mType == nsITimer::TYPE_REPEATING_SLACK) {
+      mTimeout = TimeStamp::Now() + delay;
     } else {
-      ReleaseCallback();
+      mTimeout = mTimeout + delay;
+    }
+    if (gThread) {
+      gThread->AddTimer(this);
     }
   }
 
+  mCallbackDuringFire.swap(trash);
+
   MOZ_LOG(GetTimerLog(), LogLevel::Debug,
          ("[this=%p] Took %fms to fire timer callback\n",
           this, (TimeStamp::Now() - now).ToMilliseconds()));
 }
 
 #if defined(HAVE_DLADDR) && defined(HAVE___CXA_DEMANGLE)
 #define USE_DLADDR 1
 #endif
 
 #ifdef USE_DLADDR
 #include <cxxabi.h>
 #include <dlfcn.h>
 #endif
 
 // See the big comment above GetTimerFiringsLog() to understand this code.
 void
-nsTimerImpl::LogFiring(CallbackType aCallbackType, CallbackUnion aCallback)
+nsTimerImpl::LogFiring(const Callback& aCallback, uint8_t aType, uint32_t aDelay)
 {
   const char* typeStr;
-  switch (mType) {
+  switch (aType) {
     case nsITimer::TYPE_ONE_SHOT:                   typeStr = "ONE_SHOT"; break;
     case nsITimer::TYPE_REPEATING_SLACK:            typeStr = "SLACK   "; break;
     case nsITimer::TYPE_REPEATING_PRECISE:          /* fall through */
     case nsITimer::TYPE_REPEATING_PRECISE_CAN_SKIP: typeStr = "PRECISE "; break;
     default:                              MOZ_CRASH("bad type");
   }
 
-  switch (aCallbackType) {
-    case CallbackType::Function: {
+  switch (aCallback.mType) {
+    case Callback::Type::Function: {
       bool needToFreeName = false;
       const char* annotation = "";
       const char* name;
       static const size_t buflen = 1024;
       char buf[buflen];
 
-      if (mName.is<NameString>()) {
-        name = mName.as<NameString>();
+      if (aCallback.mName.is<Callback::NameString>()) {
+        name = aCallback.mName.as<Callback::NameString>();
 
-      } else if (mName.is<NameFunc>()) {
-        mName.as<NameFunc>()(mITimer, mClosure, buf, buflen);
+      } else if (aCallback.mName.is<Callback::NameFunc>()) {
+        aCallback.mName.as<Callback::NameFunc>()(
+            mITimer, aCallback.mClosure, buf, buflen);
         name = buf;
 
       } else {
-        MOZ_ASSERT(mName.is<NameNothing>());
+        MOZ_ASSERT(aCallback.mName.is<Callback::NameNothing>());
 #ifdef USE_DLADDR
         annotation = "[from dladdr] ";
 
         Dl_info info;
-        void* addr = reinterpret_cast<void*>(aCallback.c);
+        void* addr = reinterpret_cast<void*>(aCallback.mCallback.c);
         if (dladdr(addr, &info) == 0) {
           name = "???[dladdr: failed]";
 
         } else if (info.dli_sname) {
           int status;
           name = abi::__cxa_demangle(info.dli_sname, nullptr, nullptr, &status);
           if (status == 0) {
             // Success. Because we didn't pass in a buffer to __cxa_demangle it
@@ -593,80 +591,61 @@ nsTimerImpl::LogFiring(CallbackType aCal
         }
 #else
         name = "???[dladdr is unimplemented or doesn't work well on this OS]";
 #endif
       }
 
       MOZ_LOG(GetTimerFiringsLog(), LogLevel::Debug,
               ("[%d]    fn timer (%s %5d ms): %s%s\n",
-               getpid(), typeStr, mDelay, annotation, name));
+               getpid(), typeStr, aDelay, annotation, name));
 
       if (needToFreeName) {
         free(const_cast<char*>(name));
       }
 
       break;
     }
 
-    case CallbackType::Interface: {
+    case Callback::Type::Interface: {
       MOZ_LOG(GetTimerFiringsLog(), LogLevel::Debug,
               ("[%d] iface timer (%s %5d ms): %p\n",
-               getpid(), typeStr, mDelay, aCallback.i));
+               getpid(), typeStr, aDelay, aCallback.mCallback.i));
       break;
     }
 
-    case CallbackType::Observer: {
+    case Callback::Type::Observer: {
       MOZ_LOG(GetTimerFiringsLog(), LogLevel::Debug,
               ("[%d]   obs timer (%s %5d ms): %p\n",
-               getpid(), typeStr, mDelay, aCallback.o));
+               getpid(), typeStr, aDelay, aCallback.mCallback.o));
       break;
     }
 
-    case CallbackType::Unknown:
+    case Callback::Type::Unknown:
     default: {
       MOZ_LOG(GetTimerFiringsLog(), LogLevel::Debug,
               ("[%d]   ??? timer (%s, %5d ms)\n",
-               getpid(), typeStr, mDelay));
+               getpid(), typeStr, aDelay));
       break;
     }
   }
 }
 
-void
-nsTimerImpl::SetDelayInternal(uint32_t aDelay, TimeStamp aBase)
-{
-  TimeDuration delayInterval = TimeDuration::FromMilliseconds(aDelay);
-
-  mDelay = aDelay;
-
-  mTimeout = aBase;
-
-  mTimeout += delayInterval;
-
-  if (MOZ_LOG_TEST(GetTimerLog(), LogLevel::Debug)) {
-    if (mStart.IsNull()) {
-      mStart = aBase;
-    } else {
-      mStart2 = aBase;
-    }
-  }
-}
-
 nsTimer::~nsTimer()
 {
 }
 
 size_t
 nsTimer::SizeOfIncludingThis(mozilla::MallocSizeOf aMallocSizeOf) const
 {
   return aMallocSizeOf(this);
 }
 
-/* static */ const nsTimerImpl::NameNothing nsTimerImpl::Nothing = 0;
+/* static */
+const nsTimerImpl::Callback::NameNothing nsTimerImpl::Callback::Nothing = 0;
 
 #ifdef MOZ_TASK_TRACER
 void
 nsTimerImpl::GetTLSTraceInfo()
 {
   mTracedTask.GetTLSTraceInfo();
 }
 
--- a/xpcom/threads/nsTimerImpl.h
+++ b/xpcom/threads/nsTimerImpl.h
@@ -10,16 +10,17 @@
 #include "nsITimer.h"
 #include "nsIEventTarget.h"
 #include "nsIObserver.h"
 
 #include "nsCOMPtr.h"
 
 #include "mozilla/Attributes.h"
 #include "mozilla/Logging.h"
+#include "mozilla/Mutex.h"
 #include "mozilla/TimeStamp.h"
 #include "mozilla/Variant.h"
 
 #ifdef MOZ_TASK_TRACER
 #include "TracedTaskCommon.h"
 #endif
 
 extern mozilla::LogModule* GetTimerLog();
@@ -32,148 +33,159 @@ extern mozilla::LogModule* GetTimerLog()
     {0x84, 0x27, 0xfb, 0xab, 0x44, 0xf2, 0x9b, 0xc8} \
 }
 
 // TimerThread, nsTimerEvent, and nsTimer have references to these. nsTimer has
 // a separate lifecycle so we can Cancel() the underlying timer when the user of
 // the nsTimer has let go of its last reference.
 class nsTimerImpl
 {
-  ~nsTimerImpl();
+  ~nsTimerImpl() {}
 public:
   typedef mozilla::TimeStamp TimeStamp;
 
   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());
-
-  void Fire();
+  void Fire(int32_t aGeneration);
 
 #ifdef MOZ_TASK_TRACER
   void GetTLSTraceInfo();
   mozilla::tasktracer::TracedTaskCommon GetTracedTask();
 #endif
 
   int32_t GetGeneration()
   {
     return mGeneration;
   }
 
-  enum class CallbackType : uint8_t {
-    Unknown = 0,
-    Interface = 1,
-    Function = 2,
-    Observer = 3,
-  };
-
   nsresult InitCommon(uint32_t aDelay, uint32_t aType);
 
-  void ReleaseCallback()
-  {
-    // if we're the last owner of the callback object, make
-    // sure that we don't recurse into ReleaseCallback in case
-    // the callback's destructor calls Cancel() or similar.
-    CallbackType cbType = mCallbackType;
-    mCallbackType = CallbackType::Unknown;
+  struct Callback {
+    Callback() :
+      mType(Type::Unknown),
+      mName(Nothing),
+      mClosure(nullptr)
+    {
+      mCallback.c = nullptr;
+    }
+
+    Callback(const Callback& other) = delete;
+    Callback& operator=(const Callback& other) = delete;
+
+    ~Callback()
+    {
+      if (mType == Type::Interface) {
+        NS_RELEASE(mCallback.i);
+      } else if (mType == Type::Observer) {
+        NS_RELEASE(mCallback.o);
+      }
+    }
+
+    void swap(Callback& other)
+    {
+      std::swap(mType, other.mType);
+      std::swap(mCallback, other.mCallback);
+      std::swap(mName, other.mName);
+      std::swap(mClosure, other.mClosure);
+    }
 
-    if (cbType == CallbackType::Interface) {
-      NS_RELEASE(mCallback.i);
-    } else if (cbType == CallbackType::Observer) {
-      NS_RELEASE(mCallback.o);
+    enum class Type : uint8_t {
+      Unknown = 0,
+      Interface = 1,
+      Function = 2,
+      Observer = 3,
+    };
+    Type mType;
+
+    union CallbackUnion
+    {
+      nsTimerCallbackFunc c;
+      // These refcounted references are managed manually, as they are in a union
+      nsITimerCallback* MOZ_OWNING_REF i;
+      nsIObserver* MOZ_OWNING_REF o;
+    } mCallback;
+
+    // |Name| is a tagged union type representing one of (a) nothing, (b) a
+    // string, or (c) a function. mozilla::Variant doesn't naturally handle the
+    // "nothing" case, so we define a dummy type and value (which is unused and
+    // so the exact value doesn't matter) for it.
+    typedef const int NameNothing;
+    typedef const char* NameString;
+    typedef nsTimerNameCallbackFunc NameFunc;
+    typedef mozilla::Variant<NameNothing, NameString, NameFunc> Name;
+    static const NameNothing Nothing;
+    Name mName;
+
+    void*                 mClosure;
+  };
+
+  Callback& GetCallback()
+  {
+    mMutex.AssertCurrentThreadOwns();
+    if (mCallback.mType == Callback::Type::Unknown) {
+      return mCallbackDuringFire;
     }
+
+    return mCallback;
   }
 
-  // Permanently disables this timer. This gets called when the last nsTimer
-  // ref (besides mITimer) goes away. If called from the target thread, it
-  // guarantees that the timer will not fire again. If called from anywhere
-  // else, it could race.
-  void Neuter();
-
   bool IsRepeating() const
   {
     static_assert(nsITimer::TYPE_ONE_SHOT < nsITimer::TYPE_REPEATING_SLACK,
                   "invalid ordering of timer types!");
     static_assert(
         nsITimer::TYPE_REPEATING_SLACK < nsITimer::TYPE_REPEATING_PRECISE,
         "invalid ordering of timer types!");
     static_assert(
         nsITimer::TYPE_REPEATING_PRECISE <
           nsITimer::TYPE_REPEATING_PRECISE_CAN_SKIP,
         "invalid ordering of timer types!");
     return mType >= nsITimer::TYPE_REPEATING_SLACK;
   }
 
-  bool IsRepeatingPrecisely() const
-  {
-    return mType >= nsITimer::TYPE_REPEATING_PRECISE;
-  }
-
   nsCOMPtr<nsIEventTarget> mEventTarget;
 
-  void*                 mClosure;
-
-  union CallbackUnion
-  {
-    nsTimerCallbackFunc c;
-    // These refcounted references are managed manually, as they are in a union
-    nsITimerCallback* MOZ_OWNING_REF i;
-    nsIObserver* MOZ_OWNING_REF o;
-  } mCallback;
-
-  void LogFiring(CallbackType aCallbackType, CallbackUnion aCallbackUnion);
-
-  // |Name| is a tagged union type representing one of (a) nothing, (b) a
-  // string, or (c) a function. mozilla::Variant doesn't naturally handle the
-  // "nothing" case, so we define a dummy type and value (which is unused and
-  // so the exact value doesn't matter) for it.
-  typedef const int NameNothing;
-  typedef const char* NameString;
-  typedef nsTimerNameCallbackFunc NameFunc;
-  typedef mozilla::Variant<NameNothing, NameString, NameFunc> Name;
-  static const NameNothing Nothing;
+  void LogFiring(const Callback& aCallback, uint8_t aType, uint32_t aDelay);
 
   nsresult InitWithFuncCallbackCommon(nsTimerCallbackFunc aFunc,
                                       void* aClosure,
                                       uint32_t aDelay,
                                       uint32_t aType,
-                                      Name aName);
-
-  // This is set by Init. It records the name (if there is one) for the timer,
-  // for use when logging timer firings.
-  Name mName;
+                                      Callback::Name aName);
 
   // 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.
-  CallbackType          mCallbackType;
   uint8_t               mType;
 
   // The generation number of this timer, re-generated each time the timer is
   // initialized so one-shot timers can be canceled and re-initialized by the
   // arming thread without any bad race conditions.
-  // This is only modified on the target thread, and only after removing the
-  // timer from the TimerThread. Is set on the arming thread, initially.
+  // Updated only after this timer has been removed from the timer thread.
   int32_t               mGeneration;
 
   uint32_t              mDelay;
+  // Updated only after this timer has been removed from the timer thread.
   TimeStamp             mTimeout;
 
 #ifdef MOZ_TASK_TRACER
   mozilla::tasktracer::TracedTaskCommon mTracedTask;
 #endif
 
-  TimeStamp             mStart, mStart2;
   static double         sDeltaSum;
   static double         sDeltaSumSquared;
   static double         sDeltaNum;
-  RefPtr<nsITimer>      mITimer;
+  const RefPtr<nsITimer>      mITimer;
+  mozilla::Mutex mMutex;
+  Callback              mCallback;
+  Callback              mCallbackDuringFire;
 };
 
 class nsTimer final : public nsITimer
 {
   virtual ~nsTimer();
 public:
   nsTimer() : mImpl(new nsTimerImpl(this)) {}