Bug 1151080: Rewrite NR_async_timer_set(0) to use direct dispatch. r=mt

media/mtransport/nr_timer.cpp

@@ -67,20 +67,15 @@ extern "C" {
 
 namespace mozilla {
 
-class nrappkitTimerCallback : public nsITimerCallback
-{
-public:
-  // We're going to release ourself in the callback, so we need to be threadsafe
-  NS_DECL_THREADSAFE_ISUPPORTS
-  NS_DECL_NSITIMERCALLBACK
-
-  nrappkitTimerCallback(NR_async_cb cb, void *cb_arg,
-                        const char *function, int line)
+class nrappkitCallback  {
+ public:
+  nrappkitCallback(NR_async_cb cb, void *cb_arg,
+                   const char *function, int line)
     : cb_(cb), cb_arg_(cb_arg), function_(function), line_(line) {
   }
+  virtual ~nrappkitCallback() {}
 
-private:
-  virtual ~nrappkitTimerCallback() {}
+  virtual void Cancel() = 0;
 
 protected:
   /* additional members */
@@ -90,38 +85,113 @@ protected:
   int line_;
 };
 
+class nrappkitTimerCallback : public nrappkitCallback,
+                              public nsITimerCallback {
+ public:
+  // We're going to release ourself in the callback, so we need to be threadsafe
+  NS_DECL_THREADSAFE_ISUPPORTS
+  NS_DECL_NSITIMERCALLBACK
+
+  nrappkitTimerCallback(NR_async_cb cb, void *cb_arg,
+                        const char *function, int line,
+                        nsITimer *timer)
+      : nrappkitCallback(cb, cb_arg, function, line),
+        timer_(timer) {}
+
+  virtual void Cancel() override {
+    AddRef();  // Cancelling the timer causes the callback it holds to
+               // be released. AddRef() keeps us alive.
+    timer_->Cancel();
+    timer_->Release();
+    Release(); // Will cause deletion of this object.
+  }
+
+ private:
+  nsITimer* timer_;
+  virtual ~nrappkitTimerCallback() {}
+};
+
 NS_IMPL_ISUPPORTS(nrappkitTimerCallback, nsITimerCallback)
 
 NS_IMETHODIMP nrappkitTimerCallback::Notify(nsITimer *timer) {
   r_log(LOG_GENERIC, LOG_DEBUG, "Timer callback fired (set in %s:%d)",
         function_.c_str(), line_);
-
+  MOZ_ASSERT(timer == timer_);
   cb_(0, 0, cb_arg_);
 
   // Allow the timer to go away.
   timer->Release();
   return NS_OK;
 }
+
+class nrappkitScheduledCallback : public nrappkitCallback {
+ public:
+
+  nrappkitScheduledCallback(NR_async_cb cb, void *cb_arg,
+                            const char *function, int line)
+      : nrappkitCallback(cb, cb_arg, function, line) {}
+
+  void Run() {
+    if (cb_) {
+      cb_(0, 0, cb_arg_);
+    }
+  }
+
+  virtual void Cancel() override {
+    cb_ = nullptr;
+  }
+
+  ~nrappkitScheduledCallback() {}
+};
+
 }  // close namespace
 
 
 using namespace mozilla;
 
-// These timers must only be used from the STS thread.
-// This function is a helper that enforces that.
-static void CheckSTSThread() {
+static nsCOMPtr<nsIEventTarget> GetSTSThread() {
   nsresult rv;
 
   nsCOMPtr<nsIEventTarget> sts_thread;
 
   sts_thread = do_GetService(NS_SOCKETTRANSPORTSERVICE_CONTRACTID, &rv);
-
   MOZ_ASSERT(NS_SUCCEEDED(rv));
+
+  return sts_thread;
+}
+
+// These timers must only be used from the STS thread.
+// This function is a helper that enforces that.
+static void CheckSTSThread() {
+  nsCOMPtr<nsIEventTarget> sts_thread = GetSTSThread();
+
   ASSERT_ON_THREAD(sts_thread);
 }
 
-int NR_async_timer_set(int timeout, NR_async_cb cb, void *arg, char *func,
-                       int l, void **handle) {
+static int nr_async_timer_set_zero(NR_async_cb cb, void *arg,
+                                   char *func, int l,
+                                   nrappkitCallback **handle) {
+  nrappkitScheduledCallback* callback(new nrappkitScheduledCallback(
+      cb, arg, func, l));
+
+  nsresult rv = GetSTSThread()->Dispatch(WrapRunnable(
+      nsAutoPtr<nrappkitScheduledCallback>(callback),
+      &nrappkitScheduledCallback::Run),
+                        NS_DISPATCH_NORMAL);
+  if (NS_FAILED(rv))
+    return R_FAILED;
+
+  *handle = callback;
+
+  // On exit to this function, the only strong reference to callback is in
+  // the Runnable. Because we are redispatching to the same thread,
+  // this is always safe.
+  return 0;
+}
+
+static int nr_async_timer_set_nonzero(int timeout, NR_async_cb cb, void *arg,
+                                      char *func, int l,
+                                      nrappkitCallback **handle) {
   nsresult rv;
   CheckSTSThread();
 
@@ -130,8 +200,9 @@ int NR_async_timer_set(int timeout, NR_async_cb cb, void *arg, char *func,
     return(R_FAILED);
   }
 
-  rv = timer->InitWithCallback(new nrappkitTimerCallback(cb, arg, func, l),
-                               timeout, nsITimer::TYPE_ONE_SHOT);
+  nrappkitTimerCallback* callback =
+      new nrappkitTimerCallback(cb, arg, func, l, timer);
+  rv = timer->InitWithCallback(callback, timeout, nsITimer::TYPE_ONE_SHOT);
   if (NS_FAILED(rv)) {
     return R_FAILED;
   }
@@ -139,11 +210,29 @@ int NR_async_timer_set(int timeout, NR_async_cb cb, void *arg, char *func,
   // We need an AddRef here to keep the timer alive, per the spec.
   timer->AddRef();
 
+  *handle = callback;
+
+  return 0;
+}
+
+int NR_async_timer_set(int timeout, NR_async_cb cb, void *arg,
+                       char *func, int l, void **handle) {
+  CheckSTSThread();
+
+  nrappkitCallback *callback;
+  int r;
+
+  if (!timeout) {
+    r = nr_async_timer_set_zero(cb, arg, func, l, &callback);
+  } else {
+    r = nr_async_timer_set_nonzero(timeout, cb, arg, func, l, &callback);
+  }
+
+  if (r)
+    return r;
+
   if (handle)
-    *handle = timer.get();
-  // Bug 818806: if we have no handle to the timer, we have no way to avoid
-  // it leaking (though not the callback object) if it never fires (or if
-  // we exit before it fires).
+    *handle = callback;
 
   return 0;
 }
@@ -163,11 +252,8 @@ int NR_async_timer_cancel(void *handle) {
 
   CheckSTSThread();
 
-  nsITimer *timer = static_cast<nsITimer *>(handle);
-
-  timer->Cancel();
-  // Allow the timer to go away.
-  timer->Release();
+  nrappkitCallback* callback = static_cast<nrappkitCallback *>(handle);
+  callback->Cancel();
 
   return 0;
 }

media/mtransport/test/nrappkit_unittest.cpp

@@ -44,10 +44,29 @@ class TimerTest : public ::testing::Test {
     return ret;
   }
 
+  int ArmCancelTimer(int timeout) {
+    int ret;
+
+    test_utils->sts_target()->Dispatch(
+        WrapRunnableRet(this, &TimerTest::ArmCancelTimer_w, timeout, &ret),
+        NS_DISPATCH_SYNC);
+
+    return ret;
+  }
+
   int ArmTimer_w(int timeout) {
     return NR_ASYNC_TIMER_SET(timeout, cb, this, &handle_);
   }
 
+  int ArmCancelTimer_w(int timeout) {
+    int r;
+    r = ArmTimer_w(timeout);
+    if (r)
+      return r;
+
+    return CancelTimer_w();
+  }
+
   int CancelTimer() {
     int ret;
 
@@ -74,7 +93,7 @@ class TimerTest : public ::testing::Test {
 
   int Schedule_w() {
     NR_ASYNC_SCHEDULE(cb, this);
-    
+
     return 0;
   }
 
@@ -105,6 +124,12 @@ TEST_F(TimerTest, CancelTimer) {
   ASSERT_FALSE(fired_);
 }
 
+TEST_F(TimerTest, CancelTimer0) {
+  ArmCancelTimer(0);
+  PR_Sleep(100);
+  ASSERT_FALSE(fired_);
+}
+
 TEST_F(TimerTest, ScheduleTest) {
   Schedule();
   ASSERT_TRUE_WAIT(fired_, 1000);