Clear the received slices in DownloadItemImpl when etag changed.

Clear the received slices in DownloadItemImpl when etag changed.

For parallel download, the received slices are currently passed in the
constructor of DownloadFileImpl, which is earlier than the etag check
logic in DownloadItemImpl. This raises an issue that the
DownloadFileImpl will use the out-dated received slices data. It will
block the download to refresh the data with new strong validators, and
download won't complete.

This CL cleared the received slices in DownloadItemImpl, and pass the
received slices to DownloadFileImpl via Initialize call instead of the
ctor.

Alternatively we may do it in DownloadManagerImpl, but it's probably
better to do this kind of clean up in DownloadItemImpl.


When etag changes, currently for a normal download, we update the etag
in DownloadItemImpl::UpdateValidatorsOnResumption, and
DownloadSaveInfo::offset is set to 0 in
DownloadRequestCore::HandleSuccessfulServerResponse, where we pass this
0 offset to BaseFile::Initialize to truncate the file size to 0.


BUG=706182, 644352
Review-Url: https://codereview.chromium.org/2799333002
Cr-Commit-Position: refs/heads/master@{#463417}
Committed: https://chromium.googlesource.com/chromium/src/+/583a61ad6d736c81b9f82c9e24676af19ce868e9

[xingliu, 2017-04-06 19:36:06]

Description was changed from

==========
Clear the received slices in DownloadItemImpl when etag changed.

For parallel download, the received slices are currently passed in the
constructor of DownloadFileImpl, which is earlier than the etag check
logic in DownloadItemImpl. This raises an issue that the
DownloadFileImpl will use the out-dated received slices data. It will
block the download to refresh the data with new strong validators, and
download won't complete.

Alternatively we may do it in DownloadManagerImpl, but it's probably
better to do this kind of clean up in DownloadItemImpl.


When etag changes, currently for a normal download, we update the etag
in DownloadItemImpl::UpdateValidatorsOnResumption, and
DownloadSaveInfo::offset is set to 0 in
DownloadRequestCore::HandleSuccessfulServerResponse, where we pass this
0 offset to BaseFile::Initialize to truncate the file size to 0.


BUG=706182, 644352
==========

to

==========
Clear the received slices in DownloadItemImpl when etag changed.

For parallel download, the received slices are currently passed in the
constructor of DownloadFileImpl, which is earlier than the etag check
logic in DownloadItemImpl. This raises an issue that the
DownloadFileImpl will use the out-dated received slices data. It will
block the download to refresh the data with new strong validators, and
download won't complete.

This CL cleared the received slices in DownloadItemImpl, and pass the
received slices to DownloadFileImpl via Initialize call instead of the
ctor.

Alternatively we may do it in DownloadManagerImpl, but it's probably
better to do this kind of clean up in DownloadItemImpl.


When etag changes, currently for a normal download, we update the etag
in DownloadItemImpl::UpdateValidatorsOnResumption, and
DownloadSaveInfo::offset is set to 0 in
DownloadRequestCore::HandleSuccessfulServerResponse, where we pass this
0 offset to BaseFile::Initialize to truncate the file size to 0.


BUG=706182, 644352
==========

[xingliu, 2017-04-06 19:36:35]

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

[commit-bot: I haz the power, 2017-04-06 19:37:47]

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

[xingliu, 2017-04-06 20:25:45]

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

[xingliu, 2017-04-06 20:25:46]

Hi, PTAL, thanks.

[commit-bot: I haz the power, 2017-04-06 21:04:50]

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

[commit-bot: I haz the power, 2017-04-06 21:04:51]

Dry run: This issue passed the CQ dry run.

[qinmin, 2017-04-07 18:03:49]

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

https://codereview.chromium.org/2799333002/diff/1/content/browser/download/do...
content/browser/download/download_item_impl.cc:1032: received_slices_.clear();
should we also clear the received_bytes_ after the
RecordOriginStateOnResumption().

The received_bytes_ will get cleared when DownloadFile updates its state through
a posted task. but if Chrome crashes before the update, then the etag_ and
last_modified_time_ has been updated to new values, but the received_bytes_
hasn't. If the db happens to commit the etag_ and last_modified_time_ change,
then if user restarts chrome and resumes the download, the download will not
start from offset 0.

[xingliu, 2017-04-07 20:50:08]

Thanks for the review. PTAL.

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

https://codereview.chromium.org/2799333002/diff/1/content/browser/download/do...
content/browser/download/download_item_impl.cc:1032: received_slices_.clear();
On 2017/04/07 18:03:49, qinmin wrote:
> should we also clear the received_bytes_ after the
> RecordOriginStateOnResumption().
> 
> The received_bytes_ will get cleared when DownloadFile updates its state
through
> a posted task. but if Chrome crashes before the update, then the etag_ and
> last_modified_time_ has been updated to new values, but the received_bytes_
> hasn't. If the db happens to commit the etag_ and last_modified_time_ change,
> then if user restarts chrome and resumes the download, the download will not
> start from offset 0.

Done.

Make sense, it's good to clear received slices before the new
content_disposition is set in memory. So we have a lower chance to get
inconsistency in db records.

[qinmin, 2017-04-07 21:01:47]

On 2017/04/07 20:50:08, xingliu wrote:
> Thanks for the review. PTAL.
> 
>
https://codereview.chromium.org/2799333002/diff/1/content/browser/download/do...
> File content/browser/download/download_item_impl.cc (right):
> 
>
https://codereview.chromium.org/2799333002/diff/1/content/browser/download/do...
> content/browser/download/download_item_impl.cc:1032: received_slices_.clear();
> On 2017/04/07 18:03:49, qinmin wrote:
> > should we also clear the received_bytes_ after the
> > RecordOriginStateOnResumption().
> > 
> > The received_bytes_ will get cleared when DownloadFile updates its state
> through
> > a posted task. but if Chrome crashes before the update, then the etag_ and
> > last_modified_time_ has been updated to new values, but the received_bytes_
> > hasn't. If the db happens to commit the etag_ and last_modified_time_
change,
> > then if user restarts chrome and resumes the download, the download will not
> > start from offset 0.
> 
> Done.
> 
> Make sense, it's good to clear received slices before the new
> content_disposition is set in memory. So we have a lower chance to get
> inconsistency in db records.

forget to upload the new patch?

[xingliu, 2017-04-07 22:18:03]

PTAL, thanks.

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

https://codereview.chromium.org/2799333002/diff/1/content/browser/download/do...
content/browser/download/download_item_impl.cc:1032: received_slices_.clear();
On 2017/04/07 18:03:49, qinmin wrote:
> should we also clear the received_bytes_ after the
> RecordOriginStateOnResumption().
> 
> The received_bytes_ will get cleared when DownloadFile updates its state
through
> a posted task. but if Chrome crashes before the update, then the etag_ and
> last_modified_time_ has been updated to new values, but the received_bytes_
> hasn't. If the db happens to commit the etag_ and last_modified_time_ change,
> then if user restarts chrome and resumes the download, the download will not
> start from offset 0.

Done,

Sorry that I somehow misunderstood the comment, set the received_bytes_ to 0
only when validators changed.

Also we probably shouldn't set the received_bytes_ to 0 after
RecordOriginStateOnResumption(), since when validators are not changed and
crashed before DownloadFile sets the received_bytes_, we probably should not
restart from 0.

[qinmin, 2017-04-07 23:37:44]

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

https://codereview.chromium.org/2799333002/diff/40001/content/browser/downloa...
content/browser/download/download_item_impl.cc:1033: received_bytes_ = 0;
you either need to do this after RecordOriginStateOnResumption(), or you have to
add a new boolean here, otherwise RecordOriginStateOnResumption() will record
the wrong metric.

So either:
bool is_partial = (received_bytes_ != 0);
if (etag_!=.....) {
  received_slices_.clear();
  received_bytes_ = 0;
}
RecordOriginStateOnResumption(is_partial, origin_state);


Or: 
if (etag_!=.....) {
  received_slices_.clear();
}
RecordOriginStateOnResumption(received_bytes_ != 0, origin_state);
if (etag_!=.....) {
  received_bytes_ = 0;
}

[xingliu, 2017-04-08 02:29:14]

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

https://codereview.chromium.org/2799333002/diff/40001/content/browser/downloa...
content/browser/download/download_item_impl.cc:1033: received_bytes_ = 0;
On 2017/04/07 23:37:43, qinmin wrote:
> you either need to do this after RecordOriginStateOnResumption(), or you have
to
> add a new boolean here, otherwise RecordOriginStateOnResumption() will record
> the wrong metric.
> 
> So either:
> bool is_partial = (received_bytes_ != 0);
> if (etag_!=.....) {
>   received_slices_.clear();
>   received_bytes_ = 0;
> }
> RecordOriginStateOnResumption(is_partial, origin_state);
> 
> 
> Or: 
> if (etag_!=.....) {
>   received_slices_.clear();
> }
> RecordOriginStateOnResumption(received_bytes_ != 0, origin_state);
> if (etag_!=.....) {
>   received_bytes_ = 0;
> }

Oh, I see, thanks for the suggestion, didn't notice it uses the received_bytes_
here.

[qinmin, 2017-04-08 05:55:18]

lgtm

[xingliu, 2017-04-10 17:48:31]

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

[commit-bot: I haz the power, 2017-04-10 17:49:03]

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-10 18:42:09]

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

