Bug 1157323 - Part 2: Factor nsTimerImpl into two classes, so we don't need to do racy stuff in nsTimerImpl::Release. r=froydnj
authorByron Campen [:bwc] <docfaraday@gmail.com>
Wed, 20 Jul 2016 15:16:40 -0500
changeset 315360 f814d7c1885428f36fe418d90a70e610a224848b
parent 315359 a718fc141d0a3efa8db4d09969b9c606a1d10e5c
child 315361 85739737d40d01de29d2f83d9715249900ca58d9
push id30748
push usercbook@mozilla.com
push dateWed, 28 Sep 2016 13:53:19 +0000
treeherdermozilla-central@8c84b7618840 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersfroydnj
bugs1157323
milestone52.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 1157323 - Part 2: Factor nsTimerImpl into two classes, so we don't need to do racy stuff in nsTimerImpl::Release. r=froydnj MozReview-Commit-ID: DAe4TpMqBpA
xpcom/build/XPCOMInit.cpp
xpcom/build/XPCOMModule.inc
xpcom/threads/nsTimerImpl.cpp
xpcom/threads/nsTimerImpl.h
--- a/xpcom/build/XPCOMInit.cpp
+++ b/xpcom/build/XPCOMInit.cpp
@@ -202,17 +202,17 @@ NS_GENERIC_FACTORY_CONSTRUCTOR(nsSupport
 NS_GENERIC_FACTORY_CONSTRUCTOR(nsSupportsPRInt64)
 NS_GENERIC_FACTORY_CONSTRUCTOR(nsSupportsFloat)
 NS_GENERIC_FACTORY_CONSTRUCTOR(nsSupportsDouble)
 NS_GENERIC_FACTORY_CONSTRUCTOR(nsSupportsVoid)
 NS_GENERIC_FACTORY_CONSTRUCTOR(nsSupportsInterfacePointer)
 
 NS_GENERIC_FACTORY_CONSTRUCTOR_INIT(nsConsoleService, Init)
 NS_GENERIC_FACTORY_CONSTRUCTOR(nsAtomService)
-NS_GENERIC_FACTORY_CONSTRUCTOR(nsTimerImpl)
+NS_GENERIC_FACTORY_CONSTRUCTOR(nsTimer)
 NS_GENERIC_FACTORY_CONSTRUCTOR(nsBinaryOutputStream)
 NS_GENERIC_FACTORY_CONSTRUCTOR(nsBinaryInputStream)
 NS_GENERIC_FACTORY_CONSTRUCTOR(nsStorageStream)
 NS_GENERIC_FACTORY_CONSTRUCTOR(nsVersionComparatorImpl)
 NS_GENERIC_FACTORY_CONSTRUCTOR(nsScriptableBase64Encoder)
 
 NS_GENERIC_FACTORY_CONSTRUCTOR(nsVariantCC)
 
--- a/xpcom/build/XPCOMModule.inc
+++ b/xpcom/build/XPCOMModule.inc
@@ -17,17 +17,17 @@
     COMPONENT(PERSISTENTPROPERTIES, nsPersistentProperties::Create)
 
     COMPONENT(SUPPORTSARRAY, nsSupportsArray::Create)
     COMPONENT(ARRAY, nsArrayBase::XPCOMConstructor)
     COMPONENT(CONSOLESERVICE, nsConsoleServiceConstructor)
     COMPONENT(ATOMSERVICE, nsAtomServiceConstructor)
     COMPONENT_M(OBSERVERSERVICE, nsObserverService::Create, Module::ALLOW_IN_GPU_PROCESS)
 
-    COMPONENT_M(TIMER, nsTimerImplConstructor, Module::ALLOW_IN_GPU_PROCESS)
+    COMPONENT_M(TIMER, nsTimerConstructor, Module::ALLOW_IN_GPU_PROCESS)
 
 #define COMPONENT_SUPPORTS(TYPE, Type)                                         \
   COMPONENT(SUPPORTS_##TYPE, nsSupports##Type##Constructor)
 
     COMPONENT_SUPPORTS(ID, ID)
     COMPONENT_SUPPORTS(STRING, String)
     COMPONENT_SUPPORTS(CSTRING, CString)
     COMPONENT_SUPPORTS(PRBOOL, PRBool)
--- a/xpcom/threads/nsTimerImpl.cpp
+++ b/xpcom/threads/nsTimerImpl.cpp
@@ -107,95 +107,60 @@ myNS_MeanAndStdDev(double n, double sumO
     }
     // for some reason, Windows says sqrt(0.0) is "-1.#J" (?!) so do this:
     stdDev = var != 0.0 ? sqrt(var) : 0.0;
   }
   *meanResult = mean;
   *stdDevResult = stdDev;
 }
 
