[Promise.all] Fix: call IteratorClose if Promise.resolve is not callable

PerformPromiseAll doesn't set iteratorRecord.[[Done]] to true if
Promise.resolve is not callable. This makes Promise.all call
IteratorClose.

BUG=v8:10452

Change-Id: Icbe17416a733f68ef09f1c610d715f544c2a3b8a
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2164789Reviewed-by: Shu-yu Guo <syg@chromium.org>
Commit-Queue: Marja Hölttä <marja@chromium.org>
Cr-Commit-Position: refs/heads/master@{#67416}

src/builtins/promise-all.tq

@@ -142,20 +142,18 @@ namespace promise {
     // as that guards the lookup path for the "resolve" property on the
     // Promise constructor.
     let promiseResolveFunction: JSAny = Undefined;
+    try {
       try {
         if (!IsPromiseResolveLookupChainIntact(nativeContext, constructor)) {
-        // 5. Let _promiseResolve_ be ? Get(_constructor_, `"resolve"`).
           let promiseResolve: JSAny;
-        try {
+
+          // 5. Let _promiseResolve_ be ? Get(_constructor_, `"resolve"`).
           promiseResolve = GetProperty(constructor, kResolveString);
-        } catch (e) deferred {
-          iterator::IteratorCloseOnException(iter, e) otherwise Reject;
-        }
 
           // 6. If IsCallable(_promiseResolve_) is *false*, throw a *TypeError*
           // exception.
-        promiseResolveFunction = Cast<Callable>(promiseResolve)
-            otherwise ThrowTypeError(
+          promiseResolveFunction =
+              Cast<Callable>(promiseResolve) otherwise ThrowTypeError(
                   MessageTemplate::kCalledNonCallable, 'resolve');
         }
 
@@ -163,7 +161,7 @@ namespace promise {
             nativeContext[NativeContextSlot::ITERATOR_RESULT_MAP_INDEX]);
         while (true) {
           let nextValue: JSAny;
-
+          try {
             // Let next be IteratorStep(iteratorRecord.[[Iterator]]).
             // If next is an abrupt completion, set iteratorRecord.[[Done]] to
             // true. ReturnIfAbrupt(next).
@@ -175,28 +173,28 @@ namespace promise {
             // to true.
             // ReturnIfAbrupt(nextValue).
             nextValue = iterator::IteratorValue(next, fastIteratorResultMap);
+          } catch (e) {
+            goto Reject(e);
+          }
 
           // Check if we reached the limit.
           if (index == kPropertyArrayHashFieldMax) {
             // If there are too many elements (currently more than 2**21-1),
-          // raise a RangeError here (which is caught directly and turned into
-          // a rejection) of the resulting promise. We could gracefully handle
+            // raise a RangeError here (which is caught below and turned into
+            // a rejection of the resulting promise). We could gracefully handle
             // this case as well and support more than this number of elements
             // by going to a separate function and pass the larger indices via a
             // separate context, but it doesn't seem likely that we need this,
             // and it's unclear how the rest of the system deals with 2**21 live
-          // Promises anyways.
-          try {
+            // Promises anyway.
             ThrowRangeError(MessageTemplate::kTooManyElementsInPromiseAll);
-          } catch (e) deferred {
-            iterator::IteratorCloseOnException(iter, e) otherwise Reject;
-          }
           }
 
           // Set remainingElementsCount.[[Value]] to
           //     remainingElementsCount.[[Value]] + 1.
-        const remainingElementsCount = UnsafeCast<Smi>(
-            resolveElementContext[PromiseAllResolveElementContextSlots::
+          const remainingElementsCount =
+              UnsafeCast<Smi>(resolveElementContext
+                                  [PromiseAllResolveElementContextSlots::
                                        kPromiseAllResolveElementRemainingSlot]);
           resolveElementContext[PromiseAllResolveElementContextSlots::
                                     kPromiseAllResolveElementRemainingSlot] =
@@ -242,7 +240,6 @@ namespace promise {
               IsPromiseSpeciesProtectorCellInvalid() || Is<Smi>(nextValue) ||
               !IsPromiseThenLookupChainIntact(
                   nativeContext, UnsafeCast<HeapObject>(nextValue).map)) {
-          try {
             // Let nextPromise be ? Call(constructor, _promiseResolve_, «
             // nextValue »).
             const nextPromise = CallResolve(
@@ -262,9 +259,6 @@ namespace promise {
                 SetPropertyStrict(
                     context, thenResult, kPromiseHandledBySymbol, promise);
               }
-          } catch (e) deferred {
-            iterator::IteratorCloseOnException(iter, e) otherwise Reject;
-          }
           } else {
             PerformPromiseThenImpl(
                 UnsafeCast<JSPromise>(nextValue), resolveElementFun,
@@ -274,6 +268,9 @@ namespace promise {
           // Set index to index + 1.
           index += 1;
         }
+      } catch (e) deferred {
+        iterator::IteratorCloseOnException(iter, e) otherwise Reject;
+      }
     }
     label Done {}
 

test/mjsunit/es6/promise-all-resolve-not-callable.js

+// Copyright 2020 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
+
+load('test/mjsunit/test-async.js');
+
+// Promise.all should call IteratorClose if Promise.resolve is not callable.
+
+let returnCount = 0;
+let iter = {
+  [Symbol.iterator]() {
+    return {
+      return() {
+        returnCount++;
+      }
+    };
+  }
+};
+
+Promise.resolve = "certainly not callable";
+
+testAsync(assert => {
+  assert.plan(2);
+  Promise.all(iter).then(assert.unreachable, reason => {
+    assert.equals(true, reason instanceof TypeError);
+    assert.equals(1, returnCount);
+  });
+});

test/mjsunit/es6/promise-allsettled-resolve-not-callable.js

+// Copyright 2020 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
+
+load('test/mjsunit/test-async.js');
+
+// Promise.allSettled should call IteratorClose if Promise.resolve is not callable.
+
+let returnCount = 0;
+let iter = {
+  [Symbol.iterator]() {
+    return {
+      return() {
+        returnCount++;
+      }
+    };
+  }
+};
+
+Promise.resolve = "certainly not callable";
+
+testAsync(assert => {
+  assert.plan(2);
+  Promise.allSettled(iter).then(assert.unreachable, reason => {
+    assert.equals(true, reason instanceof TypeError);
+    assert.equals(1, returnCount);
+  });
+});

test/mjsunit/es6/promise-race-resolve-not-callable.js

+// Copyright 2020 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
+
+load('test/mjsunit/test-async.js');
+
+// Promise.race should call IteratorClose if Promise.resolve is not callable.
+
+let returnCount = 0;
+let iter = {
+  [Symbol.iterator]() {
+    return {
+      return() {
+        returnCount++;
+      }
+    };
+  }
+};
+
+Promise.resolve = "certainly not callable";
+
+testAsync(assert => {
+  assert.plan(2);
+  Promise.race(iter).then(assert.unreachable, reason => {
+    assert.equals(true, reason instanceof TypeError);
+    assert.equals(1, returnCount);
+  });
+});