[es6] refactor Promise resolution

Several changes are included here:

1. Each resolution callback references shared data indicating whether
it has already been resolved or not, as described in 25.4.1.3
http://tc39.github.io/ecma262/#sec-createresolvingfunctions.
Previously this was handled exclusively by the Promise's status,
which does not work correctly with the current chaining behaviour.

2. During fulfillment, When a Promise is resolved with a thenable, the
spec chains the promises together by invoking the thenable's `then`
function with the original Promise's resolve and reject methods (per
section 25.4.2.2, or
http://tc39.github.io/ecma262/#sec-promiseresolvethenablejob, on the
next tick, regardless of whether or not there are pending tasks.

3. Adds a spec compliance fix to ensure that the Promise constructor
is only loaded once when `then()` is called, solving v8:4539 as well.
This involves refactoring PromiseChain to accept a constructor
argument. PromiseChain/PromiseDeferred will hopefully be removed soon,
simplifying the process.

BUG=v8:4162, v8:4539, v8:3237
LOG=N
R=rossberg@chromium.org, littledan@chromium.org, adamk@chromium.org

Review URL: https://codereview.chromium.org/1394463003

Cr-Commit-Position: refs/heads/master@{#32046}

src/js/promise.js

@@ -34,6 +34,32 @@ utils.Import(function(from) {
 // Status values: 0 = pending, +1 = resolved, -1 = rejected
 var lastMicrotaskId = 0;
 
+function CreateResolvingFunctions(promise) {
+  var alreadyResolved = false;
+
+  var resolve = function(value) {
+    if (alreadyResolved === true) return;
+    alreadyResolved = true;
+    if (value === promise) {
+      return PromiseReject(promise, MakeTypeError(kPromiseCyclic, value));
+    }
+    PromiseResolve(promise, value);
+  };
+
+  var reject = function(reason) {
+    if (alreadyResolved === true) return;
+    alreadyResolved = true;
+    PromiseReject(promise, reason);
+  };
+
+  return {
+    __proto__: null,
+    resolve: resolve,
+    reject: reject
+  };
+}
+
+
 var GlobalPromise = function Promise(resolver) {
   if (resolver === promiseRawSymbol) return;
   if (!%_IsConstructCall()) throw MakeTypeError(kNotAPromise, this);
@@ -42,8 +68,8 @@ var GlobalPromise = function Promise(resolver) {
   var promise = PromiseInit(this);
   try {
     %DebugPushPromise(promise, Promise, resolver);
-    resolver(function(x) { PromiseResolve(promise, x) },
-             function(r) { PromiseReject(promise, r) });
+    var callbacks = CreateResolvingFunctions(promise);
+    resolver(callbacks.resolve, callbacks.reject);
   } catch (e) {
     PromiseReject(promise, e);
   } finally {
@@ -93,7 +119,7 @@ function PromiseCoerce(constructor, x) {
       return %_Call(PromiseRejected, constructor, r);
     }
     if (IS_CALLABLE(then)) {
-      var deferred = %_Call(PromiseDeferred, constructor);
+      var deferred = NewPromiseCapability(constructor);
       try {
         %_Call(then, x, deferred.resolve, deferred.reject);
       } catch(r) {
@@ -161,7 +187,28 @@ function PromiseCreate() {
 }
 
 function PromiseResolve(promise, x) {
-  PromiseDone(promise, +1, x, promiseOnResolveSymbol)
+  if (GET_PRIVATE(promise, promiseStatusSymbol) === 0) {
+    if (IS_SPEC_OBJECT(x)) {
+      // 25.4.1.3.2 steps 8-12
+      try {
+        var then = x.then;
+      } catch (e) {
+        return PromiseReject(promise, e);
+      }
+      if (IS_CALLABLE(then)) {
+        // PromiseResolveThenableJob
+        return %EnqueueMicrotask(function() {
+          try {
+            var callbacks = CreateResolvingFunctions(promise);
+            %_Call(then, x, callbacks.resolve, callbacks.reject);
+          } catch (e) {
+            PromiseReject(promise, e);
+          }
+        });
+      }
+    }
+    PromiseDone(promise, +1, x, promiseOnResolveSymbol);
+  }
 }
 
 function PromiseReject(promise, r) {
@@ -179,18 +226,19 @@ function PromiseReject(promise, r) {
 
 // Convenience.
 
-function PromiseDeferred() {
-  if (this === GlobalPromise) {
+function NewPromiseCapability(C) {
+  if (C === GlobalPromise) {
     // Optimized case, avoid extra closure.
     var promise = PromiseInit(new GlobalPromise(promiseRawSymbol));
+    var callbacks = CreateResolvingFunctions(promise);
     return {
       promise: promise,
-      resolve: function(x) { PromiseResolve(promise, x) },
-      reject: function(r) { PromiseReject(promise, r) }
+      resolve: callbacks.resolve,
+      reject: callbacks.reject
     };
   } else {
-    var result = {promise: UNDEFINED, reject: UNDEFINED, resolve: UNDEFINED};
-    result.promise = new this(function(resolve, reject) {
+    var result = {promise: UNDEFINED, resolve: UNDEFINED, reject: UNDEFINED };
+    result.promise = new C(function(resolve, reject) {
       result.resolve = resolve;
       result.reject = reject;
     });
@@ -198,6 +246,10 @@ function PromiseDeferred() {
   }
 }
 
+function PromiseDeferred() {
+  return NewPromiseCapability(this);
+}
+
 function PromiseResolved(x) {
   if (this === GlobalPromise) {
     // Optimized case, avoid extra closure.
@@ -223,10 +275,11 @@ function PromiseRejected(r) {
 
 // Simple chaining.
 
-function PromiseChain(onResolve, onReject) {  // a.k.a. flatMap
+// PromiseChain a.k.a. flatMap
+function PromiseChainInternal(constructor, onResolve, onReject) {
   onResolve = IS_UNDEFINED(onResolve) ? PromiseIdResolveHandler : onResolve;
   onReject = IS_UNDEFINED(onReject) ? PromiseIdRejectHandler : onReject;
-  var deferred = %_Call(PromiseDeferred, this.constructor);
+  var deferred = NewPromiseCapability(constructor);
   switch (GET_PRIVATE(this, promiseStatusSymbol)) {
     case UNDEFINED:
       throw MakeTypeError(kNotAPromise, this);
@@ -258,6 +311,11 @@ function PromiseChain(onResolve, onReject) {  // a.k.a. flatMap
   return deferred.promise;
 }
 
+function PromiseChain(onResolve, onReject) {
+  return %_Call(PromiseChainInternal, this, this.constructor,
+                onResolve, onReject);
+}
+
 function PromiseCatch(onReject) {
   return this.then(UNDEFINED, onReject);
 }
@@ -270,8 +328,9 @@ function PromiseThen(onResolve, onReject) {
   var that = this;
   var constructor = this.constructor;
   return %_Call(
-    PromiseChain,
+    PromiseChainInternal,
     this,
+    constructor,
     function(x) {
       x = PromiseCoerce(constructor, x);
       if (x === that) {
@@ -299,7 +358,7 @@ function PromiseCast(x) {
 }
 
 function PromiseAll(iterable) {
-  var deferred = %_Call(PromiseDeferred, this);
+  var deferred = NewPromiseCapability(this);
   var resolutions = [];
   try {
     var count = 0;
@@ -331,7 +390,7 @@ function PromiseAll(iterable) {
 }
 
 function PromiseRace(iterable) {
-  var deferred = %_Call(PromiseDeferred, this);
+  var deferred = NewPromiseCapability(this);
   try {
     for (var value of iterable) {
       var reject = function(r) { deferred.reject(r) };

test/cctest/test-api.cc

@@ -20308,13 +20308,13 @@ TEST(PromiseThen) {
   Handle<Function> f1 = Handle<Function>::Cast(global->Get(v8_str("f1")));
   Handle<Function> f2 = Handle<Function>::Cast(global->Get(v8_str("f2")));
 
-  // Chain
-  q->Chain(f1);
-  CHECK(global->Get(v8_str("x1"))->IsNumber());
+  // TODO(caitp): remove tests once PromiseChain is removed, per bug 3237
+  /* q->Chain(f1);
+  CHECK(global->Get(v8_str2("x1"))->IsNumber());
   CHECK_EQ(0, global->Get(v8_str("x1"))->Int32Value());
   isolate->RunMicrotasks();
   CHECK(!global->Get(v8_str("x1"))->IsNumber());
-  CHECK(p->Equals(global->Get(v8_str("x1"))));
+  CHECK(p->Equals(global->Get(v8_str("x1")))); */
 
   // Then
   CompileRun("x1 = x2 = 0;");

test/mjsunit/es6/promises.js

@@ -93,13 +93,30 @@ function assertAsync(b, s) {
   --asyncAssertsExpected
 }
 
+function assertLater(f, name) {
+  assertFalse(f()); // should not be true synchronously
+  ++asyncAssertsExpected;
+  var iterations = 0;
+  function runAssertion() {
+    if (f()) {
+      print(name, "succeeded");
+      --asyncAssertsExpected;
+    } else if (iterations++ < 10) {
+      %EnqueueMicrotask(runAssertion);
+    } else {
+      %AbortJS(name + " FAILED!");
+    }
+  }
+  %EnqueueMicrotask(runAssertion);
+}
+
 function assertAsyncDone(iteration) {
   var iteration = iteration || 0;
   %EnqueueMicrotask(function() {
     if (asyncAssertsExpected === 0)
       assertAsync(true, "all")
     else if (iteration > 10)  // Shouldn't take more.
-      assertAsync(false, "all")
+      assertAsync(false, "all... " + asyncAssertsExpected)
     else
       assertAsyncDone(iteration + 1)
   });
@@ -216,6 +233,7 @@ function assertAsyncDone(iteration) {
   assertAsyncRan()
 })();
 
+/* TODO(caitp): remove tests once PromiseChain is removed, per bug 3237
 (function() {
   var p1 = Promise.accept(5)
   var p2 = Promise.accept(p1)
@@ -225,7 +243,7 @@ function assertAsyncDone(iteration) {
     assertUnreachable
   )
   assertAsyncRan()
-})();
+})();*/
 
 (function() {
   var p1 = Promise.accept(5)
@@ -520,6 +538,7 @@ function assertAsyncDone(iteration) {
   assertAsyncRan()
 })();
 
+/* TODO(caitp): remove tests once PromiseChain is removed, per bug v8:3237
 (function() {
   var p1 = Promise.accept(5)
   var p2 = Promise.accept(p1)
@@ -531,7 +550,7 @@ function assertAsyncDone(iteration) {
   )
   deferred.resolve(p2)
   assertAsyncRan()
-})();
+})(); */
 
 (function() {
   var p1 = Promise.accept(5)
@@ -572,6 +591,7 @@ function assertAsyncDone(iteration) {
   assertAsyncRan()
 })();
 
+/* TODO(caitp): remove tests once PromiseChain is removed, per bug v8:3237
 (function() {
   var p1 = Promise.accept(5)
   var p2 = {then: function(onResolve, onReject) { onResolve(p1) }}
@@ -583,7 +603,7 @@ function assertAsyncDone(iteration) {
   )
   deferred.resolve(p2)
   assertAsyncRan()
-})();
+})(); */
 
 (function() {
   var p1 = Promise.accept(5)
@@ -618,6 +638,7 @@ function assertAsyncDone(iteration) {
   assertAsyncRan()
 })();
 
+/* TODO(caitp): remove tests once PromiseChain is removed, per bug v8:3237
 (function() {
   var deferred = Promise.defer()
   var p = deferred.promise
@@ -627,8 +648,9 @@ function assertAsyncDone(iteration) {
     assertUnreachable
   )
   assertAsyncRan()
-})();
+})();*/
 
+/* TODO(caitp): remove tests once PromiseChain is removed, per bug v8:3237
 (function() {
   var deferred = Promise.defer()
   var p = deferred.promise
@@ -638,7 +660,7 @@ function assertAsyncDone(iteration) {
     function(r) { assertAsync(r instanceof TypeError, "cyclic/deferred/then") }
   )
   assertAsyncRan()
-})();
+})();*/
 
 (function() {
   Promise.all([]).chain(
@@ -1059,4 +1081,39 @@ function assertAsyncDone(iteration) {
              "subclass/resolve/descendant with transplanted own constructor");
 }());
 
+(function() {
+  var thenCalled = false;
+
+  var resolve;
+  var promise = new Promise(function(res) { resolve = res; });
+  resolve({ then() { thenCalled = true; throw new Error(); } });
+  assertLater(function() { return thenCalled; }, "resolve-with-thenable");
+});
+
+(function() {
+  var calledWith;
+
+  var resolve;
+  var p1 = (new Promise(function(res) { resolve = res; }));
+  var p2 = p1.then(function(v) {
+    return {
+      then(resolve, reject) { resolve({ then() { calledWith = v }}); }
+    };
+  });
+
+  resolve({ then(resolve) { resolve(2); } });
+  assertLater(function() { return calledWith === 2; },
+              "resolve-with-thenable2");
+})();
+
+(function() {
+  var p = Promise.resolve();
+  var callCount = 0;
+  defineProperty(p, "constructor", {
+    get: function() { ++callCount; return Promise; }
+  });
+  p.then();
+  assertEquals(1, callCount);
+})();
+
 assertAsyncDone()