Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(516)

Issue 1751603002: [Downloads] Rework how hashes are calculated for download files. (Closed)

Created:
4 years, 9 months ago by asanka
Modified:
4 years, 9 months ago
Reviewers:
Ted C, Devlin, svaldez, jam, davidben
CC:
chromium-reviews, darin-cc_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

[Downloads] Rework how hashes are calculated for download files. Prior to this CL, downloads assumed that there would be a stable serialization of hash state. This serialization was meant to be persisted and used if the download was subsequently resumed, thus avoiding rehashing the portion of the download completed so far. However, there's no standard serialization for SHA256 hash state. Any change to the serialization format would render any historical data invalid. While it's possible to come up with a reasonable and stable format for hash state, the resulting complexity isn't worth it. Fortunately, the code for persisting the hash state was never written. Hence it's not too late to change things. With this CL, downloads will behave as follows with respect to hashes: * An in-progress download will calculate the hash of the bytes written to disk so far using a crypto::SecureHash object using SHA256. * Progress notifications from DownloadItem / DownloadFile no longer report a hash state. * If the download completes successfully, DownloadFile exposes the crypto::SecureHash object representing the final hash state via the DownloadDestinationObserver interface. DownloadItemImpl uses this object to calculate the final hash which is then made available via DownloadItem::GetHash() for completed downloads. * In the event of an interruption, DownloadFile will expose the crypto::SecureHash object representing the hash state. DownloadItemImpl uses a clone of the hash state to obtain a SHA256 hash of the partial data. This hash is available via DownloadItem::GetHash() on interrupted downloads. DownloadItemImpl also keeps the crypto::SecureHash object in case the download is resumed later. * Resuming downloads pass the crypto::SecureHash object representing the partial state via DownloadSaveInfo. If a crypto::SecureHash object isn't available (e.g. because the download was restored from history), then DownloadItemImpl can optionally pass along a SHA256 hash of the partial file. If for some reason, the partial state of the download is abandoned (e.g. because of a validation error), then DownloadRequestCore destroys the cryto::SecureHash object and resets the prefix hash so that the download can restart from the beginning. * When DownloadManagerImpl receives a StartDownload() callback (which happens when a response is available for a download request), the crypto::SecureHash object passed within DownloadSaveInfo is used to construct a new DownloadFile. * A newly created DownloadFile assumes that a crypto::SecureHash object passed to it correctly represents the partial state of the partial file. * In the absence of a crypto::SecureHash object, DownloadFile reads the partial file and calculates the partial hash state in a new crypto::SecureHash object. If a prefix hash value is available, then the hash of the partial file is matched against this prefix hash. A mismatch causes a FILE_HASH_MISMATCH error which in turn causes the download to abandon its partial state and restart. These rules establish the following invariants: * All downloads calculate the SHA256 hash of its contents. * Regardless of how a download is started or resumed, by the time it is completed successfully, DownloadItem::GetHash() correctly reports the SHA256 hash of the downloaded bytes. * Regardless of how a download is started or resumed, an interrupted download with a received byte count > 0 will always report the correct SHA256 hash of the partial data in DownloadItem::GetHash(). Note that this CL doesn't add code to persist the hash for an interrupted download. In order to keep the size of the CL sane, the download history changes are going to be done in a follow up CL. BUG=7648 BUG=563684 Committed: https://crrev.com/4350f6a0843ceb70885f375f9dcf0df9c081faf0 Cr-Commit-Position: refs/heads/master@{#381158}

Patch Set 1 #

Patch Set 2 : #

Patch Set 3 : #

Patch Set 4 : #

Patch Set 5 : #

Patch Set 6 : #

Patch Set 7 : #

Patch Set 8 : #

Total comments: 3

Patch Set 9 : #

Total comments: 6

Patch Set 10 : Try to appease MSVC #

Total comments: 18

Patch Set 11 : Address first round of comments #

Total comments: 10

Patch Set 12 : Try to make hash references less confusing #

