Bug 1009569 - Promise then() must be called on a clean stack. r=bz
authorNikhil Marathe <nsm.nikhil@gmail.com>
Tue, 20 May 2014 14:21:13 -0700
changeset 204333 d489e0bc4e5dc16c423913c0e4c99d334b4a665e
parent 204332 0253f2720564f5daac9efdf45a9082328c0e4cfe
child 204334 2219d94afa5a12abdc17a5369396d9fb37bfd032
push id494
push userraliiev@mozilla.com
push dateMon, 25 Aug 2014 18:42:16 +0000
treeherdermozilla-release@a3cc3e46b571 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbz
bugs1009569
milestone32.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 1009569 - Promise then() must be called on a clean stack. r=bz
dom/promise/Promise.cpp
dom/promise/Promise.h
dom/promise/tests/test_promise.html
dom/workers/test/promise_worker.js
--- a/dom/promise/Promise.cpp
+++ b/dom/promise/Promise.cpp
@@ -163,16 +163,219 @@ public:
   bool
   WorkerRun(JSContext* aCx, WorkerPrivate* aWorkerPrivate)
   {
     RunInternal();
     return true;
   }
 };
 
+enum {
+  SLOT_PROMISE = 0,
+  SLOT_DATA
+};
+
+/*
+ * Utilities for thenable callbacks.
+ *
+ * A thenable is a { then: function(resolve, reject) { } }.
+ * `then` is called with a resolve and reject callback pair.
+ * Since only one of these should be called at most once (first call wins), the
+ * two keep a reference to each other in SLOT_DATA. When either of them is
+ * called, the references are cleared. Further calls are ignored.
+ */
+namespace {
+void
+LinkThenableCallables(JSContext* aCx, JS::Handle<JSObject*> aResolveFunc,
+                      JS::Handle<JSObject*> aRejectFunc)
+{
+  js::SetFunctionNativeReserved(aResolveFunc, SLOT_DATA,
+                                JS::ObjectValue(*aRejectFunc));
+  js::SetFunctionNativeReserved(aRejectFunc, SLOT_DATA,
+                                JS::ObjectValue(*aResolveFunc));
+}
+
+/*
+ * Returns false if callback was already called before, otherwise breaks the
+ * links and returns true.
+ */
+bool
+MarkAsCalledIfNotCalledBefore(JSContext* aCx, JS::Handle<JSObject*> aFunc)
+{
+  JS::Value otherFuncVal = js::GetFunctionNativeReserved(aFunc, SLOT_DATA);
+
+  if (!otherFuncVal.isObject()) {
+    return false;
+  }
+
+  JSObject* otherFuncObj = &otherFuncVal.toObject();
+  MOZ_ASSERT(js::GetFunctionNativeReserved(otherFuncObj, SLOT_DATA).isObject());
+
+  // Break both references.
+  js::SetFunctionNativeReserved(aFunc, SLOT_DATA, JS::UndefinedValue());
+  js::SetFunctionNativeReserved(otherFuncObj, SLOT_DATA, JS::UndefinedValue());
+
+  return true;
+}
+
+Promise*
+GetPromise(JSContext* aCx, JS::Handle<JSObject*> aFunc)
+{
+  JS::Value promiseVal = js::GetFunctionNativeReserved(aFunc, SLOT_PROMISE);
+
+  MOZ_ASSERT(promiseVal.isObject());
+
+  Promise* promise;
+  UNWRAP_OBJECT(Promise, &promiseVal.toObject(), promise);
+  return promise;
+}
+};
+
+// Equivalent to the specification's ResolvePromiseViaThenableTask.
+class ThenableResolverMixin
+{
+public:
+  ThenableResolverMixin(Promise* aPromise,
+                        JS::Handle<JSObject*> aThenable,
+                        PromiseInit* aThen)
+    : mPromise(aPromise)
+    , mThenable(CycleCollectedJSRuntime::Get()->Runtime(), aThenable)
+    , mThen(aThen)
+  {
+    MOZ_ASSERT(aPromise);
+    MOZ_COUNT_CTOR(ThenableResolverMixin);
+  }
+
+  virtual ~ThenableResolverMixin()
+  {
+    NS_ASSERT_OWNINGTHREAD(ThenableResolverMixin);
+    MOZ_COUNT_DTOR(ThenableResolverMixin);
+  }
+
+protected:
+  void
+  RunInternal()
+  {
+    NS_ASSERT_OWNINGTHREAD(ThenableResolverMixin);
+    ThreadsafeAutoJSContext cx;
+    JS::Rooted<JSObject*> wrapper(cx, mPromise->GetOrCreateWrapper(cx));
+    if (!wrapper) {
+      return;
+    }
+    JSAutoCompartment ac(cx, wrapper);
+
+    JS::Rooted<JSObject*> resolveFunc(cx,
+      mPromise->CreateThenableFunction(cx, mPromise, PromiseCallback::Resolve));
+
+    if (!resolveFunc) {
+      mPromise->HandleException(cx);
+      return;
+    }
+
+    JS::Rooted<JSObject*> rejectFunc(cx,
+      mPromise->CreateThenableFunction(cx, mPromise, PromiseCallback::Reject));
+    if (!rejectFunc) {
+      mPromise->HandleException(cx);
+      return;
+    }
+
+    LinkThenableCallables(cx, resolveFunc, rejectFunc);
+
+    ErrorResult rv;
+
+    JS::Rooted<JSObject*> rootedThenable(cx, mThenable);
+
+    mThen->Call(rootedThenable, resolveFunc, rejectFunc, rv,
+                CallbackObject::eRethrowExceptions);
+
+    rv.WouldReportJSException();
+    if (rv.IsJSException()) {
+      JS::Rooted<JS::Value> exn(cx);
+      rv.StealJSException(cx, &exn);
+
+      bool couldMarkAsCalled = MarkAsCalledIfNotCalledBefore(cx, resolveFunc);
+
+      // If we could mark as called, neither of the callbacks had been called
+      // when the exception was thrown. So we can reject the Promise.
+      if (couldMarkAsCalled) {
+        bool ok = JS_WrapValue(cx, &exn);
+        MOZ_ASSERT(ok);
+        if (!ok) {
+          NS_WARNING("Failed to wrap value into the right compartment.");
+        }
+
+        mPromise->RejectInternal(cx, exn, Promise::SyncTask);
+      }
+      // At least one of resolveFunc or rejectFunc have been called, so ignore
+      // the exception. FIXME(nsm): This should be reported to the error
+      // console though, for debugging.
+    }
+  }
+
+private:
+  nsRefPtr<Promise> mPromise;
+  JS::PersistentRooted<JSObject*> mThenable;
+  nsRefPtr<PromiseInit> mThen;
+  NS_DECL_OWNINGTHREAD;
+};
+
+// Main thread runnable to resolve thenables.
+class ThenableResolverTask MOZ_FINAL : public nsRunnable,
+                                       public ThenableResolverMixin
+{
+public:
+  ThenableResolverTask(Promise* aPromise,
+                       JS::Handle<JSObject*> aThenable,
+                       PromiseInit* aThen)
+    : ThenableResolverMixin(aPromise, aThenable, aThen)
+  {
+    MOZ_ASSERT(NS_IsMainThread());
+  }
+
+  ~ThenableResolverTask()
+  {
+    MOZ_ASSERT(NS_IsMainThread());
+  }
+
+  NS_IMETHOD Run()
+  {
+    MOZ_ASSERT(NS_IsMainThread());
+    RunInternal();
+    return NS_OK;
+  }
+};
+
+// Worker thread runnable to resolve thenables.
+class WorkerThenableResolverTask MOZ_FINAL : public WorkerSameThreadRunnable,
+                                             public ThenableResolverMixin
+{
+public:
+  WorkerThenableResolverTask(WorkerPrivate* aWorkerPrivate,
+                             Promise* aPromise,
+                             JS::Handle<JSObject*> aThenable,
+                             PromiseInit* aThen)
+    : WorkerSameThreadRunnable(aWorkerPrivate),
+      ThenableResolverMixin(aPromise, aThenable, aThen)
+  {
+    MOZ_ASSERT(aWorkerPrivate);
+    aWorkerPrivate->AssertIsOnWorkerThread();
+  }
+
+  ~WorkerThenableResolverTask()
+  {}
+
+  bool
+  WorkerRun(JSContext* aCx, WorkerPrivate* aWorkerPrivate)
+  {
+    aWorkerPrivate->AssertIsOnWorkerThread();
+    RunInternal();
+    return true;
+  }
+};
+
 // Promise
 
 NS_IMPL_CYCLE_COLLECTION_CLASS(Promise)
 
 NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN(Promise)
   tmp->MaybeReportRejectedOnce();
   NS_IMPL_CYCLE_COLLECTION_UNLINK(mGlobal)
   NS_IMPL_CYCLE_COLLECTION_UNLINK(mResolveCallbacks)
