Bug 1282366 - Improve WorkerHolder use in Runnables, r=khuey
authorAndrea Marchesini <amarchesini@mozilla.com>
Mon, 04 Jul 2016 08:19:10 +0200
changeset 303482 cc14db2645218df5f292f1eb00fdd1e9d7d83486
parent 303481 7814d47046c69629f2a5025ad925dac4b3bae404
child 303483 289f630f3b0b818d581dfbd32dd6a97038a558e0
push id79095
push useramarchesini@mozilla.com
push dateMon, 04 Jul 2016 06:19:43 +0000
treeherdermozilla-inbound@cc14db264521 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerskhuey
bugs1282366
milestone50.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1282366 - Improve WorkerHolder use in Runnables, r=khuey
dom/bindings/BindingUtils.cpp
dom/console/Console.cpp
dom/indexedDB/ActorsChild.cpp
dom/workers/WorkerHolder.cpp
dom/workers/WorkerRunnable.cpp
dom/workers/WorkerRunnable.h
--- a/dom/bindings/BindingUtils.cpp
+++ b/dom/bindings/BindingUtils.cpp
@@ -3292,103 +3292,51 @@ SetDocumentAndPageUseCounter(JSContext* 
     win->GetDocument()->SetDocumentAndPageUseCounter(aUseCounter);
   }
 }
 
 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());
 
     // Walk up to our containing page
     WorkerPrivate* wp = mWorkerPrivate;
     while (wp->GetParent()) {
       wp = wp->GetParent();
     }
 
     nsPIDOMWindowInner* window = wp->GetWindow();
     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
 
 void
 DeprecationWarning(JSContext* aCx, JSObject* aObject,
                    nsIDocument::DeprecatedOperations aOperation)
 {
--- a/dom/console/Console.cpp
+++ b/dom/console/Console.cpp
@@ -308,142 +308,68 @@ public:
   {
     JS_ClearPendingException(mCx);
   }
 
 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()
   {
     // Clear the StructuredCloneHolderBase class.
     Clear();
   }
 
   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();
 
     // Walk up to our containing page
     WorkerPrivate* wp = mWorkerPrivate;
     while (wp->GetParent()) {
       wp = wp->GetParent();
     }
 
     nsPIDOMWindowInner* window = wp->GetWindow();
     if (!window) {
       RunWindowless();
     } 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
   RunWithWindow(nsPIDOMWindowInner* aWindow)
   {
     AssertIsOnMainThread();
 
     AutoJSAPI jsapi;
@@ -486,17 +412,24 @@ private:
     // don't need a proxy here.
     global = js::UncheckedUnwrap(global);
 
     JSAutoCompartment ac(cx, global);
 
     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;
 
   // This method is called in the main-thread.
   virtual void
   RunConsole(JSContext* aCx, nsPIDOMWindowOuter* aOuterWindow,
              nsPIDOMWindowInner* aInnerWindow) = 0;
@@ -559,18 +492,16 @@ protected:
 
     if (NS_WARN_IF(!JS_WriteString(aWriter, jsString))) {
       return false;
     }
 
     return true;
   }
 
-  WorkerPrivate* mWorkerPrivate;
-
   // This must be released on the worker thread.
   RefPtr<Console> mConsole;
 
   ConsoleStructuredCloneData mClonedData;
 };
 
 // This runnable appends a CallData object into the Console queue running on
 // the main-thread.
--- a/dom/indexedDB/ActorsChild.cpp
+++ b/dom/indexedDB/ActorsChild.cpp
@@ -898,17 +898,16 @@ protected:
   ~WorkerPermissionRequestChildProcessActor()
   {}
 
   virtual bool
   Recv__delete__(const uint32_t& aPermission) override;
 };
 
 class WorkerPermissionChallenge final : public Runnable
-                                      , public WorkerHolder
 {
 public:
   WorkerPermissionChallenge(WorkerPrivate* aWorkerPrivate,
                             BackgroundFactoryRequestChild* aActor,
                             IDBFactory* aFactory,
                             const PrincipalInfo& aPrincipalInfo)
     : mWorkerPrivate(aWorkerPrivate)
     , mActor(aActor)
@@ -916,35 +915,43 @@ public:
     , mPrincipalInfo(aPrincipalInfo)
   {
     MOZ_ASSERT(mWorkerPrivate);
     MOZ_ASSERT(aActor);
     MOZ_ASSERT(aFactory);
     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
   {
     bool completed = RunInternal();
     if (completed) {
       OperationCompleted();
     }
 
     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()
   {
     if (NS_IsMainThread()) {
       RefPtr<WorkerPermissionOperationCompleted> runnable =
         new WorkerPermissionOperationCompleted(mWorkerPrivate, this);
 
       MOZ_ALWAYS_TRUE(runnable->Dispatch());
@@ -958,17 +965,17 @@ public:
 
     RefPtr<IDBFactory> factory;
     mFactory.swap(factory);
 
     mActor->SendPermissionRetry();
     mActor = nullptr;
 
     mWorkerPrivate->AssertIsOnWorkerThread();
-    ReleaseWorker();
+    mWorkerPrivate->ModifyBusyCountFromWorker(false);
   }
 
 private:
   bool
   RunInternal()
   {
     MOZ_ASSERT(NS_IsMainThread());
 
@@ -1409,23 +1416,17 @@ BackgroundFactoryRequestChild::RecvPermi
   if (!NS_IsMainThread()) {
     WorkerPrivate* workerPrivate = GetCurrentThreadWorkerPrivate();
     MOZ_ASSERT(workerPrivate);
     workerPrivate->AssertIsOnWorkerThread();
 
     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;
   nsCOMPtr<nsIPrincipal> principal =
     mozilla::ipc::PrincipalInfoToPrincipal(aPrincipalInfo, &rv);
   if (NS_WARN_IF(NS_FAILED(rv))) {
     return false;
   }
--- a/dom/workers/WorkerHolder.cpp
+++ b/dom/workers/WorkerHolder.cpp
@@ -40,14 +40,15 @@ WorkerHolder::ReleaseWorker()
   MOZ_ASSERT(mWorkerPrivate);
   ReleaseWorkerInternal();
 }
 
 void
 WorkerHolder::ReleaseWorkerInternal()
 {
   if (mWorkerPrivate) {
+    mWorkerPrivate->AssertIsOnWorkerThread();
     mWorkerPrivate->RemoveHolder(this);
     mWorkerPrivate = nullptr;
   }
 }
 
 END_WORKERS_NAMESPACE
--- a/dom/workers/WorkerRunnable.cpp
+++ b/dom/workers/WorkerRunnable.cpp
@@ -654,8 +654,92 @@ WorkerSameThreadRunnable::PostDispatch(W
   aWorkerPrivate->AssertIsOnWorkerThread();
   if (aDispatchResult) {
     DebugOnly<bool> willIncrement = aWorkerPrivate->ModifyBusyCountFromWorker(true);
     // Should never fail since if this thread is still running, so should the
     // parent and it should be able to process a control runnable.
     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());
+}
--- a/dom/workers/WorkerRunnable.h
+++ b/dom/workers/WorkerRunnable.h
@@ -404,16 +404,44 @@ public:
   // fails, or if the worker is shut down while dispatching, an error will be
   // reported on aRv.  In that case the error MUST be propagated out to script.
   void Dispatch(ErrorResult& aRv);
 
 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
 // we are bad people and should be ashamed of ourselves.  Let's hope none of
 // them happen while a worker is shutting down.
 //
 // Do NOT copy what this class is doing elsewhere.  Just don't.
 class WorkerCheckAPIExposureOnMainThreadRunnable