Patch Set 13 : Rebase on top of https://codereview.chromium.org/1781983002 since that's going in first. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+1887 lines, -1288 lines) Patch
M chrome/browser/download/download_browsertest.cc View 1 2 1 chunk +4 lines, -4 lines 0 comments Download
M chrome/browser/download/download_history.cc View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +3 lines, -0 lines 0 comments Download
M chrome/browser/download/download_history_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +1 line, -0 lines 0 comments Download
M chrome/browser/download/download_item_model.cc View 1 2 2 chunks +2 lines, -0 lines 0 comments Download
M chrome/browser/download/download_item_model_unittest.cc View 1 2 chunks +4 lines, -0 lines 0 comments Download
M chrome/browser/extensions/api/downloads/downloads_api_browsertest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +1 line, -0 lines 0 comments Download
M chrome/common/extensions/api/downloads.idl View 1 1 chunk +1 line, -0 lines 0 comments Download
M components/history/content/browser/content_history_backend_db_unittest.cc View 1 2 3 4 5 6 1 chunk +1 line, -0 lines 0 comments Download
M content/browser/download/base_file.h View 1 2 3 4 5 6 7 8 9 10 5 chunks +112 lines, -78 lines 0 comments Download
M content/browser/download/base_file.cc View 1 2 3 4 5 6 7 8 9 10 11 12 8 chunks +110 lines, -91 lines 0 comments Download
M content/browser/download/base_file_linux.cc View 1 chunk +5 lines, -2 lines 0 comments Download
M content/browser/download/base_file_mac.cc View 1 chunk +6 lines, -3 lines 0 comments Download
M content/browser/download/base_file_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 19 chunks +253 lines, -206 lines 0 comments Download
M content/browser/download/base_file_win.cc View 1 2 3 4 5 6 1 chunk +7 lines, -4 lines 0 comments Download
M content/browser/download/download_browsertest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 11 chunks +449 lines, -51 lines 0 comments Download
A + content/browser/download/download_destination_observer.h View 2 chunks +12 lines, -7 lines 0 comments Download
M content/browser/download/download_file.h View 1 3 chunks +6 lines, -23 lines 0 comments Download
M content/browser/download/download_file_factory.h View 2 chunks +5 lines, -5 lines 0 comments Download
M content/browser/download/download_file_factory.cc View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +6 lines, -8 lines 0 comments Download
M content/browser/download/download_file_impl.h View 1 4 chunks +33 lines, -24 lines 0 comments Download
M content/browser/download/download_file_impl.cc View 1 2 3 4 5 6 7 8 9 10 11 12 14 chunks +75 lines, -86 lines 0 comments Download
M content/browser/download/download_file_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 27 chunks +114 lines, -112 lines 0 comments Download
M content/browser/download/download_item_factory.h View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +1 line, -0 lines 0 comments Download
M content/browser/download/download_item_impl.h View 1 2 3 4 5 6 7 8 9 10 11 12 10 chunks +75 lines, -48 lines 0 comments Download
M content/browser/download/download_item_impl.cc View 1 2 3 4 5 6 7 8 9 10 11 12 35 chunks +170 lines, -90 lines 0 comments Download
M content/browser/download/download_item_impl_delegate.h View 1 chunk +5 lines, -0 lines 0 comments Download
M content/browser/download/download_item_impl_delegate.cc View 1 chunk +5 lines, -0 lines 0 comments Download
M content/browser/download/download_item_impl_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 55 chunks +222 lines, -131 lines 0 comments Download
M content/browser/download/download_manager_impl.h View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +1 line, -0 lines 0 comments Download
M content/browser/download/download_manager_impl.cc View 1 2 3 4 5 6 7 8 9 10 11 12 5 chunks +13 lines, -10 lines 0 comments Download
M content/browser/download/download_manager_impl_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 8 chunks +13 lines, -17 lines 0 comments Download
M content/browser/download/download_net_log_parameters.h View 3 chunks +0 lines, -3 lines 0 comments Download
M content/browser/download/download_net_log_parameters.cc View 3 chunks +0 lines, -9 lines 0 comments Download
M content/browser/download/download_request_core.cc View 1 2 3 4 5 6 7 8 9 10 11 2 chunks +4 lines, -8 lines 0 comments Download
M content/browser/download/mock_download_file.h View 1 2 3 4 1 chunk +5 lines, -4 lines 0 comments Download
M content/browser/download/save_file.h View 1 chunk +0 lines, -1 line 0 comments Download
M content/browser/download/save_file.cc View 1 2 3 4 5 6 7 8 9 10 11 12 4 chunks +8 lines, -15 lines 0 comments Download
M content/browser/download/save_package.cc View 1 2 3 4 3 chunks +5 lines, -8 lines 0 comments Download
M content/content_browser.gypi View 1 2 3 4 5 6 7 8 9 10 11 12 2 chunks +1 line, -1 line 0 comments Download
D content/public/browser/download_destination_observer.h View 1 chunk +0 lines, -39 lines 0 comments Download
M content/public/browser/download_interrupt_reason_values.h View 1 chunk +3 lines, -0 lines 0 comments Download
M content/public/browser/download_item.h View 1 2 3 4 5 6 7 8 9 10 11 12 2 chunks +4 lines, -7 lines 0 comments Download
M content/public/browser/download_manager.h View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +1 line, -0 lines 0 comments Download
M content/public/browser/download_save_info.h View 1 2 3 4 5 6 7 8 9 10 11 3 chunks +11 lines, -2 lines 0 comments Download
M content/public/browser/download_save_info.cc View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +9 lines, -0 lines 0 comments Download
M content/public/browser/download_url_parameters.h View 1 2 3 4 5 6 7 8 9 10 11 2 chunks +21 lines, -6 lines 0 comments Download
M content/public/test/mock_download_manager.h View 1 2 3 4 5 6 7 8 9 10 11 12 3 chunks +3 lines, -0 lines 0 comments Download
M content/public/test/mock_download_manager.cc View 1 2 3 4 5 6 7 8 9 10 11 12 4 chunks +4 lines, -0 lines 0 comments Download
M content/public/test/test_file_error_injector.cc View 1 2 3 4 5 6 7 8 9 10 11 12 8 chunks +31 lines, -44 lines 0 comments Download
M crypto/secure_hash.h View 2 chunks +5 lines, -15 lines 0 comments Download
M crypto/secure_hash_default.cc View 1 2 3 4 5 6 7 8 9 10 2 chunks +7 lines, -43 lines 0 comments Download
M crypto/secure_hash_openssl.cc View 1 2 3 4 5 6 7 8 9 10 2 chunks +9 lines, -46 lines 0 comments Download
M crypto/secure_hash_unittest.cc View 1 2 3 4 5 6 7 8 9 10 2 chunks +36 lines, -31 lines 0 comments Download
M crypto/third_party/nss/sha512.cc View 1 2 3 4 5 6 7 8 9 10 2 chunks +5 lines, -6 lines 0 comments Download

