Closed Bug 1237455 Opened 9 years ago Closed 9 years ago

fetch() function not always applying Headers to outbound requests.

Categories

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

46 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla46
Tracking Status
firefox42 --- wontfix
firefox43 --- wontfix
firefox44 --- wontfix
firefox45 --- fixed
firefox46 --- fixed
firefox-esr38 --- unaffected

People

(Reporter: jrconlin, Assigned: bkelly)

References

()

Details

Attachments

(5 files, 4 obsolete files)

2.17 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
2.53 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
4.93 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
3.93 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
5.18 KB, patch
bkelly
: review+
Details | Diff | Splinter Review
Using Nightly on either x64 Linux or Windows, I've noticed that fetch() does not always send out headers. It usually fails. I have tried setting headers as either a Headers object, or as a standard object, both fail equally.

Steps to reproduce:
1) visit https://evilonastick.com/push 
2) You will be asked if you wish to receive notifications, please accept.
3) Open the Web Console and refresh the page. 
Note the following lines:
```
Fetching: "https://updates.push.services.mozilla.com..." Object { method: "POST", headers: Object, body: Uint8Array[36], salt: Uint8Array[16], dh: ArrayBuffer, endpoint: "https://updates.push.services.mozil…", pheaders: Object } webpush.js:297:9
```
This is the arguments that are to be sent to the fetch() command. Using the console, you can verify that the headers are present.

```
Encryption-Key header keyid=p256dh;dh=BK9x_zoXeJ5qP4fzZDGgc_pPw6DMqvT_c2H0KuBHD7tvEvlKUK2CE3W_Pn6FdBj-Z1TsQwkK4vRPvio411L479A webpush.js:298:9
```
This is a further sanity check that the required header is indeed present.

```
Server returned 400. Probably missing headers. Try refreshing to see if fetch() will send them. webpush.js:303:25

Send Failed:  Error: fetch() failed to include headers. Refresh
Stack trace:
webpush/</<@https://evilonastick.com/push/webpush.js:306:31
promise callback*webpush/<@https://evilonastick.com/push/webpush.js:299:9
promise callback*webpush@https://evilonastick.com/push/webpush.js:241:12
promise callback*@https://evilonastick.com/push/:305:1
 webpush.js:313:18
```
And that's fetch failing. The server returns error code 400 if headers are not present.

You can examine the outbound Request object to note that the headers are not being sent. 

Please note that the page works fine using Firefox 45.
Component: General → DOM
Product: Firefox → Core
Flags: needinfo?(bkelly)
I suspect the AsyncOpen2() refactor of fetch().  Leaving need info to remind me to investigate.
Note, this is doing a CORS request with a preflight.  The CORS headers look correct to me.
> Please note that the page works fine using Firefox 45.

The AsyncOpen2 change landed in Gecko 44, so if this worked in 45, then that's not the problem.

We do have tests which set headers, and those tests are passing, so at least things work under some circumstances.

Is your test by any chance ever redirecting the request using a 302 (or similar) response? Looking at the fetch() implementation, I would expect added headers to disappear upon redirect.
Hmm.. If this is a CORS request with preflight, then you're likely not doing a redirect, so that's likely not the problem.
The server doesn't do redirects. What the server does do is check if the incoming request contains "encryption", "encryption-key" and "content-type" headers. If those aren't present, then it returns a 400. 

Like I said, I'm able to pull up the outbound request using the web console and see that the headers aren't being included in the call, even though they're included in the fetch options object.
Hey folks, this is a pretty significant issue -- but at least I see it consistently in Fennec, giving me hope it can be addressed easily.  jrconlin, could you extract a smaller test case that does not rely on Push in Fennec?  Perhaps you don't really need Push at all to see this on your existing page (https://evilonastick.com/push), but I haven't verified.
Flags: needinfo?(jrconlin)
Oddly, I'm not able to reproduce this using a simple test page. Even more oddly, I note that there are more headers going out for the simple test page than the push page. 

