Right now, JSImplemented promise-returning APIs like RTCPeerConnection - once Bug 1091898 lands - will basically carry rejection objects back out to content, whether they are: A) content-exceptions carefully thrown by PeerConnection.js, or B) unexpected chrome-exceptions thrown by bugs in our code. The latter is bad and leads to "No permission to access 'message'" errors when content JS tries to access the rejection object. STR: - Build with patches in Bug 1091898 to get promise-returning RTCPeerConnection - Add "this.nonexistentfunc();" to _createOffer in PeerConnection.js http://mxr.mozilla.org/mozilla-central/source/dom/media/PeerConnection.js?rev=ebca7b404b37#617 - Run and go to URL Actual result: - Error: Permission denied to access property 'name' line 25 - Nothing in Browser console. Expected result: - NS_ERROR_UNEXPECTED: line 30 - In Browser console: TypeError: this.dummyfunction is not a function PeerConnection.js:535:4 Desired result (maybe?): - InternalError: Unexpected error line 30 - In Browser console: TypeError: this.dummyfunction is not a function PeerConnection.js:535:4

So just to be clear, this bug is about case (B), not case (A), right? That is, the case when an unexpected exception is thrown? One possible option is that we could change CallbackObject such that the eRethrowExceptions behavior doesn't rethrow when the caller does not subsume the exception. This would require having some concept of the caller compartment, but we could do that. In this case that would be the compartment of the promise, not of the actual callback or the current compartment at the callsite, which is a bit odd. Another option would be to do this entirely in the promise code; the problem there is that if we don't subsume the exception we have to report it, but we want to report it while in the right compartment. In particular, we'd need to enter the compartment of the unwrapped callback, then SetPendingException, then report it. Totally doable, but a bit less generic. Bobby, thoughts?

