Bug 1282366 - Improve WorkerHolder use in Runnables, r=khuey

dom/bindings/BindingUtils.cpp

@@ -3297,46 +3297,23 @@ namespace {
 
 // This runnable is used to write a deprecation message from a worker to the
 // console running on the main-thread.
-class DeprecationWarningRunnable final : public Runnable
-                                       , public WorkerHolder
+class DeprecationWarningRunnable final : public WorkerProxyToMainThreadRunnable
 {
-  WorkerPrivate* mWorkerPrivate;
   nsIDocument::DeprecatedOperations mOperation;
 
 public:
   DeprecationWarningRunnable(WorkerPrivate* aWorkerPrivate,
                              nsIDocument::DeprecatedOperations aOperation)
-    : mWorkerPrivate(aWorkerPrivate)
+    : WorkerProxyToMainThreadRunnable(aWorkerPrivate)
     , mOperation(aOperation)
   {
     MOZ_ASSERT(aWorkerPrivate);
-  }
-
-  void
-  Dispatch()
-  {
-    if (NS_WARN_IF(!HoldWorker(mWorkerPrivate))) {
-      return;
-    }
-
-    if (NS_WARN_IF(NS_FAILED(NS_DispatchToMainThread(this)))) {
-      ReleaseWorker();
-      return;
-    }
-  }
-
-  virtual bool
-  Notify(Status aStatus) override
-  {
-    // We don't care about the notification. We just want to keep the
-    // mWorkerPrivate alive.
-    return true;
+    aWorkerPrivate->AssertIsOnWorkerThread();
   }
 
 private:
-
-  NS_IMETHOD
-  Run() override
+  void
+  RunOnMainThread() override
   {
     MOZ_ASSERT(NS_IsMainThread());
 
@@ -3350,40 +3327,11 @@ private:
     if (window && window->GetExtantDoc()) {
       window->GetExtantDoc()->WarnOnceAbout(mOperation);
     }
-
-    ReleaseWorkerHolder();
-    return NS_OK;
   }
 
   void
-  ReleaseWorkerHolder()
-  {
-    class ReleaseRunnable final : public MainThreadWorkerRunnable
-    {
-      RefPtr<DeprecationWarningRunnable> mRunnable;
-
-    public:
-      ReleaseRunnable(WorkerPrivate* aWorkerPrivate,
-                      DeprecationWarningRunnable* aRunnable)
-        : MainThreadWorkerRunnable(aWorkerPrivate)
-        , mRunnable(aRunnable)
-      {}
-
-      virtual bool
-      WorkerRun(JSContext* aCx, WorkerPrivate* aWorkerPrivate) override
-      {
-        MOZ_ASSERT(aWorkerPrivate);
-        aWorkerPrivate->AssertIsOnWorkerThread();
-
-        mRunnable->ReleaseWorker();
-        return true;
-      }
-    };
-
-    RefPtr<ReleaseRunnable> runnable =
-      new ReleaseRunnable(mWorkerPrivate, this);
-    NS_WARN_IF(!runnable->Dispatch());
-  }
+  RunBackOnWorkerThread() override
+  {}
 };
 
 } // anonymous namespace

dom/console/Console.cpp

@@ -313,17 +313,14 @@ private:
   JSContext* mCx;
 };
 
