Bug 1545120: Simplify this callback management stuff a little. r=froydnj a=pascalc
authorByron Campen [:bwc] <docfaraday@gmail.com>
Thu, 18 Apr 2019 14:53:18 +0000
changeset 523306 198b3248e141024d06c7603d4ad5c19f6f40cc11
parent 523305 3a1b53518d5ff472eb17f214bb02fc82b1675890
child 523307 c53e3fd7696496c92cc74ac23c361c33d883f735
push id11138
push userarchaeopteryx@coole-files.de
push dateTue, 23 Apr 2019 19:02:09 +0000
treeherdermozilla-beta@c53e3fd76964 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersfroydnj, pascalc
bugs1545120
milestone67.0
Bug 1545120: Simplify this callback management stuff a little. r=froydnj a=pascalc Differential Revision: https://phabricator.services.mozilla.com/D27914
xpcom/threads/nsTimerImpl.cpp
xpcom/threads/nsTimerImpl.h
--- a/xpcom/threads/nsTimerImpl.cpp
+++ b/xpcom/threads/nsTimerImpl.cpp
@@ -239,17 +239,18 @@ nsTimer::Release(void) {
 }
 
 nsTimerImpl::nsTimerImpl(nsITimer* aTimer, nsIEventTarget* aTarget)
     : mEventTarget(aTarget),
       mHolder(nullptr),
       mType(0),
       mGeneration(0),
       mITimer(aTimer),
-      mMutex("nsTimerImpl::mMutex") {
+      mMutex("nsTimerImpl::mMutex"),
+      mFiring(0) {
   // XXX some code creates timers during xpcom shutdown, when threads are no
   // longer available, so we cannot turn this on yet.
   // MOZ_ASSERT(mEventTarget);
 }
 
 // static
 nsresult nsTimerImpl::Startup() {
   nsresult rv;
@@ -401,18 +402,17 @@ void nsTimerImpl::CancelImpl(bool aClear
       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)) {
+    if (aClearITimer && !mFiring) {
       MOZ_RELEASE_ASSERT(
           mITimer,
           "mITimer was nulled already! "
           "This indicates that someone has messed up the refcount on nsTimer!");
       timerTrash.swap(mITimer);
     }
   }
 }
@@ -505,27 +505,29 @@ nsresult nsTimerImpl::GetAllowedEarlyFir
   *aValueOut = gThread ? gThread->AllowedEarlyFiringMicroseconds() : 0;
   return NS_OK;
 }
 
 void nsTimerImpl::Fire(int32_t aGeneration) {
   uint8_t oldType;
   uint32_t oldDelay;
   TimeStamp oldTimeout;
+  Callback callbackDuringFire;
   nsCOMPtr<nsITimer> kungFuDeathGrip;
 
   {
     // 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);
+    ++mFiring;
+    callbackDuringFire = mCallback;
     oldType = mType;
     oldDelay = mDelay.ToMilliseconds();
     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.
     kungFuDeathGrip = mITimer;
   }
@@ -546,49 +548,52 @@ void nsTimerImpl::Fire(int32_t aGenerati
             ("[this=%p] actual delay time   %4dms\n", this, oldDelay + d));
     MOZ_LOG(GetTimerLog(), LogLevel::Debug,
             ("[this=%p] (mType is %d)       -------\n", this, oldType));
     MOZ_LOG(GetTimerLog(), LogLevel::Debug,
             ("[this=%p]     delta           %4dms\n", this, d));
   }
 
   if (MOZ_LOG_TEST(GetTimerFiringsLog(), LogLevel::Debug)) {
-    LogFiring(mCallbackDuringFire, oldType, oldDelay);
+    LogFiring(callbackDuringFire, oldType, oldDelay);
   }
 