(In reply to Boris Zbarsky [:bz] from comment #1) > So just to be clear, this bug is about case (B), not case (A), right? That > is, the case when an unexpected exception is thrown? Yes.

It seems to me that, for JS-implemented WebIDL, any object-valued exception that is not subsumed by the calling page should be reported at the boundary. In theory, we could run into some weird Xray case where chrome code would do (contentWindow.someJSImplementedAPI.createOffer(...).then(...)), in which case the reject callback would not be able to see an exception it subsumes. But in practice, I think that any non-page-principaled exception thrown by an API is accidental, and we should consistently stop them at the boundary. So I think I'm for option (1) here.

Patches here and in Bug 1107592 are working great for me, except for one problem. From try in Bug 1111666 comment 7: > 15:19:40 INFO - 1244 INFO TEST-UNEXPECTED-FAIL > | /tests/dom/bindings/test/test_exceptions_from_jsimplemented.html > | Rejection should have been on line 31 - got 787, expected 31 For the promise-returning method (addIceCandidate), the line-number is that of the throw in chrome-only PeerConnection.js instead of the outer content call. For the non-promise-returning method (updateIce), the line-number is correct.

Comment on attachment 8535454 [details] [diff] [review] part 1. Change the invariants around aCompartment in CallSetup to allow passing it even when the exception handling mode is eRethrowExceptions Review of attachment 8535454 [details] [diff] [review]: ----------------------------------------------------------------- Sorry this took forever - it took me a long time to wrap my head around all of this. Now that I understand it better, I realize that this only applies to C++ callers that explicitly pass eRethrowExceptions, and is thus much less of a generic problem than I thought. And given that using the global of the promise is kind of unfortunate from an Xray perspective, I think that maybe option 2 would have been better. But certainly not enough to go back and redo everything. r=me ::: dom/bindings/CallbackObject.cpp @@ +192,5 @@ > + // Caller didn't ask us to filter for only exceptions we subsume. > + return true; > + } > + > + // On workers, we don't have nsIPrincipals to work with. But we olso only s/olso/also @@ +207,5 @@ > + // current compartment/global of mCx. > + nsIPrincipal* callerPrincipal = > + nsJSPrincipals::get(JS_GetCompartmentPrincipals(mCompartment)); > + nsIPrincipal* calleePrincipal = > + nsContentUtils::ObjectPrincipal(JS::CurrentGlobalOrNull(mCx)); This is just nsContentUtils::SubjectPrincipal(). ::: dom/bindings/CallbackObject.h @@ +87,5 @@ > // Throw an exception to the caller code if the thrown exception is a > // binding object for a DOMError or DOMException from the caller's scope, > // otherwise report it. > eRethrowContentExceptions, > + // Throw exceptions to the caller code. unless the caller compartment is nit: punctuation

Comment on attachment 8535237 [details] [diff] [review] part 3. Make Promise pass in their object compartment when calling their various callbacks Review of attachment 8535237 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/bindings/test/TestInterfaceJS.js @@ +98,5 @@ > + // compartment, since we plan to call the page-provided resolve function. > + var thenable = new this._win.Object(); > + Cu.waiveXrays(thenable).then = function() { > + noSuchMethodExistsYo3() > + } I'd do: var thenable = new Cu.cloneInto({ then: function() {noSuchmethodExistsYo3(); }, {cloneFunctions: true}); That way, the function will get automatically passed through Cu.exportFunction. ::: dom/promise/Promise.cpp @@ +214,5 @@ > + if (rv.IsJSException()) { > + rv.StealJSException(cx, &exn); > + } else { > + JSAutoCompartment ac(cx, mPromise->GlobalJSObject()); > + DebugOnly<bool> conversionResult = ToJSValue(cx, rv, &exn); This part is really confusing to me. What's going on, exactly? It looks like that ToJSValue overload just creates an empty-ish exception - is that what we want? It seems like it would be much better to produce some kind of useful greppable string like "Internal API implementation error" - that would also make the tests make more sense, since currently they just require empty string/filename. Needs comments in any case. @@ +569,3 @@ > aRv.WouldReportJSException(); > > if (aRv.IsJSException()) { Why don't we do the same dance here? ::: dom/promise/PromiseCallback.cpp @@ +210,5 @@ > > rv.WouldReportJSException(); > > // PromiseReactionTask step 7 > + if (rv.Failed()){ nit: ) { @@ +222,5 @@ > + } > + } else { > + JSAutoCompartment ac(aCx, mNextPromise->GlobalJSObject()); > + DebugOnly<bool> conversionResult = ToJSValue(aCx, rv, &value); > + MOZ_ASSERT(conversionResult); Same thing here.

> And given that using the global of the promise is kind of unfortunate from an Xray > perspective Hmm. Can you elaborate? We'd use the global of the promise in both options from comment 1, no? Note, by the way, that when Promise moves to SpiderMonkey this stuff will get _really_ exciting. I'm not quite sure exactly how it will work then, honestly, since it will no longer be going through CallbackObject bits... We may need to move some parts of this over to Xray/COW land. :( > This is just nsContentUtils::SubjectPrincipal(). Hmm. Is that because we're calling this before we reset() mAutoIncumbent/EntryScript? > That way, the function will get automatically passed through Cu.exportFunction. Won't that put the function into the page global? The intent here is to have a thenable whose "then" method is system but which has a property named "then" in the page compartment, so that when we pass it to the page-compartment resolve function the "then" property will be visible and get called. Since the function getting called is actually system, exceptions from it should not get propagated to the page, which is what the test is testing for. I _think_ using cloneInto would make all the action happen on the page, so the page would see the "noSuchMethodExistsYo3 is undefined" exception, no? > This part is really confusing to me. What's going on, exactly? What's going on is that in the case when we're not rethrowing the exception that chrome JS threw out to content we'll instead have a failed ErrorResult with an NS_ERROR_UNEXPECTED on it, coming from the callback code, which does this for PromiseInit, say: if (!JS::Call(cx, aThisVal, callable, JS::HandleValueArray::subarray(argv, 0, argc), &rval)) { aRv.Throw(NS_ERROR_UNEXPECTED); return; } We override that NS_ERROR_UNEXPECTED with the JS exception in the cases when we're rethrowing it. > It looks like that ToJSValue overload just creates an empty-ish exception It creates exactly the same exception that you'd get from a Web IDL binding method whose C++ implementation put the ErrorResult in the given state. In this case, you get exactly what you'd get if a C++ Web IDL implementation for a method declared as: [Throws] void foo(); did: aRv.Throw(NS_ERROR_UNEXPECTED); return; It's "emptyish" because the exception object XPConnect produces for NS_ERROR_UNEXPECTED is fairly sucky. But note that pretty much anything we produce here will have no useful filename, since we don't want to report the chrome filename involved to the page and there is no useful content filename associated with this stuff, since the call is happening from C++ in an async-ish way. > It seems like it would be much better to produce some kind of useful greppable string > like "Internal API implementation error" We could do that, I guess. That would need to happen in the CallSetup code at the point when we decide we're not going to propagate through the exception on the JSContext and will report it instead. We could then throw something more meaningful than NS_ERROR_UNEXPECTED on mErrorResult. I'm happy to do that as a followup. > Why don't we do the same dance here? Oh, this is interesting. Most immediately, because the dance was added due to test failures because promises were not getting rejected when they should have been. The reason the PromiseInit case did not lead to a test failure is a bit subtle. What happens there is that we're constructing a promise via new Promise, which is a fallible operation. The construction process calls the callback function passed to the constructor and uses the ErrorResult of the constructor call for this call. If the call to the callback fails, we end up returning out of the promise construction without rejecting our newly-constructed promise (because we didn't do the dance, as you say). But we _did_ place an error on the ErrorResult the bindings passed us, so the binding says "well, this promise-returning method threw; I better ignore its return value and construct a promise rejected with the exception". So it ignores the non-rejected thing we returned and constructs a new promise rejected with the exception on the ErrorResult. So the user-perceived behavior is the same as if we had done the dance: construction returns a promise, and it's rejected with the thing it should be rejected with. The dance was needed in the other cases because they were not directly called from binding code and hence any exception thrown on their on-stack ErrorResult was just getting swallowed instead of being used to reject a promise... I can either add a comment here or make it match the other cases by doing the dance and then explicitly clearing whatever exception is on the ErrorResult so the bindings code will use my newly-created rejected Promise instead of synthesizing a new one. My preference is to add a comment, I think.

(In reply to Vacation Dec 15-31 from comment #11) > > And given that using the global of the promise is kind of unfortunate from an Xray > > perspective > > Hmm. Can you elaborate? We'd use the global of the promise in both options > from comment 1, no? My impression was that we could base the subsumes check on the principal of the actual reject callback it was being passed to. So we could pass the full exception to chrome reject callbacks, and the neutered one to content callbacks (and report if there wasn't at least one chrome callback). I guess that's complicated though. > Note, by the way, that when Promise moves to SpiderMonkey this stuff will > get _really_ exciting. I'm not quite sure exactly how it will work then, > honestly, since it will no longer be going through CallbackObject bits... > We may need to move some parts of this over to Xray/COW land. :( Hopefully not COWs, since those are dead. But yes, we'll need to handle it somehow. The JS engine actually has a subsumes callback, so maybe it can be handled there. > > > This is just nsContentUtils::SubjectPrincipal(). > > Hmm. Is that because we're calling this before we reset() > mAutoIncumbent/EntryScript? Right. The subject principal is just the principal of the current compartment's global, which is exactly what you're doing here. > > That way, the function will get automatically passed through Cu.exportFunction. > > Won't that put the function into the page global? > > The intent here is to have a thenable whose "then" method is system but > which has a property named "then" in the page compartment, so that when we > pass it to the page-compartment resolve function the "then" property will be > visible and get called. Since the function getting called is actually > system, exceptions from it should not get propagated to the page, which is > what the test is testing for. I _think_ using cloneInto would make all the > action happen on the page, so the page would see the "noSuchMethodExistsYo3 > is undefined" exception, no? Using cloneInto will mean that you have a content object with a content function, and that content function is just a thin shell that forwards to the chrome function. The if the function throws, the exception that bubbles up will be a chrome exception, and the current patches would do the right thing, I think. Interestingly, this is a mark against option (2), since trying to report the exception in the compartment of the unwrapped callable would cause it to get reported in the content scope, even though it's a chrome exception. I guess we can't assume that the callable and the thrown exception will be same-origin. > > This part is really confusing to me. What's going on, exactly? > > What's going on is that in the case when we're not rethrowing the exception > that chrome JS threw out to content we'll instead have a failed ErrorResult > with an NS_ERROR_UNEXPECTED on it, coming from the callback code, which does > this for PromiseInit, say: > > if (!JS::Call(cx, aThisVal, callable, > JS::HandleValueArray::subarray(argv, 0, argc), &rval)) { > aRv.Throw(NS_ERROR_UNEXPECTED); > return; > } Ah I see - we set the rv's nsresult in codegen. I missed that part. > > It seems like it would be much better to produce some kind of useful greppable string > > like "Internal API implementation error" > > We could do that, I guess. That would need to happen in the CallSetup code > at the point when we decide we're not going to propagate through the > exception on the JSContext and will report it instead. We could then throw > something more meaningful than NS_ERROR_UNEXPECTED on mErrorResult. I'm > happy to do that as a followup. That sounds good, but I know you're busy - I'll leave it up to you. > > > Why don't we do the same dance here? > > Oh, this is interesting. > > Most immediately, because the dance was added due to test failures because > promises were not getting rejected when they should have been. > > The reason the PromiseInit case did not lead to a test failure is a bit > subtle. > > What happens there is that we're constructing a promise via new Promise, > which is a fallible operation. The construction process calls the callback > function passed to the constructor and uses the ErrorResult of the > constructor call for this call. If the call to the callback fails, we end > up returning out of the promise construction without rejecting our > newly-constructed promise (because we didn't do the dance, as you say). But > we _did_ place an error on the ErrorResult the bindings passed us, so the > binding says "well, this promise-returning method threw; I better ignore its > return value and construct a promise rejected with the exception". So it > ignores the non-rejected thing we returned and constructs a new promise > rejected with the exception on the ErrorResult. So the user-perceived > behavior is the same as if we had done the dance: construction returns a > promise, and it's rejected with the thing it should be rejected with. > > The dance was needed in the other cases because they were not directly > called from binding code and hence any exception thrown on their on-stack > ErrorResult was just getting swallowed instead of being used to reject a > promise... > > I can either add a comment here or make it match the other cases by doing > the dance and then explicitly clearing whatever exception is on the > ErrorResult so the bindings code will use my newly-created rejected Promise > instead of synthesizing a new one. My preference is to add a comment, I > think. Ok, sounds good. Please include all the detail of the above, because I'm almost certainly not going to remember it past today. ;-)

> I guess that's complicated though. Yeah. You have to make sure that different content callbacks all get the same exception, so you have to cache the content version or something... We could do it, but it'd be pretty fragile, I expect. > The subject principal is just the principal of the current compartment's global Current compartment of the stack-top JSContext, right. This last part was the part that was not obvious to me. > I guess we can't assume that the callable and the thrown exception will be same-origin. Indeed. In fact, that assumption is totally false, because we want to allow chrome code to throw content exceptions (via Xray calls on the content stuff to create them). Will do these fixups when I get back. > and that content function is just a thin shell that forwards to the chrome function Ah, cool. I'll do that then.

(In reply to Vacation Dec 15-31 from comment #13) > > I guess that's complicated though. > > Yeah. You have to make sure that different content callbacks all get the > same exception, so you have to cache the content version or something... We > could do it, but it'd be pretty fragile, I expect. > > > The subject principal is just the principal of the current compartment's global > > Current compartment of the stack-top JSContext, right. This last part was > the part that was not obvious to me. It is never valid to do anything with a cx that is not stack-top.

Comment on attachment 8535236 [details] [diff] [review] part 2. Change codegen to output an aCompartment argument on all callbacks that have an aExceptionHandling argument Review of attachment 8535236 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/bindings/Codegen.py @@ +13631,5 @@ > # CallSetup should re-throw exceptions on aRv. > args.append(Argument("ExceptionHandling", "aExceptionHandling", > "eReportExceptions")) > + # And the argument for communicating when exceptions should > + # really be rethrown. Could you elaborate a bit, because the argument itself isn't useful to decide that (ie that we use its principal to check that it subsumes the exception).

I'd be happy to file a new bug for comment 8 to accelerate the landing of this.

(In reply to Jan-Ivar Bruaroey [:jib] from comment #8) > For the promise-returning method (addIceCandidate), the line-number is that > of the throw in chrome-only PeerConnection.js instead of the outer content > call. OK, I think I see why this is happening. There are actually a few problems here. The first problem is that SavedStacks are busted for line/columne/file: they don't do the right security checks. I filed bug 1117242 on this. The second problem is that even if that were fixed we'd have some fragile behavior because DOMException caches its file/line/column after extracting them from the JS callstack for the first time, because the extraction process is not exactly fast. I ran into this in the testcases in bug 1107592 as well, where I solved it via actually cloning the DOMException in ErrorResult::ReportJSExceptionFromJSImplementation. I should have added content DOMException tests in this bug, not just pure JS exceptions. :( Or added Promise tests to bug 1107592 or something. I'll probably refactor that cloning code so I can reuse it in this bug. Need to think about it a bit. A followup is not ok here, because I think exposing the chrome script info is a security problem.

> var thenable = new Cu.cloneInto({ then: function() {noSuchmethodExistsYo3(); > }, {cloneFunctions: true}); I tried this (with this._win) as the second argument, before the options object, but it doesn't work: we end up rejecting with an object the page doesn't actually have access to, as far as I can tell. I've reverted to the code I used to have here. > It seems like it would be much better to produce some kind of useful greppable string > like "Internal API implementation error" Filed bug 1117269.

Bobby, please let me know if you can think of someone else who should review this... Another possible strategy would be to fix the SavedStacks stuff and somehow invalidate the cache in JSStackFrame when called from a different caller than the previous time. The problem is, I don't know how to save the caller from the previous time. I guess I could save the subject principal or something. Jan-Ivar, this should fix your try orange, though you'll need to move the exception-throwing back into the sync part of the method if you want to see a useful line number on the content side, of course.

Comment on attachment 8543477 [details] [diff] [review] part 4. Add a friend API to get the principals of a SavedFrame object Review of attachment 8543477 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. I don't know what the usual practices are for JSPrincipals, but should we increment its reference count when we return it?

I think for this hacky API just returning the raw pointer is ok. It's not like the pointer will outlive the JSObject keeping the JSPrincipals alive...

Any ETA on this one? I have several patches from last year blocking on it.

(In reply to Jan-Ivar Bruaroey [:jib] from comment #23) > Any ETA on this one? I have several patches from last year blocking on it. I am currently firefighting media stuff - hopefully by the end of next week if not sooner. Sorry for the lag here :-(

Comment on attachment 8543478 [details] [diff] [review] part 5. Add tests for promise rejections with content-side DOMExceptions, and fix the promise code so those tests actually pass Review of attachment 8543478 [details] [diff] [review]: ----------------------------------------------------------------- I haven't fully reviewed this, but from a high-level, I'm not really happy about making the current compartment during the StealJSException salient from a security perspective. IMO, the information stored in a JSStackFrame should correspond with the information that is visible to the object it belongs to. If we do caching, the cached values should reflect that behavior, and Xrays should manually bypass the cache. Thoughts, Boris? ::: dom/bindings/test/TestInterfaceJS.js @@ +84,5 @@ > }, > > + testPromiseWithDOMExceptionThrowingPromiseInit: function() { > + return new this._win.Promise(() => { > + throw new this._win.DOMException("We are a second DOMException", Where was the first? ::: dom/bindings/test/test_promise_rejections_from_jsimplemented.html @@ +13,5 @@ > /** Test for Bug 1107592 **/ > > SimpleTest.waitForExplicitFinish(); > > + function checkExn(lineNumber, name, message, code, filename, testNumber, exn) { Don't we want to check the stack sanitation pieces here? ::: xpcom/base/nsIException.idl @@ +15,1 @@ > [scriptable, uuid(f80ac10b-68dd-4482-a8c2-3cbe13fa89af)] I guess you don't need to rev the UUID because the method is being added to the end? What about classes that inherit from this one? @@ +34,5 @@ > AUTF8String toString(); > + > + // Return whether this stack frame can be accessed by the caller. This is > + // safe to call on non-main threads, but will always report "yes" there. > + [noscript, notxpcom, nostdcall] boolean callerCanAccess(in JSContext aCx); I'd call this CallerSubsumes, or CanCallerAccess.

> IMO, the information stored in a JSStackFrame should correspond with the information that > is visible to the object it belongs to. That would be a good theory, but isn't how JSStackFrame works right now. When I wrote this patch I was hoping we could get this in as a workaround for the JSStackFrame bustage, then change things to be saner once bug 1117242 is fixed and we figure out what to do for the DOMException caching. But I don't have an ETA on that, and wanted to unblock things here. I'm happy to file a followup bug to rip out these bits once our stackframe security story is saner. Sound good? If so, I can address your other comments and either post the updated patch or not, as you prefer. > Where was the first? In testThrowDOMException in TestInterfaceJS.js. > Don't we want to check the stack sanitation pieces here? Hmm. That's probably a good idea, yes. I'll add that. > I guess you don't need to rev the UUID because the method is being added to the end? No, I just forgot to rev it. Will do. > I'd call this CallerSubsumes, or CanCallerAccess. Can do.

(In reply to Boris Zbarsky [:bz] from comment #26) > That would be a good theory, but isn't how JSStackFrame works right now. Right, and I'm trying to determine how much more work it would be to get us there, than to do this. > When I wrote this patch I was hoping we could get this in as a workaround > for the JSStackFrame bustage, then change things to be saner once bug > 1117242 is fixed It looks like that bug is close to landing. > and we figure out what to do for the DOMException caching. Can't we just bypass the cache if we're being invoked over Xrays? > But I don't have an ETA on that, and wanted to unblock things here. > > I'm happy to file a followup bug to rip out these bits once our stackframe > security story is saner. Sound good? My beef is that this code is growing a lot of invariants that I have trouble understanding, and that every time I try to review stuff for it, I spend more than an hour just trying to figure out how everything works (this is why the review has taken forever, because that hour is very costly right now). I recognize that my limited bandwidth is unhelpful here. If you just want a rubber stamp and promise to take take care of it in the coming months, I'm willing to give that to you. If you want me to sign off on this design-wise, then it seems like we should fix bug 1117242 and the caching story. Which do you want?

> It looks like that bug is close to landing. Sure. It wasn't two weeks ago, when I wrote the patch. > Can't we just bypass the cache if we're being invoked over Xrays? As in, bypass _filling_ the cache? We could, if we can detect being called over Xrays... Can we? Speaking of which, will content get Xrays to the chrome StackFrame? That's the direction we'll need Xrays in here: the StackFrame is created in the chrome compartment but then attached to a content DOMException. So when content calls us we're in the content compartment, and presumably will wrap the chrome StackFrame into it and then make calls on it, but at that point we need Xrays to it. Which I want basically depends on Jan-Ivar's timeframes and your availability, I think. If you're going to be available enough to help make sure whatever we end up doing to make the Xray stuff work is done expeditiously (however that's defined), then I'm all in favor of that. Otherwise, I think I'll take the rubberstamp to get Jan-Ivar unblocked...

Comment on attachment 8543478 [details] [diff] [review] part 5. Add tests for promise rejections with content-side DOMExceptions, and fix the promise code so those tests actually pass Review of attachment 8543478 [details] [diff] [review]: ----------------------------------------------------------------- Ok. Per IRC discussion, rs=me with previous comments addressed. ::: dom/promise/Promise.cpp @@ +211,5 @@ > rv.WouldReportJSException(); > if (rv.Failed()) { > JS::Rooted<JS::Value> exn(cx); > if (rv.IsJSException()) { > + JSAutoCompartment ac(cx, mPromise->GlobalJSObject()); Please comment this (and the other two like it) and indicate that they affect the security check.

Thanks, appreciate it! This will let us move forward with the WebRTC API and unblocks patches.

I filed bug 1122238 as a followup on ripping out the hackery.

https://hg.mozilla.org/integration/mozilla-inbound/rev/52e8b1cf65c9 https://hg.mozilla.org/integration/mozilla-inbound/rev/50a3d2410fcd https://hg.mozilla.org/integration/mozilla-inbound/rev/a5098ce0d76d https://hg.mozilla.org/integration/mozilla-inbound/rev/40c8c95bab0b https://hg.mozilla.org/integration/mozilla-inbound/rev/a091ec8d2a04

https://hg.mozilla.org/mozilla-central/rev/52e8b1cf65c9 https://hg.mozilla.org/mozilla-central/rev/50a3d2410fcd https://hg.mozilla.org/mozilla-central/rev/a5098ce0d76d https://hg.mozilla.org/mozilla-central/rev/40c8c95bab0b https://hg.mozilla.org/mozilla-central/rev/a091ec8d2a04

Comment on attachment 8543478 [details] [diff] [review] part 5. Add tests for promise rejections with content-side DOMExceptions, and fix the promise code so those tests actually pass Review of attachment 8543478 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/base/DOMException.cpp @@ +745,5 @@ > + retval->mLocation.swap(caller); > + } > + } > + > + return ToJSValue(aCx, retval, aSanitizedValue); Missing #include "mozilla/dom/ToJSValue.h"