1430169 - Figure out what .wrappedJSObject should do on sandboxCallableProxyHandler proxies for Xrays

Status: NEW

(Core :: XPConnect, defect, P3)

Description

[Boris Zbarsky [:bzbarsky]]

See bug 1430164 comment 2.  Right now it asserts in debug builds:

  Assertion failure: js::IsCrossCompartmentWrapper(wrapper), at /home/bzbarsky/mozilla/vanilla/mozilla/js/xpconnect/wrappers/WrapperFactory.cpp:117

and in an opt build "does the right thing", kinda by accident.

Plausible behaviors:

1)  Don't expose the property.
2)  Make xpc::wrappedJSObject_getter throw on a non-wrapper.
3)  Make xpc::wrappedJSObject_getter unwrap non-cross-compartment wrappers.
4)  Make xpc::wrappedJSObject_getter unwrap all wrappers and then rewrap into
    current compartment.

Bobby, thoughts?

Comment 1

[Bobby Holley (:bholley)]

Given that the context where we do the sandboxCallableProxyHandler stuff is basically WebExtensions, where we'd really rather not expose non-standard stuff, I'm partial to (1).

I think (3) fits best with the model, but I only really care about the model if we're actually going to try to get the model right, which would mean some sort of cache of the callable proxies so that we entirely eliminate the behavior in bug 1430164 for non-constructors as well. And I'm not sure that's worth doing.

Comment 2

[Boris Zbarsky [:bzbarsky]]

> so that we entirely eliminate the behavior in bug 1430164 for non-constructors as well.

That's hard, because "alert" and "window.alert" would still end up different objects unless we do something really weird....

I can do (1).

Comment 3

[Boris Zbarsky [:bzbarsky]]

With bug 1430164 fixed, I'm not quite sure how to get into this situation.  We need to get a thing off the sandbox proto and end up with a sandboxCallableProxyHandler for an Xray.  But DOM ctors no longer get wrapped in sandboxCallableProxyHandler, DOM methods on Xrays give you a new function, not an Xray for the underlying function, and there's nothing else you can get off the window, afaict.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: Attempt to write a test, which doesn't work

Comment 5

[Tomislav Jovanovic :zombie]

(In reply to Bobby Holley (:bholley) from comment #1)
> I think (3) fits best with the model, but I only really care about the model
> if we're actually going to try to get the model right, which would mean some
> sort of cache of the callable proxies so that we entirely eliminate the
> behavior in bug 1430164 for non-constructors as well. And I'm not sure
> that's worth doing.

`alert != alert != this.alert` breaks a very fundamental assumption, and I think is worth fixing, even if the fix doesn't cover `this.alert == window.alert`.

Comment 6

[Boris Zbarsky [:bzbarsky]]

> `alert != alert != this.alert` breaks a very fundamental assumption

While true, fixing this involves a huge amount of complexity: we have to cache the wrapper we create, manage lifetimes somehow, etc, etc.  I'd like to see some evidence that it's a problem before we invest in that complexity.

In any case, that identity not holding is not what this bug is about.  This bug is about .wrappedJSObject in debug builds not doing the right thing for a sandboxCallableProxyHandler around an Xray.  And with bug 1430164 fixed, I'm not sure how to produce a sandboxCallableProxyHandler around an Xray to start with.