-class ConsoleRunnable : public Runnable
-                      , public WorkerHolder
+class ConsoleRunnable : public WorkerProxyToMainThreadRunnable
                       , public StructuredCloneHolderBase
 {
 public:
   explicit ConsoleRunnable(Console* aConsole)
-    : mWorkerPrivate(GetCurrentThreadWorkerPrivate())
+    : WorkerProxyToMainThreadRunnable(GetCurrentThreadWorkerPrivate())
     , mConsole(aConsole)
-  {
-    MOZ_ASSERT(mWorkerPrivate);
-  }
+  {}
 
   virtual
   ~ConsoleRunnable()
@@ -335,24 +332,24 @@ public:
   bool
   Dispatch(JSContext* aCx)
   {
-    if (!DispatchInternal(aCx)) {
-      ReleaseData();
+    mWorkerPrivate->AssertIsOnWorkerThread();
+
+    if (NS_WARN_IF(!PreDispatch(aCx))) {
+      RunBackOnWorkerThread();
+      return false;
+    }
+
+    if (NS_WARN_IF(!WorkerProxyToMainThreadRunnable::Dispatch())) {
+      RunBackOnWorkerThread();
       return false;
     }
 
     return true;
   }
 
-  virtual bool Notify(workers::Status aStatus) override
-  {
-    // We don't care about the notification. We just want to keep the
-    // mWorkerPrivate alive.
-    return true;
-  }
-
-private:
-  NS_IMETHOD
-  Run() override
+protected:
+  void
+  RunOnMainThread() override
   {
     AssertIsOnMainThread();
 
@@ -368,77 +365,6 @@ private:
     } else {
       RunWithWindow(window);
     }
-
-    PostDispatch();
-    return NS_OK;
-  }
-
-  bool
-  DispatchInternal(JSContext* aCx)
-  {
-    mWorkerPrivate->AssertIsOnWorkerThread();
-
-    if (NS_WARN_IF(!PreDispatch(aCx))) {
-      return false;
-    }
-
-    if (NS_WARN_IF(!HoldWorker(mWorkerPrivate))) {
-      return false;
-    }
-
-    if (NS_WARN_IF(NS_FAILED(NS_DispatchToMainThread(this)))) {
-      return false;
-    }
-
-    return true;
-  }
-
-  void
-  PostDispatch()
-  {
-    class ConsoleReleaseRunnable final : public MainThreadWorkerControlRunnable
-    {
-      RefPtr<ConsoleRunnable> mRunnable;
-
-    public:
-      ConsoleReleaseRunnable(WorkerPrivate* aWorkerPrivate,
-                             ConsoleRunnable* aRunnable)
-        : MainThreadWorkerControlRunnable(aWorkerPrivate)
-        , mRunnable(aRunnable)
-      {
-        MOZ_ASSERT(aRunnable);
-      }
-
-      // If something goes wrong, we still need to release the ConsoleCallData
-      // object. For this reason we have a custom Cancel method.
-      nsresult
-      Cancel() override
-      {
-        WorkerRun(nullptr, mWorkerPrivate);
-        return NS_OK;
-      }
-
-      virtual bool
-      WorkerRun(JSContext* aCx, workers::WorkerPrivate* aWorkerPrivate) override
-      {
-        MOZ_ASSERT(aWorkerPrivate);
-        aWorkerPrivate->AssertIsOnWorkerThread();
-
-        mRunnable->ReleaseData();
-        mRunnable->mConsole = nullptr;
-
-        mRunnable->ReleaseWorker();
-        return true;
-      }
-
-    private:
-      ~ConsoleReleaseRunnable()
-      {}
-    };
-
-    RefPtr<WorkerControlRunnable> runnable =
-      new ConsoleReleaseRunnable(mWorkerPrivate, this);
-    NS_WARN_IF(!runnable->Dispatch());
   }
 
   void
@@ -491,7 +417,14 @@ private:
     RunConsole(cx, nullptr, nullptr);
   }
 
-protected:
+  void
+  RunBackOnWorkerThread() override
+  {
+    mWorkerPrivate->AssertIsOnWorkerThread();
+    ReleaseData();
+    mConsole = nullptr;
+  }
+
   // This method is called in the owning thread of the Console object.
   virtual bool
   PreDispatch(JSContext* aCx) = 0;
@@ -564,8 +497,6 @@ protected:
     return true;
   }
 
-  WorkerPrivate* mWorkerPrivate;
-
   // This must be released on the worker thread.
   RefPtr<Console> mConsole;
 

dom/indexedDB/ActorsChild.cpp

@@ -903,7 +903,6 @@ protected:
 };
 
 class WorkerPermissionChallenge final : public Runnable
