Bug 1024765 - Part 2: Make refcounting logic around PostTimerEvent more explicit. r=bz, a=sledru
authorByron Campen [:bwc] <docfaraday@gmail.com>
Mon, 16 Jun 2014 14:13:04 -0700
changeset 207255 358992828820708e12f17a511554fe368012d058
parent 207254 b1873b86994c3972c1eec8e99a533f403cd1c729
child 207256 4e6018ad263903b6d931ec4c8ff6b2417a1a1548
push id3741
push userasasaki@mozilla.com
push dateMon, 21 Jul 2014 20:25:18 +0000
treeherdermozilla-beta@4d6f46f5af68 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbz, sledru
bugs1024765
milestone32.0a2
Bug 1024765 - Part 2: Make refcounting logic around PostTimerEvent more explicit. r=bz, a=sledru
xpcom/threads/TimerThread.cpp
xpcom/threads/nsTimerImpl.cpp
xpcom/threads/nsTimerImpl.h
--- a/xpcom/threads/TimerThread.cpp
+++ b/xpcom/threads/TimerThread.cpp
@@ -238,52 +238,57 @@ TimerThread::Run()
         if (now >= timer->mTimeout || forceRunThisTimer) {
     next:
           // NB: AddRef before the Release under RemoveTimerInternal to avoid
           // mRefCnt passing through zero, in case all other refs than the one
           // from mTimers have gone away (the last non-mTimers[i]-ref's Release
           // must be racing with us, blocked in gThread->RemoveTimer waiting
           // for TimerThread::mMonitor, under nsTimerImpl::Release.
 
-          NS_ADDREF(timer);
+          nsRefPtr<nsTimerImpl> timerRef(timer);
           RemoveTimerInternal(timer);
+          timer = nullptr;
 
           {
             // We release mMonitor around the Fire call to avoid deadlock.
             MonitorAutoUnlock unlock(mMonitor);
 
 #ifdef DEBUG_TIMERS
             if (PR_LOG_TEST(GetTimerLog(), PR_LOG_DEBUG)) {
               PR_LOG(GetTimerLog(), PR_LOG_DEBUG,
                      ("Timer thread woke up %fms from when it was supposed to\n",
-                      fabs((now - timer->mTimeout).ToMilliseconds())));
+                      fabs((now - timerRef->mTimeout).ToMilliseconds())));
             }
 #endif
 
             // We are going to let the call to PostTimerEvent here handle the
             // release of the timer so that we don't end up releasing the timer
             // on the TimerThread instead of on the thread it targets.
-            if (NS_FAILED(timer->PostTimerEvent())) {
-              nsrefcnt rc;
-              NS_RELEASE2(timer, rc);
+            timerRef = nsTimerImpl::PostTimerEvent(timerRef.forget());
+
+            if (timerRef) {
+              // We got our reference back due to an error.
+              // Unhook the nsRefPtr, and release manually so we can get the
+              // refcount.
+              nsrefcnt rc = timerRef.forget().take()->Release();
+              (void)rc;
 
               // The nsITimer interface requires that its users keep a reference
               // to the timers they use while those timers are initialized but
               // have not yet fired.  If this ever happens, it is a bug in the
               // code that created and used the timer.
               //
               // Further, note that this should never happen even with a
               // misbehaving user, because nsTimerImpl::Release checks for a
               // refcount of 1 with an armed timer (a timer whose only reference
               // is from the timer thread) and when it hits this will remove the
               // timer from the timer thread and thus destroy the last reference,
               // preventing this situation from occurring.
               MOZ_ASSERT(rc != 0, "destroyed timer off its target thread!");
             }
-            timer = nullptr;
           }
 
           if (mShutdown) {
             break;
           }
 
           // Update now, as PostTimerEvent plus the locking may have taken a
           // tick or two, and we may goto next below.
--- a/xpcom/threads/nsTimerImpl.cpp
+++ b/xpcom/threads/nsTimerImpl.cpp
@@ -107,21 +107,20 @@ public:
 
 } // anonymous namespace
 
 class nsTimerEvent : public nsRunnable
 {
 public:
   NS_IMETHOD Run();
 
-  nsTimerEvent(nsTimerImpl* aTimer, int32_t aGeneration)
-    : mTimer(dont_AddRef(aTimer))
-    , mGeneration(aGeneration)
+  nsTimerEvent()
+    : mTimer()
+    , mGeneration(0)
   {
-    // aTimer is already addref'd for us
     MOZ_COUNT_CTOR(nsTimerEvent);
 
     MOZ_ASSERT(gThread->IsOnTimerThread(),
                "nsTimer must always be allocated on the timer thread");
 
     sAllocatorUsers++;
   }
 
@@ -138,18 +137,28 @@ public:
     return sAllocator->Alloc(aSize);
   }
   void operator delete(void* aPtr)
   {
     sAllocator->Free(aPtr);
     DeleteAllocatorIfNeeded();
   }
 