-NS_IMPL_QUERY_INTERFACE(nsTimerImpl, nsITimer)
-NS_IMPL_ADDREF(nsTimerImpl)
+NS_IMPL_QUERY_INTERFACE(nsTimer, nsITimer)
+NS_IMPL_ADDREF(nsTimer)
 
 NS_IMETHODIMP_(MozExternalRefCountType)
-nsTimerImpl::Release(void)
+nsTimer::Release(void)
 {
-  nsrefcnt count;
-
-  MOZ_ASSERT(int32_t(mRefCnt) > 0, "dup release");
-  count = --mRefCnt;
-  NS_LOG_RELEASE(this, count, "nsTimerImpl");
-  if (count == 0) {
-    mRefCnt = 1; /* stabilize */
-
-    /* enable this to find non-threadsafe destructors: */
-    /* NS_ASSERT_OWNINGTHREAD(nsTimerImpl); */
-    delete this;
-    return 0;
-  }
+  nsrefcnt count = --mRefCnt;
+  NS_LOG_RELEASE(this, count, "nsTimer");
 
-  // If only one reference remains, and mArmed is set, then the ref must be
-  // from the TimerThread::mTimers array, so we Cancel this timer to remove
-  // the mTimers element, and return 0 if Cancel in fact disarmed the timer.
-  //
-  // We use an inlined version of nsTimerImpl::Cancel here to check for the
-  // NS_ERROR_NOT_AVAILABLE code returned by gThread->RemoveTimer when this
-  // timer is not found in the mTimers array -- i.e., when the timer was not
-  // in fact armed once we acquired TimerThread::mLock, in spite of mArmed
-  // being true here.  That can happen if the armed timer is being fired by
-  // TimerThread::Run as we race and test mArmed just before it is cleared by
-  // the timer thread.  If the RemoveTimer call below doesn't find this timer
-  // in the mTimers array, then the last ref to this timer is held manually
-  // and temporarily by the TimerThread, so we should fall through to the
-  // final return and return 1, not 0.
-  //
-  // The original version of this thread-based timer code kept weak refs from
-  // TimerThread::mTimers, removing this timer's weak ref in the destructor,
-  // but that leads to double-destructions in the race described above, and
-  // adding mArmed doesn't help, because destructors can't be deferred, once
-  // begun.  But by combining reference-counting and a specialized Release
-  // method with "is this timer still in the mTimers array once we acquire
-  // the TimerThread's lock" testing, we defer destruction until we're sure
-  // that only one thread has its hot little hands on this timer.
-  //
-  // Note that both approaches preclude a timer creator, and everyone else
-  // except the TimerThread who might have a strong ref, from dropping all
-  // their strong refs without implicitly canceling the timer.  Timers need
-  // non-mTimers-element strong refs to stay alive.
-
-  if (count == 1 && mArmed) {
-    mCanceled = true;
-
-    MOZ_ASSERT(gThread, "Armed timer exists after the thread timer stopped.");
-    if (NS_SUCCEEDED(gThread->RemoveTimer(this))) {
-      return 0;
-    }
+  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 = nullptr;
+  } else if (count == 0) {
+    delete this;
   }
 
   return count;
 }
 
-nsTimerImpl::nsTimerImpl() :
+nsTimerImpl::nsTimerImpl(nsITimer* aTimer) :
   mClosure(nullptr),
   mName(nsTimerImpl::Nothing),
   mCallbackType(CallbackType::Unknown),
   mFiring(false),
   mArmed(false),
   mCanceled(false),
   mGeneration(0),
-  mDelay(0)
+  mDelay(0),
+  mITimer(aTimer)
 {
+  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;
@@ -357,20 +322,37 @@ nsTimerImpl::Cancel()
     gThread->RemoveTimer(this);
   }
 
   ReleaseCallback();
 
   return NS_OK;
 }
 
