1514261 - Fix exportFunction for same-compartment realms

Status: RESOLVED FIXED

(Core :: XPConnect, enhancement)

Description

[Jan de Mooij [:jandem]]

I get this on Try with bug 1514210:

Assertion failure: js::GetObjectCompartment(obj) != js::GetContextCompartment(cx) (This should be invoked after entering the compartment but before wrapping the values), at /builds/worker/workspace/build/src/js/xpconnect/src/ExportHelpers.cpp:246

I think we're exporting a function to a same-compartment global and then we call it and assume |this| and arguments are cross-compartment.

Comment 1

[Boris Zbarsky [:bzbarsky]]

This is trying to assert that we did the JSAutoRealm but not yet the JS_WrapValue call.

This used to kinda work in the old world, because unwrappedFun and callee were in different globals.  Except when they were not.  Here's a testcase that asserts on current trunk without any changes to compartments and gobals when run in a browser sandbox:

  Cu.exportFunction(function(arg){ dump(arg); }, window)({})

This is just exporting a function to the function's existing global, then calling the result, and boom.

What we could probably assert is that either cx is same-global with js::UncheckedUnwrap(obj) or cx is different-global with obj.  I think.  Bobby, does that make sense?

Comment 2

[Kris Maglione [:kmag]]

Or we could just throw when someone tries to export a function to the same compartment. There's not really any sense in doing that

Comment 3

[Boris Zbarsky [:bzbarsky]]

The hard part with that is that when we're doing this multiple-globals-per-compartment stuff whether two globals are same-compartment or not is a bit unpredictable.

One option would be that exportFunction just returns the original function if the compartments match and otherwise do what it does now...

Comment 4

[Kris Maglione [:kmag]]

Well, what I really mean is there's not really any sense in doing it when it's same compartment or same origin. I suppose there may be some odd corner case where it's sometimes same origin, sometimes not. In which case, just returning the same function is probably fine...

Comment 5

[Boris Zbarsky [:bzbarsky]]

I think that corner case is going to be hit a good bit as we transition piecemeal to using fewer compartments.... Once the transition is done, we could reevaluate.

Comment 6

[Kris Maglione [:kmag]]

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #5)
> I think that corner case is going to be hit a good bit as we transition
> piecemeal to using fewer compartments.... Once the transition is done, we
> could reevaluate.

Yeah, fair enough. Just making sure we do something sensible in that case for now is probably best.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: When exporting functions within a single compartment, just keep using the same function

Comment 8

[Kris Maglione [:kmag]]

Comment on attachment 9031571 [details] [diff] [review]
When exporting functions within a single compartment, just keep using the same function

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

::: js/xpconnect/src/ExportHelpers.cpp
@@ +90,5 @@
>        }
>  
> +      if (!js::IsWrapper(obj)) {
> +        // We're same-compartment.  Don't set up a function forwarder; just use
> +        // our function as-is.

Maybe mention that we only really expect to see this for same-compartment realms?

@@ +450,5 @@
>        return false;
>      }
>  
> +    if (!js::IsWrapper(funObj)) {
> +      // We're same-compartment; just use funObj as-is.

And here.

Comment 9

[Bobby Holley (:bholley)]

I don't understand from skimming this bug why we can't unconditionally use a forwarder and just have the compartment change be a no-op if the function and its forwardee are same-compartment. The patch in this bug is probably fine if we need it, though it does change the semantics (i.e. putting an expando on the forwarder puts it on the forwardee, whereas normally it would't). So I'd prefer to always have a wrapper function if that's feasible.

Comment 10

[Boris Zbarsky [:bzbarsky]]

We can always use a forwarder, but then we have to drop the failing assert, because it just won't hold in same-compartment cases.  Apart from that everything else will work fine.  If you're OK with that, it's certainly simple enough to do.

Comment 11

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #10)
> We can always use a forwarder, but then we have to drop the failing assert,
> because it just won't hold in same-compartment cases.  Apart from that
> everything else will work fine.  If you're OK with that, it's certainly
> simple enough to do.

How about we just skip the block with the CheckSameOriginArg + JS_WrapValue calls in the case that the forwarder is same-compartment with the forwardee?

Comment 12

[Boris Zbarsky [:bzbarsky]]

Attachment: Skip messing around with compartments in FunctionForwarder if the forwarder is already same-compartment with the underlying callee

Comment 13

[Bobby Holley (:bholley)]

Comment on attachment 9031717 [details] [diff] [review]
Skip messing around with compartments in FunctionForwarder if the forwarder is already same-compartment with the underlying callee

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

::: js/xpconnect/src/ExportHelpers.cpp
@@ +296,5 @@
>      // here, because certain function wrappers (notably content->nsEP) are
>      // not callable.
>      JSAutoRealm ar(cx, unwrappedFun);
> +    if (js::GetObjectCompartment(unwrappedFun) !=
> +        js::GetObjectCompartment(&args.callee())) {

nit: I'd prefer to do bool crossCompartment = ....; if (crossCompartment) { ... }

To make the meaning clearer.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Done.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fca2da230042
Skip messing around with compartments in FunctionForwarder if the forwarder is already same-compartment with the underlying callee.  r=bholley

Comment 16

[Natalia Csoregi [:nataliaCs]]

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