1436276 - More problems with promises and Xrays

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[Boris Zbarsky [:bzbarsky]]

STEPS TO REPRODUCE:

1)  Set the "devtools.chrome.enabled" preference to true.
2)  Set the "devtools.debugger.remote-enabled" preference to true
3)  Open the "Browser Content Toolbox" from the devtools menu.
4)  Ensure you have a tab open that has some random web page loaded.
5)  Evaluate this string:

  tabs[0].content.navigator.credentials.create({ publicKey: { challenge: 5 } }).then(() => {}).catch((err) => tabs[0].content.alert(err))

EXPECTED RESULTS: Get an alert saying "TypeError: 'challenge' member of MakePublicKeyCredentialOptions could not be converted to any of: ArrayBufferView, ArrayBuffer."

ACTUAL RESULTS: Get an alert saying "InternalError: Promise rejection value is a non-unwrappable cross-compartment wrapper."

The exact sequence of calls matters.  If the .then() bit is taken out, the problem disappears.  If the .then().catch() is replaced with a single .then() call with two function arguments, the problem also disappears.

OK, so what's going on here?

We enter the bindings code via Xrays, in the system compartment.  The create() method throws.  This exception is converted into a Promise via ConvertExceptionToPromise.  Critically, this is all done in the _content_ compartment, because we use xpc::XrayAwareCalleeGlobal here.  Looks like we did that as part of the initial implementation in bug 983300, but without very clear reasons for why...

In any case, we call JS::CallOriginalPromiseReject which ends up in CommonStaticResolveRejectImpl and creates the promise (in the content compartment).  Then we end up calling RejectMaybeWrappedPromise and that works fine, because we have an actual PromiseObject, so the fact that we're resolving it with a value it can't unwrap is not relevant.

So now we have a content-side Promise rejected with a chrome-side extension (wrapped in an opaque security wrapper).

Next we do the .then() call, again over Xrays.  This time we land in js::OriginalPromiseThen while in the system compartment, create the new Promise in the _content_ compartment (because all that stuff is Xrayed) and call PerformPromiseThen while still in the system compartment.  We land in PerformPromiseThenWithReaction with the following:

* Current compartment is system.
* promise is content-side
* valueOrReason is the opaque security wrapper.

Then we go on to EnqueuePromiseReactionJob and create the job in the system compartment.

Later, when the job executes, it calls, indirectly, RejectMaybeWrappedPromise while still in the system compartment, which leads to the code at https://searchfox.org/mozilla-central/rev/84cea84b12145d752e50ddca6be5462c38510e35/js/src/builtin/Promise.cpp#1016-1039 which ends up switching up the rejection value.

OK, so this is not good.  What are the options here?

1)  Make it so that the promises involve are all created in the system compartment, not the page compartment in this case, by removing the XrayAwareCalleeGlobal bit..  This might well be pretty reasonable; I'm really not sure why we're creating them in the page compartment, necessarily.

2)  Make resolving a Promise with a wrapped thing it can't touch fail.  That is, do the CheckedUnwrap() even in the !IsProxy(promiseObj) case in RejectMaybeWrappedPromise.  This will make the behavior consistent in that without the .then() we would also have the exception switched out, but seems pretty sub-optimal to me.

3)  Make it so that once a promise has been rejected with a non-unwrappable thing, .then() and .catch() on it don't lose that property.  That said, this seems a little hard to enforce.  For example, what if the callback to .then() throws?  This exception _should_ get sanitized in that case, I would think.

I'm going to try a try push with #1 and see whether anything obvious fails, for a start.

Comment 1

[Boris Zbarsky [:bzbarsky]]

> 4)  Ensure you have a tab open that has some random web page loaded.

I guess it needs to be https so you have navigator.credentials.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: Bindings should create their return promises in the current compartment even when called over Xrays

OK, try looks green and this fixes the issue.  I think the new behavior makes a lot more sense.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: diff -w version of the patch

Comment 4

[Bobby Holley (:bholley)]

