New issue
Advanced search Search tips

Issue 603330 link

Starred by 2 users
Status: Fixed
Owner:
asanka@chromium.org
Closed: Apr 2016
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 1
Type: Bug

Blocking:
issue 7648



Sign in to add a comment

Large partial files can get discarded during resumption

Project Member Reported by asanka@chromium.org, Apr 13 2016 
In cases where the size of the partial file doesn't match the persisted metadata, we resolve the conflict in favor of the persisted metadata. BaseFile does this by truncating the partial file to match.

BaseFile::Initialize() -> BaseFile::CalculatePartialHash() can end up reading past the expected EOF in this case for large partial files. If this happens, BaseFile incorrectly concludes that the partial file too short and discards it.
Project Member

Comment 1 by bugdroid1@chromium.org, Apr 14 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/aeeba88e199ca02307bf5dfb651c867cfdb94a16

commit aeeba88e199ca02307bf5dfb651c867cfdb94a16
Author: asanka <asanka@chromium.org>
Date: Thu Apr 14 18:17:31 2016

[Downloads] BaseFile shouldn't read past the expected EOF for a stub file.

The partial stub file may have more data than expected due to
discrepancy between persisted metadata and the intermediate file.
Currently we resolve this conflict in favor of persisted metadata and
truncate the partial file to match. In this case, BaseFile::Initialize()
-> BaseFile::CalculatePartialHash() ended up reading past teh expected
EOF for large intermediate files. Consequently, BaseFile would conclude
incorrectly that the partial file is too short.

This patch fixes this bug by correctly stopping reading prior to the
expected EOF.

R=svaldez@chromium.org
BUG= 603330 

Review URL: https://codereview.chromium.org/1887873002

Cr-Commit-Position: refs/heads/master@{#387370}

[modify] https://crrev.com/aeeba88e199ca02307bf5dfb651c867cfdb94a16/content/browser/download/base_file.cc
[modify] https://crrev.com/aeeba88e199ca02307bf5dfb651c867cfdb94a16/content/browser/download/base_file_unittest.cc
[modify] https://crrev.com/aeeba88e199ca02307bf5dfb651c867cfdb94a16/content/browser/download/download_browsertest.cc

Comment 2 by asanka@chromium.org, Apr 14 2016

Status: Fixed (was: Assigned)

Comment 3 by asanka@chromium.org, May 10 2016

Labels: Merge-Request-51
This change has baked in Canary/Dev for a while now and is low risk.

Comment 4 by tin...@google.com, May 10 2016

Labels: -Merge-Request-51 Merge-Approved-51 Hotlist-Merge-Approved
Your change meets the bar and is auto-approved for M51 (branch: 2704)
Project Member

Comment 5 by bugdroid1@chromium.org, May 10 2016

Labels: -merge-approved-51 merge-merged-2704
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/2bb4bee49cf1ef7c9c19b4790f5caf80a77c3ccf

commit 2bb4bee49cf1ef7c9c19b4790f5caf80a77c3ccf
Author: Asanka Herath <asanka@chromium.org>
Date: Tue May 10 04:15:47 2016

[Merge M51][Downloads] BaseFile shouldn't read past the expected EOF for a stub file.

The partial stub file may have more data than expected due to
discrepancy between persisted metadata and the intermediate file.
Currently we resolve this conflict in favor of persisted metadata and
truncate the partial file to match. In this case, BaseFile::Initialize()
-> BaseFile::CalculatePartialHash() ended up reading past teh expected
EOF for large intermediate files. Consequently, BaseFile would conclude
incorrectly that the partial file is too short.

This patch fixes this bug by correctly stopping reading prior to the
expected EOF.

R=svaldez@chromium.org
BUG= 603330 

Review URL: https://codereview.chromium.org/1887873002

Cr-Commit-Position: refs/heads/master@{#387370}
(cherry picked from commit aeeba88e199ca02307bf5dfb651c867cfdb94a16)

Review URL: https://codereview.chromium.org/1956403005 .

Cr-Commit-Position: refs/branch-heads/2704@{#463}
Cr-Branched-From: 6e53600def8f60d8c632fadc70d7c1939ccea347-refs/heads/master@{#386251}

[modify] https://crrev.com/2bb4bee49cf1ef7c9c19b4790f5caf80a77c3ccf/content/browser/download/base_file.cc
[modify] https://crrev.com/2bb4bee49cf1ef7c9c19b4790f5caf80a77c3ccf/content/browser/download/base_file_unittest.cc
[modify] https://crrev.com/2bb4bee49cf1ef7c9c19b4790f5caf80a77c3ccf/content/browser/download/download_browsertest.cc
Project Member

Comment 6 by bugdroid1@chromium.org, May 10 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/9e40fc4df45be64ec6f2d3f724785807e1f473ce

commit 9e40fc4df45be64ec6f2d3f724785807e1f473ce
Author: Asanka Herath <asanka@chromium.org>
Date: Tue May 10 14:05:34 2016

[M51] Fix build after 2bb4bee49cf1ef7c9c19b4790f5caf80a77c3ccf.

TBR=asanka@chromium.org
BUG= 603330 

Review URL: https://codereview.chromium.org/1958413004 .

Cr-Commit-Position: refs/branch-heads/2704@{#469}
Cr-Branched-From: 6e53600def8f60d8c632fadc70d7c1939ccea347-refs/heads/master@{#386251}

[modify] https://crrev.com/9e40fc4df45be64ec6f2d3f724785807e1f473ce/content/browser/download/download_browsertest.cc
►

Sign in to add a comment