Bug 1130686 - Refactor PromiseHolder in the service worker clients code. r=nsm

dom/promise/Promise.cpp

@@ -1441,35 +1441,13 @@ PromiseWorkerProxy::StoreISupports(nsISupports* aSupports)
   mSupportsArray.AppendElement(supports);
 }
 
-namespace {
-
-class PromiseWorkerProxyControlRunnable final
-  : public WorkerControlRunnable
+bool
+PromiseWorkerProxyControlRunnable::WorkerRun(JSContext* aCx,
+                                             WorkerPrivate* aWorkerPrivate)
 {
-  nsRefPtr<PromiseWorkerProxy> mProxy;
-
-public:
-  PromiseWorkerProxyControlRunnable(WorkerPrivate* aWorkerPrivate,
-                                    PromiseWorkerProxy* aProxy)
-    : WorkerControlRunnable(aWorkerPrivate, WorkerThreadUnchangedBusyCount)
-    , mProxy(aProxy)
-  {
-    MOZ_ASSERT(aProxy);
-  }
-
-  virtual bool
-  WorkerRun(JSContext* aCx, WorkerPrivate* aWorkerPrivate) override
-  {
-    mProxy->CleanUp(aCx);
-    return true;
-  }
-
-private:
-  ~PromiseWorkerProxyControlRunnable()
-  {}
-};
-
-} // anonymous namespace
+  mProxy->CleanUp(aCx);
+  return true;
+}
 
 void
 PromiseWorkerProxy::RunCallback(JSContext* aCx,

dom/promise/PromiseWorkerProxy.h

@@ -11,6 +11,8 @@
 #include "mozilla/dom/workers/bindings/WorkerFeature.h"
 #include "nsProxyRelease.h"
 
+#include "WorkerRunnable.h"
+
 namespace mozilla {
 namespace dom {
 
@@ -53,8 +55,12 @@ class WorkerPrivate;
 // dispatch a runnable to the worker. Use GetWorkerPrivate() to acquire the
 // worker. This might be null! In the WorkerRunnable's WorkerRun() use
 // GetWorkerPromise() to access the Promise and resolve/reject it. Then call
-// CleanUp() on the worker
-// thread.
+// CleanUp() on the worker thread.
+//
+// IMPORTANT: Dispatching the runnable to the worker thread may fail causing
+// the promise to leak. To successfully release the promise on the
+// worker thread in this case, use |PromiseWorkerProxyControlRunnable| to
+// dispatch a control runnable that will deref the object on the correct thread.
 
 class PromiseWorkerProxy : public PromiseNativeHandler,
                            public workers::WorkerFeature
@@ -78,6 +84,17 @@ public:
 
   void CleanUp(JSContext* aCx);
 
+  Mutex& GetCleanUpLock()
+  {
+    return mCleanUpLock;
+  }
+
+  bool IsClean() const
+  {
+    mCleanUpLock.AssertCurrentThreadOwns();
+    return mCleanedUp;
+  }
+
 protected:
   virtual void ResolvedCallback(JSContext* aCx,
                                 JS::Handle<JS::Value> aValue) override;
@@ -119,6 +136,30 @@ private:
   Mutex mCleanUpLock;
 };
 
+// Helper runnable used for releasing the proxied promise when the worker
+// is not accepting runnables and the promise object would leak.
+// See the instructions above.
+class PromiseWorkerProxyControlRunnable final : public workers::WorkerControlRunnable
+{
+  nsRefPtr<PromiseWorkerProxy> mProxy;
+
+public:
+  PromiseWorkerProxyControlRunnable(workers::WorkerPrivate* aWorkerPrivate,
+                                    PromiseWorkerProxy* aProxy)
+    : WorkerControlRunnable(aWorkerPrivate, WorkerThreadUnchangedBusyCount)
+    , mProxy(aProxy)
+  {
+    MOZ_ASSERT(aProxy);
+  }
+
+  virtual bool
+  WorkerRun(JSContext* aCx, workers::WorkerPrivate* aWorkerPrivate) override;
+
+private:
+  ~PromiseWorkerProxyControlRunnable()
+  {}
+};
+
 } // namespace dom
 } // namespace mozilla
 

dom/workers/ServiceWorkerClients.cpp

@@ -4,6 +4,7 @@
  * You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 #include "mozilla/dom/Promise.h"
