756173 - XMLHttpRequest sandbox issues

Status: RESOLVED WORKSFORME

(Firefox Graveyard :: SocialAPI, defect)

Description

[Shane Caraveo (:mixedpuppy)]

We're hitting multiple problems in making XHR available to the frameworker sandbox.  

1. on osx, it seems like the iframe doesn't always have XHR, so we are unable to add it to the sandbox.
2. we're having to hack the XHR class to make it work (when it is avialable):

workerWindow.XMLHttpRequest.__exposedProps__ = [];

3. if we get past the above issues, then XHR only works with CORS enabled servers, it will not communicate back to the principle server the sandbox is created with.

Comment 1

[Shane Caraveo (:mixedpuppy)]

more background.  we're NOT using the iframe window as the sandboxPrototype and are trying to import specific functions and classes into the sandbox.  We're doing this in order to have a sandbox that roughly matches a web worker.  We cannot use a real worker because we need access to several APIs not available to the fx implementation of workers.

One fix is to simply give up on that, and use the iframe window as the sandboxPrototype.  This fixes our XHR problem, but also introduces the possibility that we will not be able to switch over to worker in the future.

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Some more information, please. What is a frameworker sandbox?
A testcase attached to this bug would be good.

Comment 3

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

Attachment: mochitest-browser-chrome test demonstrating the problem

Attaching a browser test case that demonstrates the problem.  At the risk of telling people how to suck eggs, once applied the test can be run from your obj-dir by executing:

% make -C dom/base && TEST_PATH=dom/base make -C . mochitest-browser-chrome

This test simulates what our "frameworker" does - it creates a hidden window attached to an origin, then creates a sandbox based on that window and evaluates code in that sandbox (the "worker" simulation)

It demonstrates 2 problems:

* There is a line:
  workerWindow.XMLHttpRequest.__exposedProps__ = [];

  which is necessary to get XMLHttpRequest working at all - without this we end up with a wrapper that fails to grant access to the object, so "new XMLHttpRequest()" returns undefined.

* If this hack is in place, XMLHttpRequest *almost* works, but doesn't respect the same-origin policy.  The code works as expected if you change the url to, say "http://enable-cors.org/" as that server supports CORS.  We expect it to work as written as it is attempting to fetch the exact same URL as the window has so should be in the same origin.

Comment 4

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

I meant to add - the test fails with:

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/dom/base/test/browser_xhr_sandbox.js | check the sandbox code was happy - Got ERROR: got request status of 0, expected ok

indicating the XHR completed with req.status == 0, which has all the hallmarks of being rejected due to lack of CORS headers.

Comment 5

[Shane Caraveo (:mixedpuppy)]

> * There is a line:
>   workerWindow.XMLHttpRequest.__exposedProps__ = [];


This seems to only work on windows, because, if you create multiple "frameworkers", XMLHttpRequest is not even available after the first.

Comment 6

[Bobby Holley (:bholley)]

