Bug 330128. Calling cancel() on a timer doesn't drop ref to the callback. r=brendan, sr=bzbarsky, a=beltzner
authorsayrer@gmail.com
Wed, 24 Oct 2007 12:11:41 -0700
changeset 7133 1f816e5ca7c07115ad8d5a5c908ac7ace280af49
parent 7132 6ba2fed95e002cc4e0620499aadc9e17c0fe52d9
child 7134 9eb0c777cd3b43c9eb7e440199616ca26d342a7b
push idunknown
push userunknown
push dateunknown
reviewersbrendan, bzbarsky, beltzner
bugs330128
milestone1.9a9pre
Bug 330128. Calling cancel() on a timer doesn't drop ref to the callback. r=brendan, sr=bzbarsky, a=beltzner
xpcom/threads/nsTimerImpl.cpp
xpcom/threads/nsTimerImpl.h
--- a/xpcom/threads/nsTimerImpl.cpp
+++ b/xpcom/threads/nsTimerImpl.cpp
@@ -280,16 +280,18 @@ NS_IMETHODIMP nsTimerImpl::Init(nsIObser
 
 NS_IMETHODIMP nsTimerImpl::Cancel()
 {
   mCanceled = PR_TRUE;
 
   if (gThread)
     gThread->RemoveTimer(this);
 
+  ReleaseCallback();
+
   return NS_OK;
 }
 
 NS_IMETHODIMP nsTimerImpl::SetDelay(PRUint32 aDelay)
 {
   // If we're already repeating precisely, update mTimeout now so that the
   // new delay takes effect in the future.
   if (mTimeout != 0 && mType == TYPE_REPEATING_PRECISE)
@@ -373,32 +375,55 @@ void nsTimerImpl::Fire()
     // Precise repeating timers advance mTimeout by mDelay without fail before
     // calling Fire().
     timeout -= PR_MillisecondsToInterval(mDelay);
   }
   if (gThread)
     gThread->UpdateFilter(mDelay, timeout, now);
 
   mFiring = PR_TRUE;
+  
+  // Handle callbacks that re-init the timer, but avoid leaking.
+  // See bug 330128.
+  CallbackUnion callback = mCallback;
+  PRUintn callbackType = mCallbackType;
+  if (callbackType == CALLBACK_TYPE_INTERFACE)
+    NS_ADDREF(callback.i);
+  else if (callbackType == CALLBACK_TYPE_OBSERVER)
+    NS_ADDREF(callback.o);
+  ReleaseCallback();
 
-  switch (mCallbackType) {
+  switch (callbackType) {
     case CALLBACK_TYPE_FUNC:
-      mCallback.c(this, mClosure);
+      callback.c(this, mClosure);
       break;
     case CALLBACK_TYPE_INTERFACE:
-      mCallback.i->Notify(this);
+      callback.i->Notify(this);
       break;
     case CALLBACK_TYPE_OBSERVER:
-      mCallback.o->Observe(static_cast<nsITimer*>(this),
-                           NS_TIMER_CALLBACK_TOPIC,
-                           nsnull);
+      callback.o->Observe(static_cast<nsITimer*>(this),
+                          NS_TIMER_CALLBACK_TOPIC,
+                          nsnull);
       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 == CALLBACK_TYPE_UNKNOWN && callbackType != TYPE_ONE_SHOT) {
+    mCallback = callback;
+    mCallbackType = callbackType;
+  } else {
+    // The timer was a one-shot, or the callback was reinitialized.
+    if (callbackType == CALLBACK_TYPE_INTERFACE)
+      NS_RELEASE(callback.i);
+    else if (callbackType == CALLBACK_TYPE_OBSERVER)
+      NS_RELEASE(callback.o);
+  }
+
   mFiring = PR_FALSE;
 
 #ifdef DEBUG_TIMERS
   if (PR_LOG_TEST(gTimerLog, PR_LOG_DEBUG)) {
     PR_LOG(gTimerLog, PR_LOG_DEBUG,
            ("[this=%p] Took %dms to fire timer callback\n",
             this, PR_IntervalToMilliseconds(PR_IntervalNow() - now)));
   }
--- a/xpcom/threads/nsTimerImpl.h
+++ b/xpcom/threads/nsTimerImpl.h
@@ -109,23 +109,24 @@ private:
   nsresult InitCommon(PRUint32 aType, PRUint32 aDelay);
 
   void ReleaseCallback()
   {
     if (mCallbackType == CALLBACK_TYPE_INTERFACE)
       NS_RELEASE(mCallback.i);
     else if (mCallbackType == CALLBACK_TYPE_OBSERVER)
       NS_RELEASE(mCallback.o);
+    mCallbackType = CALLBACK_TYPE_UNKNOWN;
   }
 
   nsCOMPtr<nsIThread>   mCallingThread;
 
   void *                mClosure;
 
-  union {
+  union CallbackUnion {
     nsTimerCallbackFunc c;
     nsITimerCallback *  i;
     nsIObserver *       o;
   } mCallback;
 
   // These members are set by Init (called from NS_NewTimer) and never reset.
   PRUint8               mCallbackType;