+  already_AddRefed<nsTimerImpl> ForgetTimer()
+  {
+    return mTimer.forget();
+  }
+
+  void SetTimer(already_AddRefed<nsTimerImpl> aTimer)
+  {
+    mTimer = aTimer;
+    mGeneration = mTimer->GetGeneration();
+  }
+
 private:
-  nsTimerEvent(); // Not implemented
   ~nsTimerEvent()
   {
     MOZ_COUNT_DTOR(nsTimerEvent);
 
     MOZ_ASSERT(!sCanDeleteAllocator || sAllocatorUsers > 0,
                "This will result in us attempting to deallocate the nsTimerEvent allocator twice");
     sAllocatorUsers--;
   }
@@ -699,61 +708,73 @@ nsTimerEvent::Run()
   }
 #endif
 
   mTimer->Fire();
 
   return NS_OK;
 }
 
-nsresult
-nsTimerImpl::PostTimerEvent()
+already_AddRefed<nsTimerImpl>
+nsTimerImpl::PostTimerEvent(already_AddRefed<nsTimerImpl> aTimerRef)
 {
-  if (!mEventTarget) {
+  nsRefPtr<nsTimerImpl> timer(aTimerRef);
+  if (!timer->mEventTarget) {
     NS_ERROR("Attempt to post timer event to NULL event target");
-    return NS_ERROR_NOT_INITIALIZED;
+    return timer.forget();
   }
 
   // XXX we may want to reuse this nsTimerEvent in the case of repeating timers.
 
-  // Since TimerThread addref'd 'this' for us, we don't need to addref here.
-  // We will release in destroyMyEvent.  We need to copy the generation number
-  // from this timer into the event, so we can avoid firing a timer that was
-  // re-initialized after being canceled.
+  // Since TimerThread addref'd 'timer' for us, we don't need to addref here.
+  // We will release either in ~nsTimerEvent(), or pass the reference back to
+  // the caller. We need to copy the generation number from this timer into the
+  // event, so we can avoid firing a timer that was re-initialized after being
+  // canceled.
 
-  nsRefPtr<nsTimerEvent> event = new nsTimerEvent(this, mGeneration);
+  // Note: We override operator new for this class, and the override is
+  // fallible!
+  nsRefPtr<nsTimerEvent> event = new nsTimerEvent;
   if (!event) {
-    return NS_ERROR_OUT_OF_MEMORY;
+    return timer.forget();
   }
 
 #ifdef DEBUG_TIMERS
   if (PR_LOG_TEST(GetTimerLog(), PR_LOG_DEBUG)) {
     event->mInitTime = TimeStamp::Now();
   }
 #endif
 
   // If this is a repeating precise timer, we need to calculate the time for
   // the next timer to fire before we make the callback.
-  if (IsRepeatingPrecisely()) {
-    SetDelayInternal(mDelay);
+  if (timer->IsRepeatingPrecisely()) {
+    timer->SetDelayInternal(timer->mDelay);
 
     // But only re-arm REPEATING_PRECISE timers.
-    if (gThread && mType == TYPE_REPEATING_PRECISE) {
-      nsresult rv = gThread->AddTimer(this);
+    if (gThread && timer->mType == TYPE_REPEATING_PRECISE) {
+      nsresult rv = gThread->AddTimer(timer);
       if (NS_FAILED(rv)) {
-        return rv;
+        return timer.forget();
       }
     }
   }
 
-  nsresult rv = mEventTarget->Dispatch(event, NS_DISPATCH_NORMAL);
-  if (NS_FAILED(rv) && gThread) {
-    gThread->RemoveTimer(this);
+  nsIEventTarget* target = timer->mEventTarget;
+  event->SetTimer(timer.forget());
+
+  nsresult rv = target->Dispatch(event, NS_DISPATCH_NORMAL);
+  if (NS_FAILED(rv)) {
+    timer = event->ForgetTimer();
+    if (gThread) {
+      gThread->RemoveTimer(timer);
+    }
+    return timer.forget();
   }
-  return rv;
+
+  return nullptr;
 }
 
 void
 nsTimerImpl::SetDelayInternal(uint32_t aDelay)
 {
   TimeDuration delayInterval = TimeDuration::FromMilliseconds(aDelay);
 
   mDelay = aDelay;
--- a/xpcom/threads/nsTimerImpl.h
+++ b/xpcom/threads/nsTimerImpl.h
@@ -54,17 +54,19 @@ public:
 
   static nsresult Startup();
   static void Shutdown();
 
   friend class TimerThread;
   friend struct TimerAdditionComparator;
 
   void Fire();
-  nsresult PostTimerEvent();
+  // If a failure is encountered, the reference is returned to the caller
+  static already_AddRefed<nsTimerImpl> PostTimerEvent(
+      already_AddRefed<nsTimerImpl> aTimerRef);
   void SetDelayInternal(uint32_t aDelay);
 
   NS_DECL_THREADSAFE_ISUPPORTS
   NS_DECL_NSITIMER
 
   int32_t GetGeneration()
   {
     return mGeneration;