817284 - new dom binding for windowless xhr events are broken

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Gabor Krizsanits (INACTIVE)]

I set the priority for critical because I think this is already in aurora, and it will break jetpack features if we don't fix it. Also I'm not sure that context are used properly (or at least I don't see the logic)

The problem is that if the global of an XHR instance (or later the same will be true for websocket and indexeddb) and the callback we assign to 'onload' is from the same scope, CallbackFunction::CallSetup::CallSetup will not be able to fetch a context from it, and will use the safe jscontext. The compartment is not set for the ctx and the security check fails (which makes no sense since the callback is from the very same scope as the XHR instance...). It also bothers me what guarantees in the regular case that the context we get from the global in the regular case operates on the right compartment.

Btw onprogress seemed to work fine, but it uses a bit different binding mechanism.

Example code in scratchpad (executed in browser context)
var cu = Components.utils;
var sb = cu.Sandbox("http://w3schools.com",
                    {wantXHRConstructor: true});
           
function createXHR(loc, async)
{
  var xhr = new XMLHttpRequest();
  xhr.open("GET", loc, async);
  return xhr;
}           

// Test async XHR sending
cu.evalInSandbox('var createXHR = ' + createXHR.toString(), sb);  

// sorry for the long string here...
var async = cu.evalInSandbox('var async = createXHR("http://w3schools.com/XML/cd_catalog.xml", true);async.onload=function(e){throw "loaded"};async', sb);

async.send(null);

Comment 1

[Boris Zbarsky [:bzbarsky]]

> and will use the safe jscontext.

Right.  So far things should be fine.

> The compartment is not set for the ctx 

"ctx" as in nsIScriptContext (what "ctx" usually means)?  There should be no ctx anywhere around the safe JS context.  Or do you mean the "cx" (as in the safe context)?  That should be in whatever compartment it's in at this point...

> and the security check fails

Hmm.  Which part fails?  That security check shouldn't depend on the compartment of aCx, last I checked.  Is CanExecuteScripts returning false because there is no nsIScriptContext and the function is not system?

Let me try your testcase.

Comment 2

[Boris Zbarsky [:bzbarsky]]

> Btw onprogress seemed to work fine, but it uses a bit different binding mechanism.

Er, it does?

For the rest, the function being called is not system, because it was created in the sandbox.  And the safe context has no script context, so we don't know whether script should be enabled for it and assume it should not.

So what we used to do was call SetEventHandlerToJsval with the XHR object as the "scope".  Then we did nsJSUtils::GetStaticScriptContext on the XHR's JS object to get the nsIScriptContext and JSContext to work with.  That calls GetStaticScriptGlobal() and that knows about sandboxes so would have gotten the right global, and in particular the right nsIScriptContext.

But the new code is _also_ using GetStaticScriptGlobal.  So what gives?

Comment 3

[Boris Zbarsky [:bzbarsky]]

Ah, this sandbox is using a principalholder, not a window.  So then I have no idea how this used to work.  I'll try stepping through in a build from before bug 807226, I guess.

But note that afaict the code for onprogress and onload should be identical, so I'm not sure how you're seeing onprogress work if onload doesn't.

Comment 4

[Boris Zbarsky [:bzbarsky]]

OK.  So in a build from before bug 807226 I see nsEventListenerManager::SetEventHandlerToJsval end up with a null nsIScriptContext, just like we do.  And I also see it have a false value for aExpectScriptContext, which is why it adds the handler after all.

Then we land in SetEventHandlerInternal, which goes ahead and creates and nsIDOMEventListener if there is no nsIScriptContext, which will go through XPConnect dispatch and not do the "is script allowed to run here" checks that CallEventHandler does.

I guess that's the simple way to go.  I'll cook up a patch.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: Something to play with

Still need to write a test

Comment 6

[Gabor Krizsanits (INACTIVE)]

(In reply to Boris Zbarsky (:bz) from comment #2)
> > Btw onprogress seemed to work fine, but it uses a bit different binding mechanism.
> 
> Er, it does?

Uh... sorry about this part I tried to reproduce it and couldn't I must have overlooked something during playing with various tests... 

> "ctx" as in nsIScriptContext (what "ctx" usually means)?  There should be no
> ctx anywhere around the safe JS context.  Or do you mean the "cx" (as in the
> safe context)?  That should be in whatever compartment it's in at this
> point...

I was not aware that differentiation. I meant "cx".

(In reply to Boris Zbarsky (:bz) from comment #5)
> Created attachment 687409 [details] [diff] [review]
> Something to play with
> 
> Still need to write a test

Awesome, thanks! I'm not sure I'll have time writing test this weekend but the latest on Monday I'm sure I will.

Comment 7

[Gabor Krizsanits (INACTIVE)]

Attachment: test v0

So I started writing a test for it, and the patch seem to work, however I ran into another problem... While trying to access the dom of the response xml. 

There is a check in nsNodeSH::PreCreate with some comment that fails:

  NS_ENSURE_STATE(doc->GetScriptHandlingObject(hasHadScriptHandlingObject) ||
                  hasHadScriptHandlingObject ||
                  IsPrivilegedScript());

http://mxr.mozilla.org/mozilla-central/source/dom/base/nsDOMClassInfo.cpp#7834

I did not yet have the time to investigate it, have to run for a meeting now...

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch with test

Comment 9

[Bobby Holley (:bholley)]

Comment on attachment 687835 [details] [diff] [review]
Patch with test

Given that I still think the call to CheckFunctionAccess is useless, you can do what you want here. :-)

Comment 10

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/54d8ca229142

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 687835 [details] [diff] [review]
Patch with test

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 807226 
User impact if declined: Breaks some Jetpack features.
Testing completed (on m-c, etc.): Passes additional automated tests, manual
   testing.
Risk to taking this patch (and alternatives if risky): I believe this is low-risk.
String or UUID changes made by this patch: None.

Comment 12

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/54d8ca229142

Comment 13

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/986a71639980

Comment 14

[Paul Silaghi, QA [:pauly]]

Is there something QA can do to verify this fix ?

Comment 15

[Boris Zbarsky [:bzbarsky]]

Paul, worth trying the steps in comment 0.

Comment 16

[Paul Silaghi, QA [:pauly]]

I'm going to need some more specific steps in order to reproduce this. And some actual/expected results to follow, I'm not clear about the main issue here.

Comment 17

[Gabor Krizsanits (INACTIVE)]

1. open up scratchpad
2. select environment->browser
(in case you don't have this option you might have to set a flag in about:config)
3. copy+paste+execute the script
4. open up error console

expected:
You should see an exception there: "loaded"

Comment 18

[Paul Silaghi, QA [:pauly]]

Thanks Gabor.
No error seen in error console before the fix (nightly 2012-12-05).
"Error: uncaught exception: loaded" in FF 19b5 on Win 7, Ubuntu 12.04 and Mac OS X 10.7.5. Verified fixed