1262069 - Promise resolution values should be wrapped for the promise's compartment before storing

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Kris Maglione [:kmag]]

Promise rejection values are currently stored with whatever wrappers they were passed in with, and re-wrapped for the compartment that the promise belongs to when reporting unhandled rejections, or calling handlers.

In the case of unhandled rejections, in particular, this causes issues when attempting to report an error that belongs to the same compartment as the promise, but was passed in from a different compartment:

    let sandbox = Cu.Sandbox(window);
    sandbox.Promise.reject(new sandbox.Error("error"));

    Cu.nukeSandbox(sandbox);

This leads to a message:

    JavaScript error: , line 0: uncaught exception: unknown (can't convert to string)

since the error object was stored as a cross-compartment wrapper, and that wrapper was cut when the sandbox was destroyed, meaning that the exception reporter can no longer report any useful details about it.

Comment 1

[Kris Maglione [:kmag]]

Attachment: Wrap promise resolution values before storing

Comment 2

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8738042 [details] [diff] [review]
Wrap promise resolution values before storing

I seem to recall there were actual reasons for storing the value as-is instead of wrapping into the promise compartment.  Have you run the full test suite with this patch?

In any case, this won't help with the spidermonkey promise implementation, which we're about to turn on.  As in, you're touching code that's about to become dead code.

Comment 3

[Kris Maglione [:kmag]]

(In reply to Boris Zbarsky [:bz] from comment #2)
> Comment on attachment 8738042 [details] [diff] [review]
> Wrap promise resolution values before storing
> 
> I seem to recall there were actual reasons for storing the value as-is
> instead of wrapping into the promise compartment.  Have you run the full
> test suite with this patch?

A few tests are still running, but so far most of them have passed.

I could see choosing not to re-wrap the value, on the expectation that some
handlers may be in the same compartment that fulfilled the promise. But I
don't think that should cause any actual problems, unless there are some weird
corner cases with cached WN reflectors.

Even if there are, though, I think that re-wrapping for the target compartment
is the expected behavior. A call to a promise handler for a promise in a
different compartment should be the same as any other call that passes a value
into that compartment. Any code that relies on different behavior is, I think,
broken.

> In any case, this won't help with the spidermonkey promise implementation,
> which we're about to turn on.  As in, you're touching code that's about to
> become dead code.

How soon will it be turned on?

I just tested with spidermonkey promises enabled, and I no longer get these
errors. But the unhandled rejections aren't reported, either.

From a quick read through Promise.js and Promise.cpp, it looks like the
resolution values should be wrapped for the compartment that owns the promise
before calling the self-hosted fulfillment methods, so this may not be a
problem there.

Either way, if the new implementation isn't going to be turned on in this
release, I'd still like to address this. These errors are extremely confusing
for users, and I'm worried that they may mask issues in our tests.

Comment 4

[Boris Zbarsky [:bzbarsky]]

> How soon will it be turned on?

Hopefully a few days, but I said that two weeks ago...

I guess the code you're adding comes after we've done the thenable thing.  So maybe it's OK to mess with the value at that point.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8738042 [details] [diff] [review]
Wrap promise resolution values before storing

>+  if (!JS_WrapValue(cx, &value)) {
>+    JS_ClearPendingException(cx);

You should probably set value to undefined too, or something....

And you probably want a test for whatever thing it is you care about here, so someone doesn't simplify things and re-break it.

r=me

Comment 6

[Till Schneidereit [:till]]

(In reply to Kris Maglione [:kmag] from comment #3)> From a quick read through Promise.js and Promise.cpp, it looks like the
> resolution values should be wrapped for the compartment that owns the promise
> before calling the self-hosted fulfillment methods, so this may not be a
> problem there.

Correct, it's not a problem in the new implementation.

> Either way, if the new implementation isn't going to be turned on in this
> release, I'd still like to address this. These errors are extremely confusing
> for users, and I'm worried that they may mask issues in our tests.

What bz said. I have all tests passing on Linux and OS X now, but Windows does weird things.

In any case, this seems like it's a small enough fix that landing it shouldn't hurt. Given that we want to keep open the option of disabling SM Promise if it misbehaves in bad ways on Aurora or Beta, I think it'd make sense to fix issues with the DOM implementation.

Comment 7

[Kris Maglione [:kmag]]

Thanks

Comment 8

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/fx-team/rev/bb1926280d496c73c43f89db6b7b5f8f9165504c
Bug 1262069: Wrap promise resolution values before storing. r=bz

Comment 9

[Carsten Book [:Tomcat]]

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