interrupt and resume download with CONTENT_LENGTH_MISMATCH errors

interrupt and resume download with CONTENT_LENGTH_MISMATCH errors

If a download ends with CONTENT_LENGTH_MISMATCH error, chrome treat it as completed.
However, this could happen if server closes the connection too early.
And it causes the downloaded file size to be incorrect.
And with pause/resume functionalities, this could happen more frequently.
This change treats these download as interrupted if strong validators are present.
For downloads without strong validators, auto resumption could cause them to enter a restart->interrupt->restart cycle.
As a result, download is treated as completed if there are no strong validators.

BUG=453357
Review-Url: https://codereview.chromium.org/2832223004
Cr-Commit-Position: refs/heads/master@{#467897}
Committed: https://chromium.googlesource.com/chromium/src/+/b42016cc0c9f244e44a437ca060f50ff0edab189

[qinmin, 2017-04-21 22:15:09]

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

[qinmin, 2017-04-21 22:15:10]

PTAL

asanka@, please take a look at this. For all download without strong validators,
i treat all of them as completed and we don't have content-range headers to
verify it.

[Ilya Sherman, 2017-04-21 22:45:11]

histograms.xml lgtm

[David Trainor- moved to gerrit, 2017-04-25 03:50:41]

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

https://codereview.chromium.org/2832223004/diff/1/content/browser/download/do...
content/browser/download/download_item_impl.cc:1804: if (reason == 
DOWNLOAD_INTERRUPT_REASON_SERVER_CONTENT_LENGTH_MISMATCH)
Remove extra space after ==

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

https://codereview.chromium.org/2832223004/diff/1/content/browser/download/do...
content/browser/download/download_item_impl_unittest.cc:722: // Test that a
download is resumed automatcially after a content length mismatch
automatically

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

https://codereview.chromium.org/2832223004/diff/1/content/browser/download/do...
content/browser/download/download_stats.h:110: // bytes is received when
resuming the download
is -> are.  Add '.' at end.

https://codereview.chromium.org/2832223004/diff/1/content/browser/download/do...
content/browser/download/download_stats.h:114: // bytes is received when
resuming the download.
is -> are

https://codereview.chromium.org/2832223004/diff/1/content/public/browser/down...
File content/public/browser/download_interrupt_reason_values.h (right):

https://codereview.chromium.org/2832223004/diff/1/content/public/browser/down...
content/public/browser/download_interrupt_reason_values.h:122:
INTERRUPT_REASON(SERVER_CONTENT_LENGTH_MISMATCH, 38)
Am I understanding that we only interrupt when we have a strong validator?  Do
we want to make that clear here?

[qinmin, 2017-04-25 19:27:47]

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

https://codereview.chromium.org/2832223004/diff/1/content/browser/download/do...
content/browser/download/download_item_impl.cc:1804: if (reason == 
DOWNLOAD_INTERRUPT_REASON_SERVER_CONTENT_LENGTH_MISMATCH)
On 2017/04/25 03:50:41, David Trainor-ping if over 24h wrote:
> Remove extra space after ==

Done.

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

https://codereview.chromium.org/2832223004/diff/1/content/browser/download/do...
content/browser/download/download_item_impl_unittest.cc:722: // Test that a
download is resumed automatcially after a content length mismatch
On 2017/04/25 03:50:41, David Trainor-ping if over 24h wrote:
> automatically

Done.

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

https://codereview.chromium.org/2832223004/diff/1/content/browser/download/do...
content/browser/download/download_stats.h:110: // bytes is received when
resuming the download
On 2017/04/25 03:50:41, David Trainor-ping if over 24h wrote:
> is -> are.  Add '.' at end.

Done.

https://codereview.chromium.org/2832223004/diff/1/content/browser/download/do...
content/browser/download/download_stats.h:114: // bytes is received when
resuming the download.
On 2017/04/25 03:50:41, David Trainor-ping if over 24h wrote:
> is -> are

Done.

https://codereview.chromium.org/2832223004/diff/1/content/public/browser/down...
File content/public/browser/download_interrupt_reason_values.h (right):

https://codereview.chromium.org/2832223004/diff/1/content/public/browser/down...
content/public/browser/download_interrupt_reason_values.h:122:
INTERRUPT_REASON(SERVER_CONTENT_LENGTH_MISMATCH, 38)
On 2017/04/25 03:50:41, David Trainor-ping if over 24h wrote:
> Am I understanding that we only interrupt when we have a strong validator?  Do
> we want to make that clear here?

Done.

[xingliu, 2017-04-25 19:32:05]

I found this comment in the code:
https://cs.chromium.org/chromium/src/net/url_request/url_request_http_job.cc?...

Also it links to a bug:
https://bugs.chromium.org/p/chromium/issues/detail?id=79694

does this kind of download will always be interrupted if we do this?

https://codereview.chromium.org/2832223004/diff/1/content/public/browser/down...
File content/public/browser/download_interrupt_reason_values.h (right):

https://codereview.chromium.org/2832223004/diff/1/content/public/browser/down...
content/public/browser/download_interrupt_reason_values.h:122:
INTERRUPT_REASON(SERVER_CONTENT_LENGTH_MISMATCH, 38)
FYI: We might need to change some other files, I remember extension generate
their api file based on this.

[qinmin, 2017-04-25 20:51:28]

On 2017/04/25 19:32:05, xingliu wrote:
> I found this comment in the code:
>
https://cs.chromium.org/chromium/src/net/url_request/url_request_http_job.cc?...
> 
> Also it links to a bug:
> https://bugs.chromium.org/p/chromium/issues/detail?id=79694
> 
> does this kind of download will always be interrupted if we do this?

No, if the error is cleared in ShouldFixMismatchedContentLength(), we should not
get interrupted.

> 
>
https://codereview.chromium.org/2832223004/diff/1/content/public/browser/down...
> File content/public/browser/download_interrupt_reason_values.h (right):
> 
>
https://codereview.chromium.org/2832223004/diff/1/content/public/browser/down...
> content/public/browser/download_interrupt_reason_values.h:122:
> INTERRUPT_REASON(SERVER_CONTENT_LENGTH_MISMATCH, 38)
> FYI: We might need to change some other files, I remember extension generate
> their api file based on this.

[qinmin, 2017-04-25 20:52:12]

https://codereview.chromium.org/2832223004/diff/1/content/public/browser/down...
File content/public/browser/download_interrupt_reason_values.h (right):

https://codereview.chromium.org/2832223004/diff/1/content/public/browser/down...
content/public/browser/download_interrupt_reason_values.h:122:
INTERRUPT_REASON(SERVER_CONTENT_LENGTH_MISMATCH, 38)
On 2017/04/25 19:32:05, xingliu wrote:
> FYI: We might need to change some other files, I remember extension generate
> their api file based on this.

Done. There is a chrome/common/extensions/api/downloads.idl needs to be updated

[David Trainor- moved to gerrit, 2017-04-26 16:09:01]

lgtm % xingliu@

https://codereview.chromium.org/2832223004/diff/40001/content/browser/downloa...
File content/browser/download/download_item_impl_unittest.cc (right):

https://codereview.chromium.org/2832223004/diff/40001/content/browser/downloa...
content/browser/download/download_item_impl_unittest.cc:722: // Test that a
download is resumed automatcially after a content length mismatch
Fixed the other one.  This one is still automatcially!

[xingliu, 2017-04-26 16:46:46]

lgtm

[qinmin, 2017-04-26 17:19:58]

qinmin@chromium.org changed reviewers:
+ jam@chromium.org, sky@chromium.org

[qinmin, 2017-04-26 17:19:58]

+sky@ for OWNER of components/history/content/browser/ and 
chrome/common/

+jam@ for OWNER of
content/public/browser/download_interrupt_reason_values.h

[sky, 2017-04-26 19:55:43]

LGTM

[asanka, 2017-04-26 21:17:33]

LGTM % nits

https://codereview.chromium.org/2832223004/diff/40001/chrome/app/generated_re...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/2832223004/diff/40001/chrome/app/generated_re...
chrome/app/generated_resources.grd:1387: Content length mismatch
Nit: This phrase is going to be confusing for users and translators. Even
seasoned veterans can't explain what Content-Length is supposed to be without
looking it up.

Both for translators (in the |desc| field) and for users (inline text), let's go
with something easier to parse. Something that indicates that  the browser could
only download part of the file.

Possible alternatives for STATUS: "Truncated", "Incomplete", "Partial", or
something agreeable to the UX folks. Keep in mind that all translations of this
string need to fit on the shelf.

https://codereview.chromium.org/2832223004/diff/40001/chrome/app/generated_re...
chrome/app/generated_resources.grd:1391: Received fewer bytes than the number in
the content-length header.
While we use "bytes" to explain how big the download is, it's not super
important to use the technical term here. As above, just explain that the
download is incomplete.

https://codereview.chromium.org/2832223004/diff/40001/content/browser/downloa...
File content/browser/download/download_item_impl.h (right):

https://codereview.chromium.org/2832223004/diff/40001/content/browser/downloa...
content/browser/download/download_item_impl.h:716: // The bytes received if the
download was interrupted with
Nit: Value of |received_bytes_| at the time the download was interrupted with
CONTENT_LENGTH_MISMATCH.

Otherwise it's not clear why this field exists.

Also, initialize it here. So it's clear what the default value is without
hunting down constructors.

Also, call it something like "received_bytes_at_length_mismatch".

https://codereview.chromium.org/2832223004/diff/40001/content/browser/downloa...
File content/browser/download/download_request_core.cc (right):

https://codereview.chromium.org/2832223004/diff/40001/content/browser/downloa...
content/browser/download/download_request_core.cc:514: // 1. server close the
connection too early.
The more common case isn't the server closing the connection, but a middlebox.
Imagine what happens if the UA is connecting through a proxy and the connection
between the proxy and the server breaks. The proxy isn't going to bother
emulating a network error. Instead it'll just close the connection.

Also "Server closes the connection too early." (consistent capitalization,
singular verb).

[jam, 2017-04-27 15:35:59]

lgtm

[qinmin, 2017-04-27 18:20:54]

https://codereview.chromium.org/2832223004/diff/40001/chrome/app/generated_re...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/2832223004/diff/40001/chrome/app/generated_re...
chrome/app/generated_resources.grd:1387: Content length mismatch
On 2017/04/26 21:17:33, asanka wrote:
> Nit: This phrase is going to be confusing for users and translators. Even
> seasoned veterans can't explain what Content-Length is supposed to be without
> looking it up.
> 
> Both for translators (in the |desc| field) and for users (inline text), let's
go
> with something easier to parse. Something that indicates that  the browser
could
> only download part of the file.
> 
> Possible alternatives for STATUS: "Truncated", "Incomplete", "Partial", or
> something agreeable to the UX folks. Keep in mind that all translations of
this
> string need to fit on the shelf.

Done.

https://codereview.chromium.org/2832223004/diff/40001/chrome/app/generated_re...
chrome/app/generated_resources.grd:1391: Received fewer bytes than the number in
the content-length header.
On 2017/04/26 21:17:33, asanka wrote:
> While we use "bytes" to explain how big the download is, it's not super
> important to use the technical term here. As above, just explain that the
> download is incomplete.

Done.

https://codereview.chromium.org/2832223004/diff/40001/content/browser/downloa...
File content/browser/download/download_item_impl.h (right):

https://codereview.chromium.org/2832223004/diff/40001/content/browser/downloa...
content/browser/download/download_item_impl.h:716: // The bytes received if the
download was interrupted with
On 2017/04/26 21:17:33, asanka wrote:
> Nit: Value of |received_bytes_| at the time the download was interrupted with
> CONTENT_LENGTH_MISMATCH.
> 
> Otherwise it's not clear why this field exists.
> 
> Also, initialize it here. So it's clear what the default value is without
> hunting down constructors.
> 
> Also, call it something like "received_bytes_at_length_mismatch".

Done.

https://codereview.chromium.org/2832223004/diff/40001/content/browser/downloa...
File content/browser/download/download_item_impl_unittest.cc (right):

https://codereview.chromium.org/2832223004/diff/40001/content/browser/downloa...
content/browser/download/download_item_impl_unittest.cc:722: // Test that a
download is resumed automatcially after a content length mismatch
On 2017/04/26 16:09:01, David Trainor-ping if over 24h wrote:
> Fixed the other one.  This one is still automatcially!

Done.

https://codereview.chromium.org/2832223004/diff/40001/content/browser/downloa...
File content/browser/download/download_request_core.cc (right):

https://codereview.chromium.org/2832223004/diff/40001/content/browser/downloa...
content/browser/download/download_request_core.cc:514: // 1. server close the
connection too early.
On 2017/04/26 21:17:33, asanka wrote:
> The more common case isn't the server closing the connection, but a middlebox.
> Imagine what happens if the UA is connecting through a proxy and the
connection
> between the proxy and the server breaks. The proxy isn't going to bother
> emulating a network error. Instead it'll just close the connection.
> 
> Also "Server closes the connection too early." (consistent capitalization,
> singular verb).

Done. Thanks for the info.

[qinmin, 2017-04-27 18:20:59]

The CQ bit was checked by qinmin@chromium.org

[qinmin, 2017-04-27 18:20:59]

The patchset sent to the CQ was uploaded after l-g-t-m from
isherman@chromium.org, asanka@chromium.org, dtrainor@chromium.org,
sky@chromium.org, jam@chromium.org, xingliu@chromium.org
Link to the patchset: https://codereview.chromium.org/2832223004/#ps60001
(title: "fixing nits")

[qinmin, 2017-04-27 18:21:00]

The CQ bit was unchecked by qinmin@chromium.org

[qinmin, 2017-04-27 18:21:02]

The CQ bit was checked by qinmin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-27 18:21:37]