+#include "mozilla/dom/PromiseWorkerProxy.h"
 
 #include "ServiceWorkerClient.h"
 #include "ServiceWorkerClients.h"
@@ -41,96 +42,17 @@ ServiceWorkerClients::WrapObject(JSContext* aCx, JS::Handle<JSObject*> aGivenPro
 
 namespace {
 
-// Helper class used for passing the promise between threads while
-// keeping the worker alive.
-class PromiseHolder final : public WorkerFeature
-{
-  friend class MatchAllRunnable;
-
-  NS_INLINE_DECL_THREADSAFE_REFCOUNTING(PromiseHolder)
-
-public:
-  PromiseHolder(WorkerPrivate* aWorkerPrivate,
-                Promise* aPromise)
-    : mWorkerPrivate(aWorkerPrivate),
-      mPromise(aPromise),
-      mCleanUpLock("promiseHolderCleanUpLock"),
-      mClean(false)
-  {
-    MOZ_ASSERT(mWorkerPrivate);
-    mWorkerPrivate->AssertIsOnWorkerThread();
-    MOZ_ASSERT(mPromise);
-
-    if (NS_WARN_IF(!mWorkerPrivate->AddFeature(mWorkerPrivate->GetJSContext(), this))) {
-      // Worker has been canceled and will go away.
-      // The ResolvePromiseWorkerRunnable won't run, so we can set mPromise to
-      // nullptr.
-      mPromise = nullptr;
-      mClean = true;
-    }
-  }
-
-  Promise*
-  GetPromise() const
-  {
-    return mPromise;
-  }
-
-  void
-  Clean()
-  {
-    mWorkerPrivate->AssertIsOnWorkerThread();
-
-    MutexAutoLock lock(mCleanUpLock);
-    if (mClean) {
-      return;
-    }
-
-    mPromise = nullptr;
-    mWorkerPrivate->RemoveFeature(mWorkerPrivate->GetJSContext(), this);
-    mClean = true;
-  }
-
-  bool
-  Notify(JSContext* aCx, Status aStatus)
-  {
-    mWorkerPrivate->AssertIsOnWorkerThread();
-
-    if (aStatus > Running) {
-      Clean();
-    }
-
-    return true;
-  }
-
-private:
-  ~PromiseHolder()
-  {
-    MOZ_ASSERT(mClean);
-  }
-
-  WorkerPrivate* mWorkerPrivate;
-  nsRefPtr<Promise> mPromise;
-
-  // Used to prevent race conditions on |mClean| and to ensure that either a
-  // Notify() call or a dispatch back to the worker thread occurs before
-  // this object is released.
-  Mutex mCleanUpLock;
-
-  bool mClean;
-};
-
 class ResolvePromiseWorkerRunnable final : public WorkerRunnable
 {
-  nsRefPtr<PromiseHolder> mPromiseHolder;
+  nsRefPtr<PromiseWorkerProxy> mPromiseProxy;
   nsTArray<ServiceWorkerClientInfo> mValue;
 
 public:
   ResolvePromiseWorkerRunnable(WorkerPrivate* aWorkerPrivate,
-                               PromiseHolder* aPromiseHolder,
+                               PromiseWorkerProxy* aPromiseProxy,
                                nsTArray<ServiceWorkerClientInfo>& aValue)
     : WorkerRunnable(aWorkerPrivate, WorkerThreadModifyBusyCount),
-      mPromiseHolder(aPromiseHolder)
+      mPromiseProxy(aPromiseProxy)
   {
     AssertIsOnMainThread();
     mValue.SwapElements(aValue);
@@ -142,7 +64,7 @@ public:
     MOZ_ASSERT(aWorkerPrivate);
     aWorkerPrivate->AssertIsOnWorkerThread();
 
-    Promise* promise = mPromiseHolder->GetPromise();
+    Promise* promise = mPromiseProxy->GetWorkerPromise();
     MOZ_ASSERT(promise);
 
     nsTArray<nsRefPtr<ServiceWorkerClient>> ret;
@@ -154,51 +76,23 @@ public:
     promise->MaybeResolve(ret);
 
     // release the reference on the worker thread.
-    mPromiseHolder->Clean();
+    mPromiseProxy->CleanUp(aCx);
 
     return true;
   }
 };
 
-class ReleasePromiseRunnable final : public MainThreadWorkerControlRunnable
-{
-  nsRefPtr<PromiseHolder> mPromiseHolder;
-
-public:
-  ReleasePromiseRunnable(WorkerPrivate* aWorkerPrivate,
-                         PromiseHolder* aPromiseHolder)
-    : MainThreadWorkerControlRunnable(aWorkerPrivate),
-      mPromiseHolder(aPromiseHolder)
-  { }
-
-private:
-  ~ReleasePromiseRunnable()
-  { }
-
-  bool
-  WorkerRun(JSContext* aCx, WorkerPrivate* aWorkerPrivate)
-  {
-    MOZ_ASSERT(aWorkerPrivate);
-    aWorkerPrivate->AssertIsOnWorkerThread();
-
-    mPromiseHolder->Clean();
-
-    return true;
-  }
-
-};
-
 class MatchAllRunnable final : public nsRunnable
 {
   WorkerPrivate* mWorkerPrivate;
-  nsRefPtr<PromiseHolder> mPromiseHolder;
+  nsRefPtr<PromiseWorkerProxy> mPromiseProxy;
   nsCString mScope;
 public:
   MatchAllRunnable(WorkerPrivate* aWorkerPrivate,
-                   PromiseHolder* aPromiseHolder,
+                   PromiseWorkerProxy* aPromiseProxy,
                    const nsCString& aScope)
     : mWorkerPrivate(aWorkerPrivate),
-      mPromiseHolder(aPromiseHolder),
+      mPromiseProxy(aPromiseProxy),
       mScope(aScope)
   {
     MOZ_ASSERT(aWorkerPrivate);
@@ -210,8 +104,8 @@ public:
   {
     AssertIsOnMainThread();
 
-    MutexAutoLock lock(mPromiseHolder->mCleanUpLock);
-    if (mPromiseHolder->mClean) {
+    MutexAutoLock lock(mPromiseProxy->GetCleanUpLock());
+    if (mPromiseProxy->IsClean()) {
       // Don't resolve the promise if it was already released.
       return NS_OK;
     }
@@ -221,7 +115,7 @@ public:
 
     swm->GetAllClients(mScope, result);
     nsRefPtr<ResolvePromiseWorkerRunnable> r =
-      new ResolvePromiseWorkerRunnable(mWorkerPrivate, mPromiseHolder, result);
+      new ResolvePromiseWorkerRunnable(mWorkerPrivate, mPromiseProxy, result);
 
     AutoSafeJSContext cx;
     if (r->Dispatch(cx)) {
@@ -230,11 +124,11 @@ public:
 
     // Dispatch to worker thread failed because the worker is shutting down.
     // Use a control runnable to release the runnable on the worker thread.
-    nsRefPtr<ReleasePromiseRunnable> releaseRunnable =
-      new ReleasePromiseRunnable(mWorkerPrivate, mPromiseHolder);
+    nsRefPtr<PromiseWorkerProxyControlRunnable> releaseRunnable =
+      new PromiseWorkerProxyControlRunnable(mWorkerPrivate, mPromiseProxy);
 
     if (!releaseRunnable->Dispatch(cx)) {
-      NS_RUNTIMEABORT("Failed to dispatch PromiseHolder control runnable.");
+      NS_RUNTIMEABORT("Failed to dispatch MatchAll promise control runnable.");
     }
 
     return NS_OK;
@@ -264,16 +158,16 @@ ServiceWorkerClients::MatchAll(const ClientQueryOptions& aOptions,
     return nullptr;
   }
 
-  nsRefPtr<PromiseHolder> promiseHolder = new PromiseHolder(workerPrivate,
-                                                            promise);
-  if (!promiseHolder->GetPromise()) {
+  nsRefPtr<PromiseWorkerProxy> promiseProxy =
+    PromiseWorkerProxy::Create(workerPrivate, promise);
+  if (!promiseProxy->GetWorkerPromise()) {
     // Don't dispatch if adding the worker feature failed.
     return promise.forget();
   }
 
   nsRefPtr<MatchAllRunnable> r =
     new MatchAllRunnable(workerPrivate,
-                         promiseHolder,
+                         promiseProxy,
                          NS_ConvertUTF16toUTF8(scope));
   nsresult rv = NS_DispatchToMainThread(r);