This bug is for a patchset I've been working on which adds a new implementation of XMLHttpRequest, XMLHttpRequestWeb, which is meant to: - handle both worker and non-worker cases in one class (by using FetchDriver with custom FetchResolvers). - match the XHR spec text as closely as possible (also including some spec fixes for our fetch code). - not provide extra Gecko-specific interfaces such as raw channel access, mozAnon, or -moz-chunked-arraybuffer. - fall back to the old classes for system XHRs, to help reduce churn until they can be removed. There are still some differences between this implementation and the XHR spec-text. I've been filing spec-bugs where appropriate to clear things up (with TODOs in the code), but there are still some things that differ due to other implementation quirks in Gecko, the more noteworthy being: - the sync code-paths differ a bit because we use syncloops to achieve sync XHRs, which don't quite match the spec (especially since DOM promises don't seem to work in syncloops). - rather than opening a ReadableStream to read the response, I use the existing FetchDriverObserver interface, so the code has some subtle differences due to the events it sends not quite matching the spec-text. Which ties into the next point: - the spec waits for the user to read the response before parsing it as a document or text; we've historically decoded it on-the-fly. I continue to do this (which complicates the code a bit, since the fetch driver doesn't handle text/documents the way XHRs do, so I use refactored versions of that code from the existing main-thread XHR class). There are still a few test-failures left that I can't quite figure out on my own (see https://treeherder.mozilla.org/#/jobs?repo=try&revision=d621b79361f9e0c6424f02f4a6909377bb185317): - layout/reftests/scrolling/deferred-anchor.xhtml: This is apparently related to the XHR's load group being canceled on document Stop(). The XMLHttpRequestMainThread class also hits this code-path, but its request is not (or is no longer) in the load group at that time, so it does not get canceled. I'm not familiar enough with load groups to know what's causing this difference. - /FileAPI/url/sandboxed-iframe.html /FileAPI/url/url-with-xhr.any.html /FileAPI/url/url-with-xhr.any.worker.html These are all failing because of the same actual test (the final test in the latter 2 files). That test xhr.opens a blob: url, then revokes it before calling xhr.send. Somehow, my code doesn't keep a strong reference to the blob, though I can't tell why. It may be because the XMLHttpRequestMainThread class uses an nsIURI instead of a MozURL to store mRequestURI, but just changing to an nsIURI isn't doing the trick in my class. - the service worker cache ignoreVary WPTs are failing, presumably due to a spec-change (fetch #609). If I do not clear the Content-Disposition header, then they continue to work, but I couldn't quite pin down what the issue is. (I've marked these as failing for now in one of the patches to reduce noise in my try-pushes). In addition to fixing any remaining test failures, I can think of at least three things we'd likely want to change before landing this: - gate this new XHR implementation behind a pref. - figure out if we should migrate from using a worker holder (as per baku's recent work). - perhaps see if we can skip using MozURL, given the recent work to make nsIURI thread-safe. Right now NS_NewURI only works on the main thread, so I used MozURL instead (though I had to patch it to support passing in encodings, and my Rust-fu is rather weak so my attempt likely isn't worthy of landing without a lot of work). But since this is already in some workable state, I've decided to request feedback now, rather than dragging things out indefinitely. (I'll post the actual patches ASAP on mozreview).

