From 54541df8b35f0be057507437c0f6e1413f1ccd29 Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Wed, 25 Nov 2015 15:48:09 -0500 Subject: [PATCH] Bug 1170760 part 7. Add subclassing support to Promise::Race. r=baku,efaust Note that the web platform tests don't actually have quite the behavior they're expected to per the spec yet. They will get adjusted later on as we add subclassing support to Promise.resolve and Promise.prototype.then. --- dom/bindings/Errors.msg | 1 + dom/promise/Promise.cpp | 107 +++++++++--- dom/promise/Promise.h | 5 +- dom/promise/tests/chrome.ini | 1 + dom/promise/tests/file_promise_xrays.html | 30 ++++ dom/promise/tests/mochitest.ini | 3 + dom/promise/tests/test_promise_xrays.html | 125 ++++++++++++++ dom/webidl/Promise.webidl | 9 +- testing/web-platform/meta/MANIFEST.json | 11 +- .../js/builtins/Promise-subclassing.html | 153 ++++++++++++++++++ 10 files changed, 416 insertions(+), 29 deletions(-) create mode 100644 dom/promise/tests/file_promise_xrays.html create mode 100644 dom/promise/tests/test_promise_xrays.html create mode 100644 testing/web-platform/tests/js/builtins/Promise-subclassing.html diff --git a/dom/bindings/Errors.msg b/dom/bindings/Errors.msg index 4b5a446b54c..42c1d82e205 100644 --- a/dom/bindings/Errors.msg +++ b/dom/bindings/Errors.msg @@ -87,5 +87,6 @@ MSG_DEF(MSG_ILLEGAL_PROMISE_CONSTRUCTOR, 0, JSEXN_TYPEERR, "Non-constructor valu MSG_DEF(MSG_PROMISE_CAPABILITY_HAS_SOMETHING_ALREADY, 0, JSEXN_TYPEERR, "GetCapabilitiesExecutor function already invoked with non-undefined values.") MSG_DEF(MSG_PROMISE_RESOLVE_FUNCTION_NOT_CALLABLE, 0, JSEXN_TYPEERR, "A Promise subclass passed a non-callable value as the resolve function.") MSG_DEF(MSG_PROMISE_REJECT_FUNCTION_NOT_CALLABLE, 0, JSEXN_TYPEERR, "A Promise subclass passed a non-callable value as the reject function.") +MSG_DEF(MSG_PROMISE_ARG_NOT_ITERABLE, 1, JSEXN_TYPEERR, "{0} is not iterable") MSG_DEF(MSG_SW_INSTALL_ERROR, 2, JSEXN_TYPEERR, "ServiceWorker script at {0} for scope {1} encountered an error during installation.") MSG_DEF(MSG_SW_SCRIPT_THREW, 2, JSEXN_TYPEERR, "ServiceWorker script at {0} for scope {1} threw an exception during script evaluation.") diff --git a/dom/promise/Promise.cpp b/dom/promise/Promise.cpp index 7bf0e75b80a..ce26a8f987a 100644 --- a/dom/promise/Promise.cpp +++ b/dom/promise/Promise.cpp @@ -1325,47 +1325,106 @@ Promise::All(const GlobalObject& aGlobal, return promise.forget(); } -/* static */ already_AddRefed +/* static */ void Promise::Race(const GlobalObject& aGlobal, JS::Handle aThisv, - const Sequence& aIterable, ErrorResult& aRv) + JS::Handle aIterable, JS::MutableHandle aRetval, + ErrorResult& aRv) { + // Implements http://www.ecma-international.org/ecma-262/6.0/#sec-promise.race nsCOMPtr global = do_QueryInterface(aGlobal.GetAsSupports()); if (!global) { aRv.Throw(NS_ERROR_UNEXPECTED); - return nullptr; + return; } JSContext* cx = aGlobal.Context(); - JS::Rooted obj(cx, JS::CurrentGlobalOrNull(cx)); - if (!obj) { - aRv.Throw(NS_ERROR_UNEXPECTED); - return nullptr; - } + // Steps 1-5: nothing to do. Note that the @@species bits got removed in + // https://github.com/tc39/ecma262/pull/211 + PromiseCapability capability(cx); - RefPtr promise = Create(global, aRv); + // Step 6. + NewPromiseCapability(cx, global, aThisv, true, capability, aRv); + // Step 7. if (aRv.Failed()) { - return nullptr; + return; } - RefPtr resolveCb = - new ResolvePromiseCallback(promise, obj); + MOZ_ASSERT(aThisv.isObject(), "How did NewPromiseCapability succeed?"); + JS::Rooted constructorObj(cx, &aThisv.toObject()); - RefPtr rejectCb = new RejectPromiseCallback(promise, obj); - - for (uint32_t i = 0; i < aIterable.Length(); ++i) { - JS::Rooted value(cx, aIterable.ElementAt(i)); - RefPtr nextPromise = Promise::Resolve(aGlobal, aThisv, value, aRv); - // According to spec, Resolve can throw, but our implementation never does. - // Well it does when window isn't passed on the main thread, but that is an - // implementation detail which should never be reached since we are checking - // for window above. Remove this when subclassing is supported. - MOZ_ASSERT(!aRv.Failed()); - nextPromise->AppendCallbacks(resolveCb, rejectCb); + // After this point we have a useful promise value in "capability", so just go + // ahead and put it in our retval now. Every single return path below would + // want to do that anyway. + aRetval.set(capability.PromiseValue()); + if (!MaybeWrapValue(cx, aRetval)) { + aRv.NoteJSContextException(); + return; } - return promise.forget(); + // The arguments we're going to be passing to "then" on each loop iteration. + JS::AutoValueArray<2> callbackFunctions(cx); + callbackFunctions[0].set(capability.mResolve); + callbackFunctions[1].set(capability.mReject); + + // Steps 8 and 9. + JS::ForOfIterator iter(cx); + if (!iter.init(aIterable, JS::ForOfIterator::AllowNonIterable)) { + capability.RejectWithException(cx, aRv); + return; + } + + if (!iter.valueIsIterable()) { + ThrowErrorMessage(cx, MSG_PROMISE_ARG_NOT_ITERABLE, + "Argument of Promise.race"); + capability.RejectWithException(cx, aRv); + return; + } + + // Step 10 doesn't need to be done, because ForOfIterator handles it + // for us. + + // Now we jump over to + // http://www.ecma-international.org/ecma-262/6.0/#sec-performpromiserace + // and do its steps. + JS::Rooted nextValue(cx); + while (true) { + bool done; + // Steps a, b, c, e, f, g. + if (!iter.next(&nextValue, &done)) { + capability.RejectWithException(cx, aRv); + return; + } + + // Step d. + if (done) { + // We're all set! + return; + } + + // Step h. Sadly, we can't take a shortcut here even if + // capability.mNativePromise exists, because someone could have overridden + // "resolve" on the canonical Promise constructor. + JS::Rooted nextPromise(cx); + if (!JS_CallFunctionName(cx, constructorObj, "resolve", + JS::HandleValueArray(nextValue), &nextPromise)) { + // Step i. + capability.RejectWithException(cx, aRv); + return; + } + + // Step j. And now we don't know whether nextPromise has an overridden + // "then" method, so no shortcuts here either. + JS::Rooted nextPromiseObj(cx); + JS::Rooted ignored(cx); + if (!JS_ValueToObject(cx, nextPromise, &nextPromiseObj) || + !JS_CallFunctionName(cx, nextPromiseObj, "then", callbackFunctions, + &ignored)) { + // Step k. + capability.RejectWithException(cx, aRv); + } + } } /* static */ diff --git a/dom/promise/Promise.h b/dom/promise/Promise.h index a47fe406835..62dfc22b21b 100644 --- a/dom/promise/Promise.h +++ b/dom/promise/Promise.h @@ -194,9 +194,10 @@ public: All(const GlobalObject& aGlobal, const nsTArray>& aPromiseList, ErrorResult& aRv); - static already_AddRefed + static void Race(const GlobalObject& aGlobal, JS::Handle aThisv, - const Sequence& aIterable, ErrorResult& aRv); + JS::Handle aIterable, JS::MutableHandle aRetval, + ErrorResult& aRv); static bool PromiseSpecies(JSContext* aCx, unsigned aArgc, JS::Value* aVp); diff --git a/dom/promise/tests/chrome.ini b/dom/promise/tests/chrome.ini index 52d840cdf02..a6a92289f89 100644 --- a/dom/promise/tests/chrome.ini +++ b/dom/promise/tests/chrome.ini @@ -5,3 +5,4 @@ skip-if = buildapp == 'b2g' [test_on_new_promise.html] [test_on_promise_settled.html] [test_on_promise_settled_duplicates.html] +[test_promise_xrays.html] diff --git a/dom/promise/tests/file_promise_xrays.html b/dom/promise/tests/file_promise_xrays.html new file mode 100644 index 00000000000..edc3875bbb3 --- /dev/null +++ b/dom/promise/tests/file_promise_xrays.html @@ -0,0 +1,30 @@ + + + + diff --git a/dom/promise/tests/mochitest.ini b/dom/promise/tests/mochitest.ini index db96b14812e..888bed0a24f 100644 --- a/dom/promise/tests/mochitest.ini +++ b/dom/promise/tests/mochitest.ini @@ -1,4 +1,7 @@ [DEFAULT] +# Support files for chrome tests that we want to load over HTTP need +# to go in here, not chrome.ini, apparently. +support-files = file_promise_xrays.html [test_abortable_promise.html] [test_bug883683.html] diff --git a/dom/promise/tests/test_promise_xrays.html b/dom/promise/tests/test_promise_xrays.html new file mode 100644 index 00000000000..a433f83a2f8 --- /dev/null +++ b/dom/promise/tests/test_promise_xrays.html @@ -0,0 +1,125 @@ + + + + + + Test for Bug 1170760 + + + + + +Mozilla Bug 1170760 +

+ + +
+
+
+ + diff --git a/dom/webidl/Promise.webidl b/dom/webidl/Promise.webidl index 69a768e8cbc..b22448a8d1c 100644 --- a/dom/webidl/Promise.webidl +++ b/dom/webidl/Promise.webidl @@ -42,6 +42,11 @@ interface _Promise { [NewObject] static Promise all(sequence iterable); - [NewObject] - static Promise race(sequence iterable); + // Have to use "any" (or "object", but "any" is simpler) as the type to + // support the subclassing behavior, since nothing actually requires the + // return value of PromiseSubclass.race to be a Promise object. As a result, + // we also have to do our argument conversion manually, because we want to + // convert its exceptions into rejections. + [NewObject, Throws] + static any race(optional any iterable); }; diff --git a/testing/web-platform/meta/MANIFEST.json b/testing/web-platform/meta/MANIFEST.json index 2f859419ffc..557366bb301 100644 --- a/testing/web-platform/meta/MANIFEST.json +++ b/testing/web-platform/meta/MANIFEST.json @@ -29919,7 +29919,16 @@ }, "local_changes": { "deleted": [], - "items": {}, + "items": { + "testharness": { + "js/builtins/Promise-subclassing.html": [ + { + "path": "js/builtins/Promise-subclassing.html", + "url": "/js/builtins/Promise-subclassing.html" + } + ] + } + }, "reftest_nodes": {} }, "reftest_nodes": { diff --git a/testing/web-platform/tests/js/builtins/Promise-subclassing.html b/testing/web-platform/tests/js/builtins/Promise-subclassing.html new file mode 100644 index 00000000000..7f751201210 --- /dev/null +++ b/testing/web-platform/tests/js/builtins/Promise-subclassing.html @@ -0,0 +1,153 @@ + + + + + +