Closed Bug 462707 Opened 11 years ago Closed 11 years ago

nsHttpChannel::GetEntityID should respect Accept-Ranges response header

Categories

(Core :: Networking: HTTP, defect, major)

defect
Not set
major

Tracking

()

RESOLVED FIXED
mozilla1.9.1b3

People

(Reporter: ehsan, Assigned: ehsan)

References

()

Details

(Keywords: fixed1.9.1)

Attachments

(2 files, 2 obsolete files)

Our implementation of HTTP resuming code has a major flaw, that is, the Accept-Ranges response header is not respected in nsHttpChannel::GetEntityID.

Servers which do not support resuming send "Accept-Ranges: none", and others send "Accept-Ranges: [units]" where [units] is "bytes" for all practical purposes.

Without this being fixed, servers which explicitly state that they don't support request resuming by sending "Accept-Ranges: none" are not handled by our code gracefully.  This may cause a bogus pause button to show up in the UI for the download, and also it might lead to corruption of user's downloaded files if the download is automatically paused along the way (for example, by putting the computer to sleep or closing Firefox).
It shouldn't cause corruption, because the server wouldn't send a 206 response when attempting resume, which we handle gracefully. But yeah, we shouldn't show a pause button when we can't pause.
Attached patch Patch (v1) (obsolete) — Splinter Review
Patch with a unit test.
Assignee: nobody → ehsan.akhgari
Status: NEW → ASSIGNED
Attachment #347149 - Flags: superreview?(cbiesinger)
Attachment #347149 - Flags: review?(cbiesinger)
Attached patch Patch (v1.1) (obsolete) — Splinter Review
Section 3.12 <http://www.w3.org/Protocols/rfc2616/rfc2616-sec3.html#sec3.12> says:

The only range unit defined by HTTP/1.1 is "bytes". HTTP/1.1 implementations MAY ignore ranges specified using other units.

We can only handle the bytes units, I think it be appropriate for us to ignore any other Accept-Ranges header that a server might generate.  This patch allows Accept-Ranges not be set, but if it's set, it requires its value to be equal to "bytes".

I have also expanded the tests section to cover the following four modes:
1. No Accept-Ranges       (resumable)
2. Accept-Ranges: bytes   (resumable)
3. Accept-Ranges: none    (non-resumable)
4. Accept-Ranges: foobar  (non-resumable)
Attachment #347149 - Attachment is obsolete: true
Attachment #347150 - Flags: superreview?(cbiesinger)
Attachment #347150 - Flags: review?(cbiesinger)
Attachment #347149 - Flags: superreview?(cbiesinger)
Attachment #347149 - Flags: review?(cbiesinger)
Comment on attachment 347150 [details] [diff] [review]
Patch (v1.1)

+    // Don't return an entity if the server sent the followin header:

followin -> following

+    const char* cAcceptRanges =

why the "c" prefix?

+    if (cAcceptRanges && strcmp(cAcceptRanges, "bytes")) {

no, this is wrong. a server can send a list of acceptable units here. you want nsHttp::FindToken, I think, as in http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/src/nsHttpResponseHead.cpp#641
Attachment #347150 - Flags: superreview?(cbiesinger)
Attachment #347150 - Flags: review?(cbiesinger)
Attachment #347150 - Flags: review-
Attached patch Patch (v1.2)Splinter Review
(In reply to comment #4)
> (From update of attachment 347150 [details] [diff] [review])
> +    // Don't return an entity if the server sent the followin header:
> 
> followin -> following

Fixed.

> +    const char* cAcceptRanges =
> 
> why the "c" prefix?

Just to be consistent with the rest of the strings declared in this function: |cLastMod| and |cEtag|.  I removed the "c" prefix in this version.

> +    if (cAcceptRanges && strcmp(cAcceptRanges, "bytes")) {
> 
> no, this is wrong. a server can send a list of acceptable units here. you want
> nsHttp::FindToken, I think, as in
> http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/src/nsHttpResponseHead.cpp#641

You're right.  Done that, and also added a few more unit tests for the cases where more than one unit is specified in Accept-Ranges.
Attachment #347150 - Attachment is obsolete: true
Attachment #347246 - Flags: superreview?(bzbarsky)
Attachment #347246 - Flags: review?(cbiesinger)
Attachment #347246 - Flags: superreview?(bzbarsky) → superreview?(cbiesinger)
Attachment #347246 - Flags: superreview?(cbiesinger)
Attachment #347246 - Flags: superreview+
Attachment #347246 - Flags: review?(cbiesinger)
Attachment #347246 - Flags: review+
Thanks for the reviews, Christian.  Is this something we should consider for 1.9.1?
Comment on attachment 347246 [details] [diff] [review]
Patch (v1.2)

This patch makes our HTTP protocol implementation a bit more compliant with the HTTP spec, and includes a unit test.
Attachment #347246 - Flags: approval1.9.1?
Comment on attachment 347246 [details] [diff] [review]
Patch (v1.2)

a191=beltzner
Attachment #347246 - Flags: approval1.9.1? → approval1.9.1+
http://hg.mozilla.org/mozilla-central/rev/356e3865c629
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.1b3
Flags: in-testsuite+
Keywords: fixed1.9.1
You need to log in before you can comment on or make changes to this bug.