From 0cf5031cd7a057d3ecb013e0dfda6bffacd16261 Mon Sep 17 00:00:00 2001 From: "Byron Campen [:bwc]" Date: Wed, 20 Jul 2016 15:16:40 -0500 Subject: [PATCH] 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 --HG-- extra : rebase_source : 4caeb1ffc41b0704e5c1c111ef869d8edfb6d30c --- xpcom/build/XPCOMInit.cpp | 2 +- xpcom/build/XPCOMModule.inc | 2 +- xpcom/threads/nsTimerImpl.cpp | 117 +++++++++++++++------------------- xpcom/threads/nsTimerImpl.h | 62 +++++++++++++----- 4 files changed, 100 insertions(+), 83 deletions(-) diff --git a/xpcom/build/XPCOMInit.cpp b/xpcom/build/XPCOMInit.cpp index 898f1152029b..1dda9e819964 100644 --- a/xpcom/build/XPCOMInit.cpp +++ b/xpcom/build/XPCOMInit.cpp @@ -207,7 +207,7 @@ 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) diff --git a/xpcom/build/XPCOMModule.inc b/xpcom/build/XPCOMModule.inc index e0aa6b8496e3..5513d9d9b156 100644 --- a/xpcom/build/XPCOMModule.inc +++ b/xpcom/build/XPCOMModule.inc @@ -22,7 +22,7 @@ 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) diff --git a/xpcom/threads/nsTimerImpl.cpp b/xpcom/threads/nsTimerImpl.cpp index a9594ae70e38..d07bd67f03f5 100644 --- a/xpcom/threads/nsTimerImpl.cpp +++ b/xpcom/threads/nsTimerImpl.cpp @@ -112,68 +112,30 @@ myNS_MeanAndStdDev(double n, double sumOfValues, double sumOfSquaredValues, *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; + nsrefcnt count = --mRefCnt; + NS_LOG_RELEASE(this, count, "nsTimer"); - 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); */ + 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 0; - } - - // 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; - } } return count; } -nsTimerImpl::nsTimerImpl() : +nsTimerImpl::nsTimerImpl(nsITimer* aTimer) : mClosure(nullptr), mName(nsTimerImpl::Nothing), mCallbackType(CallbackType::Unknown), @@ -181,8 +143,10 @@ nsTimerImpl::nsTimerImpl() : 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(NS_GetCurrentThread()); @@ -191,6 +155,7 @@ nsTimerImpl::nsTimerImpl() : nsTimerImpl::~nsTimerImpl() { + MOZ_COUNT_DTOR(nsTimerImpl); ReleaseCallback(); } @@ -362,10 +327,27 @@ nsTimerImpl::Cancel() 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 " @@ -518,13 +500,13 @@ nsTimerImpl::Fire() 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(this), + callback.o->Observe(mITimer, NS_TIMER_CALLBACK_TOPIC, nullptr); break; @@ -535,7 +517,7 @@ nsTimerImpl::Fire() // 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 { @@ -557,7 +539,7 @@ nsTimerImpl::Fire() // 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 @@ -583,10 +565,10 @@ 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"); } @@ -602,7 +584,7 @@ nsTimerImpl::LogFiring(CallbackType aCallbackType, CallbackUnion aCallback) name = mName.as(); } else if (mName.is()) { - mName.as()(this, mClosure, buf, buflen); + mName.as()(mITimer, mClosure, buf, buflen); name = buf; } else { @@ -704,8 +686,12 @@ nsTimerImpl::SetDelayInternal(uint32_t aDelay) } } +nsTimer::~nsTimer() +{ +} + size_t -nsTimerImpl::SizeOfIncludingThis(mozilla::MallocSizeOf aMallocSizeOf) const +nsTimer::SizeOfIncludingThis(mozilla::MallocSizeOf aMallocSizeOf) const { return aMallocSizeOf(this); } @@ -724,5 +710,6 @@ nsTimerImpl::GetTracedTask() { return mTracedTask; } + #endif diff --git a/xpcom/threads/nsTimerImpl.h b/xpcom/threads/nsTimerImpl.h index 71067ab0d5f7..afea8a2def65 100644 --- a/xpcom/threads/nsTimerImpl.h +++ b/xpcom/threads/nsTimerImpl.h @@ -32,12 +32,18 @@ extern mozilla::LogModule* GetTimerLog(); {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(); @@ -46,12 +52,6 @@ public: 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(); @@ -77,7 +77,6 @@ private: Observer = 3, }; - ~nsTimerImpl(); nsresult InitCommon(uint32_t aDelay, uint32_t aType); void ReleaseCallback() @@ -95,20 +94,29 @@ private: } } + // 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, + static_assert(nsITimer::TYPE_ONE_SHOT < nsITimer::TYPE_REPEATING_SLACK, "invalid ordering of timer types!"); - static_assert(TYPE_REPEATING_SLACK < TYPE_REPEATING_PRECISE, - "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 mEventTarget; @@ -181,6 +189,28 @@ private: static double sDeltaSum; static double sDeltaSumSquared; static double sDeltaNum; + RefPtr 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 mImpl; }; #endif /* nsTimerImpl_h___ */