Currently we set HTTP headers like User-Agent, Host, etc in HttpBaseChannel::Init(): https://dxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/HttpBaseChannel.cpp#170 This means in non-e10s mode they are set before interception. In e10s mode, however, the headers are set in the parent process after interception. The fetch spec requires us to set them after interception. See steps 8 and 9 in HTTP-network-or-cache fetch: https://fetch.spec.whatwg.org/#http-network-or-cache-fetch This is a problem for bug 1188932 because the auto-generated User-Agent header now appears on FetchEvent.request. This in turn causes any fetch(event.request) calls to require a preflight which breaks previously working code.

Patrick, do you think its safe for us to move the AddStandardRequestHeaders() call to AsyncOpen() after interception occurs? We would obviously have to make headers explicitly set before AsyncOpen() take precedence of course. Just wondering if there are other assumptions in necko I'm not aware of, though.

We should write a wpt test for this as well.

This seems to fix the test which was failing for me with the User-Agent patch. I haven't run any other tests yet. let's see if anything breaks horribly: https://treeherder.mozilla.org/#/jobs?repo=try&revision=91a2ecf1ed04 I also haven't written the WPT for this yet (mainly because I A: don't know anything about service workers, let alone their API or how to write a WPT for them, and B: don't know how to write a WPT).

Comment on attachment 8655025 [details] [diff] [review] Add standard HTTP headers after interception in non-e10s mode Review of attachment 8655025 [details] [diff] [review]: ----------------------------------------------------------------- This looks reasonable to me. I think you should get feedback/review from Patrick or another necko peer, though.

Comment on attachment 8655025 [details] [diff] [review] Add standard HTTP headers after interception in non-e10s mode Clearing the review flag until I fix up a bunch of test failures.

Comment on attachment 8656260 [details] [diff] [review] Part 1: Add standard HTTP headers after interception in non-e10s mode nevermind - apparently there are even more test failures... https://treeherder.mozilla.org/#/jobs?repo=try&revision=70dd0209af06

Comment on attachment 8656263 [details] [diff] [review] Part 2: Add a Web Platform Test to check the visibility of default headers during interception Review of attachment 8656263 [details] [diff] [review]: ----------------------------------------------------------------- Overall looks good, but I think it would be a bit nicer to postMessage() the result text back from the iframe instead of passing the Response object cross globals back to the parent window. ::: testing/web-platform/mozilla/tests/service-workers/service-worker/fetch-header-visibility.https.html @@ +17,5 @@ > + return wait_for_state(t, reg.installing, 'activated'); > + }) > + .then(function() { > + var frame = document.createElement('iframe'); > + frame.src = scope;// + '?check-ua-header'; What is the comment here? Stale debugging stuff? @@ +25,5 @@ > + // during interception > + var with_ua = new Promise(function(resolve, reject) { > + window.withUAHelper = function(response) { > + response.text().then(function(text) { > + try { I don't think you need a try/catch here since you're already instead of a .then(). Any uncaught exceptions will automatically be converted to a promise rejection. @@ +38,5 @@ > + // during interception > + var no_ua = new Promise(function(resolve, reject) { > + window.noUAHelper = function(response) { > + response.text().then(function(text) { > + try { No try/catch here, either. ::: testing/web-platform/mozilla/tests/service-workers/service-worker/resources/fetch-header-visibility-iframe.html @@ +10,5 @@ > + var noUA = fetch(uri, {}); > + > + // Dispatch back to the parent iframe > + withUA.then(parent.withUAHelper); > + noUA.then(parent.noUAHelper); Can you just postMessage() the text content back to the parent instead of calling a function directly on the window? I think thats more consistent with the other tests and less likely to have weird side-effects on other browsers.

I _think_ this will finally actually pass tests! https://treeherder.mozilla.org/#/jobs?repo=try&revision=4a8777cc9107

Now uses postMessage instead of directly interacting with the parent's window object

Since we're changing allowed headers to XHR and fetch, we should flag this for updates on MDN.

Oh, thats the other bug.

(In reply to Ben Kelly [:bkelly] from comment #12) > Since we're changing allowed headers to XHR and fetch, we should flag this > for updates on MDN. That's technically bug 1188932, not this.

Comment on attachment 8656854 [details] [diff] [review] Part 1: Add standard HTTP headers after interception in non-e10s mode Review of attachment 8656854 [details] [diff] [review]: ----------------------------------------------------------------- This looks reasonable to me, but you really need a necko peer to review it. I suggest :mcmanus or :jduell. Thanks! ::: netwerk/protocol/http/nsHttpHeaderArray.h @@ +27,5 @@ > const char *PeekHeader(nsHttpAtom header) const; > > // Used by internal setters: to set header from network use SetHeaderFromNet > nsresult SetHeader(nsHttpAtom header, const nsACString &value, > + bool merge = false, bool isDefault = false); I realize there is a boolean already in the signature here, but I think it would be nicer to use enums for these values. Bare true/false literals are hard to understand when reading the code. @@ +49,5 @@ > { > return FindHeaderValue(header, value) != nullptr; > } > > + nsresult VisitHeaders(nsIHttpHeaderVisitor *visitor, bool skipDefault = false); Again, I think enums are easier to read. @@ +70,5 @@ > > // Must be copy-constructable and assignable > struct nsEntry > { > + nsEntry() : isDefault(false) {} In theory we allow this style now: bool isDefault = false; Instead of defining a default constructor. ::: netwerk/protocol/http/nsIHttpChannel.idl @@ +143,5 @@ > + * > + * @param aVisitor > + * the header visitor instance. > + */ > + void visitNonDefaultRequestHeaders(in nsIHttpHeaderVisitor aVisitor); You need to update the uuid for the interface.

Comment on attachment 8656856 [details] [diff] [review] Part 2: Add a Web Platform Test to check the visibility of default headers during interception Review of attachment 8656856 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. Just a couple nits. r=me ::: testing/web-platform/mozilla/tests/service-workers/service-worker/resources/fetch-header-visibility-iframe.html @@ +9,5 @@ > + var withUA = fetch(uri, { headers: headers }); > + var noUA = fetch(uri, {}); > + > + // Tell the parent what's going on! > + withUA.then(function(response) { I think it would be more straightforward to do the fetch() inline here and below. And maybe change the comment from "tell the parent" to "check the custom UA case" and "check the default US case". @@ +21,5 @@ > + }).catch(function(err) { > + parent.postMessage('withUA FAIL - unexpected error: ' + err, '*'); > + }); > + > + noUA.then(function(response) { fetch() inline here.

Comment on attachment 8657203 [details] [diff] [review] Part 1: Don't expose standard HTTP headers during interception in non-e10s mode Review of attachment 8657203 [details] [diff] [review]: ----------------------------------------------------------------- sorry about being slow here on the review ::: netwerk/protocol/http/nsHttpHeaderArray.cpp @@ +48,2 @@ > } else if (merge && !IsSingletonHeader(header)) { > MergeHeader(header, entry, value); I think you need to change the variety field on the entry in this case, no?

(In reply to Patrick McManus [:mcmanus] from comment #19) > ::: netwerk/protocol/http/nsHttpHeaderArray.cpp > @@ +48,2 @@ > > } else if (merge && !IsSingletonHeader(header)) { > > MergeHeader(header, entry, value); > > I think you need to change the variety field on the entry in this case, no? MergeHeader changes the variety field in its implementation. I believe that that should cover this case.

(In reply to Michael Layzell [:mystor] from comment #20) > (In reply to Patrick McManus [:mcmanus] from comment #19) > > ::: netwerk/protocol/http/nsHttpHeaderArray.cpp > > @@ +48,2 @@ > > > } else if (merge && !IsSingletonHeader(header)) { > > > MergeHeader(header, entry, value); > > > > I think you need to change the variety field on the entry in this case, no? > > MergeHeader changes the variety field in its implementation. I believe that > that should cover this case. got it.. I expected to see it as an arg, but that makes sense to just set it to override.

https://hg.mozilla.org/integration/mozilla-inbound/rev/2cbbc360ec41 https://hg.mozilla.org/integration/mozilla-inbound/rev/10343dcbe03d

https://hg.mozilla.org/mozilla-central/rev/2cbbc360ec41 https://hg.mozilla.org/mozilla-central/rev/10343dcbe03d