Clean up promises and fix an edge case bug

This patch builds on previous Promise spec compliance work by
cleaning out some old code which existed to support
Promise.prototype.chain, rephrasing some code to correspond more
closely to the specification, and removing some incorrect brand
checking. A test is added for a bug in an edge case which was fixed.

R=rossberg
BUG=v8:3641
LOG=Y

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

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

src/js/promise.js

@@ -40,9 +40,6 @@ function CreateResolvingFunctions(promise) {
   var resolve = function(value) {
     if (alreadyResolved === true) return;
     alreadyResolved = true;
-    if (value === promise) {
-      return PromiseReject(promise, MakeTypeError(kPromiseCyclic, value));
-    }
     PromiseResolve(promise, value);
   };
 
@@ -116,37 +113,11 @@ function PromiseDone(promise, status, value, promiseQueue) {
   }
 }
 
-function PromiseCoerce(constructor, x) {
-  if (!IsPromise(x) && IS_SPEC_OBJECT(x)) {
-    var then;
-    try {
-      then = x.then;
-    } catch(r) {
-      return %_Call(PromiseRejected, constructor, r);
-    }
-    if (IS_CALLABLE(then)) {
-      var deferred = NewPromiseCapability(constructor);
-      try {
-        %_Call(then, x, deferred.resolve, deferred.reject);
-      } catch(r) {
-        deferred.reject(r);
-      }
-      return deferred.promise;
-    }
-  }
-  return x;
-}
-
 function PromiseHandle(value, handler, deferred) {
   try {
     %DebugPushPromise(deferred.promise, PromiseHandle);
     var result = handler(value);
-    if (result === deferred.promise)
-      throw MakeTypeError(kPromiseCyclic, result);
-    else if (IsPromise(result))
-      %_Call(PromiseChain, result, deferred.resolve, deferred.reject);
-    else
-      deferred.resolve(result);
+    deferred.resolve(result);
   } catch (exception) {
     try { deferred.reject(exception); } catch (e) { }
   } finally {
@@ -193,28 +164,29 @@ function PromiseCreate() {
 }
 
 function PromiseResolve(promise, x) {
-  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);
-          }
-        });
-      }
+  if (x === promise) {
+    return PromiseReject(promise, MakeTypeError(kPromiseCyclic, x));
+  }
+  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);
   }