[commit-bot: I haz the power, 2017-04-10 18:42:10]

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

[xingliu, 2017-04-10 19:02:24]

xingliu@chromium.org changed reviewers:
+ alexmos@chromium.org

[xingliu, 2017-04-10 19:02:25]

alexmos@chromium.org: Please review changes in 

content/public/test/test_file_error_injector.cc

Thank you.

[David Trainor- moved to gerrit, 2017-04-10 20:18:06]

lgtm

[alexmos, 2017-04-10 20:24:08]

content/public LGTM

[xingliu, 2017-04-10 21:26:20]

The CQ bit was checked by xingliu@chromium.org

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

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-10 22:12:42]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1491859580378790,
"parent_rev": "a3504f98584ce3e25570b963121fd51c7748b5b3", "commit_rev":
"583a61ad6d736c81b9f82c9e24676af19ce868e9"}

[commit-bot: I haz the power, 2017-04-10 22:13:39]

Description was changed from

==========
Clear the received slices in DownloadItemImpl when etag changed.

For parallel download, the received slices are currently passed in the
constructor of DownloadFileImpl, which is earlier than the etag check
logic in DownloadItemImpl. This raises an issue that the
DownloadFileImpl will use the out-dated received slices data. It will
block the download to refresh the data with new strong validators, and
download won't complete.

