[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 4db36359db1eb54de012183914ff4d7e707eef1d..121977e640e6738e695dd0b5426e9301ada21653 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 });
@@ -338,35 +338,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;

[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;
     }
 
-    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) {