set content length limit if half open range request fails

set content length limit if half open range request fails

If a range request fails, normally we should report an error.
However, if the request is half open, it means we are reaching the end of the file.
When resuming a download, we get the slice info from the history db.
However, we cannot tell if the last slice has reached the end of the stream.
As a result, a half open request is always sent.
And this request can be used to determine the content length.

BUG=644352
Review-Url: https://codereview.chromium.org/2794683002
Cr-Commit-Position: refs/heads/master@{#461595}
Committed: https://chromium.googlesource.com/chromium/src/+/3ba57766474b9e525e858ae8382d8acbdec34039

[qinmin, 2017-03-31 21:53:24]

qinmin@chromium.org changed reviewers:
+ dtrainor@chromium.org, xingliu@chromium.org

[qinmin, 2017-03-31 21:53:25]

PTAL

[xingliu, 2017-03-31 23:38:21]

https://codereview.chromium.org/2794683002/diff/1/content/browser/download/do...
File content/browser/download/download_file_impl.cc (right):

https://codereview.chromium.org/2794683002/diff/1/content/browser/download/do...
content/browser/download/download_file_impl.cc:356: << "Content length changed,
the download might have updated.";
nit%: Since content length is the size of the payload, can we change the comment
to file size changed?

https://codereview.chromium.org/2794683002/diff/1/content/browser/download/do...
content/browser/download/download_file_impl.cc:360: content_length_limit_ =
limit;
Will we hit the DCHECK above when limit < content_length_limit_?

Maybe (limit < content_length_limit_) is only for release build, and we expect
to hit a DCHECK in debug build when file size changed?

https://codereview.chromium.org/2794683002/diff/1/content/browser/download/do...
File content/browser/download/download_file_impl.h (right):

https://codereview.chromium.org/2794683002/diff/1/content/browser/download/do...
content/browser/download/download_file_impl.h:255: // Maximum length of the
file, -1 if the limit is unknown.
nit%: It's a bit confusing that people might think this is a copy of the
Content-Length header.

[qinmin, 2017-04-01 05:20:02]

https://codereview.chromium.org/2794683002/diff/1/content/browser/download/do...
File content/browser/download/download_file_impl.cc (right):

https://codereview.chromium.org/2794683002/diff/1/content/browser/download/do...
content/browser/download/download_file_impl.cc:356: << "Content length changed,
the download might have updated.";
On 2017/03/31 23:38:21, xingliu wrote:
> nit%: Since content length is the size of the payload, can we change the
comment
> to file size changed?

Done.

https://codereview.chromium.org/2794683002/diff/1/content/browser/download/do...
content/browser/download/download_file_impl.cc:360: content_length_limit_ =
limit;
On 2017/03/31 23:38:21, xingliu wrote:
> Will we hit the DCHECK above when limit < content_length_limit_?
> 
> Maybe (limit < content_length_limit_) is only for release build, and we expect
> to hit a DCHECK in debug build when file size changed?

Yes, it will cause the DCHECK failure.
The DCHECK will tell us if this really happens in development build, so we can
analyze why this would happen. For release build, we don't want to crash the
app.

One possible scenario is that the content-length header is too large (e.g 100M).
So when the parallel request is asking for 50M+, the server returns a range
error, and we will set the content_length_limit_ here. When the initial request
completes, it founds that the file is only 20M, and SetContentLengthLimit() is
called again. In this case, we should use 20M as the file size.

https://codereview.chromium.org/2794683002/diff/1/content/browser/download/do...
File content/browser/download/download_file_impl.h (right):

https://codereview.chromium.org/2794683002/diff/1/content/browser/download/do...
content/browser/download/download_file_impl.h:255: // Maximum length of the
file, -1 if the limit is unknown.
On 2017/03/31 23:38:21, xingliu wrote:
> nit%: It's a bit confusing that people might think this is a copy of the
> Content-Length header.

it is different, the limit is the upper bound, the actual file size could be
smaller than this. 
changed the variable name to potential_file_length_

[xingliu, 2017-04-03 16:51:56]

lgtm.

https://codereview.chromium.org/2794683002/diff/1/content/browser/download/do...
File content/browser/download/download_file_impl.cc (right):

https://codereview.chromium.org/2794683002/diff/1/content/browser/download/do...
content/browser/download/download_file_impl.cc:360: content_length_limit_ =
limit;
On 2017/04/01 05:20:02, qinmin wrote:
> On 2017/03/31 23:38:21, xingliu wrote:
> > Will we hit the DCHECK above when limit < content_length_limit_?
> > 
> > Maybe (limit < content_length_limit_) is only for release build, and we
expect
> > to hit a DCHECK in debug build when file size changed?
> 
> Yes, it will cause the DCHECK failure.
> The DCHECK will tell us if this really happens in development build, so we can
> analyze why this would happen. For release build, we don't want to crash the
> app.
> 
> One possible scenario is that the content-length header is too large (e.g
100M).
> So when the parallel request is asking for 50M+, the server returns a range
> error, and we will set the content_length_limit_ here. When the initial
request
> completes, it founds that the file is only 20M, and SetContentLengthLimit() is
> called again. In this case, we should use 20M as the file size. 

I see. sgtm.

https://codereview.chromium.org/2794683002/diff/1/content/browser/download/do...
File content/browser/download/download_file_impl.h (right):

https://codereview.chromium.org/2794683002/diff/1/content/browser/download/do...
content/browser/download/download_file_impl.h:255: // Maximum length of the
file, -1 if the limit is unknown.
On 2017/04/01 05:20:02, qinmin wrote:
> On 2017/03/31 23:38:21, xingliu wrote:
> > nit%: It's a bit confusing that people might think this is a copy of the
> > Content-Length header.
> 
> it is different, the limit is the upper bound, the actual file size could be
> smaller than this. 
> changed the variable name to potential_file_length_

Thanks.

[qinmin, 2017-04-03 16:54:54]

The CQ bit was checked by qinmin@chromium.org

[commit-bot: I haz the power, 2017-04-03 16:55:26]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[David Trainor- moved to gerrit, 2017-04-03 17:47:32]

https://codereview.chromium.org/2794683002/diff/20001/content/browser/downloa...
File content/browser/download/download_file.h (right):

https://codereview.chromium.org/2794683002/diff/20001/content/browser/downloa...
content/browser/download/download_file.h:78: // Sets the potential file length.
This can be called multiple times as
This comment seems really different from what the DCHECK says in the
implementation.

https://codereview.chromium.org/2794683002/diff/20001/content/browser/downloa...
File content/browser/download/download_file_impl.cc (right):

https://codereview.chromium.org/2794683002/diff/20001/content/browser/downloa...
content/browser/download/download_file_impl.cc:358: if (length <
potential_file_length_ ||
Is this condition possible with the DCHECK above?

https://codereview.chromium.org/2794683002/diff/20001/content/browser/downloa...
File content/browser/download/download_file_impl.h (right):

https://codereview.chromium.org/2794683002/diff/20001/content/browser/downloa...
content/browser/download/download_file_impl.h:256: // value will fail. The
actual file length should not be larger than this.
When would the file length be less than this if we set it once we're done the
half open request?

[qinmin, 2017-04-03 17:51:09]

The CQ bit was unchecked by qinmin@chromium.org

[qinmin, 2017-04-03 18:33:11]

https://codereview.chromium.org/2794683002/diff/20001/content/browser/downloa...
File content/browser/download/download_file.h (right):

https://codereview.chromium.org/2794683002/diff/20001/content/browser/downloa...
content/browser/download/download_file.h:78: // Sets the potential file length.
This can be called multiple times as
On 2017/04/03 17:47:31, David Trainor-ping if over 24h wrote:
> This comment seems really different from what the DCHECK says in the
> implementation.

Changed the comment for describing this call and half-open requests

https://codereview.chromium.org/2794683002/diff/20001/content/browser/downloa...
File content/browser/download/download_file_impl.cc (right):

https://codereview.chromium.org/2794683002/diff/20001/content/browser/downloa...
content/browser/download/download_file_impl.cc:358: if (length <
potential_file_length_ ||
On 2017/04/03 17:47:31, David Trainor-ping if over 24h wrote:
> Is this condition possible with the DCHECK above?

No, this is not possible.
The DCHECK above will raise a warning that something unusual happens, and here
we need to safely deal with that warning.
For example, if a file is only 100M, but the content-length header is 300M
(which is wrong), issuing a parallel request with 150M beginning offset will
trigger this call. So in this case, we will set the potential file length to
150M.
Later on when the initial request successfully completes, this call will be
triggered again with length == 100M.
This is a quite unusual situation. So the DCHECK will fail and we could see the
warnings. And the actual download completion is handled correctly.

https://codereview.chromium.org/2794683002/diff/20001/content/browser/downloa...
File content/browser/download/download_file_impl.h (right):

https://codereview.chromium.org/2794683002/diff/20001/content/browser/downloa...
content/browser/download/download_file_impl.h:256: // value will fail. The
actual file length should not be larger than this.
On 2017/04/03 17:47:31, David Trainor-ping if over 24h wrote:
> When would the file length be less than this if we set it once we're done the
> half open request?

If the length is determined upon completion of a half open request, the file
length should be equal to potential_file_length_.
However, there might not be any successful half open requests. For example, a
file is divided into 2 slices, and the slice at the end of the file finishes
first. If the browser crashes, we don't know whether the 2nd slices has
finished. So a new range request will be issued. That range request will fail
and setPotentialFileLength() will be called. So we can know that the 2nd slice
has already finished.

The issue now becomes whether the parallel request are always issued in a valid
range. If content-length is reliable, then we are ok. If somehow the file
changes, or the content-length is wrong, then the failing range request's offset
will give a larger value than the file length.

[qinmin, 2017-04-04 00:10:41]

The CQ bit was checked by qinmin@chromium.org

[qinmin, 2017-04-04 00:10:42]

The patchset sent to the CQ was uploaded after l-g-t-m from xingliu@chromium.org
Link to the patchset: https://codereview.chromium.org/2794683002/#ps40001
(title: "new comments")

[commit-bot: I haz the power, 2017-04-04 00:11:09]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-04 01:15:44]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1491264641539850,
"parent_rev": "254adfb1110eef204799e1d23206dfe8b75f6cb8", "commit_rev":
"3ba57766474b9e525e858ae8382d8acbdec34039"}

[commit-bot: I haz the power, 2017-04-04 01:16:29]

Description was changed from

==========
set content length limit if half open range request fails

If a range request fails, normally we should report an error.
However, if the request is half open, it means we are reaching the end of the
file.
When resuming a download, we get the slice info from the history db.
However, we cannot tell if the last slice has reached the end of the stream.
As a result, a half open request is always sent.
And this request can be used to determine the content length.

BUG=644352
==========

to

==========
set content length limit if half open range request fails

If a range request fails, normally we should report an error.
However, if the request is half open, it means we are reaching the end of the
file.
When resuming a download, we get the slice info from the history db.
However, we cannot tell if the last slice has reached the end of the stream.
As a result, a half open request is always sent.
And this request can be used to determine the content length.

BUG=644352

Review-Url: https://codereview.chromium.org/2794683002
Cr-Commit-Position: refs/heads/master@{#461595}
Committed:
https://chromium.googlesource.com/chromium/src/+/3ba57766474b9e525e858ae8382d...
==========

[commit-bot: I haz the power, 2017-04-04 01:16:30]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/3ba57766474b9e525e858ae8382d...