Closed Bug 1262069 Opened 4 years ago Closed 4 years ago

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

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: kmag, Assigned: kmag)

Details

Attachments

(1 file)

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.
Attachment #8738042 - Flags: review?(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.
Attachment #8738042 - Flags: review?(bzbarsky)
(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.
> 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 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
Attachment #8738042 - Flags: review+
(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.
Thanks
https://hg.mozilla.org/mozilla-central/rev/bb1926280d49
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.