Bug 1187350 - update() should return a Promise. r=ehsan,catalinb

--HG--
extra : commitid : 2SkKnobC9jo
extra : rebase_source : 5f5d5c3ca57237fb63044c66a48861a1a4bf19f3
extra : amend_source : 5ea042fc5cc2be52594eca1fd6cda2f36057b3eb

dom/webidl/ServiceWorkerRegistration.webidl

@@ -17,9 +17,10 @@ interface ServiceWorkerRegistration : EventTarget {
 
   readonly attribute USVString scope;
 
-  void update();
+  [Throws, NewObject]
+  Promise<void> update();
 
-  [Throws]
+  [Throws, NewObject]
   Promise<boolean> unregister();
 
   // event

dom/workers/ServiceWorkerManager.cpp

@@ -541,28 +541,6 @@ private:
 
 };
 
-class ServiceWorkerUpdateFinishCallback
-{
-protected:
-  virtual ~ServiceWorkerUpdateFinishCallback()
-  { }
-
-public:
-  NS_INLINE_DECL_REFCOUNTING(ServiceWorkerUpdateFinishCallback)
-
-  virtual
-  void UpdateSucceeded(ServiceWorkerRegistrationInfo* aInfo)
-  { }
-
-  virtual
-  void UpdateFailed(nsresult aStatus)
-  { }
-
-  virtual
-  void UpdateFailed(const ErrorEventInit& aDesc)
-  { }
-};
-
 class ServiceWorkerResolveWindowPromiseOnUpdateCallback final : public ServiceWorkerUpdateFinishCallback
 {
   nsRefPtr<nsPIDOMWindow> mWindow;
@@ -4174,7 +4152,8 @@ ServiceWorkerManager::InvalidateServiceWorkerRegistrationWorker(ServiceWorkerReg
 
 void
 ServiceWorkerManager::SoftUpdate(nsIPrincipal* aPrincipal,
-                                 const nsACString& aScope)
+                                 const nsACString& aScope,
+                                 ServiceWorkerUpdateFinishCallback* aCallback)
 {
   MOZ_ASSERT(aPrincipal);
 
@@ -4184,21 +4163,23 @@ ServiceWorkerManager::SoftUpdate(nsIPrincipal* aPrincipal,
     return;
   }
 
-  SoftUpdate(scopeKey, aScope);
+  SoftUpdate(scopeKey, aScope, aCallback);
 }
 
 void
 ServiceWorkerManager::SoftUpdate(const OriginAttributes& aOriginAttributes,
-                                 const nsACString& aScope)
+                                 const nsACString& aScope,
+                                 ServiceWorkerUpdateFinishCallback* aCallback)
 {
   nsAutoCString scopeKey;
   aOriginAttributes.CreateSuffix(scopeKey);
-  SoftUpdate(scopeKey, aScope);
+  SoftUpdate(scopeKey, aScope, aCallback);
 }
 
 void
 ServiceWorkerManager::SoftUpdate(const nsACString& aScopeKey,
-                                 const nsACString& aScope)
+                                 const nsACString& aScope,
+                                 ServiceWorkerUpdateFinishCallback* aCallback)
 {
   nsRefPtr<ServiceWorkerRegistrationInfo> registration =
     GetRegistration(aScopeKey, aScope);
@@ -4231,8 +4212,10 @@ ServiceWorkerManager::SoftUpdate(const nsACString& aScopeKey,
     GetOrCreateJobQueue(aScopeKey, aScope);
   MOZ_ASSERT(queue);
 
-  nsRefPtr<ServiceWorkerUpdateFinishCallback> cb =
-    new ServiceWorkerUpdateFinishCallback();
+  nsRefPtr<ServiceWorkerUpdateFinishCallback> cb(aCallback);
+  if (!cb) {
+    cb = new ServiceWorkerUpdateFinishCallback();
+  }
 
   // "Invoke Update algorithm, or its equivalent, with client, registration as
   // its argument."

dom/workers/ServiceWorkerManager.h

@@ -130,6 +130,28 @@ public:
 
 };
 
+class ServiceWorkerUpdateFinishCallback
+{
+protected:
+  virtual ~ServiceWorkerUpdateFinishCallback()
+  { }
+
+public:
+  NS_INLINE_DECL_REFCOUNTING(ServiceWorkerUpdateFinishCallback)
+
+  virtual
+  void UpdateSucceeded(ServiceWorkerRegistrationInfo* aInfo)
+  { }
+
+  virtual
+  void UpdateFailed(nsresult aStatus)
+  { }
+
+  virtual
+  void UpdateFailed(const ErrorEventInit& aDesc)
+  { }
+};
+
 /*
  * Wherever the spec treats a worker instance and a description of said worker
  * as the same thing; i.e. "Resolve foo with
@@ -301,10 +323,14 @@ public:
                      ErrorResult& aRv);
 
   void
-  SoftUpdate(nsIPrincipal* aPrincipal, const nsACString& aScope);
+  SoftUpdate(nsIPrincipal* aPrincipal,
+             const nsACString& aScope,
+             ServiceWorkerUpdateFinishCallback* aCallback = nullptr);
 
   void
-  SoftUpdate(const OriginAttributes& aOriginAttributes, const nsACString& aScope);
+  SoftUpdate(const OriginAttributes& aOriginAttributes,
+             const nsACString& aScope,
+             ServiceWorkerUpdateFinishCallback* aCallback = nullptr);
 
   void
   PropagateSoftUpdate(const OriginAttributes& aOriginAttributes,
@@ -404,7 +430,9 @@ private:
   MaybeRemoveRegistrationInfo(const nsACString& aScopeKey);
 
   void
-  SoftUpdate(const nsACString& aScopeKey, const nsACString& aScope);
+  SoftUpdate(const nsACString& aScopeKey,
+             const nsACString& aScope,
+             ServiceWorkerUpdateFinishCallback* aCallback = nullptr);
 
   already_AddRefed<ServiceWorkerRegistrationInfo>
   GetRegistration(const nsACString& aScopeKey,

dom/workers/ServiceWorkerManagerChild.cpp

@@ -42,7 +42,7 @@ ServiceWorkerManagerChild::RecvNotifySoftUpdate(
   nsRefPtr<ServiceWorkerManager> swm = ServiceWorkerManager::GetInstance();
   MOZ_ASSERT(swm);
 
-  swm->SoftUpdate(aOriginAttributes, NS_ConvertUTF16toUTF8(aScope));
+  swm->SoftUpdate(aOriginAttributes, NS_ConvertUTF16toUTF8(aScope), nullptr);
   return true;
 }
 

dom/workers/ServiceWorkerRegistration.cpp

@@ -239,28 +239,142 @@ ServiceWorkerRegistrationMainThread::InvalidateWorkers(WhichServiceWorker aWhich
 namespace {
 
 void
-UpdateInternal(nsIPrincipal* aPrincipal, const nsAString& aScope)
+UpdateInternal(nsIPrincipal* aPrincipal,
+               const nsAString& aScope,
+               ServiceWorkerUpdateFinishCallback* aCallback)
 {
   AssertIsOnMainThread();
   MOZ_ASSERT(aPrincipal);
+  MOZ_ASSERT(aCallback);
 
   nsRefPtr<ServiceWorkerManager> swm = ServiceWorkerManager::GetInstance();
   MOZ_ASSERT(swm);
 
   // The spec defines ServiceWorkerRegistration.update() exactly as Soft Update.
-  swm->SoftUpdate(aPrincipal, NS_ConvertUTF16toUTF8(aScope));
+  swm->SoftUpdate(aPrincipal, NS_ConvertUTF16toUTF8(aScope), aCallback);
 }
 
-// This Runnable needs to have a valid WorkerPrivate. For this reason it is also
-// a WorkerFeature that is registered before dispatching itself to the
-// main-thread and it's removed with ReleaseRunnable when the operation is
-// completed. This will keep the worker alive as long as necessary.
+class MainThreadUpdateCallback final : public ServiceWorkerUpdateFinishCallback
+{
+  nsRefPtr<Promise> mPromise;
+
+  ~MainThreadUpdateCallback()
+  { }
+
+public:
+  explicit MainThreadUpdateCallback(Promise* aPromise)
+    : mPromise(aPromise)
+  {
+    AssertIsOnMainThread();
+  }
+
+  void
+  UpdateSucceeded(ServiceWorkerRegistrationInfo* aRegistration) override
+  {
+    mPromise->MaybeResolve(JS::UndefinedHandleValue);
+  }
+
+  using ServiceWorkerUpdateFinishCallback::UpdateFailed;
+
+  void
+  UpdateFailed(nsresult aStatus) override
+  {
+    mPromise->MaybeReject(aStatus);
+  }
+};
+
+class UpdateResultRunnable final : public WorkerRunnable
+{
+  nsRefPtr<PromiseWorkerProxy> mPromiseProxy;
+  nsresult mStatus;
+
+  ~UpdateResultRunnable()
+  {}
+
+public:
+  UpdateResultRunnable(PromiseWorkerProxy* aPromiseProxy, nsresult aStatus)
+    : WorkerRunnable(aPromiseProxy->GetWorkerPrivate(), WorkerThreadModifyBusyCount)
+    , mPromiseProxy(aPromiseProxy)
+    , mStatus(aStatus)
+  { }
+
+  bool
+  WorkerRun(JSContext* aCx, WorkerPrivate* aWorkerPrivate) override
+  {
+    Promise* promise = mPromiseProxy->GetWorkerPromise();
+    if (NS_SUCCEEDED(mStatus)) {
+      promise->MaybeResolve(JS::UndefinedHandleValue);
+    } else {
+      promise->MaybeReject(mStatus);
+    }
+    mPromiseProxy->CleanUp(aCx);
+    return true;
+  }
+};
+
+class WorkerThreadUpdateCallback final : public ServiceWorkerUpdateFinishCallback
+{
+  nsRefPtr<PromiseWorkerProxy> mPromiseProxy;
+
+  ~WorkerThreadUpdateCallback()
+  {
+    Finish(NS_ERROR_FAILURE);
+  }
+
+public:
+  explicit WorkerThreadUpdateCallback(PromiseWorkerProxy* aPromiseProxy)
+    : mPromiseProxy(aPromiseProxy)
+  {
+    AssertIsOnMainThread();
+  }
+
+  void
+  UpdateSucceeded(ServiceWorkerRegistrationInfo* aRegistration) override
+  {
+    Finish(NS_OK);
+  }
+
+  using ServiceWorkerUpdateFinishCallback::UpdateFailed;
+
+  void
+  UpdateFailed(nsresult aStatus) override
+  {
+    Finish(aStatus);
+  }
+
+  void
+  Finish(nsresult aStatus)
+  {
+    if (!mPromiseProxy) {
+      return;
+    }
+
+    nsRefPtr<PromiseWorkerProxy> proxy = mPromiseProxy.forget();
+
+    MutexAutoLock lock(proxy->GetCleanUpLock());
+    if (proxy->IsClean()) {
+      return;
+    }
+
+    AutoJSAPI jsapi;
+    jsapi.Init();
+
+    nsRefPtr<UpdateResultRunnable> r =
+      new UpdateResultRunnable(proxy, aStatus);
+    if (!r->Dispatch(jsapi.cx())) {
+      nsRefPtr<PromiseWorkerProxyControlRunnable> r =
+        new PromiseWorkerProxyControlRunnable(proxy->GetWorkerPrivate(), proxy);
+      r->Dispatch(jsapi.cx());
+    }
+  }
+};
+
 class UpdateRunnable final : public nsRunnable
-                           , public WorkerFeature
 {
 public:
-  UpdateRunnable(WorkerPrivate* aWorkerPrivate, const nsAString& aScope)
-    : mWorkerPrivate(aWorkerPrivate)
+  UpdateRunnable(PromiseWorkerProxy* aPromiseProxy,
+                 const nsAString& aScope)
+    : mPromiseProxy(aPromiseProxy)
     , mScope(aScope)
   {}
 
@@ -268,74 +382,24 @@ public:
   Run() override
   {
     AssertIsOnMainThread();
-    UpdateInternal(mWorkerPrivate->GetPrincipal(), mScope);
+    ErrorResult result;
 
-    class ReleaseRunnable final : public MainThreadWorkerControlRunnable
-    {
-      nsRefPtr<UpdateRunnable> mFeature;
-
-    public:
-      ReleaseRunnable(WorkerPrivate* aWorkerPrivate,
-                      UpdateRunnable* aFeature)
-        : MainThreadWorkerControlRunnable(aWorkerPrivate)
-        , mFeature(aFeature)
-      {
-        MOZ_ASSERT(aFeature);
-      }
-
-      virtual bool
-      WorkerRun(JSContext* aCx,
-                workers::WorkerPrivate* aWorkerPrivate) override
-      {
-        MOZ_ASSERT(aWorkerPrivate);
-        aWorkerPrivate->AssertIsOnWorkerThread();
-
-        aWorkerPrivate->RemoveFeature(aCx, mFeature);
-        return true;
-      }
-
-    private:
-      ~ReleaseRunnable()
-      {}
-    };
-
-    nsRefPtr<WorkerControlRunnable> runnable =
-      new ReleaseRunnable(mWorkerPrivate, this);
-    runnable->Dispatch(nullptr);
+    MutexAutoLock lock(mPromiseProxy->GetCleanUpLock());
+    if (mPromiseProxy->IsClean()) {
+      return NS_OK;
+    }
 
+    nsRefPtr<WorkerThreadUpdateCallback> cb =
+      new WorkerThreadUpdateCallback(mPromiseProxy);
+    UpdateInternal(mPromiseProxy->GetWorkerPrivate()->GetPrincipal(), mScope, cb);
     return NS_OK;
   }
 
-  virtual bool Notify(JSContext* aCx, workers::Status aStatus) override
-  {
-    // We don't care about the notification. We just want to keep the
-    // mWorkerPrivate alive.
-    return true;
-  }
-
-  bool
-  Dispatch()
-  {
-    mWorkerPrivate->AssertIsOnWorkerThread();
-
-    JSContext* cx = mWorkerPrivate->GetJSContext();
-
-    if (NS_WARN_IF(!mWorkerPrivate->AddFeature(cx, this))) {
-      return false;
-    }
-
-    nsCOMPtr<nsIRunnable> that(this);
-    if (NS_WARN_IF(NS_FAILED(NS_DispatchToMainThread(this)))) {
-      NS_ASSERTION(false, "Failed to dispatch update back to MainThread in ServiceWorker");
-    }
-    return true;
-  }
-
 private:
   ~UpdateRunnable()
   {}
 
-  WorkerPrivate* mWorkerPrivate;
+  nsRefPtr<PromiseWorkerProxy> mPromiseProxy;
   const nsString mScope;
 };
 
@@ -525,13 +589,29 @@ public:
 };
 } // namespace
 
-void
-ServiceWorkerRegistrationMainThread::Update()
+already_AddRefed<Promise>
+ServiceWorkerRegistrationMainThread::Update(ErrorResult& aRv)
 {
+  AssertIsOnMainThread();
+  nsCOMPtr<nsIGlobalObject> go = do_QueryInterface(GetOwner());
+  if (!go) {
+    aRv.Throw(NS_ERROR_FAILURE);
+    return nullptr;
+  }
+
+  nsRefPtr<Promise> promise = Promise::Create(go, aRv);
+  if (NS_WARN_IF(aRv.Failed())) {
+    return nullptr;
+  }
+
   nsCOMPtr<nsIDocument> doc = GetOwner()->GetExtantDoc();
   MOZ_ASSERT(doc);
 
-  UpdateInternal(doc->NodePrincipal(), mScope);
+  nsRefPtr<MainThreadUpdateCallback> cb =
+    new MainThreadUpdateCallback(promise);
+  UpdateInternal(doc->NodePrincipal(), mScope, cb);
+
+  return promise.forget();
 }
 
 already_AddRefed<Promise>
@@ -868,17 +948,28 @@ ServiceWorkerRegistrationWorkerThread::GetActive()
   return nullptr;
 }
 
-void
-ServiceWorkerRegistrationWorkerThread::Update()
+already_AddRefed<Promise>
+ServiceWorkerRegistrationWorkerThread::Update(ErrorResult& aRv)
 {
   WorkerPrivate* worker = GetCurrentThreadWorkerPrivate();
   MOZ_ASSERT(worker);
   worker->AssertIsOnWorkerThread();
 
-  // XXX: this pattern guarantees we won't know which thread UpdateRunnable
-  // will die on (here or MainThread)
-  nsRefPtr<UpdateRunnable> r = new UpdateRunnable(worker, mScope);
-  r->Dispatch();
+  nsRefPtr<Promise> promise = Promise::Create(worker->GlobalScope(), aRv);
+  if (aRv.Failed()) {
+    return nullptr;
+  }
+
+  nsRefPtr<PromiseWorkerProxy> proxy = PromiseWorkerProxy::Create(worker, promise);
+  if (!proxy) {
+    promise->MaybeResolve(NS_ERROR_DOM_ABORT_ERR);
+    return promise.forget();
+  }
+
+  nsRefPtr<UpdateRunnable> r = new UpdateRunnable(proxy, mScope);
+  MOZ_ALWAYS_TRUE(NS_SUCCEEDED(NS_DispatchToMainThread(r)));
+
+  return promise.forget();
 }
 
 already_AddRefed<Promise>

dom/workers/ServiceWorkerRegistration.h

@@ -106,8 +106,8 @@ public:
   ServiceWorkerRegistrationMainThread(nsPIDOMWindow* aWindow,
                                       const nsAString& aScope);
 
-  void
-  Update();
+  already_AddRefed<Promise>
+  Update(ErrorResult& aRv);
 
   already_AddRefed<Promise>
   Unregister(ErrorResult& aRv);
@@ -195,8 +195,8 @@ public:
   ServiceWorkerRegistrationWorkerThread(workers::WorkerPrivate* aWorkerPrivate,
                                         const nsAString& aScope);
 
-  void
-  Update();
+  already_AddRefed<Promise>
+  Update(ErrorResult& aRv);
 
   already_AddRefed<Promise>
   Unregister(ErrorResult& aRv);

dom/workers/test/serviceworkers/test_workerUpdate.html

@@ -23,6 +23,9 @@
         if (e.data === "FINISH") {
           ok(true, "The worker has updated itself");
           resolve();
+        } else if (e.data === "FAIL") {
+          ok(false, "The worker failed to update itself");
+          resolve();
         }
       }
     });

dom/workers/test/serviceworkers/worker_update.js

@@ -2,13 +2,18 @@
 // job queueing works properly when called from the worker thread. We should
 // test actual update scenarios with a SJS test.
 onmessage = function(e) {
-  self.registration.update();
-  clients.matchAll().then(function(c) {
-    if (c.length == 0) {
-      // We cannot proceed.
-      return;
-    }
+  self.registration.update().then(function(v) {
+    return v === undefined ? 'FINISH' : 'FAIL';
+  }).catch(function(e) {
+    return 'FAIL';
+  }).then(function(result) {
+    clients.matchAll().then(function(c) {
+      if (c.length == 0) {
+        dump("!!!!!!!!!!! WORKER HAS NO CLIENTS TO FINISH TEST !!!!!!!!!!!!\n");
+        return;
+      }
 
-    c[0].postMessage('FINISH');
+      c[0].postMessage(result);
+    });
   });
 }