798660 - XMLHttpRequest, WebSocket and Worker have issues when imported to a sandbox

Status: RESOLVED WORKSFORME

(Core :: XPConnect, defect)

Description

[:Felipe Gomes (needinfo for replies!)]

Bug 756173 was originally created about this issue, but a workaround there was found and the bug marked as WORKSFORME. However the underlying issue was not fixed, so I'm filing this bug to track it.

A wrapped XMLHttpRequest doesn't seem to work when imported to a sandbox, and to make it work it needs to be unwrapped. Example (from Bug 756173 comment 27):

win = iframe.contentWindow.wrappedJSObject;
s = Cu.Sandbox(win);
s.XMLHttpRequest = win.XMLHttpRequest;

Comment 1

[:Felipe Gomes (needinfo for replies!)]

It seems that the WebSocket object also suffers from this problem. See bug 790201 comment 8 and comment 10 for details

Comment 2

[Gabor Krizsanits (INACTIVE)]

I'll just grab some info from the previous bug to start it somewhere.

(In reply to scaraveo from comment #20)
> (In reply to Gabor Krizsanits [:krizsa :gabor] from comment #16)
> 
> I tried three different things with various levels of fail.
> 
> let s = Cu.Sandbox(iframe.contentWindow);
> 
> // #1 XMLHttpRequest is undefined in the sandbox
> s.XMLHttpRequest = iframe.contentWindow.XMLHttpRequest;
> 
> // #2 throws exception
> s.xhr = new iframe.contentWindow.XMLHttpRequest();
> 
> // #3 returned value is undefined
> s.getXHR = function() {
>  return new iframe.contentWindow.XMLHttpRequest();
> }
> 
> It seems the only way I can make this work is if iframe.contentWindow is the
> proto for the sandbox.

This really bothers me. #1 should be visible #2 should not throw #3 even this should work according to my best knowledge. So something is really fishy here. I tried with #1 first and it seems that from the chrome context where this code is executed from the dom ctor is visible, it only becomes undefined when we stick it to the sandbox which has the same principal as the content. This does not make sense to me at all.
Bobby, is it possible that CrossCompartmentWrapper::set simply forgets to unwrap the Value *vp? (or more general the DirectWrapper or even deeper but you get my point...)
Because that would mean that the line in #1 assigns a chrome object (the holder of the xray) to s.XMLHttpRequest (and maybe fails somewhere). Or if that's not the case, where can I find the bits that unwraps the value (rhs) of that assignment?

Comment 3

[Bobby Holley (:bholley)]

Yeah, this should work.

If we were forgetting to wrap something, we'd get a compartment mismatch, so I doubt that's what's going on. This should be pretty easy to debug though. Just break in CrossCompartmentWrapper::set in #1 and see what's going on. We should end up with a transparent cross-compartment wrapper because the sandbox should be same-origin with the content window.

Comment 4

[Gabor Krizsanits (INACTIVE)]

Oh man... The good news is that the line #1 works actually just I made some mistakes in my script. The bad news is that we really do have a problem here. Let's take a look step by step at this example.

var sb = Components.utils.Sandbox(content.window);
sb.XMLHttpRequest = content.window.XMLHttpRequest;

Components.utils.evalInSandbox("var xhr = new XMLHttpRequest(); xhr.open('GET', location, false); xhr.send(); xhr.responseText", sb);

This is a chrome scratchpad example. (meaning it's executed from browser context)

The result is that .open is not accessible because of a cow.

So the content.window.XMLHttpRequest because of xray creates a chrome function object (that tricky one that has the native wrapper as it's parent...). Then there is a chrome object wrapper created for it at the assignement... But that's not a big problem since calling it (even with new...) should be ok. 
The problem is that XMLHttpRequestBinding _constructor thinks that the global is the chrome window! Which is weird since the chrome function object that was returned from the xray (for XMLHttpRequest) should be bound to the content window.

Can this be because from IndirectProxyHandler::construct we end up in InvokeConstructorKernel where we do this magic: args.setThis(MagicValue(JS_IS_CONSTRUCTING)); 
and that screws up the binding between the ctor and the content window?

Comment 5

[Bobby Holley (:bholley)]

Yeah, I think we should avoid passing chrome xray stuff into content. Maybe we should just do |sb.XMLHttpRequest = content.window.wrappedJSObject.XMLHttpRequest;|?

Comment 6

[Gabor Krizsanits (INACTIVE)]

(In reply to Bobby Holley (:bholley) from comment #5)
> Yeah, I think we should avoid passing chrome xray stuff into content. Maybe
> we should just do |sb.XMLHttpRequest =
> content.window.wrappedJSObject.XMLHttpRequest;|?

I think that was the original workaround... The only problem with that, that we give up the xray behavior. So if content changed the XMLHttpRequest in some way it can cause unexpected behavior. And as you pointed it out, this should just work :( It's hard to explain to people why it does not.

Comment 7

[Gabor Krizsanits (INACTIVE)]

So I think we could fairly easily identify these functions returned from xrays in compartment wrap... I wonder if there is an easy way to replace them by a version they would have got from the target compartment. So instead of wrapping it into a chrome object wrapper, we should kind of recreate them into the target compartment (with some subsume checks ofc), or re-resolve them... I'm not sure about the details but the current behavior is really counter intuitive.

Comment 8

[Bobby Holley (:bholley)]

Maybe we should use WrapperFactory::WrapForSameCompartmentXray?

Comment 9

[Gabor Krizsanits (INACTIVE)]

(In reply to Bobby Holley (:bholley) from comment #8)
> Maybe we should use WrapperFactory::WrapForSameCompartmentXray?

I'm not sure I see how would that help... 

So I had a two ideas:
A., when we find a native function object with XPC_WN_CallMethod or XPC_WN_GetterSetter, that has a parent that is a wrapper of a wn, we simply create a new function for it in the target compartment and re-wrap the parent. But that would mess up the identity checks, and we would keep creating new objects for the same property.

B., We could call a WrapperFactory::PrepareForWrapping on the parent of these functions and then use the WrapForSameCompartmentXray on them to re-resolve the property the functions is originated from since the function object knows that id
(is this what you had in mind?) but the problem is that there is no way to distinguish a simple bound native method from these functions returned from xrays. I'm not sure that is actually a problem, right now I _think_ not... So this might even actually work, although it's a "bit" messy.

Comment 10

[Bobby Holley (:bholley)]

So, I think we should only fix this for new bindings. In particular, XHR and WebSocket are already on the new bindings. If there's other stuff for which this is a problem, I'd rather expend energy converting those to new bindings rather than mucking around with XPC_WN_CallMethod etc.

Furthermore, I think we already need [1] a mechanism such that, given a callable JSObject *foo, we can determine if it actually corresponds to a native DOM method, and if so, which one (we already have tables for this kind of stuff behind the scenes to implement Xrays, so we just need a way to get to the appropriate table entry here).

[1] - We need such a mechanism so that we can fix bug 658909. Currently we rely on Xray methods being bound to make XOWs work. I want to change this so that we can dynamically determine whether a given native function is allowed to unwrap its |this| argument (by creating a link from the JSFunction to the associated NativeProperties entry, and annotating origin information in NativeProperties). This would allow people to call/apply any instance of window.postMessage onto a XOW and have it do the right thing.

However, the best way to get from the JSFunction to the NativeProperties entry isn't really clear to me. Maybe we could add a flag to JS_NewFunction that uses a different JSClass for the Function that has a reserved slot pointing to the correct entry? bz, what do you think?

Comment 11

[Boris Zbarsky [:bzbarsky]]

As far as this exact bug goes, Peter's work on Xrays for DOM interface objects and constructors is likely relevant.

As far as the rest, we can always create our functions with the two extra reserved slots jsapi already allows.  The problem is that this won't tell us whether this is one of _our_ functions.

That all said, I don't understand the problem being described. Bug 658909 shouldn't be an issue with new bindings, I would think....

Comment 12

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky (:bz) from comment #11)

> As far as the rest, we can always create our functions with the two extra
> reserved slots jsapi already allows.

Hm, I didn't know about this. How do we get the extra slots?

> The problem is that this won't tell us
> whether this is one of _our_ functions.

Presumably we could just add something to JSFunction::flags

> That all said, I don't understand the problem being described. Bug 658909
> shouldn't be an issue with new bindings, I would think....

The issue is that bound Xray methods are the only thing that make our cross-origin access story work (that's the only way you can invoke a method with a XOW as the |this| binding). So when we fix that, we need another solution for XOWs, and the best one I can think of is that the JSNative should do a dynamic lookup of the NativeProperties entry and determine whether it's allowed to be invoked cross-origin.

Comment 13

[Gabor Krizsanits (INACTIVE)]

(In reply to Bobby Holley (:bholley) from comment #10)
> So, I think we should only fix this for new bindings. In particular, XHR and
> WebSocket are already on the new bindings. If there's other stuff for which
> this is a problem, I'd rather expend energy converting those to new bindings
> rather than mucking around with XPC_WN_CallMethod etc.

I do think that this is mainly important for dom stuff, so I agree. However I don't think that the problem/solution there would differ too much...

> 
> Furthermore, I think we already need [1] a mechanism such that, given a
> callable JSObject *foo, we can determine if it actually corresponds to a
> native DOM method, and if so, which one (we already have tables for this
> kind of stuff behind the scenes to implement Xrays, so we just need a way to
> get to the appropriate table entry here).

Ok so maybe this part is different. It is really not clear to me how can one determine given a wrapped native, or in this case a native dom object, that which compartment does it belong to. Because even if it's not 'living' in one compartment strictly it is clearly originated from one. And this question will be pretty important here.

So if I understand it correctly we have the very same problem with the new bindings here. When we resolve a native prop on a dom node from content in the chrome compartment, we will get a function object living in the chrome compartment. And when we pass that object back to content compartment (where is is originated from) then we will wrap it within a chrome object wrapper, which is quite bad. (if this is not true please explain me what happens and ignore the rest of this comment)

> 
> [1] - We need such a mechanism so that we can fix bug 658909. Currently we
> rely on Xray methods being bound to make XOWs work. I want to change this so
> that we can dynamically determine whether a given native function is allowed
> to unwrap its |this| argument (by creating a link from the JSFunction to the
> associated NativeProperties entry, and annotating origin information in
> NativeProperties). This would allow people to call/apply any instance of
> window.postMessage onto a XOW and have it do the right thing.

Hmm... I thought that this already works for the new bindings? I'm interested in this topic for sure.

> 
> However, the best way to get from the JSFunction to the NativeProperties
> entry isn't really clear to me. Maybe we could add a flag to JS_NewFunction
> that uses a different JSClass for the Function that has a reserved slot
> pointing to the correct entry? bz, what do you think?

So I think the 2 slots we have here is not really enough. But I guess we can extend that by storing a pointer to some struct in one of them. I miss at least one piece of information here, the compartment from which the corresponding native/dom object was originated from, this particular function object was acquired from. So when we pass back this function object into it's original compartment, or to some other with the same (or stronger) principal than the original one, we do something else than simply wrapping it into a chrome only wrapper.

The other part I don't get is identity. I would expect that if I resolve a prop on it's own compartment on some dom object, and then from the chrome compartment and passing it back to the original compartment, the two object should be ===. Any idea how will we deal with this problem?

Comment 14

[Bobby Holley (:bholley)]

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #13)
> (In reply to Bobby Holley (:bholley) from comment #10)
> > So, I think we should only fix this for new bindings. In particular, XHR and
> > WebSocket are already on the new bindings. If there's other stuff for which
> > this is a problem, I'd rather expend energy converting those to new bindings
> > rather than mucking around with XPC_WN_CallMethod etc.
> 
> I do think that this is mainly important for dom stuff, so I agree. However
> I don't think that the problem/solution there would differ too much...

The difference between XPCWN Xrays and DOM Xrays is that we implement XPCWN Xrays by just instantiating an XPCCallContext and going through a bunch of IDL goop, whereas for the new DOM bindings we actually generate a static table of the relevant functions (grep for NativeProperties on dom/bindings). So it'll be much easier to add annotations for DOM Xrays, because we just stick what we want in CodeGen.py and python spits it out into the generated code.

> Ok so maybe this part is different. It is really not clear to me how can one
> determine given a wrapped native, or in this case a native dom object, that
> which compartment does it belong to.

Quite easily, actually. New DOM objects are wrapper-cached, meaning that they have one JS object per C++ object (everyone else gets a cross-compartment wrapper). XPCWN objects, on the other hand, might go either way, depending on the result of PreCreate. This is another important difference between the two types of Xrays here.

> So if I understand it correctly we have the very same problem with the new
> bindings here. When we resolve a native prop on a dom node from content in
> the chrome compartment, we will get a function object living in the chrome
> compartment.

Yes.

> And when we pass that object back to content compartment (where
> is is originated from) then we will wrap it within a chrome object wrapper,
> which is quite bad. (if this is not true please explain me what happens and
> ignore the rest of this comment)

This is how things work _now_. I'm proposing that we solve this problem (for new DOM bindings) by checking (in Rewrap()) whether the object is actually a native DOM method. If it is, and it doesn't have an Xray waiver, and it's not chrome-only, then we just re-resolve it using the Xray tables in the content compartment.

> Hmm... I thought that this already works for the new bindings? I'm
> interested in this topic for sure.

It does. But it will be a problem once we move something that's cross-origin accessible (Window or Location) to the new bindings, because the bound Xray methods are the only thing that currently allows cross-origin access to work.

> So I think the 2 slots we have here is not really enough.

I think one is enough, actually.

> But I guess we can
> extend that by storing a pointer to some struct in one of them.

Whenever we do something like this, it's much better to store a reference to a JSObject* than to a struct, because the former benefits from automatic garbage collection.

> I miss at
> least one piece of information here, the compartment from which the
> corresponding native/dom object was originated from, this particular
> function object was acquired from.

We already know which object the Xray-bound method comes from, via the parent chain. And once we have that, we know the canonical compartment, as mentioned earlier.

> The other part I don't get is identity. I would expect that if I resolve a
> prop on it's own compartment on some dom object, and then from the chrome
> compartment and passing it back to the original compartment, the two object
> should be ===. Any idea how will we deal with this problem?

That's an interesting question. In general, content can pollute its native functions as much as it wants, and the Xray representation needs to be clean. Now, we could make the argument that if chrome gets a clean Xray view of contentWindow.foo and passes it back to the content, the function should re-resolve itself into the local/dirty version. This would be hard for us to implement though, because content is allowed to delete methods off its prototypes and whatnot, so we don't have anywhere to find the canonical JSFunctions (just the canonical JSNatives, which we find in the table). I don't have any great ideas here - we should see if bz has any.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Once bug 778152 lands, we should retest here and see what else, if anything, needs to be done.

Comment 16

[Mark Hammond [:markh] [:mhammond]]

FTR, bug 658909 and bug 841067 introduce changes to the work arounds slightly - but as they still need special-casing, this bug remains open.

Comment 17

[Mark Hammond [:markh] [:mhammond]]

Bug 896860 is exposing 'Worker' to our FrameWorker and needs the same workaround, so expanding the scope to include it.

Comment 18

[Boris Zbarsky [:bzbarsky]]

Can someone clearly describe what the remaining issues here are, if any.  An attachment with actual code that fails, ideally.

Comment 19

[Mark Hammond [:markh] [:mhammond]]

(In reply to Not doing reviews right now from comment #18)
> Can someone clearly describe what the remaining issues here are, if any.  An
> attachment with actual code that fails, ideally.

Shane, this was about social, right? Any chance you can work out what xhr hacks remain and are necessary? IIRC there's good xhr coverage in existing tests.

Comment 20

[Shane Caraveo (:mixedpuppy)]

Mark, it's been a long time since I've looked at that stuff, so not entirely confident in what I tested.  I removed the needsWaive array, and all the xpcshell and mochitest-browser tests pass on osx.  Here's a try for more platform coverage.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=507ebac2329f

Can you take a look at the [hacky] patch, see if it joggles any memory for you?

Comment 21

[Mark Hammond [:markh] [:mhammond]]

Thanks Shane,

I think this demonstrates that nothing else needs to be done from the DOM POV, and that we should create another social bug to remove those work-arounds. The test coverage we have for XHR and WebSockets are enough to have picked up this problem if it still exists (less so coverage for Workers, but I see no reason to believe that's different, nor that it's actually used!)

So I'm closing this as WFM and opened bug 1153029 for the removal of the workarounds in social