From 114794557687aebca601c38ba0f0a52a43b44d4a Mon Sep 17 00:00:00 2001 From: janekptacijarabaci Date: Thu, 15 Mar 2018 21:12:39 +0100 Subject: Close iterator after error in Promise.{all,race} Issue #17 --- js/src/builtin/Promise.cpp | 53 ++++-- js/src/js.msg | 3 + js/src/jsapi.h | 6 + js/src/tests/ecma_6/Promise/iterator-close.js | 234 ++++++++++++++++++++++++++ js/src/vm/ForOfIterator.cpp | 51 ++++++ 5 files changed, 330 insertions(+), 17 deletions(-) create mode 100644 js/src/tests/ecma_6/Promise/iterator-close.js (limited to 'js/src') diff --git a/js/src/builtin/Promise.cpp b/js/src/builtin/Promise.cpp index 59c97e529..c781a336d 100644 --- a/js/src/builtin/Promise.cpp +++ b/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, 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(); @@ -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) { - // Step d.i. - // TODO: implement iterator closing. + if (*done) { + // Step d.i (implicit). // Step d.ii. return true; diff --git a/js/src/js.msg b/js/src/js.msg index cb5fc383b..2a818056f 100644 --- a/js/src/js.msg +++ b/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") diff --git a/js/src/jsapi.h b/js/src/jsapi.h index 2d6ff462c..a93852fa5 100644 --- a/js/src/jsapi.h +++ b/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. diff --git a/js/src/tests/ecma_6/Promise/iterator-close.js b/js/src/tests/ecma_6/Promise/iterator-close.js new file mode 100644 index 000000000..f17260d05 --- /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); diff --git a/js/src/vm/ForOfIterator.cpp b/js/src/vm/ForOfIterator.cpp index 7bd521a6a..a67b36774 100644 --- a/js/src/vm/ForOfIterator.cpp +++ b/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() { -- cgit v1.2.3