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




4 years ago
3 months ago


(Reporter: bzbarsky, Assigned: bzbarsky)



Firefox Tracking Flags

(firefox41 affected)



(3 attachments, 2 obsolete attachments)

It can already be imported, so there's no reason not to do it.
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.
Depends on: 589199
Created attachment 8604914 [details] [diff] [review]
Patch for posterity
Assignee: nobody → bzbarsky
Bug 589199 seems to have landed
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.
All add-ons using "const XMLHttpRequest" should have been rewritten due to global lexical scope for let/const. If not, it is already broken.


a year ago
Duplicate of this bug: 439286


7 months ago
Priority: -- → P5
Is this still something we want to do, bz? (I'm not sure if it squares with our efforts to minimize memory usage).
Flags: needinfo?(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?
Flags: needinfo?(bzbarsky) → needinfo?(kmaglione+bmo)
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.
Flags: needinfo?(kmaglione+bmo)
That all sounds good to me. Is there a reason to not try landing the patch, then?
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.
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.
Another option would be to just expose everything that's exposed in Window in system scopes....
Created attachment 9001479 [details] [diff] [review]
part 1.  Expose the XMLHttpRequest constructor in system scopes


5 months ago
Attachment #8604914 - Attachment is obsolete: true
Created attachment 9001480 [details] [diff] [review]
part 2.  Stop importing the XMLHttpRequest property in xpcshell test files

xpcshell has this by default now.
Created attachment 9001481 [details] [diff] [review]
part 3.  Stop importing the XMLHttpRequest property in JSMs

The JSM global has this by default now.
Created attachment 9001592 [details] [diff] [review]
part 3.  Stop importing the XMLHttpRequest property in JSMs

The JSM global has this by default now.


5 months ago
Attachment #9001481 - Attachment is obsolete: true
This got fixed in bug 1489301.
Last Resolved: 3 months ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1489301
You need to log in before you can comment on or make changes to this bug.