Closed
Bug 1200337
Opened 9 years ago
Closed 9 years ago
standard HTTP headers are added before interception in non-e10s mode
Categories
(Core :: DOM: Service Workers, defect)
Core
DOM: Service Workers
Tracking
()
RESOLVED
FIXED
mozilla43
People
(Reporter: bkelly, Assigned: nika)
References
Details
Attachments
(2 files, 5 obsolete files)
15.29 KB,
patch
|
mcmanus
:
review+
|
Details | Diff | Splinter Review |
6.79 KB,
patch
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Comment 1•9 years ago
|
||
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.
Flags: needinfo?(mcmanus)
Reporter | ||
Comment 2•9 years ago
|
||
We should write a wpt test for this as well.
Assignee | ||
Comment 3•9 years ago
|
||
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).
Attachment #8655025 -
Flags: feedback?(bkelly)
Reporter | ||
Comment 4•9 years ago
|
||
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.
Attachment #8655025 -
Flags: feedback?(bkelly) → feedback+
Assignee | ||
Updated•9 years ago
|
Attachment #8655025 -
Flags: review?(mcmanus)
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → michael
Updated•9 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 5•9 years ago
|
||
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.
Attachment #8655025 -
Flags: review?(mcmanus)
Assignee | ||
Comment 6•9 years ago
|
||
Attachment #8655025 -
Attachment is obsolete: true
Attachment #8656260 -
Flags: review?(bkelly)
Assignee | ||
Comment 7•9 years ago
|
||
Attachment #8656263 -
Flags: review?(bkelly)
Assignee | ||
Comment 8•9 years ago
|
||
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
Attachment #8656260 -
Flags: review?(bkelly)
Reporter | ||
Comment 9•9 years ago
|
||
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.
Attachment #8656263 -
Flags: review?(bkelly)
Assignee | ||
Comment 10•9 years ago
|
||
I _think_ this will finally actually pass tests! https://treeherder.mozilla.org/#/jobs?repo=try&revision=4a8777cc9107
Attachment #8656260 -
Attachment is obsolete: true
Attachment #8656854 -
Flags: review?(bkelly)
Assignee | ||
Comment 11•9 years ago
|
||
Now uses postMessage instead of directly interacting with the parent's window object
Attachment #8656263 -
Attachment is obsolete: true
Attachment #8656856 -
Flags: review?(bkelly)
Reporter | ||
Comment 12•9 years ago
|
||
Since we're changing allowed headers to XHR and fetch, we should flag this for updates on MDN.
Keywords: dev-doc-needed
Assignee | ||
Comment 14•9 years ago
|
||
(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.
Reporter | ||
Comment 15•9 years ago
|
||
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.
Attachment #8656854 -
Flags: review?(bkelly) → feedback+
Reporter | ||
Comment 16•9 years ago
|
||
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.
Attachment #8656856 -
Flags: review?(bkelly) → review+
Assignee | ||
Comment 17•9 years ago
|
||
Attachment #8656854 -
Attachment is obsolete: true
Attachment #8657203 -
Flags: review?(mcmanus)
Assignee | ||
Comment 18•9 years ago
|
||
Attachment #8656856 -
Attachment is obsolete: true
Comment 19•9 years ago
|
||
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?
Attachment #8657203 -
Flags: review?(mcmanus)
Updated•9 years ago
|
Flags: needinfo?(mcmanus)
Updated•9 years ago
|
tracking-e10s:
--- → -
Assignee | ||
Comment 20•9 years ago
|
||
(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.
Assignee | ||
Updated•9 years ago
|
Attachment #8657203 -
Flags: review?(mcmanus)
Comment 21•9 years ago
|
||
(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.
Updated•9 years ago
|
Attachment #8657203 -
Flags: review?(mcmanus) → review+
Comment 22•9 years ago
|
||
Comment 23•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/2cbbc360ec41
https://hg.mozilla.org/mozilla-central/rev/10343dcbe03d
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox43:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
You need to log in
before you can comment on or make changes to this bug.
Description
•