ServiceWorker synthesized headers break Content-Encoding

RESOLVED FIXED in Firefox 39

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: nsm, Assigned: nsm)

Tracking

(Blocks 1 bug)

unspecified
mozilla39
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox39 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

ServiceWorker intercepts a fetch, responds by calling fetch() and passing on received response. If the data was delivered using an encoding scheme such as gzip, the Content-Length in the headers does not reflect the actual content length that the Response's stream has (since that has already been decoded by Necko).

In addition, since the Content-Encoding header itself has to be preserved so that the page controlled by the SW sees the original headers, we need to disable double decoding when the response is received from an intercepted channel.
This patch does:
1) Sets ApplyConversion to false on the http channel when an interception occurs, because the Response already has a decoded stream.
2) Calculates the actual length to be expected from the Response's stream instead of using the Content-Length, so that we don't trip the assertion.

Ben, I know this doesn't deal with the 'keep body compressed so cache can benefit', but it is the simplest fix I can think of, and is required for v1. We can discuss alternatives later. Feel free to comment here with the approach you would like. Ok?
Attachment #8574944 - Flags: review?(josh)
Attachment #8574944 - Flags: review?(honzab.moz)
Assignee: nobody → nsm.nikhil
Status: NEW → ASSIGNED
Comment on attachment 8574944 [details] [diff] [review]
Disable content decoding and use decoded length on intercepted channels

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

This makes sense to me.

::: dom/workers/test/serviceworkers/fetch/fetch_tests.js
@@ +71,5 @@
> +  for (var i = 0; i < 10; ++i) {
> +    expectedResponse += "hello";
> +  }
> +  expectedResponse += "\n";
> +

nit: extraneous newline
Attachment #8574944 - Flags: review?(josh) → review+
Comment on attachment 8574944 [details] [diff] [review]
Disable content decoding and use decoded length on intercepted channels

Patrick, could you review this based on the email thread and comments here? Honza is out for a bit. Thanks!
Attachment #8574944 - Flags: review?(honzab.moz) → review?(mcmanus)
Comment on attachment 8574944 [details] [diff] [review]
Disable content decoding and use decoded length on intercepted channels

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

lgtm thanks. maybe a comment that this isn't streamable, due to the use of available() would help later when figuring out how to add that.
Attachment #8574944 - Flags: review?(mcmanus) → review+
Forgot to deal with the non-e10s case in the earlier patch.
In that case, we have to prevent cache entry from failing because of the length mismatch.
Relevant changes are in nsHttpChannel.cpp and InterceptedChannelChrome.
Attachment #8581210 - Flags: review?(mcmanus)
needs bkelly's patch from Bug 1110814 to prevent racing test failures
Depends on: 1110814
Attachment #8581210 - Flags: review?(mcmanus) → review+
Duplicate of this bug: 1146158
https://hg.mozilla.org/mozilla-central/rev/675bb5164535
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
You need to log in before you can comment on or make changes to this bug.