Close iterator after error in Promise.{all,race}

Issue #17

js/src/builtin/Promise.cpp

@@ -1369,7 +1369,8 @@ PromiseObject::create(JSContext* cx, HandleObject executor, HandleObject proto /
 
 static MOZ_MUST_USE bool PerformPromiseAll(JSContext *cx, JS::ForOfIterator& iterator,
                                            HandleObject C, HandleObject promiseObj,
-                                           HandleObject resolve, HandleObject reject);
+                                           HandleObject resolve, HandleObject reject,
+                                           bool* done);
 
 // ES2016, 25.4.4.1.
 static bool
@@ -1410,12 +1411,14 @@ Promise_static_all(JSContext* cx, unsigned argc, Value* vp)
     // Step 6 (implicit).
 
     // Step 7.
-    bool result = PerformPromiseAll(cx, iter, C, resultPromise, resolve, reject);
+    bool done;
+    bool result = PerformPromiseAll(cx, iter, C, resultPromise, resolve, reject, &done);
 
     // Step 8.
     if (!result) {
         // Step 8.a.
-        // TODO: implement iterator closing.
+        if (!done)
+            iter.closeThrow();
 
         // Step 8.b.
         return AbruptRejectPromise(cx, args, resultPromise, reject);
@@ -1598,8 +1601,11 @@ RunResolutionFunction(JSContext *cx, HandleObject resolutionFun, HandleValue res
 // ES2016, 25.4.4.1.1.
 static MOZ_MUST_USE bool
 PerformPromiseAll(JSContext *cx, JS::ForOfIterator& iterator, HandleObject C,
-                  HandleObject promiseObj, HandleObject resolve, HandleObject reject)
+                  HandleObject promiseObj, HandleObject resolve, HandleObject reject,
+                  bool* done)
 {
+    *done = false;
+
     RootedObject unwrappedPromiseObj(cx);
     if (IsWrapper(promiseObj)) {
         unwrappedPromiseObj = CheckedUnwrap(promiseObj);
@@ -1666,14 +1672,19 @@ PerformPromiseAll(JSContext *cx, JS::ForOfIterator& iterator, HandleObject C,
     RootedValue rejectFunVal(cx, ObjectOrNullValue(reject));
 
     while (true) {
-        bool done;
+        // Steps a-c, e-g.
-        // Steps a, b, c, e, f, g.
+        if (!iterator.next(&nextValue, done)) {
-        if (!iterator.next(&nextValue, &done))
+            // Steps b, f.
+            *done = true;
+
+            // Steps c, g.
             return false;
+        }
 
         // Step d.
-        if (done) {
+        if (*done) {
             // Step d.i (implicit).
+
             // Step d.ii.
             int32_t remainingCount = dataHolder->decreaseRemainingCount();
 
@@ -1822,7 +1833,8 @@ PromiseAllResolveElementFunction(JSContext* cx, unsigned argc, Value* vp)
 
 static MOZ_MUST_USE bool PerformPromiseRace(JSContext *cx, JS::ForOfIterator& iterator,
                                             HandleObject C, HandleObject promiseObj,
-                                            HandleObject resolve, HandleObject reject);
+                                            HandleObject resolve, HandleObject reject,
+                                            bool* done);
 
 // ES2016, 25.4.4.3.
 static bool
@@ -1863,12 +1875,14 @@ Promise_static_race(JSContext* cx, unsigned argc, Value* vp)
     // Step 6 (implicit).
 
     // Step 7.
-    bool result = PerformPromiseRace(cx, iter, C, resultPromise, resolve, reject);
+    bool done;
+    bool result = PerformPromiseRace(cx, iter, C, resultPromise, resolve, reject, &done);
 
     // Step 8.
     if (!result) {
         // Step 8.a.
-        // TODO: implement iterator closing.
+        if (!done)
+            iter.closeThrow();
 
         // Step 8.b.
         return AbruptRejectPromise(cx, args, resultPromise, reject);
@@ -1882,25 +1896,30 @@ Promise_static_race(JSContext* cx, unsigned argc, Value* vp)
 // ES2016, 25.4.4.3.1.
 static MOZ_MUST_USE bool
 PerformPromiseRace(JSContext *cx, JS::ForOfIterator& iterator, HandleObject C,
-                   HandleObject promiseObj, HandleObject resolve, HandleObject reject)
+                   HandleObject promiseObj, HandleObject resolve, HandleObject reject,
+                   bool* done)
 {
+    *done = false;
     MOZ_ASSERT(C->isConstructor());
     RootedValue CVal(cx, ObjectValue(*C));
 
     RootedValue nextValue(cx);
     RootedValue resolveFunVal(cx, ObjectOrNullValue(resolve));
     RootedValue rejectFunVal(cx, ObjectOrNullValue(reject));
-    bool done;
 
     while (true) {
         // Steps a-c, e-g.
-        if (!iterator.next(&nextValue, &done))
+        if (!iterator.next(&nextValue, done)) {
+            // Steps b, f.
+            *done = true;
+
+            // Steps c, g.
             return false;
+        }
 
         // Step d.
-        if (done) {
+        if (*done) {
-            // Step d.i.
+            // Step d.i (implicit).
-            // TODO: implement iterator closing.
 
             // Step d.ii.
             return true;

js/src/js.msg

@@ -579,3 +579,6 @@ MSG_DEF(JSMSG_PROMISE_CAPABILITY_HAS_SOMETHING_ALREADY, 0, JSEXN_TYPEERR, "GetCa
 MSG_DEF(JSMSG_PROMISE_RESOLVE_FUNCTION_NOT_CALLABLE,    0, JSEXN_TYPEERR, "A Promise subclass passed a non-callable value as the resolve function.")
 MSG_DEF(JSMSG_PROMISE_REJECT_FUNCTION_NOT_CALLABLE,     0, JSEXN_TYPEERR, "A Promise subclass passed a non-callable value as the reject function.")
 MSG_DEF(JSMSG_PROMISE_ERROR_IN_WRAPPED_REJECTION_REASON,0, JSEXN_INTERNALERR, "Promise rejection value is a non-unwrappable cross-compartment wrapper.")
+
+// Iterator
+MSG_DEF(JSMSG_RETURN_NOT_CALLABLE,     0, JSEXN_TYPEERR, "property 'return' of iterator is not callable")

js/src/jsapi.h

@@ -6166,6 +6166,12 @@ class MOZ_STACK_CLASS JS_PUBLIC_API(ForOfIterator) {
      */
     bool next(JS::MutableHandleValue val, bool* done);
 
+    /**
+     * Close the iterator.
+     * For the case that completion type is throw.
+     */
+    void closeThrow();
+
     /**
      * If initialized with throwOnNonCallable = false, check whether
      * the value is iterable.

js/src/tests/ecma_6/Promise/iterator-close.js

@@ -0,0 +1,234 @@
+// |reftest| skip-if(!xulRuntime.shell) -- needs drainJobQueue
+
+var BUGNUMBER = 1180306;
+var summary = 'Promise.{all,race} should close iterator on error';
+
+print(BUGNUMBER + ": " + summary);
+
+function test(ctor, props, { nextVal=undefined,
+                             nextThrowVal=undefined,
+                             modifier=undefined,
+                             rejectReason=undefined,
+                             rejectType=undefined,
+                             closed=true }) {
+    function getIterable() {
+        let iterable = {
+            closed: false,
+            [Symbol.iterator]() {
+                let iterator = {
+                    first: true,
+                    next() {
+                        if (this.first) {
+                            this.first = false;
+                            if (nextThrowVal)
+                                throw nextThrowVal;
+                            return nextVal;
+                        }
+                        return { value: undefined, done: true };
+                    },
+                    return() {
+                        iterable.closed = true;
+                        return {};
+                    }
+                };
+                if (modifier)
+                    modifier(iterator, iterable);
+
+                return iterator;
+            }
+        };
+        return iterable;
+    }
+    for (let prop of props) {
+        let iterable = getIterable();
+        let e;
+        ctor[prop](iterable).catch(e_ => { e = e_; });
+        drainJobQueue();
+        if(rejectType)
+            assertEq(e instanceof rejectType, true);
+        else
+            assertEq(e, rejectReason);
+        assertEq(iterable.closed, closed);
+    }
+}
+
+// == Error cases with close ==
+
+// ES 2017 draft 25.4.4.1.1 step 6.i.
+// ES 2017 draft 25.4.4.3.1 step 3.h.
+class MyPromiseStaticResolveGetterThrows extends Promise {
+    static get resolve() {
+        throw "static resolve getter throws";
+    }
+};
+test(MyPromiseStaticResolveGetterThrows, ["all", "race"], {
+    nextVal: { value: Promise.resolve(1), done: false },
+    rejectReason: "static resolve getter throws",
+    closed: true,
+});
+
+class MyPromiseStaticResolveThrows extends Promise {
+    static resolve() {
+        throw "static resolve throws";
+    }
+};
+test(MyPromiseStaticResolveThrows, ["all", "race"], {
+    nextVal: { value: Promise.resolve(1), done: false },
+    rejectReason: "static resolve throws",
+    closed: true,
+});
+
+// ES 2017 draft 25.4.4.1.1 step 6.q.
+// ES 2017 draft 25.4.4.3.1 step 3.i.
+class MyPromiseThenGetterThrows extends Promise {
+    static resolve() {
+        return {
+            get then() {
+                throw "then getter throws";
+            }
+        };
+    }
+};
+test(MyPromiseThenGetterThrows, ["all", "race"], {
+    nextVal: { value: Promise.resolve(1), done: false },
+    rejectReason: "then getter throws",
+    closed: true,
+});
+
+class MyPromiseThenThrows extends Promise {
+    static resolve() {
+        return {
+            then() {
+                throw "then throws";
+            }
+        };
+    }
+};
+test(MyPromiseThenThrows, ["all", "race"], {
+    nextVal: { value: Promise.resolve(1), done: false },
+    rejectReason: "then throws",
+    closed: true,
+});
+
+// ES 2017 draft 7.4.6 step 3.
+// if GetMethod fails, the thrown value should be used.
+test(MyPromiseThenThrows, ["all", "race"], {
+    nextVal: { value: Promise.resolve(1), done: false },
+    modifier: (iterator, iterable) => {
+        Object.defineProperty(iterator, "return", {
+            get: function() {
+                iterable.closed = true;
+                throw "return getter throws";
+            }
+        });
+    },
+    rejectReason: "return getter throws",
+    closed: true,
+});
+test(MyPromiseThenThrows, ["all", "race"], {
+    nextVal: { value: Promise.resolve(1), done: false },
+    modifier: (iterator, iterable) => {
+        Object.defineProperty(iterator, "return", {
+            get: function() {
+                iterable.closed = true;
+                return "non object";
+            }
+        });
+    },
+    rejectType: TypeError,
+    closed: true,
+});
+test(MyPromiseThenThrows, ["all", "race"], {
+    nextVal: { value: Promise.resolve(1), done: false },
+    modifier: (iterator, iterable) => {
+        Object.defineProperty(iterator, "return", {
+            get: function() {
+                iterable.closed = true;
+                // Non callable.
+                return {};
+            }
+        });
+    },
+    rejectType: TypeError,
+    closed: true,
+});
+
+// ES 2017 draft 7.4.6 steps 6.
+// if return method throws, the thrown value should be ignored.
+test(MyPromiseThenThrows, ["all", "race"], {
+    nextVal: { value: Promise.resolve(1), done: false },
+    modifier: (iterator, iterable) => {
+        iterator.return = function() {
+            iterable.closed = true;
+            throw "return throws";
+        };
+    },
+    rejectReason: "then throws",
+    closed: true,
+});
+
+test(MyPromiseThenThrows, ["all", "race"], {
+    nextVal: { value: Promise.resolve(1), done: false },
+    modifier: (iterator, iterable) => {
+        iterator.return = function() {
+            iterable.closed = true;
+            return "non object";
+        };
+    },
+    rejectReason: "then throws",
+    closed: true,
+});
+
+// == Error cases without close ==
+
+// ES 2017 draft 25.4.4.1.1 step 6.a.
+test(Promise, ["all", "race"], {
+    nextThrowVal: "next throws",
+    rejectReason: "next throws",
+    closed: false,
+});
+
+test(Promise, ["all", "race"], {
+    nextVal: { value: {}, get done() { throw "done getter throws"; } },
+    rejectReason: "done getter throws",
+    closed: false,
+});
+
+// ES 2017 draft 25.4.4.1.1 step 6.e.
+test(Promise, ["all", "race"], {
+    nextVal: { get value() { throw "value getter throws"; }, done: false },
+    rejectReason: "value getter throws",
+    closed: false,
+});
+
+// ES 2017 draft 25.4.4.1.1 step 6.d.iii.2.
+let first = true;
+class MyPromiseResolveThrows extends Promise {
+    constructor(executer) {
+        if (first) {
+            first = false;
+            super((resolve, reject) => {
+                executer(() => {
+                    throw "resolve throws";
+                }, reject);
+            });
+            return;
+        }
+        super(executer);
+    }
+};
+test(MyPromiseResolveThrows, ["all"], {
+    nextVal: { value: undefined, done: true },
+    rejectReason: "resolve throws",
+    closed: false,
+});
+
+// == Successful cases ==
+
+test(Promise, ["all", "race"], {
+    nextVal: { value: Promise.resolve(1), done: false },
+    closed: false,
+});
+
+if (typeof reportCompare === 'function')
+  reportCompare(true, true);

js/src/vm/ForOfIterator.cpp

@@ -151,6 +151,57 @@ ForOfIterator::next(MutableHandleValue vp, bool* done)
     return GetProperty(cx_, resultObj, resultObj, cx_->names().value, vp);
 }
 
+// ES 2017 draft 0f10dba4ad18de92d47d421f378233a2eae8f077 7.4.6.
+// When completion.[[Type]] is throw.
+void
+ForOfIterator::closeThrow()
+{
+    MOZ_ASSERT(iterator);
+
+    RootedValue completionException(cx_);
+    if (cx_->isExceptionPending()) {
+        if (!GetAndClearException(cx_, &completionException))
+            completionException.setUndefined();
+    }
+
+    // Steps 1-2 (implicit)
+
+    // Step 3 (partial).
+    RootedValue returnVal(cx_);
+    if (!GetProperty(cx_, iterator, iterator, cx_->names().return_, &returnVal))
+        return;
+
+    // Step 4.
+    if (returnVal.isUndefined()) {
+        cx_->setPendingException(completionException);
+        return;
+    }
+
+    // Step 3 (remaining part)
+    if (!returnVal.isObject()) {
+        JS_ReportErrorNumberASCII(cx_, GetErrorMessage, nullptr, JSMSG_RETURN_NOT_CALLABLE);
+        return;
+    }
+    RootedObject returnObj(cx_, &returnVal.toObject());
+    if (!returnObj->isCallable()) {
+        JS_ReportErrorNumberASCII(cx_, GetErrorMessage, nullptr, JSMSG_RETURN_NOT_CALLABLE);
+        return;
+    }
+
+    // Step 5.
+    RootedValue innerResultValue(cx_);
+    if (!js::Call(cx_, returnVal, iterator, &innerResultValue)) {
+        if (cx_->isExceptionPending())
+            cx_->clearPendingException();
+    }
+
+    // Step 6.
+    cx_->setPendingException(completionException);
+
+    // Steps 7-9 (skipped).
+    return;
+}
+
 bool
 ForOfIterator::materializeArrayIterator()
 {