[XPCOM] Simplify nsITimer API.

It's much simpler to hold a death grip on the timer than to have a fragile
"release upon cancel" construct.

2 files changed, 12 insertions, 17 deletions

diff --git a/xpcom/threads/TimerThread.cpp b/xpcom/threads/TimerThread.cpp
index c2a0284eca..a2e40c3880 100644
--- a/xpcom/threads/TimerThread.cpp
+++ b/xpcom/threads/TimerThread.cpp
@@ -140,11 +140,7 @@ public:
 
   nsresult Cancel() override
   {
-    // Since nsTimerImpl is not thread-safe, we should release |mTimer|
-    // here in the target thread to avoid race condition. Otherwise,
-    // ~nsTimerEvent() which calls nsTimerImpl::Release() could run in the
-    // timer thread and result in race condition.
-    mTimer = nullptr;
+    mTimer->Cancel();
     return NS_OK;
   }
 
@@ -269,11 +265,6 @@ nsTimerEvent::DeleteAllocatorIfNeeded()
 NS_IMETHODIMP
 nsTimerEvent::Run()
 {
-  if (!mTimer) {
-    MOZ_ASSERT(false);
-    return NS_OK;
-  }
-
   if (MOZ_LOG_TEST(GetTimerLog(), LogLevel::Debug)) {
     TimeStamp now = TimeStamp::Now();
     MOZ_LOG(GetTimerLog(), LogLevel::Debug,
@@ -283,9 +274,7 @@ nsTimerEvent::Run()
 
   mTimer->Fire(mGeneration);
 
-  // We call Cancel() to correctly release mTimer.
-  // Read more in the Cancel() implementation.
-  return Cancel();
+  return NS_OK;
 }
 
 nsresult
diff --git a/xpcom/threads/nsTimerImpl.cpp b/xpcom/threads/nsTimerImpl.cpp
index cac3e86929..2a3531a858 100644
--- a/xpcom/threads/nsTimerImpl.cpp
+++ b/xpcom/threads/nsTimerImpl.cpp
@@ -433,11 +433,13 @@ nsTimerImpl::Fire(int32_t aGeneration)
   uint8_t oldType;
   uint32_t oldDelay;
   TimeStamp oldTimeout;
+  nsCOMPtr<nsITimer> timer;
 
   {
+    MutexAutoLock lock(mMutex);
+
     // 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) {
       // This timer got rescheduled or cancelled before we fired, so ignore this
       // firing
@@ -448,6 +450,10 @@ nsTimerImpl::Fire(int32_t aGeneration)
     oldType = mType;
     oldDelay = mDelay;
     oldTimeout = mTimeout;
+    // Ensure that the nsITimer does not unhook from the nsTimerImpl during
+    // Fire; this will cause null pointer crashes if the user of the timer drops
+    // its reference, and then uses the nsITimer* passed in the callback.
+    timer = mITimer;    
   }
 
   PROFILER_LABEL("Timer", "Fire",
@@ -477,13 +483,13 @@ nsTimerImpl::Fire(int32_t aGeneration)
 
   switch (mCallbackDuringFire.mType) {
     case Callback::Type::Function:
-      mCallbackDuringFire.mCallback.c(mITimer, mCallbackDuringFire.mClosure);
+      mCallbackDuringFire.mCallback.c(timer, mCallbackDuringFire.mClosure);
       break;
     case Callback::Type::Interface:
-      mCallbackDuringFire.mCallback.i->Notify(mITimer);
+      mCallbackDuringFire.mCallback.i->Notify(timer);
       break;
     case Callback::Type::Observer:
-      mCallbackDuringFire.mCallback.o->Observe(mITimer, NS_TIMER_CALLBACK_TOPIC,
+      mCallbackDuringFire.mCallback.o->Observe(timer, NS_TIMER_CALLBACK_TOPIC,
                                                nullptr);
       break;
     default: