:Ms2ger (he/him; ⌚ UTC+1/+2) Reporter
	Description • 12 years ago

We should remove the members of nsIXMLHttpRequest, and their implementations in nsXMLHttpRequest; regardless of whether we keep the nsIXMLHttpRequest interface and the classinfo stuff around for now.

This will involve fixing up a few in-tree consumers.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 1 • 12 years ago

Binary addons use nsIXMLHttpRequest. We should not break them.

	Boris Zbarsky [:bzbarsky]
	Comment 2 • 12 years ago

Hmm.  In that case, this is probably wontfix.  :(

That said, can we simplify the code (e.g. by not supporting ArrayBufferView for send() from C++) without breaking said addons?

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 3 • 12 years ago

We should rename nsIXMLHttpRequest, and introduce a gutted version (like nsIDOMWindowInternal).

	Thomas Wisniewski Assignee
	Comment 4 • 8 years ago

Since binary addons are gone, would this be a project that's worth tackling now? I'm refactoring the XHR code as it stands, and I suspect it would be good to do as well while I'm on the topic.

	Thomas Wisniewski Assignee
	Comment 5 • 8 years ago

I guess I should have needinfo'd someone here. Any fresh thoughts on this, guys?

	Boris Zbarsky [:bzbarsky]
	Comment 6 • 8 years ago

I expect doing this is fine now.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 7 • 8 years ago

Yeah, I believe too this should be fine. The couple of C++ users in tree will need to be changed to use
XMLHttpRequest and not nsIXMLHttpRequest, but static_cast and some small tweaks to method calls should be enough there. nsIXMLHttpRequest should be marked builtinclass.

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 8 • 8 years ago

Do it!

	Thomas Wisniewski Assignee
	Comment 9 • 8 years ago

So the goal is basically to change the IDL interface to this, and then work out what breaks? (or should the "scriptable" be dropped as well?):

    [builtinclass, scriptable, uuid(6f54214c-7175-498d-9d2d-0429e38c2869)]
    interface nsIXMLHttpRequest : nsISupports
    {};


So far I see one use of nsIXMLHttpRequest that I'm not sure how to deal with:
    toolkit/mozapps/update/nsIUpdateService.idl

It passes nsIXMLHttpRequest references around to JS code, and I'm not sure how to "cast" from an nsIXMLHttpRequest to an XMLHttpRequest object in JS.

	Boris Zbarsky [:bzbarsky]
	Comment 10 • 8 years ago

> or should the "scriptable" be dropped as well?

If you drop the "scriptable" you probably need to add the interface to the list in xpcom/reflect/xptinfo/ShimInterfaceInfo.cpp.  But yes, that might be a good idea.

>So far I see one use of nsIXMLHttpRequest that I'm not sure how to deal with:
>    toolkit/mozapps/update/nsIUpdateService.idl

Just replace nsIXMLHttpRequest with nsISupports and a comment there?

> and I'm not sure how to "cast" from an nsIXMLHttpRequest to an XMLHttpRequest object in JS.

You don't.  JS will just see an XMLHttpRequest object.  As far as JS is concerned, there is really no such thing as "nsIXMLHttpRequest" anymore.

	Thomas Wisniewski Assignee
	Comment 11 • 8 years ago

Thanks for the insight! Time for another question:

The tests using xpcshellUtilsAUS.js (like downloadInterruptedByOfflineRetry.js) use MockRegistrar to force nsIUpdateChecker to use a different version of XMLHttpRequest. As expected, attempts to simply replace it directly (like "gUpdateChecker.XMLHttpRequest=mockClass") complain that I'm trying to modify a WrappedNative. Is there some other way that I could mock it?

I've tried modding the nsIUpdateChecker IDL to add a setXHRClass type of method, but unfortunately that doesn't seem to work, as the JS prototype of the class is lost along the way (so I can't use "new" or "Object.create"). I presume that's because I'm making the in-parameter an nsISupports, but I figured I'd check before wasting time on an approach that may not be desirable in the first place.

	Boris Zbarsky [:bzbarsky]
	Comment 12 • 8 years ago

OK, so we have an actual JS impl of nsIXMLHttpRequest, which is registered for the XHR contract and then the update checker ends up using that object, not a real XHR.  Is my understanding correct?

If this is just update-checker specific, we could add a way to pass a function to nsIUpdateChecker that will create the "XHR" (just an IDL method that takes a jsval for the function).  If no function is set, it would do what it does not with the contract.

If we generally want to make this work for random code that creates XHR by contract and people want to mock that, then we need to keep this interface or something.  :(

	Thomas Wisniewski Assignee
	Comment 13 • 8 years ago

>OK, so we have an actual JS impl of nsIXMLHttpRequest, which is registered for the XHR contract and then the update checker ends up using that object, not a real XHR.  Is my understanding correct?

Yes, that's what I'm seeing in the code.


>If this is just update-checker specific

It seems to be. I don't see any other uses like it in central, at least.


>(just an IDL method that takes a jsval for the function)

Alright, makes sense. Here's a patch that does that, and gets rid of the updater's dependency on nsIXMLHttpRequest. The biggest change in the patch is mostly just indenting the mock-xhr and wrapping it into a closure.

	Thomas Wisniewski Assignee
	Comment 14 • 8 years ago

And while I'm at it, here's a patch removing the use of nsIXMLHttpRequest from the rest of the non-C++ code. It's big (sorry) but the changes are all simple and repetitive. The most interesting ones are:

- removing one test that doesn't seem useful anymore.
- removing a block of now-useless code in another mock that makes it mimic an IDL interface.
- replacing two tests' use of xmlhttprequest;1 with domparser;1, since they don't care which IDL interface they're testing, just it's for a JS global property.

A try run for both patches seems like it's working out, though a couple of tests have yet to be run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=70f0cc272c8d

	Boris Zbarsky [:bzbarsky]
	Comment 15 • 8 years ago

For the second patch, I can look through it, but note that you still can't remove that method of creating an XHR, since it's used in close to 3000 places in addons if I read my addons dxr results right.  Was the goal there just general cleanup of our code, or actually removing the contact-and-createInstance bit?

	Thomas Wisniewski Assignee
	Comment 16 • 8 years ago

I certainly don't mind keeping the minimal IDL interface around for compat. I just wanted to clear out as much of it out as possible while the iron's hot.

Do you think we'd be able to get rid of the other skeleton interfaces in nsIXMLHttpRequest.idl at this point, or are they also likely to be used by addons?

	Boris Zbarsky [:bzbarsky]
	Comment 17 • 8 years ago

Comment on attachment 8779440 [details] [diff] [review]
792808-1-remove_mozapps_update_reliance_on_nsIXMLHttpRequest.diff

Sorry for the lag here, I needed some time to think things through.  :(

And even then I'm not the right reviewer for this; going to redirect to someone who knows the update service code.

>+  void setCreateXHR(in jsval factory);

This needs documentation describing the actual semantics of this function.  For example, does it change the XHR constructor on just this one update checker object, or all update checker objects?

I would argue that we should really only change it on the given object if that's all the tests need.  And we should have some way of undoing the mock (e.g. passing in null to as the factory, or a separate method of undoing it).

And we should probably name this overrideXHRFactory or something.  Will defer to an actual toolkit/updater peer on this.

	Boris Zbarsky [:bzbarsky]
	Comment 18 • 8 years ago

Comment on attachment 8779441 [details] [diff] [review]
792808-2-remove-other-non-cpp-reliance-on-nsIXMLHttpRequest.diff

This should probably be split up into separate patches for the different reviewers you will want for different parts of it...  I do think this is worth doing, though we won't be able to drop support for the old pattern until extensions move off it.

	Boris Zbarsky [:bzbarsky]
	Comment 19 • 8 years ago

> Do you think we'd be able to get rid of the other skeleton interfaces in nsIXMLHttpRequest.idl

Hmm.  nsIXMLHttpRequestEventTarget is used like so in a few addons:

  const {classes: Cc, interfaces: Ci, utils: Cu, results: Cr, Constructor: CC} = Components;
  const XMLHttpRequest = CC("@mozilla.org/xmlextras/xmlhttprequest;1","nsIXMLHttpRequestEventTarget");

We would need to check whether that starts throwing if the nsIXMLHttpRequestEventTarget interface is removed entirely; I expect it does.  :(  We could just try to get those add-ons fixed too; there are only 6 of them on AMO.

nsIXMLHttpRequestUpload is entirely unused and can go.

nsIJSXMLHttpRequest is used a bunch in addons, in various ways:

   var XMLHttpRequest = Components.Constructor("@mozilla.org/xmlextras/xmlhttprequest;1",
                                               "nsIJSXMLHttpRequest");
....
    this.request =
      Components
        .classes["@mozilla.org/xmlextras/xmlhttprequest;1"]
          .createInstance(Components.interfaces.nsIJSXMLHttpRequest);
....
  var oHttpRequest = Components.classes["@mozilla.org/xmlextras/xmlhttprequest;1"]
                               .getService(Components.interfaces.nsIJSXMLHttpRequest);

(getService, really?).
.....
  let xhr = Cc["@mozilla.org/xmlextras/xmlhttprequest;1"].createInstance(Ci.nsIJSXMLHttpRequest);
.....
  request.QueryInterface(Ci.nsIJSXMLHttpRequest).onerror = function() { self.onError(); };

etc.

	Thomas Wisniewski Assignee
	Comment 20 • 8 years ago

>Sorry for the lag here

No worries on any lag, this isn't something we need to rush.


>does it change the XHR constructor on just this one update checker object, or all update checker objects?

Internally, a different instance is created during some operations, so the factory does need to be shared among all of them (as far as I can tell). I do agree that this needs documentation, of course.


>This should probably be split up into separate patches for the different reviewers you will want for different parts of it

Alright, I'll see if I can guess on how to split it up (by module, I'm assuming.. hopefully that will be obvious enough).


>we won't be able to drop support for the old pattern until extensions move off it.

Yes, that was my guess. Thanks for checking the addon usage of the other interfaces, too!

	Boris Zbarsky [:bzbarsky]
	Comment 21 • 8 years ago

> Internally, a different instance is created during some operations

A different instance of the update checker, right?

> so the factory does need to be shared among all of them

OK.  What happens at the end of the test?  Does the MockRegistrar stuff for xhr currently get unregistered for these tests, or are they run in an environment where nothing other than the one test gets run until xpcom restart, so the contract override goes away on its own?

	Robert Strong (they/them - no direct email)
	Comment 22 • 8 years ago

Several of the update tests that use the mock xhr only use it because it is faster. For example, urlConstruction.js makes 15 xhr requests. On my system the one with the mock xhr takes under a second to run and when using xhr it takes around 30 seconds to run. I can refactor that test so it only makes 2 xhr requests and when using xhr it then takes around 5 seconds to run.

I'm taking a look at the tests that use the mock xhr to see if they can be tested without the mock xhr in the hope that this additional code for testing won't be necessary.

	Robert Strong (they/them - no direct email)
	Comment 23 • 8 years ago

I have converted all of the app update tests except for one to not use the mock xhr and I believe I can get that last test working without the mock xhr as well.

	Thomas Wisniewski Assignee
	Comment 24 • 8 years ago

Thanks for the effort! Of course this does beg the question as to why using XHRs is so much slower... has that been investigated?

	Robert Strong (they/them - no direct email)
	Comment 25 • 8 years ago

The mock xhr is bare bones and I don't think it has been investigated.

	Robert Strong (they/them - no direct email)
	Comment 26 • 8 years ago

Comment on attachment 8779440 [details] [diff] [review]
792808-1-remove_mozapps_update_reliance_on_nsIXMLHttpRequest.diff

The removal of the app update mock xhr landed on inbound today and I filed bug 1296097 to nsIXMLHttpRequest from nsIUpdateService so this patch should soon no longer be necessary.

	Thomas Wisniewski Assignee
	Comment 27 • 8 years ago

Alright, here's a rebased version of the surviving patch, with the remaining mock fixed (a much simpler fix), and a few more non-C++ references to nsIXMLHttpRequest removed.

A try run of this new version seems fine: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f0bd01878c11

Next I'll try to get this patch split up for more targetted review, as per comment 18.

	Thomas Wisniewski Assignee
	Comment 28 • 7 years ago

Just FYI, I have a patchset I've been slowly working on to purge nsIXMLHttpRequest.h and the interfaces therein, which seems to be almost ready for review. Since we're planning on dropping legacy addon support post-57, I figure it's probably worth finishing those patches up and getting rid of that code. Thoughts, bz?

	Boris Zbarsky [:bzbarsky]
	Comment 29 • 7 years ago

That makes a lot of sense to me!  Scripts don't go through that stuff anyway, and we haven't been able to have out-of-tree C++ consumers of this stuff in a while (much longer than 57).  So it can all go away to the extent that it's not used from C++ in mozilla-central or comm-central.

	Thomas Wisniewski Assignee
	Comment 44 • 7 years ago

Comment on attachment 8787992 [details] [diff] [review]
918751-throw_NetworkErrors_instead_of_failures_where_XHR_WPTs_expect_them.diff

Alright, I just pushed a batch of commits for review to change our use of "Cc.createInstance(nsIXMLHttpRequest)" to just "new XMLHttpRequest" with the appropriate global import (and similar changes). Hopefully I managed to get a reasonable set of reviewers... apologies in advance if I didn't.

	Andrea Marchesini [:baku]
	Comment 45 • 7 years ago
mozreview-review

Comment on attachment 8909854 [details]
Bug 792808 - Change dom/[base|tests|url] to import and instantiate XHRs from global properties rather than using Cc.createInstance(Ci.nsIXMLHttpRequest);

https://reviewboard.mozilla.org/r/181342/#review186582

	Nathan Froyd [:froydnj]
	Comment 46 • 7 years ago
mozreview-review

Comment on attachment 8909861 [details]
Bug 792808 - Change asan-reporter bootstrap script to import and instantiate XHRs from global properties rather than using Cc.createInstance(Ci.nsIXMLHttpRequest);

https://reviewboard.mozilla.org/r/181356/#review186584

	Dave Townsend [:mossop]
	Comment 47 • 7 years ago
mozreview-review

Comment on attachment 8909862 [details]
Bug 792808 - Change PdfStreamConverter.jsm to import and instantiate XHRs from global properties rather than using Cc.createInstance(Ci.nsIXMLHttpRequest);

https://reviewboard.mozilla.org/r/181358/#review186586

	Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)
	Comment 48 • 7 years ago
mozreview-review

Comment on attachment 8909853 [details]
Bug 792808 - Change security/manager/tools scripts to import and instantiate XHRs from global properties rather than using Cc.createInstance(Ci.nsIXMLHttpRequest);

https://reviewboard.mozilla.org/r/181340/#review186598

	Boris Zbarsky [:bzbarsky]
	Comment 49 • 7 years ago
mozreview-review

Comment on attachment 8909859 [details]
Bug 792808 - Change js/xpconnect to import and instantiate XHRs from global properties rather than using Cc.createInstance(Ci.nsIXMLHttpRequest);

https://reviewboard.mozilla.org/r/181352/#review186612

::: js/xpconnect/tests/mochitest/test_bug790732.html
(Diff revision 1)
>  
>    // Check each interface that we shim. We start by checking specific
>    // constants for a couple of interfaces, and then once it's pretty clear that
>    // it's working as intended we just check that the objects themselves are the
>    // same.
> -  is(Ci.nsIXMLHttpRequest.HEADERS_RECEIVED, XMLHttpRequest.HEADERS_RECEIVED);

This change is probably OK, but note that at the moment we expose "Components.interfaces.nsIXMLHttpRequest.HEADERS_RECEIVED" to the web.

What we probably need to do for the constants is actually add a use counter of use of Components.interfaces.nsIXMLHttpRequest from untrusted code and see whether it triggers.  I filed bug 1401260 on adding some more measurement here.

	Blake Kaplan (:mrbkap) (inactive)
	Comment 50 • 7 years ago
mozreview-review

Comment on attachment 8909864 [details]
Bug 792808 - Change test_SpecialPowersExtension.html to import and instantiate XHRs from global properties rather than using Cc.createInstance(Ci.nsIXMLHttpRequest);

https://reviewboard.mozilla.org/r/181362/#review186616

	:Gijs (he/him)
	Comment 51 • 7 years ago
mozreview-review

Comment on attachment 8909863 [details]
Bug 792808 - Change pktApi.jsm to import and instantiate XHRs from global properties rather than using Cc.createInstance(Ci.nsIXMLHttpRequest);

https://reviewboard.mozilla.org/r/181360/#review186630

	Mike Conley (:mconley) (:⚙️)
	Comment 52 • 7 years ago
mozreview-review

Comment on attachment 8909860 [details]
Bug 792808 - Change browser/[components|modules|experiments] to import and instantiate XHRs from global properties rather than using Cc.createInstance(Ci.nsIXMLHttpRequest);

https://reviewboard.mozilla.org/r/181354/#review186632

Thanks!

Should we consider adding a linting rule to ensure new usages don't creep into the tree? Or will the interface be removed soon enough that this won't matter?

	Thomas Wisniewski Assignee
	Comment 53 • 7 years ago

(In reply to Boris Zbarsky [:bz] (still digging out from vacation mail) from comment #49)
> This change is probably OK, but note that at the moment we expose
> "Components.interfaces.nsIXMLHttpRequest.HEADERS_RECEIVED" to the web.
> 
> What we probably need to do for the constants is actually add a use counter
> of use of Components.interfaces.nsIXMLHttpRequest from untrusted code and
> see whether it triggers.  I filed bug 1401260 on adding some more
> measurement here.

Could we not somehow hang the true XMLHttpRequest object off of Components.interfaces.nsIXMLHttpRequest for web content, until we're ok with getting rid of it? Or is the IDL necessary in order to do that in the first place?

(In reply to Mike Conley (:mconley) (:⚙️) from comment #52)
> Should we consider adding a linting rule to ensure new usages don't creep
> into the tree? Or will the interface be removed soon enough that this won't
> matter?

That would probably be a good idea, as given bz's comment above (and the fact that I haven't checked comm-central yet), I can't tell how long it will be before the interface can be removed. I do have patches to remove it from moz-central, at least.

Is there documentation (or a code sample) that I could use to add such a rule?

	Mike Conley (:mconley) (:⚙️)
	Comment 54 • 7 years ago

(In reply to Thomas Wisniewski from comment #53)
> 
> Is there documentation (or a code sample) that I could use to add such a
> rule?

ahal probably knows best there.

	Andrew Swan [:aswan]
	Comment 55 • 7 years ago
mozreview-review

Comment on attachment 8909851 [details]
Bug 792808 - Change toolkit/ to import and instantiate XHRs from global properties rather than using Cc.createInstance(Ci.nsIXMLHttpRequest);

https://reviewboard.mozilla.org/r/181336/#review186646

	Andrew Halberstadt [:ahal]
	Comment 56 • 7 years ago

If you're linting Javascript, you'll likely want to add a custom eslint rule, see some of the existing rules for examples on how to do this:
http://searchfox.org/mozilla-central/source/tools/lint/eslint/eslint-plugin-mozilla/lib/rules

Otherwise you may need to add a new linter to the tree. There are some general linting docs here:
https://firefox-source-docs.mozilla.org/tools/lint/index.html

	Valentin Gosu [:valentin] (he/him)
	Comment 57 • 7 years ago
mozreview-review

Comment on attachment 8909852 [details]
Bug 792808 - Change netwerk/ to import and instantiate XHRs from global properties rather than using Cc.createInstance(Ci.nsIXMLHttpRequest);

https://reviewboard.mozilla.org/r/181338/#review186660

	Boris Zbarsky [:bzbarsky]
	Comment 58 • 7 years ago

> Could we not somehow hang the true XMLHttpRequest object off of Components.interfaces.nsIXMLHttpRequest for web content

We could.  It wouldn't be very hard.  The code lives in LookupComponentsShim in nsDOMClassInfo.cpp, and we could just add a special-case of XMLHttpRequest after the loop over kInterfaceShimMap.

I guess if we're not too worried about people starting to do "new Components.interfaces.nsIXMLHttpRequest" this would be a feasible way forward, if we really want to kill off the IID and constants.  Or we could leave just the IID and constants for the moment while removing everything else from the interface...

	Thomas Wisniewski Assignee
	Comment 59 • 7 years ago

It seems that people can already do "new Components.interfaces.nsIXMLHttpRequest" (I just tried it on release builds).

In fact, that same shim in kInterfaceShimMap works as-is with my patchset, despite my having removed nsIXMLHttpRequest.h and all of its interfaces (that is, it seems to already be doing what I was suggesting).

Based on that, I should have the rest of the patchset ready for review this week (presuming that we'll want to remove those interfaces outright, rather than leaving stubs).

	Boris Zbarsky [:bzbarsky]
	Comment 60 • 7 years ago

Oh, right, we decided to not expose nsID bits to content at all and just have Ci.nsIFoo map to the Foo ctor there.

So as long as we remove chrome uses of Ci.nsIXMLHttpRequest, we should be good.

	Dragana Damjanovic [:dragana]
	Comment 61 • 7 years ago
mozreview-review

Comment on attachment 8909856 [details]
Bug 792808 - Change dom/push to import and instantiate XHRs from global properties rather than using Cc.createInstance(Ci.nsIXMLHttpRequest);

https://reviewboard.mozilla.org/r/181346/#review186982

	Jan Honza Odvarko [:Honza] (always need-info? me)
	Comment 62 • 7 years ago
mozreview-review

Comment on attachment 8909857 [details]
Bug 792808 - Change devtools/ to import and instantiate XHRs from global properties rather than using Cc.createInstance(Ci.nsIXMLHttpRequest), and the same for FormData;

https://reviewboard.mozilla.org/r/181348/#review187056

Looks reasonable to me, thanks!

Honza

	Andrea Marchesini [:baku]
	Comment 85 • 7 years ago
mozreview-review

Comment on attachment 8911516 [details]
Bug 792808 - Migrate XMLHttpRequestMainThread away from needing nsIXMLHttpRequest's constants;

https://reviewboard.mozilla.org/r/182964/#review188164

You can simply cast mState to uint16_t if you set the correct values in the mState enums.

	Andrea Marchesini [:baku]
	Comment 86 • 7 years ago
mozreview-review

Comment on attachment 8911517 [details]
Bug 792808 - Migrate XMLHttpRequestWorker from using the nsIVariant send() XHR method;

https://reviewboard.mozilla.org/r/182966/#review188166

::: dom/xhr/XMLHttpRequestWorker.cpp:1481
(Diff revision 1)
>      Read(parent, cx, &body, aRv);
>      if (NS_WARN_IF(aRv.Failed())) {
>        return;
>      }
>  
> -    aRv = xpc->JSValToVariant(cx, body, getter_AddRefs(variant));
> +    JS::MutableHandle<JS::Value> handle(&body);

You don't need this.

::: dom/xhr/XMLHttpRequestWorker.cpp:1488
(Diff revision 1)
> +    Maybe<DocumentOrBlobOrArrayBufferViewOrArrayBufferOrFormDataOrURLSearchParamsOrUSVStringArgument> holder;
> +    holder.emplace(payload.SetValue());
> +    bool done = false, failed = false, tryNext;
> +
> +    if (body.isObject()) {
> +      done = (failed = !holder.ref().TrySetToDocument(cx, handle, tryNext, false)) || !tryNext ||

pass &body here instead of handle

	Andrea Marchesini [:baku]
	Comment 87 • 7 years ago
mozreview-review

Comment on attachment 8911518 [details]
Bug 792808 - Migrate XMLHttpRequestWorker from using the XMLHttpRequestEventTarget and nsIXMLHttpRequestUpload interfaces;

https://reviewboard.mozilla.org/r/182968/#review188168

::: dom/xhr/XMLHttpRequestWorker.cpp:955
(Diff revision 1)
>  
> -  nsCOMPtr<nsIDOMEventTarget> target =
> +  DOMEventTargetHelper* targetHelper =
>      aUpload ?
> -    do_QueryInterface(mXHRUpload) :
> -    do_QueryInterface(static_cast<nsIXMLHttpRequest*>(mXHR.get()));
> -  NS_ASSERTION(target, "This should never fail!");
> +    static_cast<XMLHttpRequestUpload*>(mXHRUpload.get()) :
> +    static_cast<XMLHttpRequestEventTarget*>(mXHR.get());
> +  NS_ASSERTION(targetHelper, "This should never fail!");

MOZ_ASSERT

	Andrea Marchesini [:baku]
	Comment 88 • 7 years ago
mozreview-review

Comment on attachment 8911519 [details]
Bug 792808 - Migrate XMLHttpRequestWorker away from using other nsIXMLHttpRequest interfaces;

https://reviewboard.mozilla.org/r/182970/#review188170

::: dom/xhr/XMLHttpRequestWorker.cpp:1007
(Diff revision 1)
>  
>    bool isUploadTarget = mXHR != target;
>    ProgressEvent* progressEvent = aEvent->InternalDOMEvent()->AsProgressEvent();
>  
>    if (mInOpen && type.EqualsASCII(sEventStrings[STRING_readystatechange])) {
> -    uint16_t readyState = 0;
> +    if (mXHR->ReadyState() == 1) {

Here you can use State::opened

::: dom/xhr/XMLHttpRequestWorker.cpp:1171
(Diff revision 1)
>              argv[0].set(response);
>              obj = JS_NewArrayObject(cx, argv);
>              if (obj) {
>                transferable.setObject(*obj);
>                // Only cache the response when the readyState is DONE.
> -              if (xhr->ReadyState() == nsIXMLHttpRequest::DONE) {
> +              if (xhr->ReadyState() == 4) {

same here. State::done

::: dom/xhr/XMLHttpRequestWorker.cpp:1414
(Diff revision 1)
>  
> -  nsresult rv;
> +  ErrorResult rv;
>  
>    if (mBackgroundRequest) {
> -    rv = mProxy->mXHR->SetMozBackgroundRequest(mBackgroundRequest);
> -    NS_ENSURE_SUCCESS(rv, rv);
> +    mProxy->mXHR->SetMozBackgroundRequest(mBackgroundRequest, rv);
> +    if (rv.Failed()) {

NS_WARN_IF

::: dom/xhr/XMLHttpRequestWorker.cpp:1421
(Diff revision 1)
> +    }
>    }
>  
>    if (mWithCredentials) {
> -    rv = mProxy->mXHR->SetWithCredentials(mWithCredentials);
> -    NS_ENSURE_SUCCESS(rv, rv);
> +    mProxy->mXHR->SetWithCredentials(mWithCredentials, rv);
> +    if (rv.Failed()) {

NS_WARN_IF

::: dom/xhr/XMLHttpRequestWorker.cpp:1428
(Diff revision 1)
> +    }
>    }
>  
>    if (mTimeout) {
> -    rv = mProxy->mXHR->SetTimeout(mTimeout);
> -    NS_ENSURE_SUCCESS(rv, rv);
> +    mProxy->mXHR->SetTimeout(mTimeout, rv);
> +    if (rv.Failed()) {

NS_WARN_IF

::: dom/xhr/XMLHttpRequestWorker.cpp:1444
(Diff revision 1)
> -                     rv2);
> +                     rv);
>  
>    MOZ_ASSERT(mProxy->mInOpen);
>    mProxy->mInOpen = false;
>  
> -  if (rv2.Failed()) {
> +  if (rv.Failed()) {

NS_WARN_IF

::: dom/xhr/XMLHttpRequestWorker.cpp:1449
(Diff revision 1)
> -  if (rv2.Failed()) {
> -    return rv2.StealNSResult();
> +  if (rv.Failed()) {
> +    return rv.StealNSResult();
>    }
>  
> -  mProxy->mXHR->SetResponseType(mResponseType, rv2);
> -  if (rv2.Failed()) {
> +  mProxy->mXHR->SetResponseType(mResponseType, rv);
> +  if (rv.Failed()) {

NS_WARN_IF

::: dom/xhr/XMLHttpRequestWorker.cpp:2278
(Diff revision 1)
>    // non-racy way until the XHR state machine actually runs on this thread
>    // (bug 671047). For now we're going to let this work only if the Send()
>    // method has not been called, unless the send has been aborted.
>    if (!mProxy || (SendInProgress() &&
>                    (mProxy->mSeenLoadStart ||
> -                   mStateData.mReadyState > nsIXMLHttpRequest::OPENED))) {
> +                   mStateData.mReadyState > 1))) {

State::opened

::: dom/xhr/XMLHttpRequestWorker.cpp:2314
(Diff revision 1)
>      return;
>    }
>  
>    if (SendInProgress() &&
>        (mProxy->mSeenLoadStart ||
> -       mStateData.mReadyState > nsIXMLHttpRequest::OPENED)) {
> +       mStateData.mReadyState > 1)) {

State::opened

	Andrea Marchesini [:baku]
	Comment 89 • 7 years ago
mozreview-review

Comment on attachment 8911521 [details]
Bug 792808 - use XMLHttpRequestBinding::STATE consts in the XHR subclasses and remove XMLHttpRequest::State enum class;

https://reviewboard.mozilla.org/r/182974/#review188172

::: dom/xul/templates/nsXULTemplateQueryProcessorXML.cpp:175
(Diff revision 1)
>  
>      rv = req->Open(NS_LITERAL_CSTRING("GET"), uriStr, true,
>                     EmptyString(), EmptyString());
>      NS_ENSURE_SUCCESS(rv, rv);
>  
> -    nsCOMPtr<EventTarget> target(do_QueryInterface(req));
> +    rv = req->AddEventListener(NS_LITERAL_STRING("load"), this, false, false, 2);

Use 
void
DOMEventTargetHelper::AddEventListener(const nsAString& aType,
                                       EventListener* aListener,
                                       const AddEventListenerOptionsOrBoolean& aOptions,
                                       const Nullable<bool>& aWantsUntrusted,
                                       ErrorResult& aRv)

instead.

::: dom/xul/templates/nsXULTemplateQueryProcessorXML.cpp:182
(Diff revision 1)
>  
> -    rv = target->AddEventListener(NS_LITERAL_STRING("error"), this, false);
> +    rv = req->AddEventListener(NS_LITERAL_STRING("error"), this, false, false, 2);
>      NS_ENSURE_SUCCESS(rv, rv);
>  
> -    rv = req->Send(nullptr);
> -    NS_ENSURE_SUCCESS(rv, rv);
> +    ErrorResult rv2;
> +    const Nullable<DocumentOrBlobOrArrayBufferViewOrArrayBufferOrFormDataOrURLSearchParamsOrUSVString> nil;

here as well.

::: dom/xul/templates/nsXULTemplateQueryProcessorXML.cpp:184
(Diff revision 1)
>      NS_ENSURE_SUCCESS(rv, rv);
>  
> -    rv = req->Send(nullptr);
> -    NS_ENSURE_SUCCESS(rv, rv);
> +    ErrorResult rv2;
> +    const Nullable<DocumentOrBlobOrArrayBufferViewOrArrayBufferOrFormDataOrURLSearchParamsOrUSVString> nil;
> +    req->Send(nullptr, nil, rv2);
> +    if (rv2.Failed()) {

NS_WARN_IF

::: dom/xul/templates/nsXULTemplateQueryProcessorXML.cpp:436
(Diff revision 1)
>      nsAutoString eventType;
>      aEvent->GetType(eventType);
>  
>      if (eventType.EqualsLiteral("load") && mTemplateBuilder) {
>          NS_ASSERTION(mRequest, "request was not set");
> -        nsCOMPtr<nsIDOMDocument> doc;
> +        ErrorResult rv;

IgnoredErrorResult

::: dom/xul/templates/nsXULTemplateQueryProcessorXML.cpp:437
(Diff revision 1)
>      aEvent->GetType(eventType);
>  
>      if (eventType.EqualsLiteral("load") && mTemplateBuilder) {
>          NS_ASSERTION(mRequest, "request was not set");
> -        nsCOMPtr<nsIDOMDocument> doc;
> -        if (NS_SUCCEEDED(mRequest->GetResponseXML(getter_AddRefs(doc))))
> +        ErrorResult rv;
> +        nsIDocument* doc = mRequest->GetResponseXML(rv);

nsCOMPtr<nsIDocument> doc = ...

	Andrea Marchesini [:baku]
	Comment 90 • 7 years ago
mozreview-review

Comment on attachment 8911522 [details]
Bug 792808 - Migrate BodyExtractor away from using nsIXHRSendable;

https://reviewboard.mozilla.org/r/182976/#review188174

	Andrea Marchesini [:baku]
	Comment 91 • 7 years ago
mozreview-review

Comment on attachment 8911523 [details]
Bug 792808 - Purge nsIXMLHttpRequest, nsIXMLHttpRequestUpload, nsIXMLHttpRequestEventTarget, nsIXHRSendable XPCOM interfaces;

https://reviewboard.mozilla.org/r/182978/#review188176

::: dom/filesystem/FileSystemTaskBase.h:15
(Diff revision 1)
>  #include "mozilla/ErrorResult.h"
>  #include "mozilla/dom/FileSystemRequestParent.h"
>  #include "mozilla/dom/PFileSystemRequestChild.h"
>  #include "nsIIPCBackgroundChildCreateCallback.h"
>  #include "nsThreadUtils.h"
> +#include "nsIGlobalObject.h"

alphabetic order.

::: dom/xhr/XMLHttpRequestMainThread.cpp:1974
(Diff revision 1)
>      mResponseXML->SetChromeXHRDocURI(chromeXHRDocURI);
>      mResponseXML->SetChromeXHRDocBaseURI(chromeXHRDocBaseURI);
>  
>      // suppress parsing failure messages to console for statuses which
>      // can have empty bodies (see bug 884693).
> -    uint32_t responseStatus;
> +    ErrorResult rv2;

IgnoredErrorResult

::: dom/xhr/XMLHttpRequestMainThread.cpp:2950
(Diff revision 1)
> -                                           const nsACString& aValue)
> +                                           const nsACString& aValue,
> +                                           ErrorResult& aRv)
>  {
>    // Step 1
>    if (mState != State::opened) {
> -    return NS_ERROR_DOM_INVALID_STATE_XHR_MUST_BE_OPENED;
> +    aRv = NS_ERROR_DOM_INVALID_STATE_XHR_MUST_BE_OPENED;

aRv.Throw(...)

::: dom/xhr/XMLHttpRequestMainThread.cpp:2956
(Diff revision 1)
> +    return;
>    }
>  
>    // Step 2
>    if (mFlagSend) {
> -    return NS_ERROR_DOM_INVALID_STATE_XHR_MUST_NOT_BE_SENDING;
> +    aRv = NS_ERROR_DOM_INVALID_STATE_XHR_MUST_NOT_BE_SENDING;

aRv.Throw(...)

::: dom/xhr/XMLHttpRequestMainThread.cpp:2966
(Diff revision 1)
>    nsAutoCString value;
>    NS_TrimHTTPWhitespace(aValue, value);
>  
>    // Step 4
>    if (!NS_IsValidHTTPToken(aName) || !NS_IsReasonableHTTPHeaderValue(value)) {
> -    return NS_ERROR_DOM_INVALID_HEADER_NAME;
> +    aRv = NS_ERROR_DOM_INVALID_HEADER_NAME;

aRv.Throw(...)

::: dom/xhr/XMLHttpRequestMainThread.cpp:3107
(Diff revision 1)
> -NS_IMETHODIMP
> -XMLHttpRequestMainThread::SetMozBackgroundRequest(bool aMozBackgroundRequest)
> +void
> +XMLHttpRequestMainThread::SetMozBackgroundRequest(bool aMozBackgroundRequest,
> +                                                  ErrorResult& aRv)
>  {
>    if (!IsSystemXHR()) {
> -    return NS_ERROR_DOM_SECURITY_ERR;
> +    aRv = NS_ERROR_DOM_SECURITY_ERR;

aRv.Throw(...)

::: dom/xhr/XMLHttpRequestMainThread.cpp:3111
(Diff revision 1)
>    if (!IsSystemXHR()) {
> -    return NS_ERROR_DOM_SECURITY_ERR;
> +    aRv = NS_ERROR_DOM_SECURITY_ERR;
> +    return;
>    }
>  
> -  if (mState != State::unsent) {
> +  if (mState != State::unsent && mState != State::opened) {

why "&& mState != opened" ? it seems an unrelated change. If needed, file a separate bug.

::: dom/xhr/XMLHttpRequestMainThread.cpp:3113
(Diff revision 1)
> +    return;
>    }
>  
> -  if (mState != State::unsent) {
> +  if (mState != State::unsent && mState != State::opened) {
>      // Can't change this while we're in the middle of something.
> -    return NS_ERROR_DOM_INVALID_STATE_XHR_MUST_NOT_BE_SENDING;
> +    aRv = NS_ERROR_DOM_INVALID_STATE_XHR_MUST_NOT_BE_SENDING;

aRv.Throw(...)

	Thomas Wisniewski Assignee
	Comment 92 • 7 years ago
mozreview-review-reply

Comment on attachment 8911519 [details]
Bug 792808 - Migrate XMLHttpRequestWorker away from using other nsIXMLHttpRequest interfaces;

https://reviewboard.mozilla.org/r/182970/#review188170

> Here you can use State::opened

That won't work, as I would need to do this:  if (mXHR->ReadyState() == static_cast<uint16_t>(XMLHttpRequestMainThread::State::opened)) {

*And* I would have to move the definition of the State class up so it's not protected, but is public (so the worker code can access it).

If I'm going to do that much, I would rather clean this code up properly:
- move the State enum class up into the XMLHttpRequest class (rather than XMLHttpRequestMainThread) and make it public.
- overload XMLHttpRequest::ReadyState() to have a version which returns a State.
- change XMLHttpRequestWorker::mReadyState into a State, rather than a uint16_t.
- change the rest of the XMLHttpRequestWorker code to not use numbers like this, but rather State::opened/etc.

I don't mind doing that, even as part of this patch queue. What would you prefer? I can rebase this patch queue to make these changes the first patch in the set, or add it to the end of the set, or just leave this as-is and file a new bug to make these changes?

	Thomas Wisniewski Assignee
	Comment 93 • 7 years ago
mozreview-review-reply

Comment on attachment 8911516 [details]
Bug 792808 - Migrate XMLHttpRequestMainThread away from needing nsIXMLHttpRequest's constants;

https://reviewboard.mozilla.org/r/182964/#review188164

Ah, good point. The values do match, so I will just cast and be done with it.

	Thomas Wisniewski Assignee
	Comment 94 • 7 years ago
mozreview-review-reply

Comment on attachment 8911521 [details]
Bug 792808 - use XMLHttpRequestBinding::STATE consts in the XHR subclasses and remove XMLHttpRequest::State enum class;

https://reviewboard.mozilla.org/r/182974/#review188172

> Use 
> void
> DOMEventTargetHelper::AddEventListener(const nsAString& aType,
>                                        EventListener* aListener,
>                                        const AddEventListenerOptionsOrBoolean& aOptions,
>                                        const Nullable<bool>& aWantsUntrusted,
>                                        ErrorResult& aRv)
> 
> instead.

I get: no known conversion for argument 2 from ‘nsXULTemplateQueryProcessorXML*’ to ‘mozilla::dom::EventListener*’

How would I cast it/make it compatible with that interface? Any example code?

	Thomas Wisniewski Assignee
	Comment 95 • 7 years ago
mozreview-review-reply

Comment on attachment 8911523 [details]
Bug 792808 - Purge nsIXMLHttpRequest, nsIXMLHttpRequestUpload, nsIXMLHttpRequestEventTarget, nsIXHRSendable XPCOM interfaces;

https://reviewboard.mozilla.org/r/182978/#review188176

> why "&& mState != opened" ? it seems an unrelated change. If needed, file a separate bug.

Ah, thanks for catching this! My to-do note to address this was lost in the shuffle somehow, so this temporary investigative hack crept into the final patches :S (thank good for reviews).

Basically I have to modify this patch to keep the old SetMozBackgroundRequest IDL method around, so we can continue to not throw on main-thread XHRs when the webIDL mozBackground setter is erroneously called. With my changes it *did* throw now, which caused a test failure as some of our internal code sets mozBackground at the wrong time (I will file bugs for any such cases I find afterward).

I will also have to revert a hunk in the worker patches to keep calling this method, rather than the one which silently fails.

	Boris Zbarsky [:bzbarsky]
	Comment 96 • 7 years ago
mozreview-review-reply

Comment on attachment 8911521 [details]
Bug 792808 - use XMLHttpRequestBinding::STATE consts in the XHR subclasses and remove XMLHttpRequest::State enum class;

https://reviewboard.mozilla.org/r/182974/#review188172

> I get: no known conversion for argument 2 from ‘nsXULTemplateQueryProcessorXML*’ to ‘mozilla::dom::EventListener*’
> 
> How would I cast it/make it compatible with that interface? Any example code?

It's rather painful.  You'd need to get the JS reflector for the nsXULTemplateQueryProcessorXML, then create a dom::EventListener wrapping it, and then pass that in.  And even that might now work, because I'm not sure whether the plumbing to call the C++ nsIDOMEventListener methods when the relevant JS bits get called via EventListener exist.

For now, I recomend continuing to call the EventTarget methods that take nsIDOMEventListener arguments.  Once we finally finish converting all event targets to webidl we can make that API noscript and nicer to use from C++...

	Thomas Wisniewski Assignee
	Comment 97 • 7 years ago

Thanks bz. That does sound a bit painful.

By the way, you mentioned comm-central would need adjusting, and I've written a patch to make those adjustments. Thankfully it's as simple as the other such patches on this bug, but I'm not sure what to do with it.

I couldn't actually test it, as my pushes to try-comm-central fizzled (they never start any build tasks), and I can't get a local build to complete (I get a cryptic error: comm-central/obj-x86_64-pc-linux-gnu/dist/include/mozilla/dom/UIEventBinding.h:13:28: fatal error: nsGlobalWindow.h: No such file or directory: #include "nsGlobalWindow.h")

I've attached the patch here regardless to make things easier for comm-central, but I'm not sure what to do with it from here. Should I migrate the patch to a new bug under Thunderbird/General, or put add it to the reviewboard queue with the rest of the patches here? Or should I just leave it alone and ping someone specific to take it from here?

	Boris Zbarsky [:bzbarsky]
	Comment 98 • 7 years ago

> but I'm not sure what to do with it.

File a separate bug, attach the patch, needinfo jorgk@jorgk.com.

	Thomas Wisniewski Assignee
	Comment 99 • 7 years ago

Comment on attachment 8911640 [details] [diff] [review]
stop-using-nsIXMLHttpRequest-in-comm-central.patch

Alright, I filed bug 1402755 and have obsoleted the copy of the patch that was here.

	Andrea Marchesini [:baku]
	Comment 100 • 7 years ago
mozreview-review

Comment on attachment 8911520 [details]
Bug 792808 - Migrate nsXMLParseEndListener away from nsIXMLHttpRequest interfaces;

https://reviewboard.mozilla.org/r/182972/#review188232

::: dom/xhr/XMLHttpRequestMainThread.cpp:213
(Diff revision 1)
>      mResultJSON(JS::UndefinedValue()),
>      mResultArrayBuffer(nullptr),
>      mIsMappedArrayBuffer(false),
>      mXPCOMifier(nullptr),
> -    mEventDispatchingSuspended(false)
> +    mEventDispatchingSuspended(false),
> +    mParseEndListener(nullptr)

no needs for this. Remove it.

::: dom/xhr/XMLHttpRequestMainThread.cpp:2326
(Diff revision 1)
>    if (mIsHtml) {
>      NS_ASSERTION(!mFlagSyncLooping,
>        "We weren't supposed to support HTML parsing with XHR!");
> +    MOZ_ASSERT(!mParseEndListener,
> +               "We are already listening for parse end?");
> +    mParseEndListener = new nsXHRParseEndListener(this);

here you are changing the logic. Don't do it.
This bug is just about removing nsIXMLHttpRequest interface.

If we need this change, do it in a separate bug.

::: dom/xhr/XMLHttpRequestMainThread.cpp:2354
(Diff revision 1)
>  
>  void
>  XMLHttpRequestMainThread::OnBodyParseEnd()
>  {
>    mFlagParseBody = false;
> +  if (mParseEndListener) {

mParseEndListener = nullptr; directly.

	Josh Matthews [:jdm]
	Comment 101 • 7 years ago
mozreview-review

Comment on attachment 8909855 [details]
Bug 792808 - Change dom/system to import and instantiate XHRs from global properties rather than using Cc.createInstance(Ci.nsIXMLHttpRequest);

https://reviewboard.mozilla.org/r/181344/#review188376

	Thomas Wisniewski Assignee
	Comment 102 • 7 years ago
mozreview-review-reply

Comment on attachment 8911520 [details]
Bug 792808 - Migrate nsXMLParseEndListener away from nsIXMLHttpRequest interfaces;

https://reviewboard.mozilla.org/r/182972/#review188232

> mParseEndListener = nullptr; directly.

Now that I think about it, should I even bother with the Stop() method for mParseEndListener at all, since it's only called in the main thread XHR destructor? Perhaps I should just mParseEndListener=nullptr in that destructor instead?

	Thomas Wisniewski Assignee
	Comment 103 • 7 years ago

baku, do you mind sharing your thoughts on comment 92? I already have a patch written which I could add to the end of this patchset. It allows the use of State::X in the worker file. Is it ok with you at the end of the patchset here, or should I split it off into its own bug?

Also, given what bz said in comment 96, I'm going to stick with the code I'm using right now for AddEventListener in nsXULTemplateQueryProcessorXML, if that's alright.

	Andrea Marchesini [:baku]
	Comment 104 • 7 years ago

> - move the State enum class up into the XMLHttpRequest class (rather than
> XMLHttpRequestMainThread) and make it public.
> - overload XMLHttpRequest::ReadyState() to have a version which returns a
> State.
> - change XMLHttpRequestWorker::mReadyState into a State, rather than a
> uint16_t.
> - change the rest of the XMLHttpRequestWorker code to not use numbers like
> this, but rather State::opened/etc.

Yes, this is exactly what I had in mind. Please do this.
Add this as separate patch at the end of the queue. No rebase needed.

About comment 96, follow what bz says.

	Thomas Wisniewski Assignee
	Comment 128 • 7 years ago

Alright, I've just pushed a rebased version of the patchset which also includes the patch requested in comment 104.

Assuming that patch is fine, then we should be good to land this soon (just waiting on sebastian's review here, and on one other review for the comm-central/calendar patch in bug 1402755).

	Andrea Marchesini [:baku]
	Comment 129 • 7 years ago
mozreview-review

Comment on attachment 8911521 [details]
Bug 792808 - use XMLHttpRequestBinding::STATE consts in the XHR subclasses and remove XMLHttpRequest::State enum class;

https://reviewboard.mozilla.org/r/182974/#review189274

	Andrea Marchesini [:baku]
	Comment 130 • 7 years ago
mozreview-review

Comment on attachment 8911520 [details]
Bug 792808 - Migrate nsXMLParseEndListener away from nsIXMLHttpRequest interfaces;

https://reviewboard.mozilla.org/r/182972/#review189278

	Andrea Marchesini [:baku]
	Comment 131 • 7 years ago
mozreview-review

Comment on attachment 8912761 [details]
Bug 792808 - expose XHR state constants as XMLHttpRequest::State enum, and use them fully-qualified in the subclasses;

https://reviewboard.mozilla.org/r/184078/#review189282

::: dom/xhr/XMLHttpRequestWorker.h:26
(Diff revision 1)
>  
>  namespace workers {
>  class WorkerPrivate;
>  }
>  
> +using State = XMLHttpRequest::State;

Do you need this?

	Thomas Wisniewski Assignee
	Comment 132 • 7 years ago
mozreview-review-reply

Comment on attachment 8912761 [details]
Bug 792808 - expose XHR state constants as XMLHttpRequest::State enum, and use them fully-qualified in the subclasses;

https://reviewboard.mozilla.org/r/184078/#review189282

> Do you need this?

Unfortunately, that doesn't work. I either have to fully qualify the name everywhere (XMLHttpRequest::State::X) or use an approach like this. Do you suspect this will cause namespace clashes with our unified builds? If so maybe we should just bite the bullet and fully qualify instead..

	Thomas Wisniewski Assignee
	Comment 133 • 7 years ago

(I of course mean in my comment above that it doesn't work to simply drop that line).

	Andrea Marchesini [:baku]
	Comment 134 • 7 years ago

I prefer this:

class XMLHttpRequestWorker ...
{
   typedef XMLHttpRequest::State State;

	Thomas Wisniewski Assignee
	Comment 135 • 7 years ago

Hmm, that variant isn't working (the header files compile, but the cpp files say that "State has not been declared").

I guess to be on the safe side and not pollute the namespace, I should just fully-qualify with XMLHttpRequest::State::X in the cpp files?

	Andrea Marchesini [:baku]
	Comment 136 • 7 years ago

> I guess to be on the safe side and not pollute the namespace, I should just
> fully-qualify with XMLHttpRequest::State::X in the cpp files?

good to me.

	Thomas Wisniewski Assignee
	Comment 160 • 7 years ago

bz, I just noticed that this mochitest fails with my patchset: js/xpconnect/tests/mochitest/test_bug790732.html

It's this specific test that's failing:

  SpecialPowers.Ci.hasOwnProperty("nsIXMLHttpRequest")

However, when I open a tab in the live browser, the equivalent assertion holds true in its console:

   Components.interfaces.hasOwnProperty("nsIXMLHttpRequest")

Do you think this is this something to worry about, or should I just skip nsIXMLHttpRequest in that specific test?

	Boris Zbarsky [:bzbarsky]
	Comment 161 • 7 years ago

So there are two different Components.interfaces things: the one exposed to content and the one exposed to chrome.

test_bug790732.html tests both.  It tests Ci.nsIXMLHttpRequest to test the web-exposed thing and tests SpecialPowers.Ci.nsIXMLHttpRequest to test the chrome-exposed thing.

In this bug we're removing "nsIXMLHttpRequest" from the chrome-exposed thing (because we know it's not used there), but not removing the web-exposed thing yet.  So yeah, we should explicitly skip it, with a comment, in this test.

	Thomas Wisniewski Assignee
	Comment 185 • 7 years ago

Alright, I've corrected my changes to that file so it's removing the test for the chrome-specific case, rather than the web-content case. Thanks again, bz.

I'm doing a try-run just in case something else sticks out, but it looks like we're just down to waiting on the review here and in bug 1402755 before we can decide whether/when to land this.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=70e6afbf405cedf6b58001663c7e28ddb738f932