e.g. for the WebPush page:
Host: updates.push.services.mozilla.com
   User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:46.0) Gecko/20100101 Firefox/46.0
   Accept: */*
   Accept-Language: en-US,en;q=0.5
   Accept-Encoding: gzip, deflate, br
   Content-Length: 36
   Referer: https://evilonastick.com/push_dev/
   Connection: keep-alive

vs. the simple test page:
 + Host: updates.push.services.mozilla.com
   User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:46.0) Gecko/20100101 Firefox/46.0
   Accept: */*
   Accept-Language: en-US,en;q=0.5
   Accept-Encoding: gzip, deflate, br
   encryption-key: keyid=p256dh;dh=RandoStringOfCrap-gZ4xNvJnSUd6xd8MWpiZ0l8HN5jGb-52bM0z2nQoos7WArEUQ59gwpq4NIbZ4qNmgpPwE
   encryption: keyid=p256;salt=AnotherRandoString1234
   Content-Encoding: aesgcm128
 + Content-Type: text/plain;charset=UTF-8
   Referer: http://192.168.40.128/fetchTest/
 + Origin: http://192.168.40.128
   Content-Length: 29
   Connection: keep-alive

I'm not sure why there's a difference between the two, or why the webpush fetcher is not including the additional headers. 

Will continue to poke at both to see what I can determine might be causing the problem.
Flags: needinfo?(jrconlin)
Ok, REALLY odd. 

Not sure if the problem has gone away or if this is a work around, but I've applied the following and things seem to work. Sort of.

If I compose the Uint8Array as a string buffer, that seems to let the headers get added. 

e.g.

   let body = "";
   results.payload.forEach(x=>{ body += String.fromCharCode(x); });

and then passing body as the body element of options. 

This works if I shift reload the page. 

If I just reload, however, the headers are not applied. (Shift reloading again still resolves correctly). 

https://jrconlin.github.io/WebPushDataTestPage/
has been updated with the latest changes.
FWIW this is still on my list to investigate.  It's just been a hectic week.  Sorry for the delay.
I can reproduce this... Sometimes refreshing gives me headers and sometimes it doesn't.  Very odd.
Assignee: nobody → bkelly
Status: NEW → ASSIGNED
Flags: needinfo?(bkelly)
This is whats happening:

1) Because there is a service worker registered we are firing a fetch event.
2) The fetch event is not intercepted because the SW is push-only.
3) FetchEventRunnable::ResumeRequest::Run() triggers an internal redirect on the channel to start the processing again.
4) The internal redirect does not appears to copy the non-standard headers to the new channel.

This is why a shift-reload works.  It bypasses the service worker.

Seems like we should be preserving all headers on these redirects...
:bkelly, Oh, man! Good hunting on that one. That also explains why the stand-alone wasn't reproducing the error. Thanks!
Patrick suggests that internal redirects should always preserve headers.  I will try writing a patch for that and a test for this particular method of triggering it in service workers.
I just tested that this is happening as far back as beta 44.  We should consider uplifting it there if there is time.  Otherwise, we'll just get it into 45 and let the trains ride.
Side question:  Is there a reason your website is trying to redirect https requests to http?

  https://www.dropbox.com/s/vgsyq0qgwbr0fky/Screenshot%202016-01-15%2012.53.59.png?dl=0

I guess maybe its the /push to /push/ normalization?
Flags: needinfo?(jrconlin)
Yeah, I think that's a normalization error. I'll see if I can fix things up to not do that.
Flags: needinfo?(jrconlin)
(In reply to JR Conlin [:jrconlin,:jconlin] from comment #16)
Hrm, might be a cloudproxy issue confusing things. I note that going to https://evilonastick.com/push/ does not bring in http resources, nor try to redirect to http. 

The page doesn't enforce either http or https for any link, relying on the default page protocol.
Honza, do you know why internal redirects do not copy the original channel's headers?  It seems odd we would strip the site's custom headers from HSTS upgrades, for example.
Flags: needinfo?(honzab.moz)
Also note that this only happens with non-e10s service workers because in e10s mode we "resume" the channel in a different way.
It seems headers are propagated across redirects by client redirect callbacks.  For example, xhr sets the headers explicitly here:

  https://dxr.mozilla.org/mozilla-central/source/dom/base/nsXMLHttpRequest.cpp#3356

We don't get this callback for the internal redirect.

Also, I think there is a separate bug that fetch() does not propagate headers on redirects.

I'm going to write some tests.
This test shows we fail to send headers for normal redirects in fetch().
This test reroutes test_fetch_cors.js through an empty service worker which exposes the issue in comment 0.  All requests end up going through the ResumeRequest() code path.

We have about 139 failures in the test with this empty service worker.
Since FetchDriver gets redirect callbacks, I don't have to do anything special for internal redirects.
Flags: needinfo?(honzab.moz)
Comment on attachment 8709449 [details] [diff] [review]
P1 Make file_CrossSiteXHR_server.sjs check headers on redirects. r=ehsan

This improves our main cors/header sjs test fixture by:

1) Making it give a 500 response if a header is missing instead of throwing in the sjs.  This is basically just for better error messages.
2) Propagating the expected headers when redirecting through hops.
Attachment #8709449 - Flags: review?(ehsan)
Comment on attachment 8709451 [details] [diff] [review]
P2 Test headers on redirects in fetch mochitests. r=ehsan

