diff --git a/dom/bindings/Bindings.conf b/dom/bindings/Bindings.conf index 9d583a738c5..b5c2953d444 100644 --- a/dom/bindings/Bindings.conf +++ b/dom/bindings/Bindings.conf @@ -1949,7 +1949,13 @@ DOMInterfaces = { # Keep this in sync with TestExampleInterface 'headerFile': 'TestBindingHeader.h', 'register': False - } + }, + +'TestInterfaceWithPromiseConstructorArg' : { + 'headerFile': 'TestBindingHeader.h', + 'register': False, + }, + } # These are temporary, until they've been converted to use new DOM bindings diff --git a/dom/bindings/Codegen.py b/dom/bindings/Codegen.py index c48b19d1109..e92167f7fa7 100644 --- a/dom/bindings/Codegen.py +++ b/dom/bindings/Codegen.py @@ -5093,14 +5093,94 @@ def getJSToNativeConversionInfo(type, descriptorProvider, failureCode=None, templateBody += 'static_assert(IsRefcounted<%s>::value, "We can only store refcounted classes.");' % typeName if isPromise: + # Per spec, what we're supposed to do is take the original + # Promise.resolve and call it with the original Promise as this + # value to make a Promise out of whatever value we actually have + # here. The question is which global we should use. There are + # several cases to consider: + # + # 1) Normal call to API with a Promise argument. This is a case the + # spec covers, and we should be using the current Realm's + # Promise. That means the current compartment. + # 2) Call to API with a Promise argument over Xrays. In practice, + # this sort of thing seems to be used for giving an API + # implementation a way to wait for conclusion of an asyc + # operation, _not_ to expose the Promise to content code. So we + # probably want to allow callers to use such an API in a + # "natural" way, by passing chrome-side promises; indeed, that + # may be all that the caller has to represent their async + # operation. That means we really need to do the + # Promise.resolve() in the caller (chrome) compartment: if we do + # it in the content compartment, we will try to call .then() on + # the chrome promise while in the content compartment, which will + # throw and we'll just get a rejected Promise. Note that this is + # also the reason why a caller who has a chrome Promise + # representing an async operation can't itself convert it to a + # content-side Promise (at least not without some serious + # gyrations). + # 3) Promise return value from a callback or callback interface. + # This is in theory a case the spec covers but in practice it + # really doesn't define behavior here because it doesn't define + # what Realm we're in after the callback returns, which is when + # the argument conversion happens. We will use the current + # compartment, which is the compartment of the callable (which + # may itself be a cross-compartment wrapper itself), which makes + # as much sense as anything else. In practice, such an API would + # once again be providing a Promise to signal completion of an + # operation, which would then not be exposed to anyone other than + # our own implementation code. + # 4) Return value from a JS-implemented interface. In this case we + # have a problem. Our current compartment is the compartment of + # the JS implementation. But if the JS implementation returned + # a page-side Promise (which is a totally sane thing to do, and + # in fact the right thing to do given that this return value is + # going right to content script) then we don't want to + # Promise.resolve with our current compartment Promise, because + # that will wrap it up in a chrome-side Promise, which is + # decidedly _not_ what's desired here. So in that case we + # should really unwrap the return value and use the global of + # the result. CheckedUnwrap should be good enough for that; if + # it fails, then we're failing unwrap while in a + # system-privileged compartment, so presumably we have a dead + # object wrapper. Just error out. Do NOT fall back to using + # the current compartment instead: that will return a + # system-privileged rejected (because getting .then inside + # resolve() failed) Promise to the caller, which they won't be + # able to touch. That's not helpful. If we error out, on the + # other hand, they will get a content-side rejected promise. + # Same thing if the value returned is not even an object. + if isCallbackReturnValue == "JSImpl": + # Case 4 above. Note that globalObj defaults to the current + # compartment global. Note that we don't use $*{exceptionCode} + # here because that will try to aRv.Throw(NS_ERROR_UNEXPECTED) + # which we don't really want here. + assert exceptionCode == "aRv.Throw(NS_ERROR_UNEXPECTED);\nreturn nullptr;\n" + getPromiseGlobal = fill( + """ + if (!$${val}.isObject()) { + aRv.ThrowTypeError(NS_LITERAL_STRING("${sourceDescription}")); + return nullptr; + } + JSObject* unwrappedVal = js::CheckedUnwrap(&$${val}.toObject()); + if (!unwrappedVal) { + // A slight lie, but not much of one, for a dead object wrapper. + aRv.ThrowTypeError(NS_LITERAL_STRING("${sourceDescription}")); + return nullptr; + } + globalObj = js::GetGlobalForObjectCrossCompartment(unwrappedVal); + """, + sourceDescription=sourceDescription) + else: + getPromiseGlobal = "" + templateBody = fill( """ - { // Scope for our GlobalObject and ErrorResult + { // Scope for our GlobalObject, ErrorResult, JSAutoCompartment, + // etc. - // Might as well use CurrentGlobalOrNull here; that will at - // least give us the same behavior as if the caller just called - // Promise.resolve() themselves. JS::Rooted globalObj(cx, JS::CurrentGlobalOrNull(cx)); + $*{getPromiseGlobal} + JSAutoCompartment ac(cx, globalObj); GlobalObject promiseGlobal(cx, globalObj); if (promiseGlobal.Failed()) { $*{exceptionCode} @@ -5112,12 +5192,25 @@ def getJSToNativeConversionInfo(type, descriptorProvider, failureCode=None, $*{exceptionCode} } JS::Rooted resolveThisv(cx, JS::ObjectValue(*promiseCtor)); - $${declName} = Promise::Resolve(promiseGlobal, resolveThisv, $${val}, promiseRv); + JS::Rooted resolveResult(cx); + JS::Rooted valueToResolve(cx, $${val}); + if (!JS_WrapValue(cx, &valueToResolve)) { + $*{exceptionCode} + } + Promise::Resolve(promiseGlobal, resolveThisv, valueToResolve, + &resolveResult, promiseRv); if (promiseRv.MaybeSetPendingException(cx)) { $*{exceptionCode} } + nsresult unwrapRv = UNWRAP_OBJECT(Promise, &resolveResult.toObject(), $${declName}); + if (NS_FAILED(unwrapRv)) { // Quite odd + promiseRv.Throw(unwrapRv); + promiseRv.MaybeSetPendingException(cx); + $*{exceptionCode} + } } """, + getPromiseGlobal=getPromiseGlobal, exceptionCode=exceptionCode) elif not descriptor.skipGen and not descriptor.interface.isConsequential() and not descriptor.interface.isExternal(): if failureCode is not None: diff --git a/dom/bindings/test/TestBindingHeader.h b/dom/bindings/test/TestBindingHeader.h index eead656bb35..88bba7069bd 100644 --- a/dom/bindings/test/TestBindingHeader.h +++ b/dom/bindings/test/TestBindingHeader.h @@ -1371,6 +1371,18 @@ public: virtual nsISupports* GetParentObject(); }; +class TestInterfaceWithPromiseConstructorArg : public nsISupports, public nsWrapperCache +{ +public: + NS_DECL_ISUPPORTS + + static + already_AddRefed + Constructor(const GlobalObject&, Promise&, ErrorResult&); + + virtual nsISupports* GetParentObject(); +}; + } // namespace dom } // namespace mozilla diff --git a/dom/bindings/test/TestCodeGen.webidl b/dom/bindings/test/TestCodeGen.webidl index f96e359fe71..c779f4f14c1 100644 --- a/dom/bindings/test/TestCodeGen.webidl +++ b/dom/bindings/test/TestCodeGen.webidl @@ -55,6 +55,7 @@ callback interface TestCallbackInterface { void passVariadicNullableTypedArray(Float32Array?... arg); Uint8Array receiveUint8Array(); attribute Uint8Array uint8ArrayAttr; + Promise receivePromise(); }; callback interface TestSingleOperationCallbackInterface { @@ -1034,6 +1035,9 @@ dictionary Dict : ParentDict { CustomEventInit customEventInit; TestDictionaryTypedef dictionaryTypedef; + + Promise promise; + sequence> promiseSequence; }; dictionary ParentDict : GrandparentDict { @@ -1161,3 +1165,7 @@ interface TestDeprecatedInterface { static void alsoDeprecated(); }; + +[Constructor(Promise promise)] +interface TestInterfaceWithPromiseConstructorArg { +}; diff --git a/dom/promise/Promise.cpp b/dom/promise/Promise.cpp index 8554f2d4112..c54cfd7520c 100644 --- a/dom/promise/Promise.cpp +++ b/dom/promise/Promise.cpp @@ -1006,34 +1006,77 @@ Promise::NewPromiseCapability(JSContext* aCx, nsIGlobalObject* aGlobal, // Step 11 doesn't need anything, since the PromiseCapability was passed in. } -/* static */ already_AddRefed +/* static */ void Promise::Resolve(const GlobalObject& aGlobal, JS::Handle aThisv, - JS::Handle aValue, ErrorResult& aRv) + JS::Handle aValue, + JS::MutableHandle aRetval, ErrorResult& aRv) { - // If a Promise was passed, just return it. - if (aValue.isObject()) { - JS::Rooted valueObj(aGlobal.Context(), &aValue.toObject()); - Promise* nextPromise; - nsresult rv = UNWRAP_OBJECT(Promise, valueObj, nextPromise); + // Implementation of + // http://www.ecma-international.org/ecma-262/6.0/#sec-promise.resolve - if (NS_SUCCEEDED(rv)) { - RefPtr addRefed = nextPromise; - return addRefed.forget(); - } - } + JSContext* cx = aGlobal.Context(); nsCOMPtr global = do_QueryInterface(aGlobal.GetAsSupports()); if (!global) { aRv.Throw(NS_ERROR_UNEXPECTED); - return nullptr; + return; } - RefPtr p = Resolve(global, aGlobal.Context(), aValue, aRv); - if (p) { - p->mFullfillmentStack = p->mAllocationStack; + // Steps 1 and 2. + if (!aThisv.isObject()) { + aRv.ThrowTypeError(); + return; } - return p.forget(); + + // Step 3. If a Promise was passed and matches our constructor, just return it. + if (aValue.isObject()) { + JS::Rooted valueObj(cx, &aValue.toObject()); + Promise* nextPromise; + nsresult rv = UNWRAP_OBJECT(Promise, valueObj, nextPromise); + + if (NS_SUCCEEDED(rv)) { + JS::Rooted constructor(cx); + if (!JS_GetProperty(cx, valueObj, "constructor", &constructor)) { + aRv.NoteJSContextException(); + return; + } + + // Cheat instead of calling JS_SameValue, since we know one's an object. + if (aThisv == constructor) { + aRetval.setObject(*valueObj); + return; + } + } + } + + // Step 4. + PromiseCapability capability(cx); + NewPromiseCapability(cx, global, aThisv, false, capability, aRv); + // Step 5. + if (aRv.Failed()) { + return; + } + + // Step 6. + Promise* p = capability.mNativePromise; + if (p) { + p->MaybeResolveInternal(cx, aValue); + p->mFullfillmentStack = p->mAllocationStack; + } else { + JS::Rooted value(cx, aValue); + JS::Rooted ignored(cx); + if (!JS::Call(cx, JS::UndefinedHandleValue /* thisVal */, + capability.mResolve, JS::HandleValueArray(value), + &ignored)) { + // Step 7. + aRv.NoteJSContextException(); + return; + } + } + + // Step 8. + aRetval.set(capability.PromiseValue()); } /* static */ already_AddRefed diff --git a/dom/promise/Promise.h b/dom/promise/Promise.h index 81d9166fb62..40a95286865 100644 --- a/dom/promise/Promise.h +++ b/dom/promise/Promise.h @@ -163,9 +163,10 @@ public: Constructor(const GlobalObject& aGlobal, PromiseInit& aInit, ErrorResult& aRv, JS::Handle aDesiredProto); - static already_AddRefed + static void Resolve(const GlobalObject& aGlobal, JS::Handle aThisv, - JS::Handle aValue, ErrorResult& aRv); + JS::Handle aValue, + JS::MutableHandle aRetval, ErrorResult& aRv); static already_AddRefed Resolve(nsIGlobalObject* aGlobal, JSContext* aCx, diff --git a/dom/promise/tests/test_bug883683.html b/dom/promise/tests/test_bug883683.html index 229844c3f79..866fdd6a5df 100644 --- a/dom/promise/tests/test_bug883683.html +++ b/dom/promise/tests/test_bug883683.html @@ -20,7 +20,7 @@ function runTest() { [{}, {}, {}, {}, {}].reduce(Promise.reject); ok(true, "No leaks with reject?"); - [{}, {}, {}, {}, {}].reduce(Promise.resolve); + [{}, {}, {}, {}, {}].reduce(Promise.resolve.bind(Promise)); ok(true, "No leaks with resolve?"); [{}, {}, {}, {}, {}].reduce(function(a, b, c, d) { return new Promise(function(r1, r2) { throw a; }); }); diff --git a/dom/promise/tests/test_promise_xrays.html b/dom/promise/tests/test_promise_xrays.html index e6c2484bd51..601e8f4e999 100644 --- a/dom/promise/tests/test_promise_xrays.html +++ b/dom/promise/tests/test_promise_xrays.html @@ -173,6 +173,38 @@ function testAll5() { ).then(nextTest); } +function testResolve1() { + var p = win.Promise.resolve(5); + ok(p instanceof win.Promise, "Promise.resolve should return a promise"); + p.then( + function(arg) { + is(arg, 5, "Should get correct Promise.resolve value"); + }, + function(e) { + ok(false, "testAll5 threw exception: " + e); + } + ).then(nextTest); +} + +function testResolve2() { + var p = win.Promise.resolve(5); + var q = win.Promise.resolve(p); + is(q, p, "Promise.resolve should just pass through Promise values"); + nextTest(); +} + +function testResolve3() { + var p = win.Promise.resolve(Promise.resolve(5)); + p.then( + function(arg) { + is(arg, 5, "Promise.resolve with chrome Promise should work"); + }, + function(e) { + ok(false, "Promise.resolve with chrome Promise should not fail"); + } + ).then(nextTest); +} + var tests = [ testLoadComplete, testHaveXray, @@ -185,6 +217,9 @@ var tests = [ testAll3, testAll4, testAll5, + testResolve1, + testResolve2, + testResolve3, ]; function nextTest() { diff --git a/dom/webidl/Promise.webidl b/dom/webidl/Promise.webidl index 33f3512854c..70221b40ab2 100644 --- a/dom/webidl/Promise.webidl +++ b/dom/webidl/Promise.webidl @@ -20,13 +20,11 @@ callback AnyCallback = any (any value); Exposed=(Window,Worker,System)] // Need to escape "Promise" so it's treated as an identifier. interface _Promise { - // Disable the static methods when the interface object is supposed to be - // disabled, just in case some code decides to walk over to .constructor from - // the proto of a promise object or someone screws up and manages to create a - // Promise object in this scope without having resolved the interface object - // first. - [NewObject] - static Promise resolve(optional any value); + // 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.resolve/reject to be a Promise object. + [NewObject, Throws] + static any resolve(optional any value); [NewObject] static Promise reject(optional any value); diff --git a/testing/web-platform/tests/js/builtins/Promise-subclassing.html b/testing/web-platform/tests/js/builtins/Promise-subclassing.html index 2e0f0448dce..5aaa0d7d81b 100644 --- a/testing/web-platform/tests/js/builtins/Promise-subclassing.html +++ b/testing/web-platform/tests/js/builtins/Promise-subclassing.html @@ -127,8 +127,10 @@ promise_test(function testPromiseRace() { var p = LoggingPromise.race(new LoggingIterable([1, 2])); var log = takeLog(); assert_array_equals(log, ["Race 1", "Constructor 1", - "Next 1", "Resolve 1", - "Next 2", "Resolve 2", + "Next 1", "Resolve 1", "Constructor 2", + "Then 1", + "Next 2", "Resolve 2", "Constructor 3", + "Then 2", "Next 3"]); assert_true(p instanceof LoggingPromise); return p.then(function(arg) { @@ -141,8 +143,10 @@ promise_test(function testPromiseRaceNoSpecies() { var p = SpeciesLessPromise.race(new LoggingIterable([1, 2])); var log = takeLog(); assert_array_equals(log, ["Race 1", "Constructor 1", - "Next 1", "Resolve 1", - "Next 2", "Resolve 2", + "Next 1", "Resolve 1", "Constructor 2", + "Then 1", + "Next 2", "Resolve 2", "Constructor 3", + "Then 2", "Next 3"]); assert_true(p instanceof SpeciesLessPromise); return p.then(function(arg) { @@ -155,8 +159,10 @@ promise_test(function testPromiseAll() { var p = LoggingPromise.all(new LoggingIterable([1, 2])); var log = takeLog(); assert_array_equals(log, ["All 1", "Constructor 1", - "Next 1", "Resolve 1", - "Next 2", "Resolve 2", + "Next 1", "Resolve 1", "Constructor 2", + "Then 1", + "Next 2", "Resolve 2", "Constructor 3", + "Then 2", "Next 3"]); assert_true(p instanceof LoggingPromise); return p.then(function(arg) { @@ -164,4 +170,40 @@ promise_test(function testPromiseAll() { }); }, "Promise.all behavior"); +promise_test(function testPromiseResolve() { + clearLog(); + var p = LoggingPromise.resolve(5); + var log = takeLog(); + assert_array_equals(log, ["Resolve 1", "Constructor 1"]); + var q = LoggingPromise.resolve(p); + assert_equals(p, q, + "Promise.resolve with same constructor should preserve identity"); + log = takeLog(); + assert_array_equals(log, ["Resolve 1"]); + + var r = Promise.resolve(p); + assert_not_equals(p, r, + "Promise.resolve with different constructor should " + + "create a new Promise instance (1)") + var s = Promise.resolve(6); + var u = LoggingPromise.resolve(s); + log = takeLog(); + assert_array_equals(log, ["Resolve 1", "Constructor 1"]); + assert_not_equals(s, u, + "Promise.resolve with different constructor should " + + "create a new Promise instance (2)") + + Object.defineProperty(s, "constructor", { value: LoggingPromise }); + var v = LoggingPromise.resolve(s); + log = takeLog(); + assert_array_equals(log, ["Resolve 1"]); + assert_equals(v, s, "Faking the .constructor should work"); + assert_false(v instanceof LoggingPromise); + + var results = Promise.all([p, q, r, s, u, v]); + return results.then(function(arg) { + assert_array_equals(arg, [5, 5, 5, 6, 6, 6]); + }); +}, "Promise.resolve behavior"); + diff --git a/toolkit/components/asyncshutdown/tests/xpcshell/test_AsyncShutdown.js b/toolkit/components/asyncshutdown/tests/xpcshell/test_AsyncShutdown.js index 4ff96d6001c..44ac76326e5 100644 --- a/toolkit/components/asyncshutdown/tests/xpcshell/test_AsyncShutdown.js +++ b/toolkit/components/asyncshutdown/tests/xpcshell/test_AsyncShutdown.js @@ -148,7 +148,7 @@ add_task(function* test_phase_removeBlocker() { do_print("Attempt to remove a blocker after wait"); lock = makeLock(kind); - blocker = Promise.resolve; + blocker = Promise.resolve.bind(Promise); yield lock.wait(); do_remove_blocker(lock, blocker, false);