+  PromiseDone(promise, +1, x, promiseOnResolveSymbol);
 }
 
 function PromiseReject(promise, r) {
@@ -245,6 +217,7 @@ function NewPromiseCapability(C) {
   } else {
     var result = {promise: UNDEFINED, resolve: UNDEFINED, reject: UNDEFINED };
     result.promise = new C(function(resolve, reject) {
+      // TODO(littledan): Check for resolve and reject being not undefined
       result.resolve = resolve;
       result.reject = reject;
     });
@@ -257,15 +230,13 @@ function PromiseDeferred() {
 }
 
 function PromiseResolved(x) {
-  if (this === GlobalPromise) {
-    // Optimized case, avoid extra closure.
-    return PromiseCreateAndSet(+1, x);
-  } else {
-    return new this(function(resolve, reject) { resolve(x) });
-  }
+  return %_Call(PromiseCast, this, x);
 }
 
 function PromiseRejected(r) {
+  if (!IS_SPEC_OBJECT(this)) {
+    throw MakeTypeError(kCalledOnNonObject, PromiseRejected);
+  }
   var promise;
   if (this === GlobalPromise) {
     // Optimized case, avoid extra closure.
@@ -279,15 +250,18 @@ function PromiseRejected(r) {
   return promise;
 }
 
-// Simple chaining.
+// Multi-unwrapped chaining with thenable coercion.
 
-// PromiseChain a.k.a. flatMap
-function PromiseChainInternal(constructor, onResolve, onReject) {
-  onResolve = IS_UNDEFINED(onResolve) ? PromiseIdResolveHandler : onResolve;
-  onReject = IS_UNDEFINED(onReject) ? PromiseIdRejectHandler : onReject;
+function PromiseThen(onResolve, onReject) {
+  var constructor = this.constructor;
+  onResolve = IS_CALLABLE(onResolve) ? onResolve : PromiseIdResolveHandler;
+  onReject = IS_CALLABLE(onReject) ? onReject : PromiseIdRejectHandler;
   var deferred = NewPromiseCapability(constructor);
   switch (GET_PRIVATE(this, promiseStatusSymbol)) {
     case UNDEFINED:
+      // TODO(littledan): The type check should be called before
+      // constructing NewPromiseCapability; this is observable when
+      // erroneously copying this method to other classes.
       throw MakeTypeError(kNotAPromise, this);
     case 0:  // Pending
       GET_PRIVATE(this, promiseOnResolveSymbol).push(onResolve, deferred);
@@ -317,47 +291,25 @@ function PromiseChainInternal(constructor, onResolve, onReject) {
   return deferred.promise;
 }
 
+// Chain is left around for now as an alias for then
 function PromiseChain(onResolve, onReject) {
-  return %_Call(PromiseChainInternal, this, this.constructor,
-                onResolve, onReject);
+  return %_Call(PromiseThen, this, onResolve, onReject);
 }
 
 function PromiseCatch(onReject) {
   return this.then(UNDEFINED, onReject);
 }
 
-// Multi-unwrapped chaining with thenable coercion.
-
-function PromiseThen(onResolve, onReject) {
-  onResolve = IS_CALLABLE(onResolve) ? onResolve : PromiseIdResolveHandler;
-  onReject = IS_CALLABLE(onReject) ? onReject : PromiseIdRejectHandler;
-  var that = this;
-  var constructor = this.constructor;
-  return %_Call(
-    PromiseChainInternal,
-    this,
-    constructor,
-    function(x) {
-      x = PromiseCoerce(constructor, x);
-      if (x === that) {
-        return onReject(MakeTypeError(kPromiseCyclic, x));
-      } else if (IsPromise(x)) {
-        return x.then(onResolve, onReject);
-      } else {
-        return onResolve(x);
-      }
-    },
-    onReject
-  );
-}
-
 // Combinators.
 
 function PromiseCast(x) {
+  if (!IS_SPEC_OBJECT(this)) {
+    throw MakeTypeError(kCalledOnNonObject, PromiseCast);
+  }
   if (IsPromise(x) && x.constructor === this) {
     return x;
   } else {
-    return new this(function(resolve) { resolve(x) });
+    return new this(function(resolve, reject) { resolve(x) });
   }
 }
 

test/mjsunit/es6/promises.js

@@ -193,8 +193,9 @@ function assertAsyncDone(iteration) {
   var p1 = Promise.accept(5)
   var p2 = Promise.accept(p1)
   var p3 = Promise.accept(p2)
+  // Note: Chain now has then-style semantics, here and in future tests.
   p3.chain(
-    function(x) { assertAsync(x === p2, "resolved/chain") },
+    function(x) { assertAsync(x === 5, "resolved/chain") },
     assertUnreachable
   )
   assertAsyncRan()
@@ -216,8 +217,8 @@ function assertAsyncDone(iteration) {
   var p2 = Promise.accept(p1)
   var p3 = Promise.accept(p2)
   p3.chain(
-    function(x) { assertAsync(x === p2, "rejected/chain") },
-    assertUnreachable
+    assertUnreachable,
+    function(x) { assertAsync(x === 5, "rejected/chain") }
   )
   assertAsyncRan()
 })();
@@ -233,17 +234,16 @@ 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)
   var p3 = Promise.accept(p2)
   p3.chain(function(x) { return x }, assertUnreachable).chain(
-    function(x) { assertAsync(x === p1, "resolved/chain/chain") },
+    function(x) { assertAsync(x === 5, "resolved/chain/chain") },
     assertUnreachable
   )
   assertAsyncRan()
-})();*/
+})();
 
 (function() {
   var p1 = Promise.accept(5)
@@ -371,7 +371,7 @@ function assertAsyncDone(iteration) {
   var p2 = {then: function(onResolve, onReject) { onResolve(p1) }}
   var p3 = Promise.accept(p2)
   p3.chain(
-    function(x) { assertAsync(x === p2, "resolved/thenable/chain") },
+    function(x) { assertAsync(x === 5, "resolved/thenable/chain") },
     assertUnreachable
   )
   assertAsyncRan()
@@ -393,8 +393,8 @@ function assertAsyncDone(iteration) {
   var p2 = {then: function(onResolve, onReject) { onResolve(p1) }}
   var p3 = Promise.accept(p2)
   p3.chain(
-    function(x) { assertAsync(x === p2, "rejected/thenable/chain") },
-    assertUnreachable
+    assertUnreachable,
+    function(x) { assertAsync(x === 5, "rejected/thenable/chain") }
   )
   assertAsyncRan()
 })();
@@ -416,7 +416,7 @@ function assertAsyncDone(iteration) {
   var p2 = Promise.accept(p1)
   var p3 = Promise.accept(p2)
   p3.chain(
-    function(x) { assertAsync(x === p2, "chain/resolve") },
+    function(x) { assertAsync(x === 5, "chain/resolve") },
     assertUnreachable
   )
   deferred.resolve(5)
@@ -426,8 +426,8 @@ function assertAsyncDone(iteration) {
 (function() {
   var deferred = Promise.defer()
   var p1 = deferred.promise
-  var p2 = Promise.accept(p1)
-  var p3 = Promise.accept(p2)
+  var p2 = Promise.resolve(p1)
+  var p3 = Promise.resolve(p2)
   p3.then(
     function(x) { assertAsync(x === 5, "then/resolve") },
     assertUnreachable
@@ -442,8 +442,8 @@ function assertAsyncDone(iteration) {
   var p2 = Promise.accept(p1)
   var p3 = Promise.accept(p2)
   p3.chain(
-    function(x) { assertAsync(x === p2, "chain/reject") },
-    assertUnreachable
+    assertUnreachable,
+    function(x) { assertAsync(x === 5, "chain/reject") }
   )
   deferred.reject(5)
   assertAsyncRan()
@@ -492,7 +492,7 @@ function assertAsyncDone(iteration) {
   var p2 = {then: function(onResolve, onReject) { onResolve(p1) }}
   var p3 = Promise.accept(p2)
   p3.chain(
-    function(x) { assertAsync(x === p2, "chain/resolve/thenable") },
+    function(x) { assertAsync(x === 5, "chain/resolve/thenable") },
     assertUnreachable
   )
   deferred.resolve(5)
@@ -518,8 +518,8 @@ function assertAsyncDone(iteration) {
   var p2 = {then: function(onResolve, onReject) { onResolve(p1) }}
   var p3 = Promise.accept(p2)
   p3.chain(
-    function(x) { assertAsync(x === p2, "chain/reject/thenable") },
-    assertUnreachable
+    assertUnreachable,
+    function(x) { assertAsync(x === 5, "chain/reject/thenable") }
   )
   deferred.reject(5)
   assertAsyncRan()
@@ -538,19 +538,18 @@ 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)
   var deferred = Promise.defer()
   var p3 = deferred.promise
   p3.chain(
-    function(x) { assertAsync(x === p2, "chain/resolve2") },
+    function(x) { assertAsync(x === 5, "chain/resolve2") },
     assertUnreachable
   )
   deferred.resolve(p2)
   assertAsyncRan()
-})(); */
+})();
 
 (function() {
   var p1 = Promise.accept(5)
@@ -591,19 +590,18 @@ 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) }}
   var deferred = Promise.defer()
   var p3 = deferred.promise
   p3.chain(
-    function(x) { assertAsync(x === p2, "chain/resolve/thenable2") },
+    function(x) { assertAsync(x === 5, "chain/resolve/thenable2") },
     assertUnreachable
   )
   deferred.resolve(p2)
   assertAsyncRan()
-})(); */
+})();
 
 (function() {
   var p1 = Promise.accept(5)
@@ -638,19 +636,17 @@ 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
   deferred.resolve(p)
   p.chain(
-    function(x) { assertAsync(x === p, "cyclic/deferred/chain") },
-    assertUnreachable
+    assertUnreachable,
+    function(r) { assertAsync(r instanceof TypeError, "cyclic/deferred/then") }
   )
   assertAsyncRan()
-})();*/
+})();
 