-  switch (mCallbackDuringFire.mType) {
+  switch (callbackDuringFire.mType) {
     case Callback::Type::Function:
-      mCallbackDuringFire.mCallback.c(mITimer, mCallbackDuringFire.mClosure);
+      callbackDuringFire.mCallback.c(mITimer, callbackDuringFire.mClosure);
       break;
     case Callback::Type::Interface:
-      mCallbackDuringFire.mCallback.i->Notify(mITimer);
+      callbackDuringFire.mCallback.i->Notify(mITimer);
       break;
     case Callback::Type::Observer:
-      mCallbackDuringFire.mCallback.o->Observe(mITimer, NS_TIMER_CALLBACK_TOPIC,
-                                               nullptr);
+      callbackDuringFire.mCallback.o->Observe(mITimer, NS_TIMER_CALLBACK_TOPIC,
+                                              nullptr);
       break;
     default:;
   }
 
-  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);
-    if (IsSlack()) {
-      mTimeout = TimeStamp::Now() + mDelay;
+  if (aGeneration == mGeneration) {
+    if (IsRepeating()) {
+      // Repeating timer has not been re-init or canceled; reschedule
+      if (IsSlack()) {
+        mTimeout = TimeStamp::Now() + mDelay;
+      } else {
+        mTimeout = mTimeout + mDelay;
+      }
+      if (gThread) {
+        gThread->AddTimer(this);
+      }
     } else {
-      mTimeout = mTimeout + mDelay;
-    }
-    if (gThread) {
-      gThread->AddTimer(this);
+      // Non-repeating timer that has not been re-scheduled. Clear.
+      mCallback.clear();
     }
   }
 
-  mCallbackDuringFire.swap(trash);
+  --mFiring;
 
   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
--- a/xpcom/threads/nsTimerImpl.h
+++ b/xpcom/threads/nsTimerImpl.h
@@ -62,40 +62,68 @@ class nsTimerImpl {
 
   int32_t GetGeneration() { return mGeneration; }
 
   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(const Callback& other) : Callback() { *this = other; }
+
+    enum class Type : uint8_t {
+      Unknown = 0,
+      Interface = 1,
+      Function = 2,
+      Observer = 3,
+    };
 
-    ~Callback() {
+    Callback& operator=(const Callback& other) {
+      if (this != &other) {
+        clear();
+        mType = other.mType;
+        switch (mType) {
+          case Type::Unknown:
+            break;
+          case Type::Interface:
+            mCallback.i = other.mCallback.i;
+            NS_ADDREF(mCallback.i);
+            break;
+          case Type::Function:
+            mCallback.c = other.mCallback.c;
+            break;
+          case Type::Observer:
+            mCallback.o = other.mCallback.o;
+            NS_ADDREF(mCallback.o);
+            break;
+        }
+        mName = other.mName;
+        mClosure = other.mClosure;
+      }
+      return *this;
+    }
+
+    ~Callback() { clear(); }
+
+    void clear() {
       if (mType == Type::Interface) {
         NS_RELEASE(mCallback.i);
       } else if (mType == Type::Observer) {
         NS_RELEASE(mCallback.o);
       }
+      mType = Type::Unknown;
     }
 
     void swap(Callback& other) {
       std::swap(mType, other.mType);
       std::swap(mCallback, other.mCallback);
       std::swap(mName, other.mName);
       std::swap(mClosure, other.mClosure);
     }
 
-    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;
@@ -118,20 +146,16 @@ class nsTimerImpl {
   nsresult InitCommon(uint32_t aDelayMS, uint32_t aType,
                       Callback&& newCallback);
 
   nsresult InitCommon(const mozilla::TimeDuration& aDelay, uint32_t aType,
                       Callback&& newCallback);
 
   Callback& GetCallback() {
     mMutex.AssertCurrentThreadOwns();
-    if (mCallback.mType == Callback::Type::Unknown) {
-      return mCallbackDuringFire;
-    }
-
     return mCallback;
   }
 
   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,
@@ -188,17 +212,18 @@ class nsTimerImpl {
 #endif
 
   static double sDeltaSum;
   static double sDeltaSumSquared;
   static double sDeltaNum;
   RefPtr<nsITimer> mITimer;
   mozilla::Mutex mMutex;
   Callback mCallback;
-  Callback mCallbackDuringFire;
+  // Counter because in rare cases we can Fire reentrantly
+  unsigned int mFiring;
 };
 
 class nsTimer final : public nsITimer {
   explicit nsTimer(nsIEventTarget* aTarget)
       : mImpl(new nsTimerImpl(this, aTarget)) {}
 
   virtual ~nsTimer();