Bug 1157323 - Part 2: Factor nsTimerImpl into two classes, so we don't need to do racy stuff in nsTimerImpl::Release. draft
authorByron Campen [:bwc] <docfaraday@gmail.com>
Wed, 20 Jul 2016 15:16:40 -0500
changeset 417173 16bfb247826fffdbc508e559fade3fdbac29976e
parent 417172 3df22718358899ab015d28d1c387e7cc6c0f99ca
child 417174 86690a6c6411d2eb6ad12017ee52b74b7db3d2fa
push id30354
push userbcampen@mozilla.com
push dateFri, 23 Sep 2016 19:02:51 +0000
bugs1157323
milestone52.0a1
Bug 1157323 - Part 2: Factor nsTimerImpl into two classes, so we don't need to do racy stuff in nsTimerImpl::Release. 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___ */