This makes us specify expected headers in test_fetch_cors.js for redirects.  There were already headers in the cors case, but I had to add a test case for no-cors.
Attachment #8709451 - Flags: review?(ehsan)
Comment on attachment 8709452 [details] [diff] [review]
P3 Add a version of test_fetch_cors that reroutes through an empty service worker. r=ehsan

Add new tests that are similar to the *_sw_reroute.html tests, but instead of doing evt.respondWith(fetch(evt.request)) it instead uses an empty service worker.  This tests the internal redirect used by ResumeRequest() and reflects what the original reporter's push-only service worker was doing.
Attachment #8709452 - Flags: review?(ehsan)
Comment on attachment 8709453 [details] [diff] [review]
P4 Create helper method to set fetch request headers. r=ehsan

Refactor by moving header setting code into a separate method.

Note, this has a slight functional change in that we silently skip setting the origin header if nsContentUtils::GetUTFOrigin() fails.  The original code rejected the fetch().  I can restore that behavior if you like, but I don't think it really matters in practice.
Attachment #8709453 - Flags: review?(ehsan)
Comment on attachment 8709454 [details] [diff] [review]
P5 Set headers on fetch() redirects. r=ehsan

This puts the FetchDriver::AsyncOnChannelRedirected() callback back in place so we can set headers on redirect.  I basically restored the previous setup code and just modified it to call the new SetRequestHeaders() helper method.
Attachment #8709454 - Flags: review?(ehsan)
Attachment #8709449 - Flags: review?(ehsan) → review+
Attachment #8709451 - Flags: review?(ehsan) → review+
Comment on attachment 8709452 [details] [diff] [review]
P3 Add a version of test_fetch_cors that reroutes through an empty service worker. r=ehsan

Review of attachment 8709452 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/tests/mochitest/fetch/mochitest.ini
@@ +30,5 @@
>  [test_fetch_basic.html]
>  skip-if = (e10s && debug && os == 'win')
>  [test_fetch_basic_sw_reroute.html]
>  skip-if = buildapp == 'b2g' || (e10s && debug && os == 'win') # Bug 1137683
> +[test_fetch_basic_sw_empty_reroute.html]

I wouldn't be surprised if you would need to copy the above skip-if here.  This will definitely not work on b2g at least.

@@ +35,5 @@
>  [test_fetch_basic_http.html]
>  skip-if = (e10s && debug && os == 'win')
>  [test_fetch_basic_http_sw_reroute.html]
>  skip-if = buildapp == 'b2g' || (e10s && debug && os == 'win') # Bug 1137683
> +[test_fetch_basic_http_sw_empty_reroute.html]

Ditto.

@@ +40,5 @@
>  [test_fetch_cors.html]
>  skip-if = buildapp == 'b2g' || (toolkit == 'android' && debug) || (e10s && debug && os == 'win') # Bug 1210552 && 1210282
>  [test_fetch_cors_sw_reroute.html]
>  skip-if = buildapp == 'b2g' || (toolkit == 'android' && debug) || (e10s && debug && os == 'win') # Bug 1137683 && 1210282
> +[test_fetch_cors_sw_empty_reroute.html]

And this.

