From 70708278664a3ecc4146c05613438f3bad2a3113 Mon Sep 17 00:00:00 2001
From: JW Wang <jwwang@mozilla.com>
Date: Fri, 18 Mar 2016 11:27:15 +0800
Subject: [PATCH] Bug 1257063 - Don't destruct the runnable inside the lock
 when TaskQueue::Dispatch fails. r=bobbyholley.

MozReview-Commit-ID: I5KvfHnLUIS
---
 dom/media/FlushableTaskQueue.cpp | 20 +++++++++++++-------
 xpcom/threads/TaskQueue.cpp      | 13 +++++++------
 xpcom/threads/TaskQueue.h        | 18 +++++++++++++-----
 3 files changed, 33 insertions(+), 18 deletions(-)

diff --git a/dom/media/FlushableTaskQueue.cpp b/dom/media/FlushableTaskQueue.cpp
index 6443045fdd84..b28340d964d5 100644
--- a/dom/media/FlushableTaskQueue.cpp
+++ b/dom/media/FlushableTaskQueue.cpp
@@ -20,13 +20,19 @@ FlushableTaskQueue::Flush()
 nsresult
 FlushableTaskQueue::FlushAndDispatch(already_AddRefed<nsIRunnable> aRunnable)
 {
-  MonitorAutoLock mon(mQueueMonitor);
-  AutoSetFlushing autoFlush(this);
-  FlushLocked();
-  nsCOMPtr<nsIRunnable> r = dont_AddRef(aRunnable.take());
-  nsresult rv = DispatchLocked(r.forget(), IgnoreFlushing, AssertDispatchSuccess);
-  NS_ENSURE_SUCCESS(rv, rv);
-  AwaitIdleLocked();
+  nsCOMPtr<nsIRunnable> r = aRunnable;
+  {
+    MonitorAutoLock mon(mQueueMonitor);
+    AutoSetFlushing autoFlush(this);
+    FlushLocked();
+    nsresult rv = DispatchLocked(/* passed by ref */r, IgnoreFlushing, AssertDispatchSuccess);
+    NS_ENSURE_SUCCESS(rv, rv);
+    AwaitIdleLocked();
+  }
+  // If the ownership of |r| is not transferred in DispatchLocked() due to
+  // dispatch failure, it will be deleted here outside the lock. We do so
+  // since the destructor of the runnable might access TaskQueue and result
+  // in deadlocks.
   return NS_OK;
 }
 
diff --git a/xpcom/threads/TaskQueue.cpp b/xpcom/threads/TaskQueue.cpp
index 944dfe78f203..169323a9c6eb 100644
--- a/xpcom/threads/TaskQueue.cpp
+++ b/xpcom/threads/TaskQueue.cpp
@@ -39,15 +39,16 @@ TaskQueue::TailDispatcher()
   return *mTailDispatcher;
 }
 
+// Note aRunnable is passed by ref to support conditional ownership transfer.
+// See Dispatch() in TaskQueue.h for more details.
 nsresult
-TaskQueue::DispatchLocked(already_AddRefed<nsIRunnable> aRunnable,
-                               DispatchMode aMode, DispatchFailureHandling aFailureHandling,
-                               DispatchReason aReason)
+TaskQueue::DispatchLocked(nsCOMPtr<nsIRunnable>& aRunnable,
+                          DispatchMode aMode, DispatchFailureHandling aFailureHandling,
+                          DispatchReason aReason)
 {
-  nsCOMPtr<nsIRunnable> r = aRunnable;
   AbstractThread* currentThread;
   if (aReason != TailDispatch && (currentThread = GetCurrent()) && RequiresTailDispatch(currentThread)) {
-    currentThread->TailDispatcher().AddTask(this, r.forget(), aFailureHandling);
+    currentThread->TailDispatcher().AddTask(this, aRunnable.forget(), aFailureHandling);
     return NS_OK;
   }
 
@@ -58,7 +59,7 @@ TaskQueue::DispatchLocked(already_AddRefed<nsIRunnable> aRunnable,
   if (mIsShutdown) {
     return NS_ERROR_FAILURE;
   }
-  mTasks.push(r.forget());
+  mTasks.push(aRunnable.forget());
   if (mIsRunning) {
     return NS_OK;
   }
diff --git a/xpcom/threads/TaskQueue.h b/xpcom/threads/TaskQueue.h
index 7d68231c9da0..279652f66888 100644
--- a/xpcom/threads/TaskQueue.h
+++ b/xpcom/threads/TaskQueue.h
@@ -43,10 +43,17 @@ public:
                 DispatchFailureHandling aFailureHandling = AssertDispatchSuccess,
                 DispatchReason aReason = NormalDispatch) override
   {
-    MonitorAutoLock mon(mQueueMonitor);
-    nsresult rv = DispatchLocked(Move(aRunnable), AbortIfFlushing, aFailureHandling, aReason);
-    MOZ_DIAGNOSTIC_ASSERT(aFailureHandling == DontAssertDispatchSuccess || NS_SUCCEEDED(rv));
-    Unused << rv;
+    nsCOMPtr<nsIRunnable> r = aRunnable;
+    {
+      MonitorAutoLock mon(mQueueMonitor);
+      nsresult rv = DispatchLocked(/* passed by ref */r, AbortIfFlushing, aFailureHandling, aReason);
+      MOZ_DIAGNOSTIC_ASSERT(aFailureHandling == DontAssertDispatchSuccess || NS_SUCCEEDED(rv));
+      Unused << rv;
+    }
+    // If the ownership of |r| is not transferred in DispatchLocked() due to
+    // dispatch failure, it will be deleted here outside the lock. We do so
+    // since the destructor of the runnable might access TaskQueue and result
+    // in deadlocks.
   }
 
   // Puts the queue in a shutdown state and returns immediately. The queue will
@@ -81,7 +88,8 @@ protected:
 
   enum DispatchMode { AbortIfFlushing, IgnoreFlushing };
 
-  nsresult DispatchLocked(already_AddRefed<nsIRunnable> aRunnable, DispatchMode aMode,
+  nsresult DispatchLocked(nsCOMPtr<nsIRunnable>& aRunnable,
+                          DispatchMode aMode,
                           DispatchFailureHandling aFailureHandling,
                           DispatchReason aReason = NormalDispatch);