462707 - nsHttpChannel::GetEntityID should respect Accept-Ranges response header

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[(no longer active)]

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).

Comment 1

[Christian :Biesinger (don't email me, ping me on IRC)]

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.

Comment 2

[(no longer active)]

Attachment: Patch (v1)

Patch with a unit test.

Comment 3

[(no longer active)]

Attachment: Patch (v1.1)

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)

Comment 4

[Christian :Biesinger (don't email me, ping me on IRC)]

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

Comment 5

[(no longer active)]

Attachment: Patch (v1.2)

(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.

Comment 6

[(no longer active)]

Thanks for the reviews, Christian.  Is this something we should consider for 1.9.1?

Comment 7

[(no longer active)]

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.

Comment 8

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 347246 [details] [diff] [review]
Patch (v1.2)

a191=beltzner

Comment 9

[(no longer active)]

Attachment: mq patch for check-in

Comment 10

[Shawn Wilsher :sdwilsh]

http://hg.mozilla.org/mozilla-central/rev/356e3865c629