Depends on Patchset:

Messages

Total messages: 27 (10 generated)
asanka
Apologies for the size of the CL. Unfortunately it's a bit of a tangled mess. ...
4 years, 9 months ago (2016-03-04 22:08:06 UTC) #5
Devlin
Wow, I'm lucky here. ;) Extensions lg so far, but I'm gonna hold off on ...
4 years, 9 months ago (2016-03-04 22:49:22 UTC) #6
davidben
crypto lgtm. A nit: > However, there's no standard serialization for SHA256 hashes. Any change ...
4 years, 9 months ago (2016-03-08 22:12:03 UTC) #7
svaldez
Mostly just a few comment nits from my first pass. I'll take another pass in ...
4 years, 9 months ago (2016-03-09 19:27:34 UTC) #8
asanka
On 2016/03/08 at 22:12:03, davidben wrote: > crypto lgtm. > > A nit: > > ...
4 years, 9 months ago (2016-03-10 16:47:58 UTC) #10
asanka
https://codereview.chromium.org/1751603002/diff/160001/chrome/browser/download/download_browsertest.cc File chrome/browser/download/download_browsertest.cc (right): https://codereview.chromium.org/1751603002/diff/160001/chrome/browser/download/download_browsertest.cc#newcode1721 chrome/browser/download/download_browsertest.cc:1721: EXPECT_EQ(0, row1.received_bytes); // There's no ETag. So the intermediate ...
4 years, 9 months ago (2016-03-10 16:48:09 UTC) #11
Ted C
On 2016/03/10 16:48:09, asanka wrote: > https://codereview.chromium.org/1751603002/diff/160001/chrome/browser/download/download_browsertest.cc > File chrome/browser/download/download_browsertest.cc (right): > > https://codereview.chromium.org/1751603002/diff/160001/chrome/browser/download/download_browsertest.cc#newcode1721 > ...
4 years, 9 months ago (2016-03-10 17:30:03 UTC) #12
svaldez
Few more nits, probably mostly ignorable. It'd be nice if we could merge hash and ...
4 years, 9 months ago (2016-03-10 17:45:34 UTC) #13
Devlin
extensions lgtm
4 years, 9 months ago (2016-03-10 17:51:50 UTC) #14
asanka
https://codereview.chromium.org/1751603002/diff/200001/content/browser/download/base_file_unittest.cc File content/browser/download/base_file_unittest.cc (right): https://codereview.chromium.org/1751603002/diff/200001/content/browser/download/base_file_unittest.cc#newcode126 content/browser/download/base_file_unittest.cc:126: BaseFile file((net::BoundNetLog())); On 2016/03/10 at 17:45:33, svaldez wrote: > ...
4 years, 9 months ago (2016-03-11 16:25:09 UTC) #15
svaldez
lgtm
4 years, 9 months ago (2016-03-11 16:30:27 UTC) #16
asanka
Thanks everyone! jam: Could you have a look? I've summarized the changes that need your ...
4 years, 9 months ago (2016-03-11 16:47:45 UTC) #17
jam
On 2016/03/11 16:47:45, asanka wrote: > Thanks everyone! > > jam: Could you have a ...
4 years, 9 months ago (2016-03-14 16:23:52 UTC) #18
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1751603002/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1751603002/240001
4 years, 9 months ago (2016-03-15 01:13:05 UTC) #20
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1751603002/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1751603002/240001
4 years, 9 months ago (2016-03-15 01:47:13 UTC) #24
commit-bot: I haz the power
Committed patchset #13 (id:240001)
4 years, 9 months ago (2016-03-15 02:41:06 UTC) #25
commit-bot: I haz the power
4 years, 9 months ago (2016-03-15 02:42:26 UTC) #27
Message was sent while issue was closed.
Patchset 13 (id:??) landed as
https://crrev.com/4350f6a0843ceb70885f375f9dcf0df9c081faf0
Cr-Commit-Position: refs/heads/master@{#381158}

Powered by Google App Engine
This is Rietveld 408576698