Revert of Clean up promises and fix an edge case bug (patchset #4 id:60001 of...

Revert of Clean up promises and fix an edge case bug (patchset #4 id:60001 of https://codereview.chromium.org/1488783002/ )

Reason for revert:
[Sheriff] Breaks layout tests:
https://build.chromium.org/p/client.v8.fyi/builders/V8-Blink%20Linux%2064/builds/3266

Please request rebase upstream first.

Original issue's description:
> 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
>
> Committed: https://crrev.com/1deb89c8fd3cb69714ae0a24e3b5a4e78f6b73b4
> Cr-Commit-Position: refs/heads/master@{#32627}

TBR=rossberg@chromium.org,caitpotter88@gmail.com,littledan@chromium.org
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=v8:3641

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

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

src/js/promise.js

@@ -40,6 +40,9 @@ 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);
   };
 
@@ -113,11 +116,37 @@ 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);
-    deferred.resolve(result);
+    if (result === deferred.promise)
+      throw MakeTypeError(kPromiseCyclic, result);
+    else if (IsPromise(result))
+      %_Call(PromiseChain, result, deferred.resolve, deferred.reject);
+    else
+      deferred.resolve(result);
   } catch (exception) {
     try { deferred.reject(exception); } catch (e) { }
   } finally {
@@ -164,29 +193,28 @@ function PromiseCreate() {
 }
 
 function PromiseResolve(promise, x) {
-  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);
-        }
-      });
+  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);
   }
-  PromiseDone(promise, +1, x, promiseOnResolveSymbol);
 }
 
 function PromiseReject(promise, r) {
@@ -217,7 +245,6 @@ 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;
     });
@@ -230,13 +257,15 @@ function PromiseDeferred() {
 }
 
 function PromiseResolved(x) {
-  return %_Call(PromiseCast, this, x);
+  if (this === GlobalPromise) {
+    // Optimized case, avoid extra closure.
+    return PromiseCreateAndSet(+1, x);
+  } else {
+    return new this(function(resolve, reject) { resolve(x) });
+  }
 }
 
 function PromiseRejected(r) {
-  if (!IS_SPEC_OBJECT(this)) {
-    throw MakeTypeError(kCalledOnNonObject, PromiseRejected);
-  }
   var promise;
   if (this === GlobalPromise) {
     // Optimized case, avoid extra closure.
@@ -250,18 +279,15 @@ function PromiseRejected(r) {
   return promise;
 }
 
-// Multi-unwrapped chaining with thenable coercion.
+// Simple chaining.
 
-function PromiseThen(onResolve, onReject) {
-  var constructor = this.constructor;
-  onResolve = IS_CALLABLE(onResolve) ? onResolve : PromiseIdResolveHandler;
-  onReject = IS_CALLABLE(onReject) ? onReject : PromiseIdRejectHandler;
+// PromiseChain a.k.a. flatMap
+function PromiseChainInternal(constructor, onResolve, onReject) {
+  onResolve = IS_UNDEFINED(onResolve) ? PromiseIdResolveHandler : onResolve;
+  onReject = IS_UNDEFINED(onReject) ? PromiseIdRejectHandler : onReject;
   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);
@@ -291,25 +317,47 @@ function PromiseThen(onResolve, onReject) {
   return deferred.promise;
 }
 
-// Chain is left around for now as an alias for then
 function PromiseChain(onResolve, onReject) {
-  return %_Call(PromiseThen, this, onResolve, onReject);
+  return %_Call(PromiseChainInternal, this, this.constructor,
+                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, reject) { resolve(x) });
+    return new this(function(resolve) { resolve(x) });
   }
 }
 

test/mjsunit/es6/promises.js

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

test/mjsunit/mjsunit.status

@@ -54,12 +54,6 @@
   # 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();