(In reply to Mark Hammond (:markh) from comment #3)

> * There is a line:
>   workerWindow.XMLHttpRequest.__exposedProps__ = [];


> * If this hack is in place, XMLHttpRequest *almost* works, but doesn't
> respect the same-origin policy.  The code works as expected if you change
> the url to, say "http://enable-cors.org/" as that server supports CORS.  We
> expect it to work as written as it is attempting to fetch the exact same URL
> as the window has so should be in the same origin.

Wait, is the worker being attached to a chrome window or to a content window? __exposedProps__ only makes sense for chrome objects being exposed to content.

More importantly, this hack doesn't make much sense to me:
1 - COWs are always callable anyway, regardless of __exposedProps__
2 - This code effectively grants zero permissions.

Also, FWIW, __exposedProps__ is supposed to be an object, not an array.

Comment 7

[Shane Caraveo (:mixedpuppy)]

(In reply to Bobby Holley (:bholley) from comment #6)
> (In reply to Mark Hammond (:markh) from comment #3)
> 
> > * There is a line:
> >   workerWindow.XMLHttpRequest.__exposedProps__ = [];
> 
> 
> > * If this hack is in place, XMLHttpRequest *almost* works, but doesn't
> > respect the same-origin policy.  The code works as expected if you change
> > the url to, say "http://enable-cors.org/" as that server supports CORS.  We
> > expect it to work as written as it is attempting to fetch the exact same URL
> > as the window has so should be in the same origin.
> 
> Wait, is the worker being attached to a chrome window or to a content
> window? 

content window.  

> __exposedProps__ only makes sense for chrome objects being exposed
> to content.
> 
> More importantly, this hack doesn't make much sense to me:
> 1 - COWs are always callable anyway, regardless of __exposedProps__
> 2 - This code effectively grants zero permissions.
> 
> Also, FWIW, __exposedProps__ is supposed to be an object, not an array.

until Mark changed exposedProps, "new XMLHttpRequest()" returned undefined.

Once he changed it, he got back a request object that ONLY worked with CORS enabled.  The hack shouldn't make sense, but it works around a bug.

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Did you try ´new somewindowobject.XMLHttpRequest();´ ?

Comment 9

[Shane Caraveo (:mixedpuppy)]

(In reply to Olli Pettay [:smaug] from comment #8)
> Did you try ´new somewindowobject.XMLHttpRequest();´ ?

the window is intentionally not available to the sandbox, so no, we cannot do that.

Comment 10

[Bobby Holley (:bholley)]

I finally looked into the __exposedProps__ stuff. For some inexplicable reason, the wrapper code treats |construct| differently from |call|. I'm going to see if I can fix that.

Comment 11

[Bobby Holley (:bholley)]

Also, sorry this took so long - I've been pretty swamped these days :-(

Comment 12

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

(In reply to Bobby Holley (:bholley) from comment #11)
> Also, sorry this took so long - I've been pretty swamped these days :-(

No problem - thanks for getting back to it!

Note the the __exposedProps__ thing is annoying, but is a work-around.  The more pressing issue from our POV is what seems to be the same-origin problems that happen trying to get the data after we've made the work-around :)

Comment 13

[Bobby Holley (:bholley)]

(In reply to Mark Hammond (:markh) from comment #12)
> Note the the __exposedProps__ thing is annoying, but is a work-around.  The
> more pressing issue from our POV is what seems to be the same-origin
> problems that happen trying to get the data after we've made the work-around
> :)

That's probably something you're going to want smaug/gabor's help with.

Comment 14

[Shane Caraveo (:mixedpuppy)]

__exposedProps__ is not really a workaround, since it only fixes things on windows.   Touching exposedProps on osx caused a js exception.

Comment 15

[Bobby Holley (:bholley)]

(In reply to Shane Caraveo (:mixedpuppy) from comment #14)
> __exposedProps__ is not really a workaround, since it only fixes things on
> windows.   Touching exposedProps on osx caused a js exception.

Fix just landed FWIW.

Comment 16

[Gabor Krizsanits (INACTIVE)]

So part 5 of bug 734891 will add an optional XHR constructor to sandbox, and I will soon land the patch, but it will still take some time util it lands. On the other hand this is a case that I think jetpack folks already solved so I CC Alex.

And just out of curiosity, if you try creating the XHR first and export that to the sandbox instead of the constructor... does that change anything? I just don't know if the constructor when exported will be bound to the window object or not, and if not maybe the XHR object will be initialized differently...

Comment 17

[Alexandre Poirot [:ochameau]]

We are doing different things in Jetpack. We are using sandboxPrototype and the sandbox global object is a proxy mapping to the content window.

Having said that, I'm wondering if there is any difference between:
  sandbox.importFunction(workerWindow.XMLHttpRequest, "XMLHttpRequest");
and
  sandbox.XMLHttpRequest = workerWindow.XMLHttpRequest;
or
  var sandbox = Cu.Sandbox(workerWindow, {XMLHttpRequest: workerWindow.XMLHttpRequest});
?

(we are closer to the last pattern in jetpack)

Finally, I was wondering which XHR contructor is going to be called:
- nsXMLHttpRequest::Init()
http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsXMLHttpRequest.cpp#488
- nsXMLHttpRequest::Init(nsIPrincipal* ,nsIScriptContext*,
                         nsPIDOMWindow*, nsIURI* aBaseURI)
http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsXMLHttpRequest.cpp#529
- nsXMLHttpRequest::Initialize(nsISupports*, JSContext*, JSObject*,
                               PRUint32 , jsval*)
http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsXMLHttpRequest.cpp#545

I'd imagine that this bug is related to one of these methods fetching an unexpected global window or script principal?
We are used to face similar bugs when using multiple DOM features in sandboxes, where it is always expected that the global object is nsIPDOMWindow:
http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsXMLHttpRequest.cpp#517
(for example, bug 733035)

Comment 18

[Bobby Holley (:bholley)]

importFunction is unnecessary since FF4.

Comment 19

[scaraveo]

(In reply to Bobby Holley (:bholley) from comment #15)
> (In reply to Shane Caraveo (:mixedpuppy) from comment #14)
> > __exposedProps__ is not really a workaround, since it only fixes things on
> > windows.   Touching exposedProps on osx caused a js exception.
> 
> Fix just landed FWIW.

I just ran a test against this fix, I still see the same issue.

Comment 20

[scaraveo]

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #16)
> And just out of curiosity, if you try creating the XHR first and export that
> to the sandbox instead of the constructor... does that change anything? I
> just don't know if the constructor when exported will be bound to the window
> object or not, and if not maybe the XHR object will be initialized
> differently...

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.

Comment 21

[Bobby Holley (:bholley)]

(In reply to scaraveo from comment #19)
> I just ran a test against this fix, I still see the same issue.

Do you mean that the __exposedProps__ = {} hack is still necessary? Or that the test still fails? The latter is expected, the former would be very surprising.

Comment 22

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

For me, it's the latter.  It looks like bug 734891 is what we now need.

Comment 23

[Shane Caraveo (:mixedpuppy)]

ochameau helped me find a solution, using iframe.contentWindow.wrappedJSObject fixes this issue:

let Cu = Components.utils;

let proto = {
  XMLHttpRequest: content.wrappedJSObject.XMLHttpRequest,
  location: String(content.location)
};
let s = Cu.Sandbox(content, {sandboxPrototype: proto});

Cu.reportError(Cu.evalInSandbox("var xhr = new XMLHttpRequest(); xhr.open('GET', location, false); xhr.send(); xhr.responseText", s));

Comment 24

[Alexandre Poirot [:ochameau]]

So it looks like the real issue is that XMLHttpRequest disappeared from window object xraywrappers, but not all xraywrappers ... only xrays made for system principal. It still works fine for xrays made for content principal.

As always, here is a scratchpad test case:
  var Cu = Components.utils;
  var s = Cu.Sandbox(content, {sandboxPrototype: {window: content}});
  Cu.reportError("from chrome: " + content.XMLHttpRequest);
  Cu.reportError("from content: " + Cu.evalInSandbox("window.XMLHttpRequest", s));
  // Just to ensure that we have an xray. Yes, we have one!
  Cu.reportError("isxray: " + Cu.evalInSandbox("window.wrappedJSObject", s));

content.XMLHttpRequest is undefined from browser.xul context,
but still works fine for window.XMLHttpRequest, in sandbox context.
That difference explain why Jetpack is still working fine whereas Shane and Mark are facing issues.

Comment 25

[Blake Kaplan (:mrbkap) (inactive)]

How do we attach new DOM binding constructors to the global object? One possibility is that they're not visible to Xray wrappers.

Comment 26

[Boris Zbarsky [:bzbarsky]]

> How do we attach new DOM binding constructors to the global object

Just via the resolve hook, iirc.  See also bug 741267.  I guess the tests there don't actually use xrays?

Comment 27

[Shane Caraveo (:mixedpuppy)]

I'm going to close this, we've solved our problem by:

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

This is part of the implementation in bug 762569.