Dry run: 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-27 18:35:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-27 18:35:34]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[qinmin, 2017-04-27 23:41:02]

Patchset #5 (id:80001) has been deleted

[qinmin, 2017-04-28 04:14:46]

The CQ bit was checked by qinmin@chromium.org

[qinmin, 2017-04-28 04:14:47]

The patchset sent to the CQ was uploaded after l-g-t-m from
isherman@chromium.org, asanka@chromium.org, dtrainor@chromium.org,
sky@chromium.org, jam@chromium.org, xingliu@chromium.org
Link to the patchset: https://codereview.chromium.org/2832223004/#ps100001
(title: "fix tests")

[commit-bot: I haz the power, 2017-04-28 04:15:23]

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-28 05:23:32]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1493352886351370,
"parent_rev": "99d003caf2d7876a7e402a07caa94a5c24ccb922", "commit_rev":
"b42016cc0c9f244e44a437ca060f50ff0edab189"}

[commit-bot: I haz the power, 2017-04-28 05:23:44]

Description was changed from

==========
interrupt and resume download with CONTENT_LENGTH_MISMATCH errors

If a download ends with CONTENT_LENGTH_MISMATCH error, chrome treat it as
completed.
However, this could happen if server closes the connection too early.
And it causes the downloaded file size to be incorrect.
And with pause/resume functionalities, this could happen more frequently.
This change treats these download as interrupted if strong validators are
present.
For downloads without strong validators, auto resumption could cause them to
enter a restart->interrupt->restart cycle.
As a result, download is treated as completed if there are no strong validators.

BUG=453357
==========

to

==========
interrupt and resume download with CONTENT_LENGTH_MISMATCH errors

If a download ends with CONTENT_LENGTH_MISMATCH error, chrome treat it as
completed.
However, this could happen if server closes the connection too early.
And it causes the downloaded file size to be incorrect.
And with pause/resume functionalities, this could happen more frequently.
This change treats these download as interrupted if strong validators are
present.
For downloads without strong validators, auto resumption could cause them to
enter a restart->interrupt->restart cycle.
As a result, download is treated as completed if there are no strong validators.

BUG=453357

Review-Url: https://codereview.chromium.org/2832223004
Cr-Commit-Position: refs/heads/master@{#467897}
Committed:
https://chromium.googlesource.com/chromium/src/+/b42016cc0c9f244e44a437ca060f...
==========

[commit-bot: I haz the power, 2017-04-28 05:23:45]

Committed patchset #5 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/b42016cc0c9f244e44a437ca060f...