[promise] Make Promise.all match spec, and always respect [[AlreadyResolved]]

src/js/promise.js

 // Copyright 2012 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.
 
 (function(global, utils, extrasUtils) {
 
 "use strict";
 
 %CheckIsBootstrapping();
 
@@ skipping 48 matching lines @@
 
 var GlobalPromise = function Promise(resolver) {
   if (resolver === promiseRawSymbol) {
     return %NewObject(GlobalPromise, new.target);
   }
   if (IS_UNDEFINED(new.target)) throw MakeTypeError(kNotAPromise, this);
   if (!IS_CALLABLE(resolver))
     throw MakeTypeError(kResolverNotAFunction, resolver);
 
   var promise = PromiseInit(%NewObject(GlobalPromise, new.target));
+  var callbacks = CreateResolvingFunctions(promise);
 
   try {
     %DebugPushPromise(promise, Promise);
-    var callbacks = CreateResolvingFunctions(promise);
     resolver(callbacks.resolve, callbacks.reject);
   } catch (e) {
-    PromiseReject(promise, e);
+    %_Call(callbacks.reject, UNDEFINED, e);
   } finally {
     %DebugPopPromise();
   }
 
   return promise;
 }
 
 // Core functionality.
 
 function PromiseSet(promise, status, value, onResolve, onReject) {
@@ skipping 88 matching lines @@
     } catch (e) {
       return PromiseReject(promise, e);
     }
     if (IS_CALLABLE(then)) {
       // PromiseResolveThenableJob
       var id, name, instrumenting = DEBUG_IS_ACTIVE;
       %EnqueueMicrotask(function() {
         if (instrumenting) {
           %DebugAsyncTaskEvent({ type: "willHandle", id: id, name: name });
         }
+        var callbacks = CreateResolvingFunctions(promise);
         try {
-          var callbacks = CreateResolvingFunctions(promise);
           %_Call(then, x, callbacks.resolve, callbacks.reject);
         } catch (e) {
-          PromiseReject(promise, e);
+          %_Call(callbacks.reject, UNDEFINED, e);
         }
         if (instrumenting) {
           %DebugAsyncTaskEvent({ type: "didHandle", id: id, name: name });
         }
       });
       if (instrumenting) {
         id = ++lastMicrotaskId;
         name = "PromseResolveThenableJob";
         %DebugAsyncTaskEvent({ type: "enqueue", id: id, name: name });
       }
@@ skipping 132 matching lines @@
     throw MakeTypeError(kCalledOnNonObject, PromiseCast);
   }
   if (IsPromise(x) && x.constructor === this) return x;
 
   var promiseCapability = NewPromiseCapability(this);
   var resolveResult = %_Call(promiseCapability.resolve, UNDEFINED, x);
   return promiseCapability.promise;
 }
 
 function PromiseAll(iterable) {
+  if (!IS_RECEIVER(this)) {
+    throw MakeTypeError(kCalledOnNonObject, "Promise.all");
+  }
+
   var deferred = NewPromiseCapability(this);
-  var resolutions = [];
+  var resolutions = new InternalArray();
   try {
-    var count = 0;
+    var count = 1;
     var i = 0;
     for (var value of iterable) {
-      var reject = function(r) { deferred.reject(r) };
-      this.resolve(value).then(
-          // Nested scope to get closure over current i.
-          // TODO(arv): Use an inner let binding once available.
-          (function(i) {
-            return function(x) {
-              resolutions[i] = x;
-              if (--count === 0) deferred.resolve(resolutions);
-            }
-          })(i), reject);
-      SET_PRIVATE(reject, promiseCombinedDeferredSymbol, deferred);
+      resolutions[i] = UNDEFINED;

[adamk, 2016/01/07 19:12:10]

Why is this needed?

[caitp (gmail), 2016/01/07 19:20:35]

I _think_ the reason is to make sure `hasOwnProperty(someIndex)` returns true,
even if the value is undefined.

It's in the spec as "Append undefined to values", and IIRC is asserted by
test262 (can't recall which test though)

[adamk, 2016/01/07 19:29:30]

Ah, ok. Can you just use .push()?

[caitp (gmail), 2016/01/07 19:37:59]

it seems to pass everything without it, presumably because of how
%MoveArrayContents() works. I've just removed the line

+      var nextPromise = this.resolve(value);
+      ++count;
+      nextPromise.then(
+          CreateResolveElementFunction(i, resolutions, deferred),
+          deferred.reject);
+      SET_PRIVATE(deferred.reject, promiseCombinedDeferredSymbol, deferred);
       ++i;
-      ++count;
     }
 
-    if (count === 0) {
-      deferred.resolve(resolutions);
+    // 6.d
+    if (--count === 0) {
+      var valuesArray = [];
+      %MoveArrayContents(resolutions, valuesArray);
+      %_Call(deferred.resolve, UNDEFINED, valuesArray);
     }
 
   } catch (e) {
-    deferred.reject(e)
+    %_Call(deferred.reject, UNDEFINED, e);
   }
   return deferred.promise;
+
+  function CreateResolveElementFunction(index, values, promiseCapability) {

[adamk, 2016/01/07 19:12:10]

I think I agree with Dan that this would read better if it were declared above
its use (it's especially odd-looking to me to see code after a return).

[caitp (gmail), 2016/01/07 19:20:36]

ok

[caitp (gmail), 2016/01/07 19:37:59]

Done.

+    var alreadyCalled = false;
+    return function(x) {
+      if (alreadyCalled === true) return;
+      alreadyCalled = true;
+      values[index] = x;
+      if (--count === 0) {
+        var valuesArray = [];
+        %MoveArrayContents(values, valuesArray);
+        %_Call(promiseCapability.resolve, UNDEFINED, valuesArray);
+      }
+    };
+  }
 }
 
 function PromiseRace(iterable) {
   if (!IS_RECEIVER(this)) {
     throw MakeTypeError(kCalledOnNonObject, PromiseRace);
   }
 
   var deferred = NewPromiseCapability(this);
   try {
     for (var value of iterable) {
@@ skipping 78 matching lines @@
 [PromiseChain, PromiseDeferred, PromiseResolved].forEach(
     fn => %FunctionRemovePrototype(fn));
 
 utils.Export(function(to) {
   to.PromiseChain = PromiseChain;
   to.PromiseDeferred = PromiseDeferred;
   to.PromiseResolved = PromiseResolved;
 });
 
 })