Closed Bug 792808 Opened 8 years ago Closed 2 years ago

Gut nsIXMLHttpRequest

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: Ms2ger, Assigned: wisniewskit)

References

(Blocks 1 open bug)

Details

Attachments

(22 files, 5 obsolete files)

59 bytes, text/x-review-board-request
aswan
: review+
Details
59 bytes, text/x-review-board-request
valentin
: review+
Details
59 bytes, text/x-review-board-request
keeler
: review+
Details
59 bytes, text/x-review-board-request
baku
: review+
Details
59 bytes, text/x-review-board-request
jdm
: review+
Details
59 bytes, text/x-review-board-request
dragana
: review+
Details
59 bytes, text/x-review-board-request
Honza
: review+
Details
59 bytes, text/x-review-board-request
sebastian
: review+
Details
59 bytes, text/x-review-board-request
bzbarsky
: review+
Details
59 bytes, text/x-review-board-request
mconley
: review+
Details
59 bytes, text/x-review-board-request
froydnj
: review+
Details
59 bytes, text/x-review-board-request
mossop
: review+
Details
59 bytes, text/x-review-board-request
Gijs
: review+
Details
59 bytes, text/x-review-board-request
mrbkap
: review+
Details
59 bytes, text/x-review-board-request
baku
: review+
Details
59 bytes, text/x-review-board-request
baku
: review+
Details
59 bytes, text/x-review-board-request
baku
: review+
Details
59 bytes, text/x-review-board-request
baku
: review+
Details
59 bytes, text/x-review-board-request
baku
: review+
Details
59 bytes, text/x-review-board-request
baku
: review+
Details
59 bytes, text/x-review-board-request
baku
: review+
Details
59 bytes, text/x-review-board-request
baku
: review+
Details
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.
Binary addons use nsIXMLHttpRequest. We should not break them.
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?
We should rename nsIXMLHttpRequest, and introduce a gutted version (like nsIDOMWindowInternal).
Component: DOM: Mozilla Extensions → DOM
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.
I guess I should have needinfo'd someone here. Any fresh thoughts on this, guys?
Flags: needinfo?(khuey)
Flags: needinfo?(bzbarsky)
I expect doing this is fine now.
Flags: needinfo?(bzbarsky) → needinfo?(bugs)
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.
Flags: needinfo?(bugs)
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.
> 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.
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.
Flags: needinfo?(bzbarsky)
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.  :(
Flags: needinfo?(bzbarsky)
>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.
Attachment #8779440 - Flags: review?(bzbarsky)
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
Assignee: nobody → wisniewskit
Status: NEW → ASSIGNED
Attachment #8779441 - Flags: review?(bzbarsky)
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?
Flags: needinfo?(wisniewskit)
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?
Flags: needinfo?(wisniewskit)
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.
Attachment #8779440 - Flags: review?(bzbarsky) → review?(robert.strong.bugs)
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.
Attachment #8779441 - Flags: review?(bzbarsky)
> 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.
>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!
> 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?
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.
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.
Thanks for the effort! Of course this does beg the question as to why using XHRs is so much slower... has that been investigated?
The mock xhr is bare bones and I don't think it has been investigated.
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.
Attachment #8779440 - Attachment is obsolete: true
Attachment #8779440 - Flags: review?(robert.strong.bugs)
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.
Attachment #8779441 - Attachment is obsolete: true
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?
Flags: needinfo?(bzbarsky)
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.
Flags: needinfo?(bzbarsky)
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.
Attachment #8787992 - Attachment is obsolete: true
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
Attachment #8909854 - Flags: review?(amarchesini) → 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
Attachment #8909861 - Flags: review?(nfroyd) → 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
Attachment #8909862 - Flags: review?(dtownsend) → 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
Attachment #8909853 - Flags: review?(dkeeler) → 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.
Attachment #8909859 - Flags: review?(bzbarsky) → 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
Attachment #8909864 - Flags: review?(mrbkap) → 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
Attachment #8909863 - Flags: review?(gijskruitbosch+bugs) → 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?
Attachment #8909860 - Flags: review?(mconley) → review+
(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?
Flags: needinfo?(mconley)
Flags: needinfo?(bzbarsky)
(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.
Flags: needinfo?(mconley) → needinfo?(ahalberstadt)
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
Attachment #8909851 - Flags: review?(aswan) → review+
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
Flags: needinfo?(ahalberstadt)
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
Attachment #8909852 - Flags: review?(valentin.gosu) → review+
> 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...
Flags: needinfo?(bzbarsky)
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).
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.
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
Attachment #8909856 - Flags: review?(dd.mozilla) → 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
Attachment #8909857 - Flags: review?(odvarko) → 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.
Attachment #8911516 - Flags: review?(amarchesini) → 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
Attachment #8911517 - Flags: review?(amarchesini) → 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
Attachment #8911518 - Flags: review?(amarchesini) → 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
Attachment #8911519 - Flags: review?(amarchesini) → 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 = ...
Attachment #8911521 - Flags: review?(amarchesini) → review-
Comment on attachment 8911522 [details]
Bug 792808 - Migrate BodyExtractor away from using nsIXHRSendable;

https://reviewboard.mozilla.org/r/182976/#review188174
Attachment #8911522 - Flags: review?(amarchesini) → 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(...)
Attachment #8911523 - Flags: review?(amarchesini) → review+
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?
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.
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?
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.
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++...
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?
> but I'm not sure what to do with it.

File a separate bug, attach the patch, needinfo jorgk@jorgk.com.
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.
Attachment #8911640 - Attachment is obsolete: true
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.
Attachment #8911520 - Flags: review?(amarchesini) → 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
Attachment #8909855 - Flags: review?(josh) → review+
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?
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.
Flags: needinfo?(amarchesini)
> - 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.
Flags: needinfo?(amarchesini)
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).
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
Attachment #8911521 - Flags: review?(amarchesini) → review+
Comment on attachment 8911520 [details]
Bug 792808 - Migrate nsXMLParseEndListener away from nsIXMLHttpRequest interfaces;

https://reviewboard.mozilla.org/r/182972/#review189278
Attachment #8911520 - Flags: review?(amarchesini) → 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?
Attachment #8912761 - Flags: review?(amarchesini) → 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

> 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..
(I of course mean in my comment above that it doesn't work to simply drop that line).
Flags: needinfo?(amarchesini)
I prefer this:

class XMLHttpRequestWorker ...
{
   typedef XMLHttpRequest::State State;
Flags: needinfo?(amarchesini)
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?
Flags: needinfo?(amarchesini)
> 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.
Flags: needinfo?(amarchesini)
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?
Flags: needinfo?(bzbarsky)
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.
Flags: needinfo?(bzbarsky)
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
Blocks: 1387169
Depends on: 1402755
Comment on attachment 8909858 [details]
Bug 792808 - Change mobile/android/ to import and instantiate XHRs from global properties rather than using Cc.createInstance(Ci.nsIXMLHttpRequest);

https://reviewboard.mozilla.org/r/181350/#review199494
Attachment #8909858 - Flags: review?(s.kaspari) → review+
Nick, I just did a fresh try-run of my patchset here, and I'm confused by the TV failures in test_http2.js:

>TEST-UNEXPECTED-FAIL | netwerk/test/unit/test_http2.js | Http2MultiplexListener.prototype.onStopRequest - [Http2MultiplexListener.prototype.onStopRequest : 125] false == true

Which is this line in Http2MultiplexListener.prototype.onStopRequest:

>do_check_true(this.onDataAvailableFired);

I don't think I ran this tier-2 test in my previous try run a couple of months ago [2] so I guess I didn't notice this failure until now. Regardless, I can't reproduce this failure locally: that test always fails with a different NS_ERROR_ABORT code elsewhere, even on a clean m-c build without my patches:

>TEST_STATUS: Thread-1 testOnStopRequest FAIL [testOnStopRequest : 80] false == true

Which is this line in Http2CheckListener.prototype.onStopRequest:

>do_check_true(Components.isSuccessCode(status));

Is there something special I ought to be doing to run that test locally? Or might you have some other clue/insight to help me understand why onDataAvailable might be failing due to my patchset (I'm guessing that it would likely be the second patch with the netwerk/ changes that would affect it)?

[1] new try-run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f6a1eb09899d29434b22fd3c6b0054b4bb7a2205
[2] old try-run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=70e6afbf405cedf6b58001663c7e28ddb738f932
Flags: needinfo?(hurley)
(In reply to Thomas Wisniewski from comment #187)

The TV failures are... confusing, at best. There shouldn't be any reason those would fail while the regular xpcshell tests succeed (though this is, literally, the first time I've even heard of/noticed TV, so I could very well be missing something).

However,
 
> Is there something special I ought to be doing to run that test locally?

Yep. You need to have node installed (version 5 or higher), and then set MOZ_NODE_PATH=/path/to/node in your env that mach runs under. One weird thing, though, is that these tests should've failed to run (with an explanatory message!) if that wasn't installed/set or it failed to run the node-http2 server for some reason. But, this wouldn't be the first time that some change made the failure mode of this situation unexpectedly bad, so it doesn't really mean anything that you didn't get a reasonable error message :)
Flags: needinfo?(hurley)
Thanks! Of course, that test file passes just fine for me locally, now that I've set MOZ_NODE_PATH (also on Linux, with any without my patchset). Knowing that, do you suspect that I should check with someone else before I just presume these failures aren't really related to my patches (as I still can't see why they would cause that onDataAvailableFired failure)?
Flags: needinfo?(hurley)
(In reply to Thomas Wisniewski from comment #189)
> Thanks! Of course, that test file passes just fine for me locally, now that
> I've set MOZ_NODE_PATH (also on Linux, with any without my patchset).
> Knowing that, do you suspect that I should check with someone else before I
> just presume these failures aren't really related to my patches (as I still
> can't see why they would cause that onDataAvailableFired failure)?

So... I don't know why the patch in question would cause onDataAvailable to not fire, but that does seem to be the case. I pushed 2 try runs, one a relatively recent (last week or so) mozilla-central, and the one with just the changes to test_http2.js from your patch above, and it behaves just like you're seeing on your try run - TV fails, while regular xpcshell tests succeed.

I don't know if TV does anything different from regular xpcshell tests, but figuring that out would be a good first step to seeing why that fails when regular xpcshell tests succeed. It may be something wrong with the TV setup, or just that the alterations in TV are exposing a latent bug, I don't know for sure. Definitely worth investigating before landing these patches, though.
Flags: needinfo?(hurley)
Alright, thanks. Unfortunately, I won't have time to deal with that anytime soon, so unless there's someone who can help figure it out, I'm going to have to put this on the backburner again. (note that SS/screenshots is also failing on Stylo-disabled builds, and I can't tell what the deal is there, either).
Duplicate of this bug: 1085060
So I've recently found some time to rebase these patches, and to investigate the causes of these TV tier-2 failures.
It turns out that it's this constructor-change in netwerk/test/unit/test_http2.js :
>function addCertOverride(host, port, bits) {
>  // my patch changes the following to: var req = new XMLHttpRequest();
>  var req = Cc["@mozilla.org/xmlextras/xmlhttprequest;1"]
>            .createInstance(Ci.nsIXMLHttpRequest);
>  try {
>    var url;
>    if (port) {
>      url = "https://" + host + ":" + port + "/";
>    } else {
>      url = "https://" + host + "/";
>    }
>    req.open("GET", url, false);
>    req.channel.notificationCallbacks = new CertOverrideListener(host, port, bits);
>    req.send(null);
>  } catch (e) {
>    // This will fail since the server is not trusted yet
>  } 
>}

Somehow, that change causes the test to fail on try runs (but not when running the test locally).

From what I'm seeing, when addCertOverride is in effect the CertOverrideListener's notifyCertProblem is supposed to kick in and dump "Certificate Override in place" to the log. Indeed, that's what a failing try log reveals [1]:
>INFO -  PID 1053 | Certificate Override in place
>INFO -  "CONSOLE_MESSAGE: (error) [JavaScript Error: "localhost:45296 uses an invalid security certificate.
>INFO -  The certificate is only valid for the following names:
>INFO -    foo.example.com, alt1.example.com, alt2.example.com
>INFO -  Error code: <a id="errorCode" title="SSL_ERROR_BAD_CERT_DOMAIN">SSL_ERROR_BAD_CERT_DOMAIN</a>

However, this notice doesn't appear in try-run logs without my patches [2] (that is, in try-runs with a clean tree). It only appears on try-runs when I change the XHR's constructor to "new XMLHttpRequest()".

Even stranger, the notice appears for me on local runs of the tests, no matter which constructor the code is using (the test also passes regardless of which constructor is being used).

Is it possible that the tier-2 config has something weird that bypasses such listeners?


To cap off the strangeness, the actual test failure being logged is this:
>TEST-UNEXPECTED-FAIL | netwerk/test/unit/test_http2.js | Http2MultiplexListener.prototype.onStopRequest - [Http2MultiplexListener.prototype.onStopRequest : 124] false == true

That is, this line:
>Assert.ok(this.buffer == multiplexContent);

What's in this.buffer is 1024 zeros, while 32*1024 are expected (according to a try-run dumping that info).
I'm... not sure how that would relate to the constructor change or the bad cert listener, though.


bz, I'm not sure who to ping for info here, do you have any suggestions?


[1] Try-run with the one-line change, which fails and doesn't display the cert override notice:
    https://treeherder.mozilla.org/#/jobs?repo=try&revision=5ec57ced841be532daf6746cb4ad15fecd09dc69&selectedJob=161848411
[2] Clean try-run that doesn't fail and doesn't display the cert override notice:
    https://treeherder.mozilla.org/#/jobs?repo=try&revision=fdbf6618ecdfb0329f14ea4b6a65392c67a326b0&selectedJob=161858980
Flags: needinfo?(bzbarsky)
I'm not quite sure who to ping.  Maybe whoever wrote/reviewed the test?

That said, in terms of actual behavior differences between the createInstance path and the constructor path, they are:

1) createInstance always calls XMLHttpRequest::Construct with the system principal.  The constructor uses the principal of the global the constructor came from.  I would expect that's also the system principal here, but it would be good to check, in XMLHttpRequest::Constructor, whether "principal" is the system principal here.

2) createInstance associates the XHR object with the privileged junk scope.  The constructor associates it with the actual global the constructor came from.  It would be interesting to change XMLHttpRequest::Constructor to pass "xpc::NativeGlobal(xpc::PrivilegedJunkScope())" instead of "global" to req->Construct() and see how that affects this test.

One thing I am wondering is whether we somehow end up doing an _async_ request from addCertOverride or something, in the failing case.  I don't see why we would, though...
Flags: needinfo?(bzbarsky)
(In reply to Thomas Wisniewski from comment #193)
> So I've recently found some time to rebase these patches, and to investigate
> the causes of these TV tier-2 failures.
>
> Somehow, that change causes the test to fail on try runs (but not when
> running the test locally).

Asking the dumb questions just to be sure, you've run this locally ** with --verify ** ? --verify runs the test repeatedly in various fashions, which may be influencing how/when/why this happens.
Ah, thanks gijs, that does seem to be what I was missing! I can not only reproduce this failure locally now, I can't avoid it whether my patches are applied or not (that is, regardless of which method of XHR construction I use). The first --verify test-run passes, but all subsequent ones fail.

I also noticed this time that the test's testOnStartRequest is logging that the channel has a non-OK status on the failing runs, which is one of two values depending on the test-run:
>ERROR Channel should have a success code! (2147500036)
>ERROR Channel should have a success code! (2152398861)

I'm not sure what that means, but since it's happening during the HTTP multiplex tests, maybe there's a race condition the test is hitting?

It looks like the related test code hasn't really changed since it was introduced in bug 950768, so I'll ping :mcmanus to see if he has any ideas.

For the record, I'm running the test locally with this (Linux) command:
>MOZ_NODE_PATH=/usr/bin/node ./mach xpcshell-test netwerk/test/unit/test_http2.js --verify
Flags: needinfo?(mcmanus)
(In reply to Thomas Wisniewski from comment #196)
> Ah, thanks gijs, that does seem to be what I was missing! I can not only
> reproduce this failure locally now, I can't avoid it whether my patches are
> applied or not (that is, regardless of which method of XHR construction I
> use). The first --verify test-run passes, but all subsequent ones fail.

On infra, TV only runs changed tests. That means that your "clean" try run doesn't run the relevant test against --verify (it's too expensive to run all the tests in TV mode all the time).

So really, you're just running into a pre-existing issue with the test in that it breaks when run under --verify conditions.

If this is the only failure it doesn't block you (that's why it's tier-2) and you should land anyway, and file a followup bug to fix the test so it also passes if run under --verify.
https://developer.mozilla.org/en-US/docs/Mozilla/QA/Test_Verification covers some of the --verify stuff in more detail if you're interested.
>ERROR Channel should have a success code! (2147500036)

That's 0x80004004 == NS_ERROR_ABORT.

>ERROR Channel should have a success code! (2152398861)

Looks like 0x804b000d == NS_ERROR_CONNECTION_REFUSED
all of the tests that use the http2 server break under --verify because --verify stops the CI http2server between phases :) - that's bug 1437529 which should be landing right now.

I think its possible some subset of http2 tests are not stateless on the server side and will break anyhow (though that bug improves things considerably).. if you identify that please do file a bug and cc me, but that's a pre-existing test-only problem and shouldn't block you. (--erify was never verified to work with all the existing tests..)
Flags: needinfo?(mcmanus)
Thanks folks! For the record, it looks like the CONNECTION_REFUSED is happening from here in netwerk/base/nsSocketTransport2.cpp (at least my logging seems to be hitting this case during the tests):

>     // Treat EACCES as a soft error since (at least on Linux) connect() returns
>     // EACCES when an IPv6 connection is blocked by a firewall. See bug 270784.
>     case PR_NO_ACCESS_RIGHTS_ERROR:
>         rv = NS_ERROR_CONNECTION_REFUSED;
>         break;

I'll post my rebased patches later so we can decide whether they need any final reviews, or if the r+s can just carry over.
Attachment #8912761 - Attachment is obsolete: true
Alright, I've rebased my patches and run them through try, revealing only what seem to be unrelated intermittents and the tier2 TV failures that previous comments suggest should not block landing these patches:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c1f304d2cfba4d59d0611da8dd380ad8fc59f1dc

bz, do you feel it's worth having anyone do a re-review of the patches, given that it's been a few months since this rebase? (FWIW, I didn't notice anything that seemed interesting while rebasing).
Flags: needinfo?(bzbarsky)
Was there any non-mechanical rebasing at all?  It might be worth having someone spot-check the places where you had to manually rebase, if any.
Flags: needinfo?(bzbarsky)
It was all mechanical. The most interesting part was that one new use of the nsIXMLHttpRequest constructor crept into toolkit/components/places/tests/unit/test_bookmarks_html_escape_entities.js (which can be seen right at the bottom of the interdiff: https://reviewboard.mozilla.org/r/181336/diff/5-6/)

Since that's exactly the same kind of change I've made all over the place in patch #1 reviewed by aswan (and the test still passes), I don't suspect that it's necessary to review it, but I don't mind pinging aswan to check it just in case?
You need to Cu.importGlobalProperties(["XMLHttpRequest"]) in test_bookmarks_html_escape_entities.js, I would think, like other unit tests in the diff.  But maybe whatever environment it's running in has the thing imported already?

There's no need to get the mechanical bits looked at again.
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #227)
> But maybe whatever environment it's running in has the thing imported already?

That's likely be the case (as it's an xpcshell-test and still passing), but I've gone ahead and added the import anyway just in case. (I had to rebase anyway).

Do you think I should I go ahead and request check-in for this patchset, or is there anything else that should be done/tried first?
If it's passing try, request check-in.
Alright, requesting checkin.
Keywords: checkin-needed
This can't be landed until all patches at https://reviewboard.mozilla.org/r/181334/ have a green "r+" _on the left_. The "!1" can be fixed by closing the issue, the "r?" require a review (if I remember correct, this happens if someone cancels a review).
Flags: needinfo?(wisniewskit)
Keywords: checkin-needed
>This can't be landed until all patches at https://reviewboard.mozilla.org/r/181334/ have a green "r+" _on the left_

Ah, thanks, that slipped past me somehow.

bz, are you ok with this patch landing despite your comment there? https://reviewboard.mozilla.org/r/181352/#issue-summary

jdm, you r+d this part, mind finalizing the review? https://reviewboard.mozilla.org/r/181344/diff/7#index_header

dragana, likewise you r+d this part, mind finalizing it? https://reviewboard.mozilla.org/r/181346/diff/7#index_header
Flags: needinfo?(wisniewskit)
Flags: needinfo?(josh)
Flags: needinfo?(dd.mozilla)
Flags: needinfo?(bzbarsky)
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/#review226628
I don't know what I can do to convince mozreview that I'm a suitable reviewer :/
Flags: needinfo?(josh)
> bz, are you ok with this patch landing despite your comment there? https://reviewboard.mozilla.org/r/181352/#issue-summary

Yes, though that patch has nothing to do with the file I made that comment on...
Flags: needinfo?(bzbarsky)
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/#review226642
Attachment #8909855 - Flags: 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/#review226644
Attachment #8909856 - Flags: review+
I just stamped all the patches, so I think you should be good to go now...
Flags: needinfo?(dd.mozilla)
Thanks a ton, bz! Re-requesting checkin...
Keywords: checkin-needed
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 69accfc13709132ee8f4fbf69dc1530bbfc37093 -d c76cfa405b0e: rebasing 447718:69accfc13709 "Bug 792808 - Change toolkit/ to import and instantiate XHRs from global properties rather than using Cc.createInstance(Ci.nsIXMLHttpRequest); r=aswan"
merging toolkit/components/search/nsSearchService.js
rebasing 447719:4fe7e54a2289 "Bug 792808 - Change netwerk/ to import and instantiate XHRs from global properties rather than using Cc.createInstance(Ci.nsIXMLHttpRequest); r=valentin"
rebasing 447720:79e421dd9af4 "Bug 792808 - Change security/manager/tools scripts to import and instantiate XHRs from global properties rather than using Cc.createInstance(Ci.nsIXMLHttpRequest); r=keeler"
rebasing 447721:16529629241c "Bug 792808 - Change dom/[base|tests|url] to import and instantiate XHRs from global properties rather than using Cc.createInstance(Ci.nsIXMLHttpRequest); r=baku"
rebasing 447722:c40c2c48c73b "Bug 792808 - Change dom/system to import and instantiate XHRs from global properties rather than using Cc.createInstance(Ci.nsIXMLHttpRequest); r=bz,jdm"
rebasing 447723:ca98d498da9b "Bug 792808 - Change dom/push to import and instantiate XHRs from global properties rather than using Cc.createInstance(Ci.nsIXMLHttpRequest); r=bz,dragana"
rebasing 447724:136e2040c1f9 "Bug 792808 - Change devtools/ to import and instantiate XHRs from global properties rather than using Cc.createInstance(Ci.nsIXMLHttpRequest), and the same for FormData; r=Honza"
merging devtools/shared/builtin-modules.js
warning: conflicts while merging devtools/shared/builtin-modules.js! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Flags: needinfo?(wisniewskit)
Keywords: checkin-needed
Rebased, checkin re-requested.

It seems there is a lot of churn lately due to how quickly we're removing the nsI* interfaces in the parent bug, so hopefully this time the check-in will stick.
Flags: needinfo?(wisniewskit)
Keywords: checkin-needed
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/d8198d56a460
Change toolkit/ to import and instantiate XHRs from global properties rather than using Cc.createInstance(Ci.nsIXMLHttpRequest); r=aswan
https://hg.mozilla.org/integration/autoland/rev/654d03b562b3
Change netwerk/ to import and instantiate XHRs from global properties rather than using Cc.createInstance(Ci.nsIXMLHttpRequest); r=valentin
https://hg.mozilla.org/integration/autoland/rev/8ad43afebd65
Change security/manager/tools scripts to import and instantiate XHRs from global properties rather than using Cc.createInstance(Ci.nsIXMLHttpRequest); r=keeler
https://hg.mozilla.org/integration/autoland/rev/96efd7a65055
Change dom/[base|tests|url] to import and instantiate XHRs from global properties rather than using Cc.createInstance(Ci.nsIXMLHttpRequest); r=baku
https://hg.mozilla.org/integration/autoland/rev/41e250a79eeb
Change dom/system to import and instantiate XHRs from global properties rather than using Cc.createInstance(Ci.nsIXMLHttpRequest); r=bz,jdm
https://hg.mozilla.org/integration/autoland/rev/2caa8dcfef0f
Change dom/push to import and instantiate XHRs from global properties rather than using Cc.createInstance(Ci.nsIXMLHttpRequest); r=bz,dragana
https://hg.mozilla.org/integration/autoland/rev/b2aa9ec1a163
Change devtools/ to import and instantiate XHRs from global properties rather than using Cc.createInstance(Ci.nsIXMLHttpRequest), and the same for FormData; r=Honza
https://hg.mozilla.org/integration/autoland/rev/4d3bc187d05c
Change mobile/android/ to import and instantiate XHRs from global properties rather than using Cc.createInstance(Ci.nsIXMLHttpRequest); r=sebastian
https://hg.mozilla.org/integration/autoland/rev/eafbaf250b12
Change js/xpconnect to import and instantiate XHRs from global properties rather than using Cc.createInstance(Ci.nsIXMLHttpRequest); r=bz
https://hg.mozilla.org/integration/autoland/rev/ed441df27dad
Change browser/[components|modules|experiments] to import and instantiate XHRs from global properties rather than using Cc.createInstance(Ci.nsIXMLHttpRequest); r=mconley
https://hg.mozilla.org/integration/autoland/rev/27b15eb83e9d
Change asan-reporter bootstrap script to import and instantiate XHRs from global properties rather than using Cc.createInstance(Ci.nsIXMLHttpRequest); r=froydnj
https://hg.mozilla.org/integration/autoland/rev/56f9de9da69a
Change PdfStreamConverter.jsm to import and instantiate XHRs from global properties rather than using Cc.createInstance(Ci.nsIXMLHttpRequest); r=mossop
https://hg.mozilla.org/integration/autoland/rev/89ffff270fb7
Change pktApi.jsm to import and instantiate XHRs from global properties rather than using Cc.createInstance(Ci.nsIXMLHttpRequest); r=Gijs
https://hg.mozilla.org/integration/autoland/rev/b4d46a5196fe
Change test_SpecialPowersExtension.html to import and instantiate XHRs from global properties rather than using Cc.createInstance(Ci.nsIXMLHttpRequest); r=mrbkap
https://hg.mozilla.org/integration/autoland/rev/a4b33f7c71be
Migrate XMLHttpRequestMainThread away from needing nsIXMLHttpRequest's constants; r=baku
https://hg.mozilla.org/integration/autoland/rev/05e1fd3789a8
Migrate XMLHttpRequestWorker from using the nsIVariant send() XHR method; r=baku
https://hg.mozilla.org/integration/autoland/rev/53963f486f15
Migrate XMLHttpRequestWorker from using the XMLHttpRequestEventTarget and nsIXMLHttpRequestUpload interfaces; r=baku
https://hg.mozilla.org/integration/autoland/rev/9feca4755b7e
Migrate XMLHttpRequestWorker away from using other nsIXMLHttpRequest interfaces; r=baku
https://hg.mozilla.org/integration/autoland/rev/73a52aae80bc
Migrate nsXMLParseEndListener away from nsIXMLHttpRequest interfaces; r=baku
https://hg.mozilla.org/integration/autoland/rev/b23feec78952
Migrate BodyExtractor away from using nsIXHRSendable; r=baku
https://hg.mozilla.org/integration/autoland/rev/2c9c1b0ef8c3
Purge nsIXMLHttpRequest, nsIXMLHttpRequestUpload, nsIXMLHttpRequestEventTarget, nsIXHRSendable XPCOM interfaces; r=baku
https://hg.mozilla.org/integration/autoland/rev/88a0a583d4ca
use XMLHttpRequestBinding::STATE consts in the XHR subclasses and remove XMLHttpRequest::State enum class; r=baku
Keywords: checkin-needed
Pushed by archaeopteryx@coole-files.de:
https://hg.mozilla.org/integration/autoland/rev/560e1ce07505
switch to webidl XHR: add missing comma detected by eslint. r=eslint-fix on a CLOSED TREE