Bug 1416724 - part 1 - AbstractThread::Dispatch should propage errors if failing the dispatching of Runnables, r=jwwang

xpcom/tests/gtest/TestMozPromise.cpp

@@ -9,6 +9,7 @@
 
 #include "mozilla/TaskQueue.h"
 #include "mozilla/MozPromise.h"
+#include "mozilla/Unused.h"
 
 #include "nsISupportsImpl.h"
 #include "mozilla/SharedThreadPool.h"
@@ -60,12 +61,11 @@ public:
 
     if (--mIterations == 0) {
       mPromise->ResolveOrReject(mValue, __func__);
-    } else {
+      return NS_OK;
-      nsCOMPtr<nsIRunnable> r = this;
-      mTaskQueue->Dispatch(r.forget());
     }
 
-    return NS_OK;
+    nsCOMPtr<nsIRunnable> r = this;
+    return mTaskQueue->Dispatch(r.forget());
   }
 
   void Cancel() {
@@ -87,7 +87,7 @@ void
 RunOnTaskQueue(TaskQueue* aQueue, FunctionType aFun)
 {
   nsCOMPtr<nsIRunnable> r = NS_NewRunnableFunction("RunOnTaskQueue", aFun);
-  aQueue->Dispatch(r.forget());
+  Unused << aQueue->Dispatch(r.forget());
 }
 
 // std::function can't come soon enough. :-(
@@ -162,11 +162,11 @@ TEST(MozPromise, AsyncResolve)
     RefPtr<DelayedResolveOrReject> c = new DelayedResolveOrReject(queue, p, RRValue::MakeReject(32.0), 7);
 
     nsCOMPtr<nsIRunnable> ref = a.get();
-    queue->Dispatch(ref.forget());
+    Unused << queue->Dispatch(ref.forget());
     ref = b.get();
-    queue->Dispatch(ref.forget());
+    Unused << queue->Dispatch(ref.forget());
     ref = c.get();
-    queue->Dispatch(ref.forget());
+    Unused << queue->Dispatch(ref.forget());
 
     p->Then(queue, __func__, [queue, a, b, c] (int aResolveValue) -> void {
       EXPECT_EQ(aResolveValue, 42);
@@ -197,7 +197,7 @@ TEST(MozPromise, CompletionPromises)
       [queue] (int aVal) -> RefPtr<TestPromise> {
         RefPtr<TestPromise::Private> p = new TestPromise::Private(__func__);
         nsCOMPtr<nsIRunnable> resolver = new DelayedResolveOrReject(queue, p, RRValue::MakeResolve(aVal - 8), 10);
-        queue->Dispatch(resolver.forget());
+        Unused << queue->Dispatch(resolver.forget());
         return RefPtr<TestPromise>(p);
       },
       DO_FAIL)

xpcom/tests/gtest/TestStateWatching.cpp

@@ -8,6 +8,7 @@
 #include "mozilla/SharedThreadPool.h"
 #include "mozilla/StateWatching.h"
 #include "mozilla/TaskQueue.h"
+#include "mozilla/Unused.h"
 #include "nsISupportsImpl.h"
 #include "VideoUtils.h"
 
@@ -32,7 +33,7 @@ TEST(WatchManager, Shutdown)
   WatchManager<Foo> manager(p, queue);
   Watchable<bool> notifier(false, "notifier");
 
-  queue->Dispatch(NS_NewRunnableFunction(
+  Unused << queue->Dispatch(NS_NewRunnableFunction(
     "TestStateWatching::WatchManager_Shutdown_Test::TestBody", [&]() {
       manager.Watch(notifier, &Foo::Notify);
       notifier = true;    // Trigger the call to Foo::Notify().

xpcom/tests/gtest/TestTaskQueue.cpp

@@ -7,6 +7,7 @@
 #include "gtest/gtest.h"
 #include "mozilla/SharedThreadPool.h"
 #include "mozilla/TaskQueue.h"
+#include "mozilla/Unused.h"
 #include "VideoUtils.h"
 
 namespace TestTaskQueue {
@@ -29,15 +30,15 @@ TEST(TaskQueue, EventOrder)
 
   // We expect task1 happens before task3.
   for (int i = 0; i < 10000; ++i) {
-    tq1->Dispatch(
+    Unused << tq1->Dispatch(
       NS_NewRunnableFunction(
         "TestTaskQueue::TaskQueue_EventOrder_Test::TestBody",
         [&]() {
-          tq2->Dispatch(NS_NewRunnableFunction(
+          Unused << tq2->Dispatch(NS_NewRunnableFunction(
             "TestTaskQueue::TaskQueue_EventOrder_Test::TestBody",
             []() { // task0
             }));
-          tq3->Dispatch(NS_NewRunnableFunction(
+          Unused << tq3->Dispatch(NS_NewRunnableFunction(
             "TestTaskQueue::TaskQueue_EventOrder_Test::TestBody",
             [&]() { // task1
               EXPECT_EQ(1, ++counter);
@@ -46,10 +47,10 @@ TEST(TaskQueue, EventOrder)
               ++sync;
               mon.Notify();
             }));
-          tq2->Dispatch(NS_NewRunnableFunction(
+          Unused << tq2->Dispatch(NS_NewRunnableFunction(
             "TestTaskQueue::TaskQueue_EventOrder_Test::TestBody",
             [&]() { // task2
-              tq3->Dispatch(NS_NewRunnableFunction(
+              Unused << tq3->Dispatch(NS_NewRunnableFunction(
                 "TestTaskQueue::TaskQueue_EventOrder_Test::TestBody",
                 [&]() { // task3
                   EXPECT_EQ(0, --counter);
@@ -60,7 +61,6 @@ TEST(TaskQueue, EventOrder)
                 }));
             }));
         }),
-      AbstractThread::AssertDispatchSuccess,
       AbstractThread::TailDispatch);
 
     // Ensure task1 and task3 are done before next loop.

xpcom/threads/AbstractThread.cpp

@@ -47,19 +47,15 @@ public:
   }
 
   virtual nsresult Dispatch(already_AddRefed<nsIRunnable> aRunnable,
-                            DispatchFailureHandling aFailureHandling = AssertDispatchSuccess,
                             DispatchReason aReason = NormalDispatch) override
   {
     AbstractThread* currentThread;
     if (aReason != TailDispatch && (currentThread = GetCurrent()) && RequiresTailDispatch(currentThread)) {
-      currentThread->TailDispatcher().AddTask(this, Move(aRunnable), aFailureHandling);
+      return currentThread->TailDispatcher().AddTask(this, Move(aRunnable));
-      return NS_OK;
     }
 
     RefPtr<nsIRunnable> runner(new Runner(this, Move(aRunnable), false /* already drained by TaskGroupRunnable  */));
-    nsresult rv = mTarget->Dispatch(runner.forget(), NS_DISPATCH_NORMAL);
+    return mTarget->Dispatch(runner.forget(), NS_DISPATCH_NORMAL);
-    MOZ_DIAGNOSTIC_ASSERT(aFailureHandling == DontAssertDispatchSuccess || NS_SUCCEEDED(rv));
-    return rv;
   }
 
   // Prevent a GCC warning about the other overload of Dispatch being hidden.
@@ -223,8 +219,7 @@ AbstractThread::DispatchFromScript(nsIRunnable* aEvent, uint32_t aFlags)
 NS_IMETHODIMP
 AbstractThread::Dispatch(already_AddRefed<nsIRunnable> aEvent, uint32_t aFlags)
 {
-  Dispatch(Move(aEvent), DontAssertDispatchSuccess, NormalDispatch);
+  return Dispatch(Move(aEvent), NormalDispatch);
-  return NS_OK;
 }
 
 NS_IMETHODIMP
@@ -234,12 +229,14 @@ AbstractThread::DelayedDispatch(already_AddRefed<nsIRunnable> aEvent,
   return NS_ERROR_NOT_IMPLEMENTED;
 }
 
-void
+nsresult
 AbstractThread::TailDispatchTasksFor(AbstractThread* aThread)
 {
   if (MightHaveTailTasks()) {
-    TailDispatcher().DispatchTasksFor(aThread);
+    return TailDispatcher().DispatchTasksFor(aThread);
   }
+
+  return NS_OK;
 }
 
 bool

xpcom/threads/AbstractThread.h

@@ -67,10 +67,8 @@ public:
   NS_IMETHOD DispatchFromScript(nsIRunnable *event, uint32_t flags) override;
   NS_IMETHOD DelayedDispatch(already_AddRefed<nsIRunnable> event, uint32_t delay) override;
 
-  enum DispatchFailureHandling { AssertDispatchSuccess, DontAssertDispatchSuccess };
   enum DispatchReason { NormalDispatch, TailDispatch };
   virtual nsresult Dispatch(already_AddRefed<nsIRunnable> aRunnable,
-                            DispatchFailureHandling aHandling = AssertDispatchSuccess,
                             DispatchReason aReason = NormalDispatch) = 0;
 
   virtual bool IsCurrentThreadIn() = 0;
@@ -89,7 +87,7 @@ public:
   // Helper functions for methods on the tail TasklDispatcher. These check
   // HasTailTasks to avoid allocating a TailDispatcher if it isn't
   // needed.
-  void TailDispatchTasksFor(AbstractThread* aThread);
+  nsresult TailDispatchTasksFor(AbstractThread* aThread);
   bool HasTailTasksFor(AbstractThread* aThread);
 
   // Returns true if this supports the tail dispatcher.

xpcom/threads/StateMirroring.h

@@ -155,8 +155,7 @@ private:
         mMirrors[i]->OwnerThread()->Dispatch(
           NewRunnableMethod("AbstractMirror::NotifyDisconnected",
                             mMirrors[i],
-                            &AbstractMirror<T>::NotifyDisconnected),
+                            &AbstractMirror<T>::NotifyDisconnected));
-          AbstractThread::DontAssertDispatchSuccess);
       }
       mMirrors.Clear();
     }
@@ -338,7 +337,7 @@ private:
           aCanonical,
           &AbstractCanonical<T>::AddMirror,
           this);
-      aCanonical->OwnerThread()->Dispatch(r.forget(), AbstractThread::DontAssertDispatchSuccess);
+      aCanonical->OwnerThread()->Dispatch(r.forget());
       mCanonical = aCanonical;
     }
   public:
@@ -357,7 +356,7 @@ private:
           mCanonical,
           &AbstractCanonical<T>::RemoveMirror,
           this);
-      mCanonical->OwnerThread()->Dispatch(r.forget(), AbstractThread::DontAssertDispatchSuccess);
+      mCanonical->OwnerThread()->Dispatch(r.forget());
       mCanonical = nullptr;
     }
 

xpcom/threads/TaskDispatcher.h

@@ -54,11 +54,10 @@ public:
 
   // Regular tasks are dispatched asynchronously, and run after state change
   // tasks.
-  virtual void AddTask(AbstractThread* aThread,
+  virtual nsresult AddTask(AbstractThread* aThread,
-                       already_AddRefed<nsIRunnable> aRunnable,
+                           already_AddRefed<nsIRunnable> aRunnable) = 0;
-                       AbstractThread::DispatchFailureHandling aFailureHandling = AbstractThread::AssertDispatchSuccess) = 0;
 
-  virtual void DispatchTasksFor(AbstractThread* aThread) = 0;
+  virtual nsresult DispatchTasksFor(AbstractThread* aThread) = 0;
   virtual bool HasTasksFor(AbstractThread* aThread) = 0;
   virtual void DrainDirectTasks() = 0;
 };
@@ -122,9 +121,8 @@ public:
     EnsureTaskGroup(aThread).mStateChangeTasks.AppendElement(r.forget());
   }
 
-  void AddTask(AbstractThread* aThread,
+  nsresult AddTask(AbstractThread* aThread,
-               already_AddRefed<nsIRunnable> aRunnable,
+                   already_AddRefed<nsIRunnable> aRunnable) override
-               AbstractThread::DispatchFailureHandling aFailureHandling) override
   {
     nsCOMPtr<nsIRunnable> r = aRunnable;
     MOZ_RELEASE_ASSERT(r);
@@ -139,11 +137,7 @@ public:
     PerThreadTaskGroup& group = *mTaskGroups.LastElement();
     group.mRegularTasks.AppendElement(r.forget());
 
-    // The task group needs to assert dispatch success if any of the runnables
+    return NS_OK;
-    // it's dispatching want to assert it.
-    if (aFailureHandling == AbstractThread::AssertDispatchSuccess) {
-      group.mFailureHandling = AbstractThread::AssertDispatchSuccess;
-    }
   }
 
   bool HasTasksFor(AbstractThread* aThread) override
@@ -152,15 +146,27 @@ public:
            (aThread == AbstractThread::GetCurrent() && HaveDirectTasks());
   }
 
-  void DispatchTasksFor(AbstractThread* aThread) override
+  nsresult DispatchTasksFor(AbstractThread* aThread) override
   {
+    nsresult rv = NS_OK;
+
     // Dispatch all groups that match |aThread|.
     for (size_t i = 0; i < mTaskGroups.Length(); ++i) {
       if (mTaskGroups[i]->mThread == aThread) {
-        DispatchTaskGroup(Move(mTaskGroups[i]));
+        nsresult rv2 = DispatchTaskGroup(Move(mTaskGroups[i]));
+
+        if (NS_WARN_IF(NS_FAILED(rv2)) && NS_SUCCEEDED(rv)) {
+          // We should try our best to call DispatchTaskGroup() as much as
+          // possible and return an error if any of DispatchTaskGroup() calls
+          // failed.
+          rv = rv2;
+        }
+
         mTaskGroups.RemoveElementAt(i--);
       }
     }
+
+    return rv;
   }
 
 private:
@@ -169,7 +175,7 @@ private:
   {
   public:
     explicit PerThreadTaskGroup(AbstractThread* aThread)
-      : mThread(aThread), mFailureHandling(AbstractThread::DontAssertDispatchSuccess)
+      : mThread(aThread)
     {
       MOZ_COUNT_CTOR(PerThreadTaskGroup);
     }
@@ -179,7 +185,6 @@ private:
     RefPtr<AbstractThread> mThread;
     nsTArray<nsCOMPtr<nsIRunnable>> mStateChangeTasks;
     nsTArray<nsCOMPtr<nsIRunnable>> mRegularTasks;
-    AbstractThread::DispatchFailureHandling mFailureHandling;
   };
 
   class TaskGroupRunnable : public Runnable
@@ -250,15 +255,14 @@ private:
     return nullptr;
   }
 
-  void DispatchTaskGroup(UniquePtr<PerThreadTaskGroup> aGroup)
+  nsresult DispatchTaskGroup(UniquePtr<PerThreadTaskGroup> aGroup)
   {
     RefPtr<AbstractThread> thread = aGroup->mThread;
 
-    AbstractThread::DispatchFailureHandling failureHandling = aGroup->mFailureHandling;
     AbstractThread::DispatchReason reason = mIsTailDispatcher ? AbstractThread::TailDispatch
                                                               : AbstractThread::NormalDispatch;
     nsCOMPtr<nsIRunnable> r = new TaskGroupRunnable(Move(aGroup));
-    thread->Dispatch(r.forget(), failureHandling, reason);
+    return thread->Dispatch(r.forget(), reason);
   }
 
   // Direct tasks. We use a Maybe<> because (a) this class is hot, (b)

xpcom/threads/TaskQueue.cpp

@@ -39,7 +39,6 @@ public:
     nsCOMPtr<nsIRunnable> runnable = aEvent;
     MonitorAutoLock mon(mTaskQueue->mQueueMonitor);
     return mTaskQueue->DispatchLocked(/* passed by ref */runnable,
-                                      DontAssertDispatchSuccess,
                                       NormalDispatch);
   }
 
@@ -106,7 +105,6 @@ TaskQueue::TailDispatcher()
 // See Dispatch() in TaskQueue.h for more details.
 nsresult
 TaskQueue::DispatchLocked(nsCOMPtr<nsIRunnable>& aRunnable,
-                          DispatchFailureHandling aFailureHandling,
                           DispatchReason aReason)
 {
   mQueueMonitor.AssertCurrentThreadOwns();
@@ -116,8 +114,7 @@ TaskQueue::DispatchLocked(nsCOMPtr<nsIRunnable>& aRunnable,
 
   AbstractThread* currentThread;
   if (aReason != TailDispatch && (currentThread = GetCurrent()) && RequiresTailDispatch(currentThread)) {
-    currentThread->TailDispatcher().AddTask(this, aRunnable.forget(), aFailureHandling);
+    return currentThread->TailDispatcher().AddTask(this, aRunnable.forget());
-    return NS_OK;
   }
 
   mTasks.push(aRunnable.forget());

xpcom/threads/TaskQueue.h

@@ -61,20 +61,14 @@ public:
 
   TaskQueue* AsTaskQueue() override { return this; }
 
-  nsresult Dispatch(already_AddRefed<nsIRunnable> aRunnable,
+  MOZ_MUST_USE nsresult
-                    DispatchFailureHandling aFailureHandling = AssertDispatchSuccess,
+  Dispatch(already_AddRefed<nsIRunnable> aRunnable,
-                    DispatchReason aReason = NormalDispatch) override
+           DispatchReason aReason = NormalDispatch) override
   {
     nsCOMPtr<nsIRunnable> r = aRunnable;
     {
       MonitorAutoLock mon(mQueueMonitor);
-      nsresult rv = DispatchLocked(/* passed by ref */r, aFailureHandling, aReason);
+      return DispatchLocked(/* passed by ref */r, aReason);
-#if defined(DEBUG) || !defined(RELEASE_OR_BETA) || defined(EARLY_BETA_OR_EARLIER)
-      if (NS_FAILED(rv) && aFailureHandling == AssertDispatchSuccess) {
-        MOZ_CRASH_UNSAFE_PRINTF("%s: Dispatch failed. rv=%x", mName, uint32_t(rv));
-      }
-#endif
-      return 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
@@ -121,7 +115,6 @@ protected:
   void AwaitIdleLocked();
 
   nsresult DispatchLocked(nsCOMPtr<nsIRunnable>& aRunnable,
-                          DispatchFailureHandling aFailureHandling,
                           DispatchReason aReason = NormalDispatch);
 
   void MaybeResolveShutdown()