1164188 - Expose the XMLHttpRequest (XHR) constructor in system scopes by default

Status: RESOLVED DUPLICATE of bug 1489301

(Core :: DOM: Core & HTML, defect, P5)

Description

[Boris Zbarsky [:bzbarsky]]

It can already be imported, so there's no reason not to do it.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Hrm.  So maybe we can't do this because of addons.  :(  There's "const XMLHttpRequest" all over addon code.  :(

Mostly from "resources/api-utils/lib/self!.js" and "boostrap.js", which I think is addon SDK stuff, so _maybe_ wouldn't be an issue?  But there are some other uses too, looks like.  This is why we can't have nice things.

Once bug 589199 is fixed we can do this.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch for posterity

Comment 3

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Bug 589199 seems to have landed

Comment 4

[Boris Zbarsky [:bzbarsky]]

Yeah, need to test that the way that was actually fixed for addons will still work with the "const XMLHttpRequest" stuff...  I'm not quite sure how to go about doing that, and this is pretty low-priority for me.  If someone wants to do the testing and get the patch in, please feel free to steal.

Comment 5

[Masatoshi Kimura [:emk]]

All add-ons using "const XMLHttpRequest" should have been rewritten due to global lexical scope for let/const. If not, it is already broken.

Comment 7

[Thomas Wisniewski [:twisniewski]]

Is this still something we want to do, bz? (I'm not sure if it squares with our efforts to minimize memory usage).

Comment 8

[Boris Zbarsky [:bzbarsky]]

So first off [Exposed=System] adds a bit of data to the table used by ResolveSystemBinding.  It doesn't really cause memory usage other than that if the global in question doesn't use the relevant binding.

Past that, [Exposed=System] exposes the API in three places:

* ContentFrameMessageManager globals
* ProcessGlobal globals
* BackstagePass globals

The first two are going to go away in bug 1480244.  For BackstagePass, it looks like we create globals using it in:

1) ipc::XPCShellEnvironment::Init
2) mozJSComponentLoader::CreateLoaderGlobal
3) XRE_XPCShellMain

We don't really care about #1 and #3.  CreateLoaderGlobal can be called from:

a) mozJSComponentLoader::GetSharedGlobal
b) mozJSComponentLoader::PrepareObjectForLocation

There's only one shared global.  PrepareObjectForLocation uses the shared global unless ReuseGlobal(aURI) returns false.  Since we set pref("jsloader.shareGlobal", true), we use the shared global for everything except "resource://gre/modules/commonjs/toolkit/loader.js", resource://gre/modules/jsdebugger.jsm, and resource://specialpowers/*.

In a release build I expect we don't have specialpowers bits.  The "resource://gre/modules/commonjs/toolkit/loader.js" thing no longer exists.  So apart from the shared global we'd only need to worry about exposing in jsdebugger.jsm...

I suspect we should in fact just expose this to System.  Kris, any objections?

Comment 9

[Kris Maglione [:kmag]]

Agreed, I can't think of any reason not to do this. If anything, it will probably save memory by preventing people from eagerly importing it before it's needed.

Comment 10

[Thomas Wisniewski [:twisniewski]]

That all sounds good to me. Is there a reason to not try landing the patch, then?

Comment 11

[Boris Zbarsky [:bzbarsky]]

Try run of patch plus some cleanup is at https://treeherder.mozilla.org/#/jobs?repo=try&revision=cb8be8f421554bbe50aaad92116d25827ada3fe8

Once I make sure it's green I will post things for review.

Comment 12

[Boris Zbarsky [:bzbarsky]]

OK, so the try run is very orange, because if I expose XHR to System, then the Window-only-exposed .responseXML getter is _not_ exposed to System and getting .responseXML from XHRs fails in system scopes.

And exposing that getter to System requires that its return type (Document) be exposed to System.

And that requires, by the same argument, that the return types of all properties of Document, and all consequential interfaces of all those types, and all the return types of those, etc, be exposed to System.

So we end up needing to expose a lot of things to System.  I'll post a patch that does that, in case we want to.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Another option would be to just expose everything that's exposed in Window in system scopes....

Comment 14

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Expose the XMLHttpRequest constructor in system scopes

Comment 15

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Stop importing the XMLHttpRequest property in xpcshell test files

xpcshell has this by default now.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Attachment: part 3. Stop importing the XMLHttpRequest property in JSMs

The JSM global has this by default now.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Attachment: part 3. Stop importing the XMLHttpRequest property in JSMs

The JSM global has this by default now.

Comment 18

[Boris Zbarsky [:bzbarsky]]

This got fixed in bug 1489301.