commit 24b9acce7908ed422611abb09b77ae5ae222ce0a Author: Byron Campen [:bwc] docfaraday@gmail.com Date: Mon Jun 16 14:13:04 2014 -0700

Bug 1024765 - Part 2: Make refcounting logic around PostTimerEvent more explicit. r=bz, a=sledru --- xpcom/threads/TimerThread.cpp | 17 +++++++---- xpcom/threads/nsTimerImpl.cpp | 67 +++++++++++++++++++++++++++-------------- xpcom/threads/nsTimerImpl.h | 4 ++- 3 files changed, 59 insertions(+), 29 deletions(-)

diff --git a/xpcom/threads/TimerThread.cpp b/xpcom/threads/TimerThread.cpp index f8a7cb7..ebf5751 100644 --- a/xpcom/threads/TimerThread.cpp +++ b/xpcom/threads/TimerThread.cpp @@ -211,8 +211,9 @@ NS_IMETHODIMP TimerThread::Run() // must be racing with us, blocked in gThread->RemoveTimer waiting // for TimerThread::mMonitor, under nsTimerImpl::Release.

- NS_ADDREF(timer); + nsRefPtr<nsTimerImpl> timerRef(timer); RemoveTimerInternal(timer); + timer = nullptr;

{ // We release mMonitor around the Fire call to avoid deadlock. @@ -222,16 +223,21 @@ NS_IMETHODIMP TimerThread::Run() if (PR_LOG_TEST(GetTimerLog(), PR_LOG_DEBUG)) { PR_LOG(GetTimerLog(), PR_LOG_DEBUG, ("Timer thread woke up %fms from when it was supposed to\n", - fabs((now - timer->mTimeout).ToMilliseconds()))); + fabs((now - timerRef->mTimeout).ToMilliseconds()))); } #endif

// We are going to let the call to PostTimerEvent here handle the // release of the timer so that we don't end up releasing the timer // on the TimerThread instead of on the thread it targets. - if (NS_FAILED(timer->PostTimerEvent())) { - nsrefcnt rc; - NS_RELEASE2(timer, rc); + timerRef = nsTimerImpl::PostTimerEvent(timerRef.forget()); + + if (timerRef) { + // We got our reference back due to an error. + // Unhook the nsRefPtr, and release manually so we can get the + // refcount. + nsrefcnt rc = timerRef.forget().get()->Release(); + (void)rc;

// The nsITimer interface requires that its users keep a reference // to the timers they use while those timers are initialized but @@ -246,7 +252,6 @@ NS_IMETHODIMP TimerThread::Run() // preventing this situation from occurring. MOZ_ASSERT(rc != 0, "destroyed timer off its target thread!"); } - timer = nullptr; }

if (mShutdown) diff --git a/xpcom/threads/nsTimerImpl.cpp b/xpcom/threads/nsTimerImpl.cpp index fd4b391..717f280 100644 --- a/xpcom/threads/nsTimerImpl.cpp +++ b/xpcom/threads/nsTimerImpl.cpp @@ -105,9 +105,10 @@ class nsTimerEvent : public nsRunnable { public: NS_IMETHOD Run();

- nsTimerEvent(nsTimerImpl *timer, int32_t generation) - : mTimer(dont_AddRef(timer)), mGeneration(generation) { - // timer is already addref'd for us + nsTimerEvent() + : mTimer() + , mGeneration(0) + { MOZ_COUNT_CTOR(nsTimerEvent);

MOZ_ASSERT(gThread->IsOnTimerThread(), @@ -132,8 +133,18 @@ public: DeleteAllocatorIfNeeded(); }

+ already_AddRefed<nsTimerImpl> ForgetTimer() + { + return mTimer.forget(); + } + + void SetTimer(already_AddRefed<nsTimerImpl> aTimer) + { + mTimer = aTimer; + mGeneration = mTimer->GetGeneration(); + } + private: - nsTimerEvent(); // Not implemented ~nsTimerEvent() { MOZ_COUNT_DTOR(nsTimerEvent);

@@ -629,23 +640,27 @@ NS_IMETHODIMP nsTimerEvent::Run() return NS_OK; }

-nsresult nsTimerImpl::PostTimerEvent() +already_AddRefed<nsTimerImpl> nsTimerImpl::PostTimerEvent(already_AddRefed<nsTimerImpl> aTimerRef) { - if (!mEventTarget) { + nsRefPtr<nsTimerImpl> timer(aTimerRef); + if (!timer->mEventTarget) { NS_ERROR("Attempt to post timer event to NULL event target"); - return NS_ERROR_NOT_INITIALIZED; + return timer.forget(); }

// XXX we may want to reuse this nsTimerEvent in the case of repeating timers.

- // Since TimerThread addref'd 'this' for us, we don't need to addref here. - // We will release in destroyMyEvent. We need to copy the generation number - // from this timer into the event, so we can avoid firing a timer that was - // re-initialized after being canceled. + // Since TimerThread addref'd 'timer' for us, we don't need to addref here. + // We will release either in ~nsTimerEvent(), or pass the reference back to + // the caller. We need to copy the generation number from this timer into the + // event, so we can avoid firing a timer that was re-initialized after being + // canceled.

- nsRefPtr<nsTimerEvent> event = new nsTimerEvent(this, mGeneration); + // Note: We override operator new for this class, and the override is + // fallible! + nsRefPtr<nsTimerEvent> event = new nsTimerEvent; if (!event) - return NS_ERROR_OUT_OF_MEMORY; + return timer.forget();

#ifdef DEBUG_TIMERS if (PR_LOG_TEST(GetTimerLog(), PR_LOG_DEBUG)) { @@ -655,21 +670,29 @@ nsresult nsTimerImpl::PostTimerEvent()

// If this is a repeating precise timer, we need to calculate the time for // the next timer to fire before we make the callback. - if (IsRepeatingPrecisely()) { - SetDelayInternal(mDelay); + if (timer->IsRepeatingPrecisely()) { + timer->SetDelayInternal(timer->mDelay);

// But only re-arm REPEATING_PRECISE timers. - if (gThread && mType == TYPE_REPEATING_PRECISE) { - nsresult rv = gThread->AddTimer(this); + if (gThread && timer->mType == TYPE_REPEATING_PRECISE) { + nsresult rv = gThread->AddTimer(timer); if (NS_FAILED(rv)) - return rv; + return timer.forget(); } }

- nsresult rv = mEventTarget->Dispatch(event, NS_DISPATCH_NORMAL); - if (NS_FAILED(rv) && gThread) - gThread->RemoveTimer(this); - return rv; + nsIEventTarget* target = timer->mEventTarget; + event->SetTimer(timer.forget()); + + nsresult rv = target->Dispatch(event, NS_DISPATCH_NORMAL); + if (NS_FAILED(rv)) { + timer = event->ForgetTimer(); + if (gThread) + gThread->RemoveTimer(timer); + } + return timer.forget(); + + return nullptr; }

void nsTimerImpl::SetDelayInternal(uint32_t aDelay) diff --git a/xpcom/threads/nsTimerImpl.h b/xpcom/threads/nsTimerImpl.h index c318cf1..4a24060 100644 --- a/xpcom/threads/nsTimerImpl.h +++ b/xpcom/threads/nsTimerImpl.h @@ -54,7 +54,9 @@ public: friend struct TimerAdditionComparator;

void Fire(); - nsresult PostTimerEvent(); + // If a failure is encountered, the reference is returned to the caller + static already_AddRefed<nsTimerImpl> PostTimerEvent( + already_AddRefed<nsTimerImpl> aTimerRef); void SetDelayInternal(uint32_t aDelay);

NS_DECL_ISUPPORTS