Bug 1180306 - Part 3: Close iterator after error in Promise.{all,race}. r=till
authorTooru Fujisawa <arai_a@mac.com>
Sat, 19 Nov 2016 01:13:08 +0900
changeset 323447 cad8259d02777425759f822e4f1c9221c0fc7ceb
parent 323446 f861cf587ad19b39da9d1676e32e31df366dc606
child 323448 5ff6465aee720453bff02829ff5e73360e5d0c70
push id30978
push usercbook@mozilla.com
push dateMon, 21 Nov 2016 14:44:46 +0000
treeherdermozilla-central@0534254e9a40 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerstill
bugs1180306
milestone53.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1180306 - Part 3: Close iterator after error in Promise.{all,race}. r=till
js/src/builtin/Promise.cpp
js/src/js.msg
js/src/jsapi.h
js/src/tests/ecma_6/Promise/iterator-close.js
js/src/vm/ForOfIterator.cpp
--- a/js/src/builtin/Promise.cpp
+++ b/js/src/builtin/Promise.cpp
@@ -1348,17 +1348,18 @@ PromiseObject::create(JSContext* cx, Han
     JS::dbg::onNewPromise(cx, promise);
 
     // Step 11.
     return promise;
 }
 
 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
 Promise_static_all(JSContext* cx, unsigned argc, Value* vp)
 {
     CallArgs args = CallArgsFromVp(argc, vp);
     RootedValue iterable(cx, args.get(0));
 
@@ -1389,22 +1390,24 @@ Promise_static_all(JSContext* cx, unsign
         JS_ReportErrorNumberASCII(cx, GetErrorMessage, nullptr, JSMSG_NOT_ITERABLE,
                                   "Argument of Promise.all");
         return AbruptRejectPromise(cx, args, resultPromise, reject);
     }
 
     // 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);
     }
 
     // Step 9.
     args.rval().setObject(*resultPromise);
     return true;
@@ -1577,18 +1580,21 @@ RunResolutionFunction(JSContext *cx, Han
         return true;
     return RejectMaybeWrappedPromise(cx, promiseObj, result);
 
 }
 
 // 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);
         MOZ_ASSERT(unwrappedPromiseObj);
     }
 
     // Step 1.
     MOZ_ASSERT(C->isConstructor());
@@ -1645,24 +1651,29 @@ PerformPromiseAll(JSContext *cx, JS::For
     uint32_t index = 0;
 
     // Step 6.
     RootedValue nextValue(cx);
     RootedId indexId(cx);
     RootedValue rejectFunVal(cx, ObjectOrNullValue(reject));
 
     while (true) {
-        bool done;
-        // Steps a, b, c, e, f, g.
-        if (!iterator.next(&nextValue, &done))
+        // Steps a-c, e-g.
+        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();
 
             // Steps d.iii-iv.
             if (remainingCount == 0) {
                 if (resolve) {
                     return RunResolutionFunction(cx, resolve, valuesArrayVal, ResolveMode,
                                                  promiseObj);
@@ -1801,17 +1812,18 @@ PromiseAllResolveElementFunction(JSConte
 
     // Step 11.
     args.rval().setUndefined();
     return true;
 }
 
 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
 Promise_static_race(JSContext* cx, unsigned argc, Value* vp)
 {
     CallArgs args = CallArgsFromVp(argc, vp);
     RootedValue iterable(cx, args.get(0));
 
@@ -1842,54 +1854,61 @@ Promise_static_race(JSContext* cx, unsig
         JS_ReportErrorNumberASCII(cx, GetErrorMessage, nullptr, JSMSG_NOT_ITERABLE,
                                   "Argument of Promise.race");
         return AbruptRejectPromise(cx, args, resultPromise, reject);
     }
 
     // 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);
     }
 
     // Step 9.
     args.rval().setObject(*resultPromise);
     return true;
 }
 
 // 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) {
-            // Step d.i.
-            // TODO: implement iterator closing.
+        if (*done) {
+            // Step d.i (implicit).
 
             // Step d.ii.
             return true;
         }
 
         // Step h.
         // Sadly, because someone could have overridden
         // "resolve" on the canonical Promise constructor.
--- a/js/src/js.msg
+++ b/js/src/js.msg
@@ -578,8 +578,11 @@ MSG_DEF(JSMSG_MODULE_INSTANTIATE_FAILED,
 MSG_DEF(JSMSG_BAD_MODULE_STATE,          0, JSEXN_INTERNALERR, "module record in unexpected state")
 
 // Promise
 MSG_DEF(JSMSG_CANNOT_RESOLVE_PROMISE_WITH_ITSELF,       0, JSEXN_TYPEERR, "A promise cannot be resolved with itself.")
 MSG_DEF(JSMSG_PROMISE_CAPABILITY_HAS_SOMETHING_ALREADY, 0, JSEXN_TYPEERR, "GetCapabilitiesExecutor function already invoked with non-undefined values.")
 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")
--- a/js/src/jsapi.h
+++ b/js/src/jsapi.h
@@ -6131,16 +6131,22 @@ class MOZ_STACK_CLASS JS_PUBLIC_API(ForO
 
     /**
      * Get the next value from the iterator.  If false *done is true
      * after this call, do not examine val.
      */
     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.
      */
     bool valueIsIterable() const {
         return iterator;
     }
 
   private:
new file mode 100644
--- /dev/null
+++ b/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);
--- a/js/src/vm/ForOfIterator.cpp
+++ b/js/src/vm/ForOfIterator.cpp
@@ -146,16 +146,67 @@ ForOfIterator::next(MutableHandleValue v
     if (*done) {
         vp.setUndefined();
         return true;
     }
 
     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()
 {
     MOZ_ASSERT(index != NOT_ARRAY);
 
     HandlePropertyName name = cx_->names().ArrayValuesAt;
     RootedValue val(cx_);
     if (!GlobalObject::getSelfHostedFunction(cx_, cx_->global(), name, name, 1, &val))