[Downloads] Do not store error responses during resumption.

[Downloads] Do not store error responses during resumption.

If a server or an intermediary sends a redirect, error, or other
unexpected response during resumption, don't store it or discard the
partial download state. Instead the download item will flag itself as
interrupted again and will require an externally initiated Resume() call
to try again.

Such responses are common where captive portals or other middle boxes
are present. If such cases aren't explicitly handled, an untimely
resumption attempt could discard partial state.

As a side-effect of this patch, downloads will also not discard partial
state due to other server errors during resumption (e.g. 500).

BUG=7648
R=svaldez@chromium.org, davidben@chromium.org, rdevlin.cronin@chromium.org
TBR=sky@chromium.org

Committed: https://crrev.com/edb920a2e81a6be07d4ef30da09e3f2e9debe0c1
Cr-Commit-Position: refs/heads/master@{#376625}

[asanka, 2016-01-13 02:40:11]

Description was changed from

==========
Don't store response for redirects.

BUG=7648
==========

to

==========
[Downloads] Do not store error responses during resumption.

If a server or an intermediary sends an error or other unexpected
response during resumption, don't store it or discard the partial
download state. Instead the download item will flag itself as
interrupted again and require an externally initiated Resume() call to
try again.

Such responses are common where captive portals or other middle boxes
are present. If such cases aren't explicitly handled, an untimely
resumption attempt could discard partial state.

As a side-effect of this patch, downloads will also not discard partial
state due to other server errors during resumption (e.g. 500).

BUG=7648
==========

[asanka, 2016-01-13 02:40:43]

Description was changed from

==========
[Downloads] Do not store error responses during resumption.

If a server or an intermediary sends an error or other unexpected
response during resumption, don't store it or discard the partial
download state. Instead the download item will flag itself as
interrupted again and require an externally initiated Resume() call to
try again.

Such responses are common where captive portals or other middle boxes
are present. If such cases aren't explicitly handled, an untimely
resumption attempt could discard partial state.

As a side-effect of this patch, downloads will also not discard partial
state due to other server errors during resumption (e.g. 500).

BUG=7648
==========

to

==========
[Downloads] Do not store error responses during resumption.

If a server or an intermediary sends an error or other unexpected
response during resumption, don't store it or discard the partial
download state. Instead the download item will flag itself as
interrupted again and require an externally initiated Resume() call to
try again.

Such responses are common where captive portals or other middle boxes
are present. If such cases aren't explicitly handled, an untimely
resumption attempt could discard partial state.

As a side-effect of this patch, downloads will also not discard partial
state due to other server errors during resumption (e.g. 500).

BUG=7648
==========

[asanka, 2016-01-13 02:42:21]

asanka@chromium.org changed reviewers:
+ davidben@chromium.org, rdevlin.cronin@chromium.org, svaldez@chromium.org

[asanka, 2016-01-13 02:46:04]

PTAL?

svaldez:
  all the things

rdevlin.cronin:
  chrome/common/extensions/api/downloads.idl

davidben:
  content/public/browser/download_interrupt_reason_values.h
  content/public/test/test_download_request_handler.cc

[svaldez, 2016-01-13 17:29:18]

Took a brief look at the tests, though I didn't verify that the asserts are
correct.

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

https://codereview.chromium.org/1544603003/diff/20001/chrome/app/generated_re...
chrome/app/generated_resources.grd:1638: +      <message
name="IDS_DOWNLOAD_INTERRUPTED_STATUS_UNREACHABLE"
Might want to reword both of these if they also apply to servers that need
state/session to access the download. Unreachable might be too strong an
indicator for the server's state.

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

https://codereview.chromium.org/1544603003/diff/20001/content/browser/downloa...
content/browser/download/download_interrupt_reasons_impl.cc:87: case
net::ERR_NETWORK_CHANGED:
Might want to split this out to a separate CL if this actually affects other
codepaths. Otherwise its probably fine here.

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

https://codereview.chromium.org/1544603003/diff/20001/content/browser/downloa...
content/browser/download/download_item_impl.cc:376: UpdateObservers();
Does this need to go at the bottom of "AutoResumeIfValid"?

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

https://codereview.chromium.org/1544603003/diff/20001/content/browser/downloa...
content/browser/download/url_downloader.cc:158:
request_->CancelWithError(net::ERR_ABORTED);
Might actually want to change this to a more appropriate error code.

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

https://codereview.chromium.org/1544603003/diff/20001/content/browser/downloa...
content/browser/download/url_downloader.h:68: void Done();
Can we call it Destroy since this doesn't necessarily mean the request was
completed.

https://codereview.chromium.org/1544603003/diff/20001/content/public/test/tes...
File content/public/test/test_download_request_handler.cc (right):

https://codereview.chromium.org/1544603003/diff/20001/content/public/test/tes...
content/public/test/test_download_request_handler.cc:316:
should_report_completed_request_ = false;
Seems unnecessary since we only call this on destruction?

[Devlin, 2016-01-13 17:39:56]

downloads.idl lgtm

[davidben, 2016-01-13 19:48:36]

On 2016/01/13 02:46:04, asanka wrote:
> davidben:
>   content/public/browser/download_interrupt_reason_values.h
>   content/public/test/test_download_request_handler.cc

lgtm. I didn't look at the other files.

[asanka, 2016-01-28 02:24:17]

I pulled out most of the initiation handling into the resurrected CL at
https://codereview.chromium.org/148133007/ . It got a bit hairy after I realized
that we want to route initiation errors through DownloadItem rather than bypass
it. The current code on trunk is also incorrect.

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

https://codereview.chromium.org/1544603003/diff/20001/chrome/app/generated_re...
chrome/app/generated_resources.grd:1638: +      <message
name="IDS_DOWNLOAD_INTERRUPTED_STATUS_UNREACHABLE"
On 2016/01/13 17:29:18, svaldez wrote:
> Might want to reword both of these if they also apply to servers that need
> state/session to access the download. Unreachable might be too strong an
> indicator for the server's state.

I only intend to return this error when we encounter an unexpected redirect. (No
longer returning this error from the SSL cert case since that may happen due to
lots of other reasons).

Could you explain what you mean above?

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

https://codereview.chromium.org/1544603003/diff/20001/content/browser/downloa...
content/browser/download/download_interrupt_reasons_impl.cc:87: case
net::ERR_NETWORK_CHANGED:
On 2016/01/13 17:29:18, svaldez wrote:
> Might want to split this out to a separate CL if this actually affects other
> codepaths. Otherwise its probably fine here.

Makes sense. Done.

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

https://codereview.chromium.org/1544603003/diff/20001/content/browser/downloa...
content/browser/download/download_item_impl.cc:376: UpdateObservers();
On 2016/01/13 17:29:18, svaldez wrote:
> Does this need to go at the bottom of "AutoResumeIfValid"?

Nope. AutoResumeIfValid() may or may not resume the download depending on
whether the download can be resumed automatically. Since it's called after the
download is interrupted, the observers should be updated regardless of what
AutoResumeIfValid() does. Since it's a bit out of place for AutoResumeIfValid()
to update observers if it doesn't do anything, we've placed the
UpdateObservers() call after the AutoResumeIfValid() call. That way, when the
observers are updated, they will see that the download is either interrupted or
{in-progress but with a potentially new interrupt reason}.

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

https://codereview.chromium.org/1544603003/diff/20001/content/browser/downloa...
content/browser/download/url_downloader.cc:158:
request_->CancelWithError(net::ERR_ABORTED);
On 2016/01/13 17:29:18, svaldez wrote:
> Might actually want to change this to a more appropriate error code.

Changed to ERR_UNSAFE_REDIRECT which is the error code used by URLRequest to
indicate a redirect that was deemed unsafe by the URLRequestJob.

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

https://codereview.chromium.org/1544603003/diff/20001/content/browser/downloa...
content/browser/download/url_downloader.h:68: void Done();
On 2016/01/13 17:29:18, svaldez wrote:
> Can we call it Destroy since this doesn't necessarily mean the request was
> completed.

Done.

https://codereview.chromium.org/1544603003/diff/20001/content/public/test/tes...
File content/public/test/test_download_request_handler.cc (right):

https://codereview.chromium.org/1544603003/diff/20001/content/public/test/tes...
content/public/test/test_download_request_handler.cc:316:
should_report_completed_request_ = false;
On 2016/01/13 17:29:18, svaldez wrote:
> Seems unnecessary since we only call this on destruction?

We also call ReportCompletedRequest when the URLRequest client reads the entire
body.

However, in retrospect, should_report_completed_request_ is a bit clumsy. I
replaced it with a running count of bytes that were successfully read and then
we are calling ReportCompletedRequest() only once during destruction.

[svaldez, 2016-01-28 21:53:46]

Is this then going to wait on that one?

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

https://codereview.chromium.org/1544603003/diff/20001/chrome/app/generated_re...
chrome/app/generated_resources.grd:1638: +      <message
name="IDS_DOWNLOAD_INTERRUPTED_STATUS_UNREACHABLE"
On 2016/01/28 02:24:17, asanka wrote:
> On 2016/01/13 17:29:18, svaldez wrote:
> > Might want to reword both of these if they also apply to servers that need
> > state/session to access the download. Unreachable might be too strong an
> > indicator for the server's state.
> 
> I only intend to return this error when we encounter an unexpected redirect.
(No
> longer returning this error from the SSL cert case since that may happen due
to
> lots of other reasons).
> 
> Could you explain what you mean above?

In that case the text is probably fine.

[asanka, 2016-02-03 03:07:16]

Yup. This CL is going to wait on https://codereview.chromium.org/148133007/ . In
order to reliably handle a bad response correctly I had to rework how
programmatic downloads work. Earlier Chrome was dropping downloads on the floor
if the network request failed without seeing an OnResponseStarted(). The
dependent CL changes this so that a DownloadItem is always created unless the
download is blocked by a throttler or cancelled by the user. It was a pretty
substantial change, so let's wait until it goes through the review cycle.

[asanka, 2016-02-12 16:24:35]

svaldez: Could you take a look? Things are settling upstream. So I don't expect
there to be any changes required here.

[svaldez, 2016-02-12 17:40:52]

lgtm, though you might want to clarify the CL description that this also deals
with redirects, since its unclear whether redirects really fall into the
category of error responses.

[asanka, 2016-02-12 23:51:43]

Description was changed from

==========
[Downloads] Do not store error responses during resumption.

If a server or an intermediary sends an error or other unexpected
response during resumption, don't store it or discard the partial
download state. Instead the download item will flag itself as
interrupted again and require an externally initiated Resume() call to
try again.

Such responses are common where captive portals or other middle boxes
are present. If such cases aren't explicitly handled, an untimely
resumption attempt could discard partial state.

As a side-effect of this patch, downloads will also not discard partial
state due to other server errors during resumption (e.g. 500).

BUG=7648
==========

to

==========
[Downloads] Do not store error responses during resumption.

If a server or an intermediary sends a redirect, error, or other
unexpected response during resumption, don't store it or discard the
partial download state. Instead the download item will flag itself as
interrupted again and will require an externally initiated Resume() call
to try again.

Such responses are common where captive portals or other middle boxes
are present. If such cases aren't explicitly handled, an untimely
resumption attempt could discard partial state.

As a side-effect of this patch, downloads will also not discard partial
state due to other server errors during resumption (e.g. 500).

BUG=7648
==========

[asanka, 2016-02-20 01:36:48]

Description was changed from

==========
[Downloads] Do not store error responses during resumption.

If a server or an intermediary sends a redirect, error, or other
unexpected response during resumption, don't store it or discard the
partial download state. Instead the download item will flag itself as
interrupted again and will require an externally initiated Resume() call
to try again.

Such responses are common where captive portals or other middle boxes
are present. If such cases aren't explicitly handled, an untimely
resumption attempt could discard partial state.

As a side-effect of this patch, downloads will also not discard partial
state due to other server errors during resumption (e.g. 500).

BUG=7648
==========

to

==========
[Downloads] Do not store error responses during resumption.

If a server or an intermediary sends a redirect, error, or other
unexpected response during resumption, don't store it or discard the
partial download state. Instead the download item will flag itself as
interrupted again and will require an externally initiated Resume() call
to try again.

Such responses are common where captive portals or other middle boxes
are present. If such cases aren't explicitly handled, an untimely
resumption attempt could discard partial state.

As a side-effect of this patch, downloads will also not discard partial
state due to other server errors during resumption (e.g. 500).

BUG=7648
R=svaldez@chromium.org, davidben@chromium.org, rdevlin.cronin@chromium.org
TBR=sky@chromium.org
==========

[asanka, 2016-02-20 01:36:49]

asanka@chromium.org changed reviewers:
+ sky@chromium.org

[asanka, 2016-02-20 01:38:44]

Thanks!

TBR += sky: I added the new interrupt reason to the list of values historically
used by the downloads history DB. This list is currently at
content_history_backend_db_unittest.cc.

[asanka, 2016-02-20 02:46:28]

The CQ bit was checked by asanka@chromium.org

[asanka, 2016-02-20 02:46:29]

The patchset sent to the CQ was uploaded after l-g-t-m from
rdevlin.cronin@chromium.org, davidben@chromium.org, svaldez@chromium.org
Link to the patchset: https://codereview.chromium.org/1544603003/#ps140001
(title: " ")

[commit-bot: I haz the power, 2016-02-20 02:47:40]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1544603003/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1544603003/140001

[commit-bot: I haz the power, 2016-02-20 05:19:24]

Description was changed from

==========
[Downloads] Do not store error responses during resumption.

If a server or an intermediary sends a redirect, error, or other
unexpected response during resumption, don't store it or discard the
partial download state. Instead the download item will flag itself as
interrupted again and will require an externally initiated Resume() call
to try again.

Such responses are common where captive portals or other middle boxes
are present. If such cases aren't explicitly handled, an untimely
resumption attempt could discard partial state.

As a side-effect of this patch, downloads will also not discard partial
state due to other server errors during resumption (e.g. 500).

BUG=7648
R=svaldez@chromium.org, davidben@chromium.org, rdevlin.cronin@chromium.org
TBR=sky@chromium.org
==========

to

==========
[Downloads] Do not store error responses during resumption.

If a server or an intermediary sends a redirect, error, or other
unexpected response during resumption, don't store it or discard the
partial download state. Instead the download item will flag itself as
interrupted again and will require an externally initiated Resume() call
to try again.

Such responses are common where captive portals or other middle boxes
are present. If such cases aren't explicitly handled, an untimely
resumption attempt could discard partial state.

As a side-effect of this patch, downloads will also not discard partial
state due to other server errors during resumption (e.g. 500).

BUG=7648
R=svaldez@chromium.org, davidben@chromium.org, rdevlin.cronin@chromium.org
TBR=sky@chromium.org
==========

[commit-bot: I haz the power, 2016-02-20 05:19:25]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-02-20 05:20:52]

