Bug 982726 - Patch 1.3 - Fix crash caused by a race condition in PromiseHolder. r=nsm
☠☠ backed out by 7cdcfb2bc1ce ☠ ☠
authorCatalin Badea <catalin.badea392@gmail.com>
Mon, 27 Oct 2014 14:51:00 +0100
changeset 212605 0ae1b3474b225c49b8ab398a423db4caa202354a
parent 212604 26cd857611b8a662f005c2b7975955a69ca9549a
child 212606 33ee15f0fbcc0b3ab98253e621acdbdfc16f0553
push id27721
push usercbook@mozilla.com
push dateTue, 28 Oct 2014 14:55:05 +0000
treeherdermozilla-central@c0ddb1b098ec [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersnsm
bugs982726
milestone36.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 982726 - Patch 1.3 - Fix crash caused by a race condition in PromiseHolder. r=nsm
dom/workers/ServiceWorkerClients.cpp
--- a/dom/workers/ServiceWorkerClients.cpp
+++ b/dom/workers/ServiceWorkerClients.cpp
@@ -40,49 +40,53 @@ ServiceWorkerClients::WrapObject(JSConte
 }
 
 namespace {
 
 // Helper class used for passing the promise between threads while
 // keeping the worker alive.
 class PromiseHolder MOZ_FINAL : public WorkerFeature
 {
+  friend class GetServicedRunnable;
+
   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*
-  Get() const
+  GetPromise() const
   {
     return mPromise;
   }
 
   void
   Clean()
   {
     mWorkerPrivate->AssertIsOnWorkerThread();
 
+    MutexAutoLock lock(mCleanUpLock);
     if (mClean) {
       return;
     }
 
     mPromise = nullptr;
     mWorkerPrivate->RemoveFeature(mWorkerPrivate->GetJSContext(), this);
     mClean = true;
   }
@@ -103,16 +107,21 @@ 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 MOZ_FINAL : public WorkerRunnable
 {
   nsRefPtr<PromiseHolder> mPromiseHolder;
   nsAutoPtr<nsTArray<uint64_t>> mValue;
 
@@ -128,17 +137,17 @@ public:
   }
 
   bool
   WorkerRun(JSContext* aCx, WorkerPrivate* aWorkerPrivate)
   {
     MOZ_ASSERT(aWorkerPrivate);
     aWorkerPrivate->AssertIsOnWorkerThread();
 
-    Promise* promise = mPromiseHolder->Get();
+    Promise* promise = mPromiseHolder->GetPromise();
     MOZ_ASSERT(promise);
 
     nsTArray<nsRefPtr<ServiceWorkerClient>> ret;
     for (size_t i = 0; i < mValue->Length(); i++) {
       ret.AppendElement(nsRefPtr<ServiceWorkerClient>(
             new ServiceWorkerClient(promise->GetParentObject(),
                                     mValue->ElementAt(i))));
     }
@@ -149,35 +158,41 @@ public:
 
     return true;
   }
 };
 
 class GetServicedRunnable MOZ_FINAL : public nsRunnable
 {
   WorkerPrivate* mWorkerPrivate;
+  nsRefPtr<PromiseHolder> mPromiseHolder;
   nsCString mScope;
-  nsRefPtr<PromiseHolder> mPromiseHolder;
 public:
   GetServicedRunnable(WorkerPrivate* aWorkerPrivate,
-                      Promise* aPromise,
+                      PromiseHolder* aPromiseHolder,
                       const nsCString& aScope)
     : mWorkerPrivate(aWorkerPrivate),
+      mPromiseHolder(aPromiseHolder),
       mScope(aScope)
   {
     MOZ_ASSERT(aWorkerPrivate);
     aWorkerPrivate->AssertIsOnWorkerThread();
-    mPromiseHolder = new PromiseHolder(aWorkerPrivate, aPromise);
   }
 
   NS_IMETHOD
   Run() MOZ_OVERRIDE
   {
     AssertIsOnMainThread();
 
+    MutexAutoLock lock(mPromiseHolder->mCleanUpLock);
+    if (mPromiseHolder->mClean) {
+      // Don't resolve the promise if it was already released.
+      return NS_OK;
+    }
+
     nsRefPtr<ServiceWorkerManager> swm = ServiceWorkerManager::GetInstance();
     nsAutoPtr<nsTArray<uint64_t>> result(new nsTArray<uint64_t>());
 
     swm->GetServicedClients(mScope, result);
     nsRefPtr<ResolvePromiseWorkerRunnable> r =
       new ResolvePromiseWorkerRunnable(mWorkerPrivate, mPromiseHolder, result);
 
     AutoSafeJSContext cx;
@@ -198,18 +213,27 @@ ServiceWorkerClients::GetServiced(ErrorR
   DOMString scope;
   mWorkerScope->GetScope(scope);
 
   nsRefPtr<Promise> promise = Promise::Create(mWorkerScope, aRv);
   if (NS_WARN_IF(aRv.Failed())) {
     return nullptr;
   }
 
+  nsRefPtr<PromiseHolder> promiseHolder = new PromiseHolder(workerPrivate,
+                                                            promise);
+  if (!promiseHolder->GetPromise()) {
+    // Don't dispatch if adding the worker feature failed.
+    return promise.forget();
+  }
+
   nsRefPtr<GetServicedRunnable> r =
-    new GetServicedRunnable(workerPrivate, promise, NS_ConvertUTF16toUTF8(scope));
+    new GetServicedRunnable(workerPrivate,
+                            promiseHolder,
+                            NS_ConvertUTF16toUTF8(scope));
   nsresult rv = NS_DispatchToMainThread(r);
 
   if (NS_WARN_IF(NS_FAILED(rv))) {
     promise->MaybeReject(NS_ERROR_NOT_AVAILABLE);
   }
 
   return promise.forget();
 }