@@ -262,21 +465,16 @@ Promise::MaybeResolve(JSContext* aCx,
 
 void
 Promise::MaybeReject(JSContext* aCx,
                      JS::Handle<JS::Value> aValue)
 {
   MaybeRejectInternal(aCx, aValue);
 }
 
-enum {
-  SLOT_PROMISE = 0,
-  SLOT_DATA
-};
-
 /* static */ bool
 Promise::JSCallback(JSContext* aCx, unsigned aArgc, JS::Value* aVp)
 {
   JS::CallArgs args = CallArgsFromVp(aArgc, aVp);
 
   JS::Rooted<JS::Value> v(aCx,
                           js::GetFunctionNativeReserved(&args.callee(),
                                                         SLOT_PROMISE));
@@ -295,72 +493,16 @@ Promise::JSCallback(JSContext* aCx, unsi
   } else {
     promise->MaybeRejectInternal(aCx, args.get(0));
   }
 
   return true;
 }
 
 /*
- * Utilities for thenable callbacks.
- *
- * A thenable is a { then: function(resolve, reject) { } }.
- * `then` is called with a resolve and reject callback pair.
- * Since only one of these should be called at most once (first call wins), the
- * two keep a reference to each other in SLOT_DATA. When either of them is
- * called, the references are cleared. Further calls are ignored.
- */
-namespace {
-void
-LinkThenableCallables(JSContext* aCx, JS::Handle<JSObject*> aResolveFunc,
-                      JS::Handle<JSObject*> aRejectFunc)
-{
-  js::SetFunctionNativeReserved(aResolveFunc, SLOT_DATA,
-                                JS::ObjectValue(*aRejectFunc));
-  js::SetFunctionNativeReserved(aRejectFunc, SLOT_DATA,
-                                JS::ObjectValue(*aResolveFunc));
-}
-
-/*
- * Returns false if callback was already called before, otherwise breaks the
- * links and returns true.
- */
-bool
-MarkAsCalledIfNotCalledBefore(JSContext* aCx, JS::Handle<JSObject*> aFunc)
-{
-  JS::Value otherFuncVal = js::GetFunctionNativeReserved(aFunc, SLOT_DATA);
-
-  if (!otherFuncVal.isObject()) {
-    return false;
-  }
-
-  JSObject* otherFuncObj = &otherFuncVal.toObject();
-  MOZ_ASSERT(js::GetFunctionNativeReserved(otherFuncObj, SLOT_DATA).isObject());
-
-  // Break both references.
-  js::SetFunctionNativeReserved(aFunc, SLOT_DATA, JS::UndefinedValue());
-  js::SetFunctionNativeReserved(otherFuncObj, SLOT_DATA, JS::UndefinedValue());
-
-  return true;
-}
-
-Promise*
-GetPromise(JSContext* aCx, JS::Handle<JSObject*> aFunc)
-{
-  JS::Value promiseVal = js::GetFunctionNativeReserved(aFunc, SLOT_PROMISE);
-
-  MOZ_ASSERT(promiseVal.isObject());
-
-  Promise* promise;
-  UNWRAP_OBJECT(Promise, &promiseVal.toObject(), promise);
-  return promise;
-}
-};
-
-/*
  * Common bits of (JSCallbackThenableResolver/JSCallbackThenableRejecter).
  * Resolves/rejects the Promise if it is ok to do so, based on whether either of
  * the callbacks have been called before or not.
  */
 /* static */ bool
 Promise::ThenableResolverCommon(JSContext* aCx, uint32_t aTask,
                                 unsigned aArgc, JS::Value* aVp)
 {
@@ -977,62 +1119,30 @@ Promise::ResolveInternal(JSContext* aCx,
     // Thenables.
     JS::Rooted<JS::Value> then(aCx);
     if (!JS_GetProperty(aCx, valueObj, "then", &then)) {
       HandleException(aCx);
       return;
     }
 
     if (then.isObject() && JS_ObjectIsCallable(aCx, &then.toObject())) {
-      JS::Rooted<JSObject*> resolveFunc(aCx,
-        CreateThenableFunction(aCx, this, PromiseCallback::Resolve));
-
-      if (!resolveFunc) {
-        HandleException(aCx);
-        return;
-      }
-
-      JS::Rooted<JSObject*> rejectFunc(aCx,
-        CreateThenableFunction(aCx, this, PromiseCallback::Reject));
-      if (!rejectFunc) {
-        HandleException(aCx);
-        return;
-      }
-
-      LinkThenableCallables(aCx, resolveFunc, rejectFunc);
-
+      // This is the then() function of the thenable aValueObj.
       JS::Rooted<JSObject*> thenObj(aCx, &then.toObject());
       nsRefPtr<PromiseInit> thenCallback =
         new PromiseInit(thenObj, mozilla::dom::GetIncumbentGlobal());
-
-      ErrorResult rv;
-      thenCallback->Call(valueObj, resolveFunc, rejectFunc,
-                         rv, CallbackObject::eRethrowExceptions);
-      rv.WouldReportJSException();
-
-      if (rv.IsJSException()) {
-        JS::Rooted<JS::Value> exn(aCx);
-        rv.StealJSException(aCx, &exn);
-
-        bool couldMarkAsCalled = MarkAsCalledIfNotCalledBefore(aCx, resolveFunc);
-
-        // If we could mark as called, neither of the callbacks had been called
-        // when the exception was thrown. So we can reject the Promise.
-        if (couldMarkAsCalled) {
-          bool ok = JS_WrapValue(aCx, &exn);
-          MOZ_ASSERT(ok);
-          if (!ok) {
-            NS_WARNING("Failed to wrap value into the right compartment.");
-          }
-
-          RejectInternal(aCx, exn, Promise::SyncTask);
-        }
-        // At least one of resolveFunc or rejectFunc have been called, so ignore
-        // the exception. FIXME(nsm): This should be reported to the error
-        // console though, for debugging.
+      if (NS_IsMainThread()) {
+        nsRefPtr<ThenableResolverTask> task =
+          new ThenableResolverTask(this, valueObj, thenCallback);
+        NS_DispatchToCurrentThread(task);
+      } else {
+        WorkerPrivate* worker = GetCurrentThreadWorkerPrivate();
+        MOZ_ASSERT(worker);
+        nsRefPtr<WorkerThenableResolverTask> task =
+          new WorkerThenableResolverTask(worker, this, valueObj, thenCallback);
+        task->Dispatch(worker->GetJSContext());
       }
 
       return;
     }
   }
 
   // If the synchronous flag is set, process our resolve callbacks with
   // value. Otherwise, the synchronous flag is unset, queue a task to process
--- a/dom/promise/Promise.h
+++ b/dom/promise/Promise.h
@@ -54,16 +54,17 @@ class Promise MOZ_FINAL : public nsISupp
   friend class PromiseResolverMixin;
   friend class PromiseResolverTask;
   friend class PromiseTask;
   friend class PromiseReportRejectFeature;
   friend class PromiseWorkerProxy;
   friend class PromiseWorkerProxyRunnable;
   friend class RejectPromiseCallback;
   friend class ResolvePromiseCallback;
+  friend class ThenableResolverMixin;
   friend class WorkerPromiseResolverTask;
   friend class WorkerPromiseTask;
   friend class WrapperPromiseCallback;
 
   ~Promise();
 
 public:
   NS_DECL_CYCLE_COLLECTING_ISUPPORTS
--- a/dom/promise/tests/test_promise.html
+++ b/dom/promise/tests/test_promise.html
@@ -613,16 +613,42 @@ function promiseResolvePromise() {
   ok(cast instanceof Promise, "Should cast to a Promise.");
   is(cast, original, "Should return original Promise.");
   cast.then(function(v) {
     is(v, true, "Should resolve to true.");
     runTest();
   });
 }
 
+// Bug 1009569.
+// Ensure that thenables are run on a clean stack asynchronously.
+// Test case adopted from
+// https://gist.github.com/getify/d64bb01751b50ed6b281#file-bug1-js.
+function promiseResolveThenableCleanStack() {
+  function immed(s) { x++; s(); }
+  function incX(){ x++; }
+
+  var x = 0;
+  var thenable = { then: immed };
+  var results = [];
+
+  Promise.resolve(thenable).then(incX);
+  results.push(x);
+
+  // check what happens after all "next cycle" steps
+  // have had a chance to complete
+  setTimeout(function(){
+    results.push(x);
+    // Result should be [0, 2] since `thenable` will be called async.
+    is(results[0], 0, "Expected thenable to be called asynchronously");
+    is(results[1], 2, "Expected thenable to be called asynchronously");
+    runTest();
+  },1000);
+}
+
 var tests = [ promiseResolve, promiseReject,
               promiseException, promiseGC, promiseAsync,
               promiseDoubleThen, promiseThenException,
               promiseThenCatchThen, promiseRejectThenCatchThen,
               promiseRejectThenCatchThen2,
               promiseRejectThenCatchExceptionThen,
               promiseThenCatchOrderingResolve,
               promiseThenCatchOrderingReject,
@@ -643,16 +669,17 @@ var tests = [ promiseResolve, promiseRej
               promiseThenableThrowsAfterCallback,
               promiseThenableRejectThenResolve,
               promiseWithThenReplaced,
               promiseStrictHandlers,
               promiseStrictExecutorThisArg,
               promiseResolveArray,
               promiseResolveThenable,
               promiseResolvePromise,
+              promiseResolveThenableCleanStack,
             ];
 
 function runTest() {
   if (!tests.length) {
     SimpleTest.finish();
     return;
   }
 
--- a/dom/workers/test/promise_worker.js
+++ b/dom/workers/test/promise_worker.js
@@ -649,16 +649,42 @@ function promiseResolvePromise() {
   ok(cast instanceof Promise, "Should cast to a Promise.");
   is(cast, original, "Should return original Promise.");
   cast.then(function(v) {
     is(v, true, "Should resolve to true.");
     runTest();
   });
 }
 
+// Bug 1009569.
+// Ensure that thenables are run on a clean stack asynchronously.
+// Test case adopted from
+// https://gist.github.com/getify/d64bb01751b50ed6b281#file-bug1-js.
+function promiseResolveThenableCleanStack() {
+  function immed(s) { x++; s(); }
+  function incX(){ x++; }
+
+  var x = 0;
+  var thenable = { then: immed };
+  var results = [];
+
+  Promise.resolve(thenable).then(incX);
+  results.push(x);
+
+  // check what happens after all "next cycle" steps
+  // have had a chance to complete
+  setTimeout(function(){
+    results.push(x);
+    // Result should be [0, 2] since `thenable` will be called async.
+    is(results[0], 0, "Expected thenable to be called asynchronously");
+    is(results[1], 2, "Expected thenable to be called asynchronously");
+    runTest();
+  },1000);
+}
+
 var tests = [
     promiseResolve,
     promiseReject,
     promiseException,
     promiseAsync,
     promiseDoubleThen,
     promiseThenException,
     promiseThenCatchThen,
@@ -693,16 +719,18 @@ var tests = [
     promiseRaceValuesArray,
     promiseRacePromiseArray,
     promiseRaceReject,
     promiseRaceThrow,
 
     promiseResolveArray,
     promiseResolveThenable,
     promiseResolvePromise,
+
+    promiseResolveThenableCleanStack,
 ];
 
 function runTest() {
   if (!tests.length) {
     postMessage({ type: 'finish' });
     return;
   }