Description was changed from

==========
[Downloads] Do not store error responses during resumption.

If a server or an intermediary sends a redirect, error, or other
unexpected response during resumption, don't store it or discard the
partial download state. Instead the download item will flag itself as
interrupted again and will require an externally initiated Resume() call
to try again.

Such responses are common where captive portals or other middle boxes
are present. If such cases aren't explicitly handled, an untimely
resumption attempt could discard partial state.

As a side-effect of this patch, downloads will also not discard partial
state due to other server errors during resumption (e.g. 500).

BUG=7648
R=svaldez@chromium.org, davidben@chromium.org, rdevlin.cronin@chromium.org
TBR=sky@chromium.org
==========

to

==========
[Downloads] Do not store error responses during resumption.

If a server or an intermediary sends a redirect, error, or other
unexpected response during resumption, don't store it or discard the
partial download state. Instead the download item will flag itself as
interrupted again and will require an externally initiated Resume() call
to try again.

Such responses are common where captive portals or other middle boxes
are present. If such cases aren't explicitly handled, an untimely
resumption attempt could discard partial state.

As a side-effect of this patch, downloads will also not discard partial
state due to other server errors during resumption (e.g. 500).

BUG=7648
R=svaldez@chromium.org, davidben@chromium.org, rdevlin.cronin@chromium.org
TBR=sky@chromium.org

Committed: https://crrev.com/edb920a2e81a6be07d4ef30da09e3f2e9debe0c1
Cr-Commit-Position: refs/heads/master@{#376625}
==========

[commit-bot: I haz the power, 2016-02-20 05:20:53]

Patchset 8 (id:??) landed as
https://crrev.com/edb920a2e81a6be07d4ef30da09e3f2e9debe0c1
Cr-Commit-Position: refs/heads/master@{#376625}