::: dom/tests/mochitest/fetch/sw_empty_reroute.js
@@ +16,5 @@
>              ["dom.serviceWorkers.testing.enabled", true],
>              ["dom.serviceWorkers.exemptFromPerDomainMax", true]]
>    }, function() {
>      navigator.serviceWorker.ready.then(setupSW);
> +    navigator.serviceWorker.register("empty.js", {scope: "/"});

Can you please add a comment to the beginning of sw_reroute.js to mention that if that file is changed, this one should probably change as well, and vice versa?

Even better, you can avoid the need for sw_empty_reroute.js completely by improving the logic in testScript.  For example, here you can decide the URL to the service worker by checking |location.href.includes("sw_empty_reroute.html")| and use "empty.js" instead of "reroute.js" in that case.

Either works for me!
Attachment #8709452 - Flags: review?(ehsan) → review+
Attachment #8709453 - Flags: review?(ehsan) → review+
Comment on attachment 8709454 [details] [diff] [review]
P5 Set headers on fetch() redirects. r=ehsan

Review of attachment 8709454 [details] [diff] [review]:
-----------------------------------------------------------------

Can you also file a Necko follow-up bug to clarify what should happen in the case of other internal redirects, such as the ones used for secure upgrades?  Thanks!
Attachment #8709454 - Flags: review?(ehsan) → review+
(In reply to :Ehsan Akhgari from comment #36)
> Can you also file a Necko follow-up bug to clarify what should happen in the
> case of other internal redirects, such as the ones used for secure upgrades?
> Thanks!

This should be automatically handled by the code making the network request via the normal AsyncOnChannelRedirect() callback.  If a caller, like xhr or fetch, is adding headers for normal redirects then they also add them for internal redirects.  So its totally up to the behavior the creator of the channel wants.  I don't think we have to do anything special for internal redirects in necko.

Does that make sense?
Flags: needinfo?(ehsan)
(In reply to :Ehsan Akhgari from comment #35)
> >  [test_fetch_basic_sw_reroute.html]
> >  skip-if = buildapp == 'b2g' || (e10s && debug && os == 'win') # Bug 1137683
> > +[test_fetch_basic_sw_empty_reroute.html]
> 
> I wouldn't be surprised if you would need to copy the above skip-if here. 
> This will definitely not work on b2g at least.

Try suggests I only need to skip b2g.
Comment on attachment 8709449 [details] [diff] [review]
P1 Make file_CrossSiteXHR_server.sjs check headers on redirects. r=ehsan

Approval Request Comment
[Feature/regressing bug #]: Service workers and fetch()
[User impact if declined]: Users will see broken behavior on sites that use custom headers.  Even if their service worker is for push events and does not intercept, the headers will be stripped from fetch() network requests.  Similarly, any fetch() request that gets redirected will have custom headers stripped.
[Describe test coverage new/current, TreeHerder]:  Tests included.
[Risks and why]: Minimal.  Only effects service workers and fetch().
[String/UUID change made/needed]: None.
Attachment #8709449 - Flags: approval-mozilla-beta?
Attachment #8709449 - Flags: approval-mozilla-aurora?
Comment on attachment 8709451 [details] [diff] [review]
P2 Test headers on redirects in fetch mochitests. r=ehsan

See comment 41.
Attachment #8709451 - Flags: approval-mozilla-beta?
Attachment #8709451 - Flags: approval-mozilla-aurora?
Comment on attachment 8709453 [details] [diff] [review]
P4 Create helper method to set fetch request headers. r=ehsan

See comment 41.
Attachment #8709453 - Flags: approval-mozilla-beta?
Attachment #8709453 - Flags: approval-mozilla-aurora?
Comment on attachment 8709454 [details] [diff] [review]
P5 Set headers on fetch() redirects. r=ehsan

See comment 41.
Attachment #8709454 - Flags: approval-mozilla-beta?
Attachment #8709454 - Flags: approval-mozilla-aurora?
Comment on attachment 8709631 [details] [diff] [review]
P3 Add a version of test_fetch_cors that reroutes through an empty service worker. r=ehsan

See comment 41.
Attachment #8709631 - Flags: approval-mozilla-beta?
Attachment #8709631 - Flags: approval-mozilla-aurora?
The fetch() redirect bits still affect FF39 to FF43.  The service worker issue does not become a problem until it ships in FF44.
Hi Ben, did you mean for these to get uplifted to Beta44? We are in RC mode and 4 days away from going live. Unless these are ship-stoppers, I cannot take them in Fx44. And even if they were ship-stoppers, this late in the cycle, I would suggest disabling Service Workers feature instead of taking a complex fix like this. Sorry!
Flags: needinfo?(bkelly)
Comment on attachment 8709449 [details] [diff] [review]
P1 Make file_CrossSiteXHR_server.sjs check headers on redirects. r=ehsan

This does not meet the Fx44 RC bar, please see comment 47.
Attachment #8709449 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
Comment on attachment 8709451 [details] [diff] [review]
P2 Test headers on redirects in fetch mochitests. r=ehsan

This does not meet the Fx44 RC bar, please see comment 47.
Attachment #8709451 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
Comment on attachment 8709453 [details] [diff] [review]
P4 Create helper method to set fetch request headers. r=ehsan

This does not meet the Fx44 RC bar, please see comment 47.
Attachment #8709453 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
Comment on attachment 8709454 [details] [diff] [review]
P5 Set headers on fetch() redirects. r=ehsan

This does not meet the Fx44 RC bar, please see comment 47.
Attachment #8709454 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
Comment on attachment 8709631 [details] [diff] [review]
P3 Add a version of test_fetch_cors that reroutes through an empty service worker. r=ehsan

This does not meet the Fx44 RC bar, please see comment 47.
Attachment #8709631 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
No problem on the beta-.  I just wanted to ask in case.
Flags: needinfo?(bkelly)
(In reply to Ben Kelly [:bkelly] from comment #37)
> (In reply to :Ehsan Akhgari from comment #36)
> > Can you also file a Necko follow-up bug to clarify what should happen in the
> > case of other internal redirects, such as the ones used for secure upgrades?
> > Thanks!
> 
> This should be automatically handled by the code making the network request
> via the normal AsyncOnChannelRedirect() callback.  If a caller, like xhr or
> fetch, is adding headers for normal redirects then they also add them for
> internal redirects.  So its totally up to the behavior the creator of the
> channel wants.  I don't think we have to do anything special for internal
> redirects in necko.
> 
> Does that make sense?

Depends on what the intended API is.  I still find it very surprising that the default behavior in the case of redirects is to lose the request headers.  We definitely have quite a few call sites that do not setup AsyncOnChannelRedirect() callbacks to propagate the headers, so whatever the intention is, our code is not expecting this in many places.

<https://dxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#550>
<https://dxr.mozilla.org/mozilla-central/source/dom/base/nsScriptLoader.cpp#336>
<https://dxr.mozilla.org/mozilla-central/source/dom/html/HTMLMediaElement.cpp#4487>

(Note that these are just a few example call sites I found in <1 minute.  I didn't do an exhastive search.)
Flags: needinfo?(ehsan)
(Having the Necko discussion in a place with Necko folks is probably better!  I filed bug 1240929 myself.)
(In reply to :Ehsan Akhgari from comment #54)
> Depends on what the intended API is.  I still find it very surprising that
> the default behavior in the case of redirects is to lose the request
> headers.  We definitely have quite a few call sites that do not setup
> AsyncOnChannelRedirect() callbacks to propagate the headers, so whatever the
> intention is, our code is not expecting this in many places.
> 
> <https://dxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.
> cpp#550>
> <https://dxr.mozilla.org/mozilla-central/source/dom/base/nsScriptLoader.
> cpp#336>
> <https://dxr.mozilla.org/mozilla-central/source/dom/html/HTMLMediaElement.
> cpp#4487>
> 
> (Note that these are just a few example call sites I found in <1 minute.  I
> didn't do an exhastive search.)

If these sites require non-standard headers to be propagates across redirects then they should be doing so via a redirect callback.  Its pretty clear to me thats the API.

If you want to argue this should change, feel free to file a bug against necko.  But I don't really see a problem with the current situation.

For example, in this case I think its a media bug that it twiddles headers, but doesn't handle that on redirect.  Of course, I have no idea if these media channels even support redirects.
(In reply to Ben Kelly [:bkelly] from comment #56)
> If these sites require non-standard headers to be propagates across
> redirects then they should be doing so via a redirect callback.  Its pretty
> clear to me thats the API.
> 
> If you want to argue this should change, feel free to file a bug against
> necko.  But I don't really see a problem with the current situation.
> 
> For example, in this case I think its a media bug that it twiddles headers,
> but doesn't handle that on redirect.  Of course, I have no idea if these
> media channels even support redirects.

Responded in bug 1240929 comment 1.
Depends on: 1241020
Comment on attachment 8709631 [details] [diff] [review]
P3 Add a version of test_fetch_cors that reroutes through an empty service worker. r=ehsan

Let's take them in aurora/45.
Attachment #8709631 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8709454 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8709453 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8709451 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8709449 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
ben, this has problems uplifting to aurora, could you rebase this for aurora ? thanks!
Flags: needinfo?(bkelly)
Flags: needinfo?(bkelly) → needinfo?(ehsan)
Flags: needinfo?(ehsan)
Can verify that things are working on the test page. Thanks all!
Tested this in a couple of firefox versions and am concerned this bug still affects Firefox 45.

45.0b1 released today fails for me (1243453):

OK:
2016-01-25-00-40-10-mozilla-aurora/firefox-45.0a2.en-US.linux-x86_64
2016-01-28-00-40-12-mozilla-aurora/firefox-46.0a2.en-US.linux-x86_64
2016-01-28-03-02-08-mozilla-central/firefox-47.0a1.en-US.linux-x86_64
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: