1137591 - crash in xul.dll@0x1a42790 | SandboxCreateXMLHttpRequest trying to Cu.importGlobalProperties XMLHttpRequest onto a Window and then use it

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

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

Attachment: readinglist-auth.js

This bug was filed from the Socorro interface and is 
report bp-2faefff1-db61-4ba7-ade1-af5e62150227.
=============================================================

On a locally built nightly and from the normal nightly channel I can reliably crash when I execute the attached file in a "browser" scratchpad.

Comment 1

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

oops - I meant to mention that the script assumes a Firefox Account user is logged in :(  I doubt it's related to that though.

Comment 2

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

Attachment: crashing-xhr.js

A version with no FxA requirements.

Comment 3

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

mozregression tells me:

> Got as far as we can go bisecting nightlies...
> Ensuring we have enough metadata to get a pushlog...
> Last good revision: cbe4f69c2e9c (2014-05-27)
> First bad revision: e017c15325ae (2014-05-28)
> Pushlog:
> https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=cbe4f69c2e9c&tochange=e017c15325ae
> ...
> Oh noes, no (more) inbound revisions :(
> do you want to bisect further by fetching the repository and building? (y or n) n

Given the age I didn't bother going further.

Comment 4

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

I've only a "release" build handy, but in my debugger, the object passed to QI at http://hg.mozilla.org/mozilla-central/annotate/df3daecd381f/js/xpconnect/src/Sandbox.cpp#l207 appears to have a garbage vtable

Comment 5

[Boris Zbarsky [:bzbarsky]]

This script is doing this:

  Cu.importGlobalProperties(["XMLHttpRequest"]);

This is meant to be used inside sandboxes and maybe JSMs, but the script is being run in a window global.  We should probably make importGlobalProperties throw in that situation to start with.

Anyway, moving right along....  GlobalProperties::Define for the XMLHttpRequest case defines SandboxCreateXMLHttpRequest as the ctor.  This function in assumes that the private value of the global is going to be an nsISupports.  But Web IDL objects, including Window, have no private value, so in a debug build you get a nice assert and in an opt build you get garbage.

Since we do want to move at least backstagepass to Web IDL, if not sandboxes, we should also change SandboxCreateXMLHttpRequest to handle this situation.

That said, I wonder why we have SandboxCreateXMLHttpRequest at all.  Why can we not use the normal XHR constructor in sandboxes/jsms?  Or can we at this point?

Comment 6

[Boris Zbarsky [:bzbarsky]]

And of course the regression range here is when we turned on webidl bindings for window.

Comment 7

[Bobby Holley (:bholley)]

(In reply to Not doing reviews right now from comment #5)
> This script is doing this:
> 
>   Cu.importGlobalProperties(["XMLHttpRequest"]);
> 
> This is meant to be used inside sandboxes and maybe JSMs, but the script is
> being run in a window global.  We should probably make
> importGlobalProperties throw in that situation to start with.

Yes.

> Anyway, moving right along....  GlobalProperties::Define for the
> XMLHttpRequest case defines SandboxCreateXMLHttpRequest as the ctor.  This
> function in assumes that the private value of the global is going to be an
> nsISupports.  But Web IDL objects, including Window, have no private value,
> so in a debug build you get a nice assert and in an opt build you get
> garbage.
> 
> Since we do want to move at least backstagepass to Web IDL, if not
> sandboxes, we should also change SandboxCreateXMLHttpRequest to handle this
> situation.

Agreed.
> That said, I wonder why we have SandboxCreateXMLHttpRequest at all.  Why can
> we not use the normal XHR constructor in sandboxes/jsms?  Or can we at this
> point?

I believe it predates WebIDL XHR. It's worth making sure that the WebIDL constructor doesn't expect Windows or Documents and works for System Principal though.

Comment 8

[Boris Zbarsky [:bzbarsky]]

OK, so the Web IDL ctor....

The binding part doesn't expect anything interesting, obviously.

nsXMLHttpRequest::Constructor expects an nsIGlobalObject and nsIScriptObjectPrincipal, which should be fine.  There _will_ be a slight behavior change though: the Web IDL ctor will use the principal of the global the ctor lives in (so of the sandbox) while SandboxCreateXMLHttpRequest uses the subject principal (which could in theory be different if we can have Xrays into a sandbox via which we invoke the ctor, right?).  Seems to me that this behavior change is OK.

Also, the subject principal and current compartment in the XHR C++ code will be the sandbox, not the maybe-Xray caller, in the new setup.  It doesn't look to me like XHR ever uses those, so it's ok.

Apart from that, both the current codepath via the 5-arg form of nsXMLHttpRequest::Init with null passed for the last two args and the new codepath via the web idl ctor end up doing exactly the same things.

Comment 9

[Bobby Holley (:bholley)]

(In reply to Not doing reviews right now from comment #8)
> nsXMLHttpRequest::Constructor expects an nsIGlobalObject and
> nsIScriptObjectPrincipal, which should be fine.  There _will_ be a slight
> behavior change though: the Web IDL ctor will use the principal of the
> global the ctor lives in (so of the sandbox) while
> SandboxCreateXMLHttpRequest uses the subject principal (which could in
> theory be different if we can have Xrays into a sandbox via which we invoke
> the ctor, right?).  Seems to me that this behavior change is OK.
> 
> Also, the subject principal and current compartment in the XHR C++ code will
> be the sandbox, not the maybe-Xray caller, in the new setup.  It doesn't
> look to me like XHR ever uses those, so it's ok.

Hm, why is that? Shouldn't WebIDL ctors do the proper Xray semantics? I seem to remember some fussing about this (you and me and peterv had a call about it 2 years ago), but I always forget what we settled on.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Get rid of SandboxCreateXMLHttpRequest and just use the normal XHR constructor in sandboxes if someone asks for XHR there

Comment 11

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Throw if someone does Cu.importGlobalProperties in a Window scope

Comment 12

[Boris Zbarsky [:bzbarsky]]

> but I always forget what we settled on.

We settled on ctors entering the compartment of the global they live in and wrapping all arguments into that compartment.

Comment 13

[Boris Zbarsky [:bzbarsky]]

OK, the throwing bit is ... troublesome.  I get test failures from some mochitest-chrome tests that include specialpowersAPI.js (which uses importGlobalProperties).  There are also explicit calls to importGlobalProperties in other mochitest-chrome tests...

I'll see if I can fix the tests to stop doing this.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Attachment: Test fixes to not importGlobalProperties in a Window

Comment 15

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d4f1b7962af4
https://hg.mozilla.org/integration/mozilla-inbound/rev/2a034f03f773

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d4f1b7962af4
https://hg.mozilla.org/mozilla-central/rev/2a034f03f773