-/* TODO(caitp): remove tests once PromiseChain is removed, per bug v8:3237
 (function() {
   var deferred = Promise.defer()
   var p = deferred.promise
@@ -660,7 +656,7 @@ function assertAsyncDone(iteration) {
     function(r) { assertAsync(r instanceof TypeError, "cyclic/deferred/then") }
   )
   assertAsyncRan()
-})();*/
+})();
 
 (function() {
   Promise.all([]).chain(
@@ -763,7 +759,6 @@ function assertAsyncDone(iteration) {
   assertAsyncRan()
 })();
 
-
 (function() {
   'use strict';
   var getCalls = 0;

test/mjsunit/mjsunit.status

@@ -54,6 +54,12 @@
   # Issue 3784: setters-on-elements is flaky
   'setters-on-elements': [PASS, FAIL],
 
+  # Issue 3641: The new 'then' semantics suppress some exceptions.
+  # These tests may be changed or removed when 'chain' is deprecated.
+  'es6/debug-promises/reject-with-throw-in-reject': [FAIL],
+  'es6/debug-promises/reject-with-undefined-reject': [FAIL],
+  'es6/debug-promises/reject-with-invalid-reject': [FAIL],
+
   ##############################################################################
   # TurboFan compiler failures.
 

test/mjsunit/regress/regress-3641.js

+// Copyright 2015 the V8 project authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+// Flags: --allow-natives-syntax
+
+// If a Promise's then method is overridden, that should be respected
+// even if the promise is already resolved. x's resolution function is
+// only called by Promise.resolve(); there shouldn't be a resolution
+// check before when calling x.then.
+
+
+// Async assert framework copied from mjsunit/es6/promises.js
+
+var asyncAssertsExpected = 0;
+
+function assertAsyncRan() { ++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... " + asyncAssertsExpected)
+    else
+      assertAsyncDone(iteration + 1)
+  });
+}
+
+// End async assert framework
+
+var y;
+var x = Promise.resolve();
+x.then = () => { y = true; }
+Promise.resolve().then(() => x);
+assertLater(() => y === true, "y === true");
+
+assertAsyncDone();