Bug 1545120: Simplify this callback management stuff a little. r=froydnj
authorByron Campen [:bwc] <docfaraday@gmail.com>
Thu, 18 Apr 2019 14:53:18 +0000
changeset 470258 bd62ad8bf26d170a68d703f13f43328bafa491d8
parent 470257 9661c95fa5df762563f877eeff248f996ef0c3a3
child 470259 f7772ec802b23b6d621d9abccb0e4e218dc235e6
push id83625
push userbcampen@mozilla.com
push dateFri, 19 Apr 2019 20:29:17 +0000
treeherderautoland@bd62ad8bf26d [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersfroydnj
bugs1545120
milestone68.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 1545120: Simplify this callback management stuff a little. r=froydnj 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();