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
--- 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___ */