This CL cleared the received slices in DownloadItemImpl, and pass the
received slices to DownloadFileImpl via Initialize call instead of the
ctor.

Alternatively we may do it in DownloadManagerImpl, but it's probably
better to do this kind of clean up in DownloadItemImpl.


When etag changes, currently for a normal download, we update the etag
in DownloadItemImpl::UpdateValidatorsOnResumption, and
DownloadSaveInfo::offset is set to 0 in
DownloadRequestCore::HandleSuccessfulServerResponse, where we pass this
0 offset to BaseFile::Initialize to truncate the file size to 0.


BUG=706182, 644352
==========

to

==========
Clear the received slices in DownloadItemImpl when etag changed.

For parallel download, the received slices are currently passed in the
constructor of DownloadFileImpl, which is earlier than the etag check
logic in DownloadItemImpl. This raises an issue that the
DownloadFileImpl will use the out-dated received slices data. It will
block the download to refresh the data with new strong validators, and
download won't complete.

This CL cleared the received slices in DownloadItemImpl, and pass the
received slices to DownloadFileImpl via Initialize call instead of the
ctor.

Alternatively we may do it in DownloadManagerImpl, but it's probably
better to do this kind of clean up in DownloadItemImpl.


When etag changes, currently for a normal download, we update the etag
in DownloadItemImpl::UpdateValidatorsOnResumption, and
DownloadSaveInfo::offset is set to 0 in
DownloadRequestCore::HandleSuccessfulServerResponse, where we pass this
0 offset to BaseFile::Initialize to truncate the file size to 0.


BUG=706182, 644352

Review-Url: https://codereview.chromium.org/2799333002
Cr-Commit-Position: refs/heads/master@{#463417}
Committed:
https://chromium.googlesource.com/chromium/src/+/583a61ad6d736c81b9f82c9e2467...
==========

[commit-bot: I haz the power, 2017-04-10 22:13:40]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/583a61ad6d736c81b9f82c9e2467...