Comment on attachment 8949063 [details] [diff] [review]
Bindings should create their return promises in the current compartment even when called over Xrays

Review of attachment 8949063 [details] [diff] [review]:
-----------------------------------------------------------------

So, one argument for the existing behavior is that we will consistently get a promise in the target compartment. Under the new scheme, we'll get an Xrayed promise if the API successfully dispatches the request but then rejects it, whereas we'll get a local (chrome) promise if the failure occurs before the DOM API mints its return promise. And for a number of cases, the distinction between whether a failure is recorded before or after the promise is minted seems kind of arbitrary (though maybe it's in the spec?).

So it seems like the problems we're seeing here might still crop up with rejections that happen to be delivered async (do those tend to be DOM Exceptions, or something else?). That said, this only affects chrome, makes the implementation simpler, and improves the ergonomics for consumers in a lot of corner cases. So I think I'm fine with it, but am curious as to your thoughts on the above.

Comment 5

[Boris Zbarsky [:bzbarsky]]

> Under the new scheme, we'll get an Xrayed promise if the API
> successfully dispatches the request but then rejects it, whereas
> we'll get a local (chrome) promise if the failure occurs before
> the DOM API mints its return promise.

That depends on how the API creates its return promises.

If it creates them in the _relevant_ global, then yes.  If it creates them in the _current_ global, then no.

Note that this is also generally web-observable.  If I do windowA.someObj.someAPI.call(windowB.someObj) and check whether I get a promise from windowA or windowB I can observe whether it was created from the relevant or current global.

In theory, specs should be defining how this happens.  In practice, it's a mess.  https://github.com/w3ctag/promises-guide/issues/46 sort of tracks the spec-level problems.  But in short, I would argue that any function that caches the to-be-returned promise should use the relevant global and any function that just mints a new thing each time should use the current global.  I'm sure what we do right now looks nothing like that.  Maybe we should make a point of going through our Promise-returning APIs and making sure they _do_ look like that.  I'd sort of been waiting for this to get settled in the specs before doing that work, especially because I don't have 100% confidence in what the spec will end up saying.

> And for a number of cases, the distinction between whether a failure is
> recorded before or after the promise is minted seems kind of arbitrary
> (though maybe it's in the spec?).

For things happening at the binding layer, they're specced to be "before the promise is minted", effectively.

For algorithm steps, we do treat "throw on ErrorResult" and "return a rejected promise" somewhat interchangeably.  Again, if we did the promise-creation in the current global this would be a nonissue.

That said, while we do end up with the inconsistent behavior you noted, this _specific_ problem should not come up.  That's because if the API impl does the rejection itself it will create the exception in either its own compartment (the "relevant global") or the compartment of the Promise, depending on how it goes about doing that.  So in all cases the Promise will subsume the rejection value and .then() will work fine.  The case observed here of a less-trusted promise with a more-trusted rejection value won't arise, afaict.

Comment 6

[Boris Zbarsky [:bzbarsky]]

I guess my point is:

1) Specs really should define whether a promise return value is created from the
   relevant or current global, under all circumstances.
2) In the Xray case, current global is the xray compartment and relevant global
   is the target compartment.
3) Therefore, if the specs actually specify this stuff and we follow the specs
   we will get predictable and consistent Xray behavior, for any given API.

plus the bit about how either way we should end up avoiding this specific issue.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fa2042520eb9
Bindings should create their return promises in the current compartment even when called over Xrays.  r=bholley

Comment 8

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/fa2042520eb9

Comment 9

[Joel Maher ( :jmaher ) (UTC -8)]

:bz, I see you wrote new tests here that are only run on debug, but I cannot see a comment as to why- should we consider running these on opt?

Comment 10

[Boris Zbarsky [:bzbarsky]]

The TestFunctions interface is only compiled into the binary in debug builds (and only if --enable-tests). See https://searchfox.org/mozilla-central/rev/d302c3058330a57f238be4062fddea629311ce66/dom/bindings/moz.build#123-128

Comment 11

[Joel Maher ( :jmaher ) (UTC -8)]

thanks for pointing that out!