Comment on attachment 8963178 [details] Bug 1449613 part 4 - Generalize fetch body consumer observers so they can work with syncloops and specify blob mimetypes. https://reviewboard.mozilla.org/r/232024/#review237574 Code analysis found 4 defects in this patch: - 4 defects found by clang-tidy You can run this analysis locally with: - `./mach static-analysis check path/to/file.cpp` (C/C++) If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: dom/fetch/FetchConsumer.cpp:225 (Diff revision 1) > template <class Derived> > -class AbortConsumeBodyControlRunnable final : public MainThreadWorkerControlRunnable > +class AbortConsumeBodyControlRunnable : public SyncOrNonSyncRunnable<Derived> > { > RefPtr<FetchBodyConsumer<Derived>> mFetchBodyConsumer; > > + AbortConsumeBodyControlRunnable<Derived>( Error: Bad implicit conversion constructor for 'abortconsumebodycontrolrunnable' [clang-tidy: mozilla-implicit-constructor] ::: dom/fetch/FetchConsumer.cpp:225 (Diff revision 1) > template <class Derived> > -class AbortConsumeBodyControlRunnable final : public MainThreadWorkerControlRunnable > +class AbortConsumeBodyControlRunnable : public SyncOrNonSyncRunnable<Derived> > { > RefPtr<FetchBodyConsumer<Derived>> mFetchBodyConsumer; > > + AbortConsumeBodyControlRunnable<Derived>( Error: Bad implicit conversion constructor for 'abortconsumebodycontrolrunnable' [clang-tidy: mozilla-implicit-constructor] ::: dom/fetch/FetchConsumer.cpp:344 (Diff revision 1) > -class AbortConsumeBlobBodyControlRunnable final > +class AbortConsumeBlobBodyControlRunnable : public SyncOrNonSyncRunnable<Derived> > - : public MainThreadWorkerControlRunnable > { > RefPtr<FetchBodyConsumer<Derived>> mFetchBodyConsumer; > > -public: > + AbortConsumeBlobBodyControlRunnable<Derived>( Error: Bad implicit conversion constructor for 'abortconsumeblobbodycontrolrunnable' [clang-tidy: mozilla-implicit-constructor] ::: dom/fetch/FetchConsumer.cpp:344 (Diff revision 1) > -class AbortConsumeBlobBodyControlRunnable final > +class AbortConsumeBlobBodyControlRunnable : public SyncOrNonSyncRunnable<Derived> > - : public MainThreadWorkerControlRunnable > { > RefPtr<FetchBodyConsumer<Derived>> mFetchBodyConsumer; > > -public: > + AbortConsumeBlobBodyControlRunnable<Derived>( Error: Bad implicit conversion constructor for 'abortconsumeblobbodycontrolrunnable' [clang-tidy: mozilla-implicit-constructor]

Thank you for all the hard work here, Tom!

Can you please enumerate the patches? Just to review them in the correct order. Just add a "part X". Thanks!

Definitely, though the patches are already in the correct order in Bugzilla, if you'd like to start taking a quick look already.

Comment on attachment 8963176 [details] Bug 1449613 - Enable support for ReadableStream bodies in the fetch driver and for XMLHttpRequests. https://reviewboard.mozilla.org/r/232020/#review237614 ::: dom/fetch/BodyExtractor.cpp:173 (Diff revision 1) > + nsIGlobalObject* aGlobal) const > +{ > + MOZ_ASSERT(aCx); > + MOZ_ASSERT(aGlobal); > + > + *aContentLength = -1; This is wrong. We are not able to send streams at the moment because we do not support unlimited upload streams yet. This is the reason why you cannot use ReadableStream with Fetch, Request and so on, but just Response. Can you remove this patch from this bug? I do think you want ReadableStream in necko be a blocker for XHR refactoring.

Comment on attachment 8963177 [details] Bug 1449613 part 2 - Fix fetch header support to better-match the spec: https://reviewboard.mozilla.org/r/232022/#review237618 If this is a bug in the current implementation, maybe we can file a separate bug for it and land this patch immediately. What do you think?

Comment on attachment 8963184 [details] Bug 1449613 part 1 - Constify a couple of DOMEventTargetHelper methods. https://reviewboard.mozilla.org/r/232036/#review237632 Also this patch can land immediately.

>Can you remove this patch from this bug? Yes, I'll do so. >Maybe we can file a separate bug for it and land this patch immediately. >Also this patch can land immediately. Sure, let's split those off. (I'll file bugs ASAP).

Alright, I've removed the ReadableStream patch from this set, rebased, fixed the commit messages, fixed the missed "explicit" the commit bot pointed out, and placed the two patches that will be split out at the start of the set. I'll split those two patches into separate bugs the next chance I have.

In some good news, I figured out what was causing the ignoreVary serviceworker cache WPT failures while I was splitting off the fetch-header patch into its own bug. So that leaves us with the reftest and blob URL failures in comment 0 (and any others my try-run may have missed).