[Promise.allSettled] Fix [[AlreadyCalled]] checking in element closures

syg · Commit Bot · commit 26df3fdc2521 · 2020-07-16T16:35:25.000Z
Bug: chromium:1105318 Change-Id: I7b1c57b7ff7beaaa53c19a270d5a8c36b11baf17 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2301082 Reviewed-by: Sathya Gunasekaran <gsathya@chromium.org> Commit-Queue: Shu-yu Guo <syg@chromium.org> Cr-Commit-Position: refs/heads/master@{#68903}
diff --git a/src/builtins/promise-all-element-closure.tq b/src/builtins/promise-all-element-closure.tq
@@ -82,7 +82,8 @@ const kPropertyArrayHashFieldMax: constexpr int31
 
 transitioning macro PromiseAllResolveElementClosure<F: type>(
     implicit context: Context)(
-    value: JSAny, function: JSFunction, wrapResultFunctor: F): JSAny {
+    value: JSAny, function: JSFunction, wrapResultFunctor: F,
+    hasResolveAndRejectClosures: constexpr bool): JSAny {
   // We use the {function}s context as the marker to remember whether this
   // resolve element closure was already called. It points to the resolve
   // element context (which is a FunctionContext) until it was called the
@@ -98,10 +99,6 @@ transitioning macro PromiseAllResolveElementClosure<F: type>(
   const nativeContext = LoadNativeContext(context);
   function.context = nativeContext;
 
-  // Update the value depending on whether Promise.all or
-  // Promise.allSettled is called.
-  const updatedValue = wrapResultFunctor.Call(nativeContext, value);
-
   // Determine the index from the {function}.
   assert(kPropertyArrayNoHashSentinel == 0);
   const identityHash =
@@ -123,6 +120,27 @@ transitioning macro PromiseAllResolveElementClosure<F: type>(
       context.elements[PromiseAllResolveElementContextSlots::
                            kPromiseAllResolveElementValuesSlot] = values;
     }
+
+  // Promise.allSettled, for each input element, has both a resolve and a reject
+  // closure that share an [[AlreadyCalled]] boolean. That is, the input element
+  // can only be settled once: after resolve is called, reject returns early,
+  // and vice versa. Using {function}'s context as the marker only tracks
+  // per-closure instead of per-element. When the second resolve/reject closure
+  // is called on the same index, values.object[index] will already exist and
+  // will not be the hole value. In that case, return early. Everything up to
+  // this point is not yet observable to user code. This is not a problem for
+  // Promise.all since Promise.all has a single resolve closure (no reject) per
+  // element.
+  if (hasResolveAndRejectClosures) {
+    if (values.objects[index] != TheHole) deferred {
+        return Undefined;
+      }
+  }
+
+  // Update the value depending on whether Promise.all or
+  // Promise.allSettled is called.
+  const updatedValue = wrapResultFunctor.Call(nativeContext, value);
+
   values.objects[index] = updatedValue;
 
   remainingElementsCount = remainingElementsCount - 1;
@@ -148,22 +166,22 @@ PromiseAllResolveElementClosure(
     js-implicit context: Context, receiver: JSAny,
     target: JSFunction)(value: JSAny): JSAny {
   return PromiseAllResolveElementClosure(
-      value, target, PromiseAllWrapResultAsFulfilledFunctor{});
+      value, target, PromiseAllWrapResultAsFulfilledFunctor{}, false);
 }
 
 transitioning javascript builtin
 PromiseAllSettledResolveElementClosure(
     js-implicit context: Context, receiver: JSAny,
     target: JSFunction)(value: JSAny): JSAny {
   return PromiseAllResolveElementClosure(
-      value, target, PromiseAllSettledWrapResultAsFulfilledFunctor{});
+      value, target, PromiseAllSettledWrapResultAsFulfilledFunctor{}, true);
 }
 
 transitioning javascript builtin
 PromiseAllSettledRejectElementClosure(
     js-implicit context: Context, receiver: JSAny,
     target: JSFunction)(value: JSAny): JSAny {
   return PromiseAllResolveElementClosure(
-      value, target, PromiseAllSettledWrapResultAsRejectedFunctor{});
+      value, target, PromiseAllSettledWrapResultAsRejectedFunctor{}, true);
 }
 }
