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

src/js/promise.js

Index: src/js/promise.js
diff --git a/src/js/promise.js b/src/js/promise.js
index a165817b7160ec3798f34e01917f5f4a8467a27b..c85f93034c8a713ed021e84cc358e612598bbd32 100644
--- a/src/js/promise.js
+++ b/src/js/promise.js
@@ -66,13 +66,13 @@ var GlobalPromise = function Promise(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();
   }
@@ -181,11 +181,11 @@ function PromiseResolve(promise, x) {
         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 });
@@ -327,35 +327,51 @@ function PromiseCast(x) {
 }
 
 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);
-      ++i;
+      resolutions[i] = UNDEFINED;
+      var nextPromise = this.resolve(value);
       ++count;
+      nextPromise.then(
+          CreateResolveElementFunction(i, resolutions, deferred),
+          deferred.reject);
+      SET_PRIVATE(deferred.reject, promiseCombinedDeferredSymbol, deferred);
+      ++i;
     }
 
-    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) {

[Dan Ehrenberg, 2016/01/04 18:18:39]

Does this need to be a nested function? Our js code tends to define top-level
functions, and this doesn't seem to close over anything.

[caitp (gmail), 2016/01/04 18:20:08]

It doesn't, but the function isn't really needed by anything else. No problem
moving it out, though

[caitp (gmail), 2016/01/04 19:26:52]

Actually, I'm wrong --- `count` is captured, so it would need to be refactored
more in order to keep working

+    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) {