-                                      , public WorkerHolder
 {
 public:
   WorkerPermissionChallenge(WorkerPrivate* aWorkerPrivate,
@@ -921,6 +920,22 @@ public:
     mWorkerPrivate->AssertIsOnWorkerThread();
   }
 
+  bool
+  Dispatch()
+  {
+    mWorkerPrivate->AssertIsOnWorkerThread();
+    if (NS_WARN_IF(!mWorkerPrivate->ModifyBusyCountFromWorker(true))) {
+      return false;
+    }
+
+    if (NS_WARN_IF(NS_FAILED(NS_DispatchToMainThread(this)))) {
+      mWorkerPrivate->ModifyBusyCountFromWorker(false);
+      return false;
+    }
+
+    return true;
+  }
+
   NS_IMETHOD
   Run() override
   {
@@ -932,14 +947,6 @@ public:
     return NS_OK;
   }
 
-  virtual bool
-  Notify(workers::Status aStatus) override
-  {
-    // We don't care about the notification. We just want to keep the
-    // mWorkerPrivate alive.
-    return true;
-  }
-
   void
   OperationCompleted()
   {
@@ -963,7 +970,7 @@ public:
     mActor = nullptr;
 
     mWorkerPrivate->AssertIsOnWorkerThread();
-    ReleaseWorker();
+    mWorkerPrivate->ModifyBusyCountFromWorker(false);
   }
 
 private:
@@ -1414,13 +1421,7 @@ BackgroundFactoryRequestChild::RecvPermissionChallenge(
     RefPtr<WorkerPermissionChallenge> challenge =
       new WorkerPermissionChallenge(workerPrivate, this, mFactory,
                                     aPrincipalInfo);
-
-    if (NS_WARN_IF(!challenge->HoldWorker(workerPrivate))) {
-      return false;
-    }
-
-    MOZ_ALWAYS_SUCCEEDS(NS_DispatchToMainThread(challenge));
-    return true;
+    return challenge->Dispatch();
   }
 
   nsresult rv;

dom/workers/WorkerHolder.cpp

@@ -45,6 +45,7 @@ void
 WorkerHolder::ReleaseWorkerInternal()
 {
   if (mWorkerPrivate) {
+    mWorkerPrivate->AssertIsOnWorkerThread();
     mWorkerPrivate->RemoveHolder(this);
     mWorkerPrivate = nullptr;
   }

dom/workers/WorkerRunnable.cpp

@@ -659,3 +659,87 @@ WorkerSameThreadRunnable::PostDispatch(WorkerPrivate* aWorkerPrivate,
     MOZ_ASSERT(willIncrement);
   }
 }
+
+WorkerProxyToMainThreadRunnable::WorkerProxyToMainThreadRunnable(WorkerPrivate* aWorkerPrivate)
+  : mWorkerPrivate(aWorkerPrivate)
+{
+  MOZ_ASSERT(mWorkerPrivate);
+  mWorkerPrivate->AssertIsOnWorkerThread();
+}
+
+WorkerProxyToMainThreadRunnable::~WorkerProxyToMainThreadRunnable()
+{}
+
+bool
+WorkerProxyToMainThreadRunnable::Dispatch()
+{
+  mWorkerPrivate->AssertIsOnWorkerThread();
+
+  if (NS_WARN_IF(!mWorkerPrivate->ModifyBusyCountFromWorker(true))) {
+    RunBackOnWorkerThread();
+    return false;
+  }
+
+  if (NS_WARN_IF(NS_FAILED(NS_DispatchToMainThread(this)))) {
+    mWorkerPrivate->ModifyBusyCountFromWorker(false);
+    RunBackOnWorkerThread();
+    return false;
+  }
+
+  return true;
+}
+
+NS_IMETHODIMP
+WorkerProxyToMainThreadRunnable::Run()
+{
+  AssertIsOnMainThread();
+  RunOnMainThread();
+  PostDispatchOnMainThread();
+  return NS_OK;
+}
+
+void
+WorkerProxyToMainThreadRunnable::PostDispatchOnMainThread()
+{
+  class ReleaseRunnable final : public MainThreadWorkerControlRunnable
+  {
+    RefPtr<WorkerProxyToMainThreadRunnable> mRunnable;
+
+  public:
+    ReleaseRunnable(WorkerPrivate* aWorkerPrivate,
+                    WorkerProxyToMainThreadRunnable* aRunnable)
+      : MainThreadWorkerControlRunnable(aWorkerPrivate)
+      , mRunnable(aRunnable)
+    {
+      MOZ_ASSERT(aRunnable);
+    }
+
+    // We must call RunBackOnWorkerThread() also if the runnable is cancelled.
+    nsresult
+    Cancel() override
+    {
+      WorkerRun(nullptr, mWorkerPrivate);
+      return NS_OK;
+    }
+
+    virtual bool
+    WorkerRun(JSContext* aCx, workers::WorkerPrivate* aWorkerPrivate) override
+    {
+      MOZ_ASSERT(aWorkerPrivate);
+      aWorkerPrivate->AssertIsOnWorkerThread();
+
+      mRunnable->RunBackOnWorkerThread();
+
+      aWorkerPrivate->ModifyBusyCountFromWorker(true);
+      return true;
+    }
+
+  private:
+    ~ReleaseRunnable()
+    {}
+  };
+
+  RefPtr<WorkerControlRunnable> runnable =
+    new ReleaseRunnable(mWorkerPrivate, this);
+  NS_WARN_IF(!runnable->Dispatch());
+}

dom/workers/WorkerRunnable.h

@@ -409,6 +409,34 @@ private:
   NS_IMETHOD Run() override;
 };
 
+// This runnable is an helper class for dispatching something from a worker
+// thread to the main-thread and back to the worker-thread. During this
+// operation, this class will keep the worker alive.
+class WorkerProxyToMainThreadRunnable : public Runnable
+{
+protected:
+  explicit WorkerProxyToMainThreadRunnable(WorkerPrivate* aWorkerPrivate);
+
+  virtual ~WorkerProxyToMainThreadRunnable();
+
+  // First this method is called on the main-thread.
+  virtual void RunOnMainThread() = 0;
+
+  // After this second method is called on the worker-thread.
+  virtual void RunBackOnWorkerThread() = 0;
+
+public:
+  bool Dispatch();
+
+private:
+  NS_IMETHOD Run() override;
+
+  void PostDispatchOnMainThread();
+
+protected:
+  WorkerPrivate* mWorkerPrivate;
+};
+
 // Class for checking API exposure.  This totally violates the "MUST" in the
 // comments on WorkerMainThreadRunnable::Dispatch, because API exposure checks
 // can't throw.  Maybe we should change it so they _could_ throw.  But for now