+void
+nsTimerImpl::Neuter()
+{
+  if (gThread) {
+    gThread->RemoveTimer(this);
+  }
+
+  // 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.
+  ++mGeneration;
+
+  // Breaks cycles when TimerEvents are in the queue of a thread that is no
+  // longer running.
+  mEventTarget = nullptr;
+}
+
 NS_IMETHODIMP
 nsTimerImpl::SetDelay(uint32_t aDelay)
 {
-  if (mCallbackType == CallbackType::Unknown && mType == TYPE_ONE_SHOT) {
+  if (mCallbackType == CallbackType::Unknown && mType == nsITimer::TYPE_ONE_SHOT) {
     // 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;
   }
 
   SetDelayInternal(aDelay);
@@ -513,34 +495,34 @@ nsTimerImpl::Fire()
   ReleaseCallback();
 
   if (MOZ_LOG_TEST(GetTimerFiringsLog(), LogLevel::Debug)) {
     LogFiring(callbackType, callback);
   }
 
   switch (callbackType) {
     case CallbackType::Function:
-      callback.c(this, mClosure);
+      callback.c(mITimer, mClosure);
       break;
     case CallbackType::Interface:
-      callback.i->Notify(this);
+      callback.i->Notify(mITimer);
       break;
     case CallbackType::Observer:
-      callback.o->Observe(static_cast<nsITimer*>(this),
+      callback.o->Observe(mITimer,
                           NS_TIMER_CALLBACK_TOPIC,
                           nullptr);
       break;
     default:
       ;
   }
 
   // If the callback didn't re-init the timer, and it's not a one-shot timer,
   // restore the callback state.
   if (mCallbackType == CallbackType::Unknown &&
-      mType != TYPE_ONE_SHOT && !mCanceled) {
+      mType != nsITimer::TYPE_ONE_SHOT && !mCanceled) {
     mCallback = callback;
     mCallbackType = callbackType;
   } else {
     // The timer was a one-shot, or the callback was reinitialized.
     if (callbackType == CallbackType::Interface) {
       NS_RELEASE(callback.i);
     } else if (callbackType == CallbackType::Observer) {
       NS_RELEASE(callback.o);
@@ -552,17 +534,17 @@ nsTimerImpl::Fire()
 
   MOZ_LOG(GetTimerLog(), LogLevel::Debug,
          ("[this=%p] Took %fms to fire timer callback\n",
           this, (TimeStamp::Now() - now).ToMilliseconds()));
 
   // Reschedule repeating timers, but make sure that we aren't armed already
   // (which can happen if the callback reinitialized the timer).
   if (IsRepeating() && !mArmed) {
-    if (mType == TYPE_REPEATING_SLACK) {
+    if (mType == nsITimer::TYPE_REPEATING_SLACK) {
       SetDelayInternal(mDelay);  // force mTimeout to be recomputed.  For
     }
     // REPEATING_PRECISE_CAN_SKIP timers this has
     // already happened.
     if (gThread) {
       gThread->AddTimer(this);
     }
   }
@@ -578,36 +560,36 @@ nsTimerImpl::Fire()
 #endif
 
 // See the big comment above GetTimerFiringsLog() to understand this code.
 void
 nsTimerImpl::LogFiring(CallbackType aCallbackType, CallbackUnion aCallback)
 {
   const char* typeStr;
   switch (mType) {
-    case TYPE_ONE_SHOT:                   typeStr = "ONE_SHOT"; break;
-    case TYPE_REPEATING_SLACK:            typeStr = "SLACK   "; break;
-    case TYPE_REPEATING_PRECISE:          /* fall through */
-    case TYPE_REPEATING_PRECISE_CAN_SKIP: typeStr = "PRECISE "; break;
+    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: {
       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>();
 
       } else if (mName.is<NameFunc>()) {
-        mName.as<NameFunc>()(this, mClosure, buf, buflen);
+        mName.as<NameFunc>()(mITimer, mClosure, buf, buflen);
         name = buf;
 
       } else {
         MOZ_ASSERT(mName.is<NameNothing>());
 #ifdef USE_DLADDR
         annotation = "[from dladdr] ";
 
         Dl_info info;
@@ -699,18 +681,22 @@ nsTimerImpl::SetDelayInternal(uint32_t a
     if (mStart.IsNull()) {
       mStart = now;
     } else {
       mStart2 = now;
     }
   }
 }
 
+nsTimer::~nsTimer()
+{
+}
+
 size_t
-nsTimerImpl::SizeOfIncludingThis(mozilla::MallocSizeOf aMallocSizeOf) const
+nsTimer::SizeOfIncludingThis(mozilla::MallocSizeOf aMallocSizeOf) const
 {
   return aMallocSizeOf(this);
 }
 
 /* static */ const nsTimerImpl::NameNothing nsTimerImpl::Nothing = 0;
 
 #ifdef MOZ_TASK_TRACER
 void
@@ -719,10 +705,11 @@ nsTimerImpl::GetTLSTraceInfo()
   mTracedTask.GetTLSTraceInfo();
 }
 
 TracedTaskCommon
 nsTimerImpl::GetTracedTask()
 {
   return mTracedTask;
 }
+
 #endif
 
--- a/xpcom/threads/nsTimerImpl.h
+++ b/xpcom/threads/nsTimerImpl.h
@@ -27,36 +27,36 @@ extern mozilla::LogModule* GetTimerLog()
 #define NS_TIMER_CID \
 { /* 5ff24248-1dd2-11b2-8427-fbab44f29bc8 */         \
      0x5ff24248,                                     \
      0x1dd2,                                         \
      0x11b2,                                         \
     {0x84, 0x27, 0xfb, 0xab, 0x44, 0xf2, 0x9b, 0xc8} \
 }
 
-class nsTimerImpl final : public nsITimer
+// 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();
 public:
   typedef mozilla::TimeStamp TimeStamp;
 
-  nsTimerImpl();
+  explicit nsTimerImpl(nsITimer* aTimer);
+  NS_INLINE_DECL_THREADSAFE_REFCOUNTING(nsTimerImpl)
+  NS_DECL_NON_VIRTUAL_NSITIMER
 
   static nsresult Startup();
   static void Shutdown();
 
   friend class TimerThread;
   friend class nsTimerEvent;
   friend struct TimerAdditionComparator;
 
-  NS_DECL_THREADSAFE_ISUPPORTS
-  NS_DECL_NSITIMER
-
-  virtual size_t SizeOfIncludingThis(mozilla::MallocSizeOf aMallocSizeOf) const override;
-
-private:
   void SetDelayInternal(uint32_t aDelay);
 
   void Fire();
 
 #ifdef MOZ_TASK_TRACER
   void GetTLSTraceInfo();
   mozilla::tasktracer::TracedTaskCommon GetTracedTask();
 #endif
@@ -72,17 +72,16 @@ private:
 
   enum class CallbackType : uint8_t {
     Unknown = 0,
     Interface = 1,
     Function = 2,
     Observer = 3,
   };
 
-  ~nsTimerImpl();
   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;
@@ -90,30 +89,39 @@ private:
 
     if (cbType == CallbackType::Interface) {
       NS_RELEASE(mCallback.i);
     } else if (cbType == CallbackType::Observer) {
       NS_RELEASE(mCallback.o);
     }
   }
 
+  // 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(TYPE_ONE_SHOT < TYPE_REPEATING_SLACK,
-                  "invalid ordering of timer types!");
-    static_assert(TYPE_REPEATING_SLACK < TYPE_REPEATING_PRECISE,
+    static_assert(nsITimer::TYPE_ONE_SHOT < nsITimer::TYPE_REPEATING_SLACK,
                   "invalid ordering of timer types!");
-    static_assert(TYPE_REPEATING_PRECISE < TYPE_REPEATING_PRECISE_CAN_SKIP,
-                  "invalid ordering of timer types!");
-    return mType >= TYPE_REPEATING_SLACK;
+    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 >= TYPE_REPEATING_PRECISE;
+    return mType >= nsITimer::TYPE_REPEATING_PRECISE;
   }
 
   nsCOMPtr<nsIEventTarget> mEventTarget;
 
   void*                 mClosure;
 
   union CallbackUnion
   {
@@ -176,11 +184,33 @@ private:
 #ifdef MOZ_TASK_TRACER
   mozilla::tasktracer::TracedTaskCommon mTracedTask;
 #endif
 
   TimeStamp             mStart, mStart2;
   static double         sDeltaSum;
   static double         sDeltaSumSquared;
   static double         sDeltaNum;
+  RefPtr<nsITimer>      mITimer;
+};
+
+class nsTimer final : public nsITimer
+{
+  virtual ~nsTimer();
+public:
+  nsTimer() : mImpl(new nsTimerImpl(this)) {}
+
+  friend class TimerThread;
+  friend class nsTimerEvent;
+  friend struct TimerAdditionComparator;
+
+  NS_DECL_THREADSAFE_ISUPPORTS
+  NS_FORWARD_SAFE_NSITIMER(mImpl);
+
+  virtual size_t SizeOfIncludingThis(mozilla::MallocSizeOf aMallocSizeOf) const override;
+
+private:
+  // nsTimerImpl holds a strong ref to us. When our refcount goes to 1, we will
+  // null this to break the cycle.
+  RefPtr<nsTimerImpl> mImpl;
 };
 
 #endif /* nsTimerImpl_h___ */