Commit bdc7250e authored by asanka's avatar asanka Committed by Commit bot

[Downloads] Avoid resetting SecureHash state across an interruption.

DownloadFileImpl/BaseFile was causing the SecureHash object to be
finalized when a download was interrupted. This caused the hash state to
be incorrect and subsequently the hash resulting from resuming the
download was also incorrect.

BUG=7648
BUG=581164

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

Cr-Commit-Position: refs/heads/master@{#371428}
parent 0905b871
...@@ -203,7 +203,11 @@ void BaseFile::Finish() { ...@@ -203,7 +203,11 @@ void BaseFile::Finish() {
if (calculate_hash_) if (calculate_hash_)
secure_hash_->Finish(sha256_hash_, crypto::kSHA256Length); secure_hash_->Finish(sha256_hash_, crypto::kSHA256Length);
Close();
}
void BaseFile::FinishWithError() {
DCHECK_CURRENTLY_ON(BrowserThread::FILE);
Close(); Close();
} }
......
...@@ -75,6 +75,11 @@ class CONTENT_EXPORT BaseFile { ...@@ -75,6 +75,11 @@ class CONTENT_EXPORT BaseFile {
// Indicate that the download has finished. No new data will be received. // Indicate that the download has finished. No new data will be received.
void Finish(); void Finish();
// Indicate that the download is being aborted due to an error. This is
// identical to Finish() with the exception that the hash state will not be
// finalized.
void FinishWithError();
// Set the client guid which will be used to identify the app to the // Set the client guid which will be used to identify the app to the
// system AV scanning function. Should be called before // system AV scanning function. Should be called before
// AnnotateWithSourceInformation() to take effect. // AnnotateWithSourceInformation() to take effect.
......
...@@ -383,6 +383,8 @@ class TestShellDownloadManagerDelegate : public ShellDownloadManagerDelegate { ...@@ -383,6 +383,8 @@ class TestShellDownloadManagerDelegate : public ShellDownloadManagerDelegate {
return true; return true;
} }
bool GenerateFileHash() override { return true; }
void SetDelayedOpen(bool delay) { void SetDelayedOpen(bool delay) {
delay_download_open_ = delay; delay_download_open_ = delay;
} }
...@@ -1399,6 +1401,59 @@ IN_PROC_BROWSER_TEST_P(DownloadResumptionContentTest, ...@@ -1399,6 +1401,59 @@ IN_PROC_BROWSER_TEST_P(DownloadResumptionContentTest,
EXPECT_EQ(download->GetState(), DownloadItem::COMPLETE); EXPECT_EQ(download->GetState(), DownloadItem::COMPLETE);
} }
IN_PROC_BROWSER_TEST_P(DownloadResumptionContentTest, Resume_Hash) {
using InjectedError = TestDownloadRequestHandler::InjectedError;
const char kExpectedHash[] =
"\xa7\x44\x49\x86\x24\xc6\x84\x6c\x89\xdf\xd8\xec\xa0\xe0\x61\x12\xdc\x80"
"\x13\xf2\x83\x49\xa9\x14\x52\x32\xf0\x95\x20\xca\x5b\x30";
std::string expected_hash(kExpectedHash);
TestDownloadRequestHandler request_handler;
TestDownloadRequestHandler::Parameters parameters;
// As a control, let's try GetHash() on an uninterrupted download.
request_handler.StartServing(parameters);
DownloadItem* uninterrupted_download(StartDownloadAndReturnItem(
initiator_shell_for_resumption(), request_handler.url()));
WaitForCompletion(uninterrupted_download);
EXPECT_EQ(expected_hash, uninterrupted_download->GetHash());
// Now with interruptions.
parameters.injected_errors.push(
InjectedError(100, net::ERR_CONNECTION_RESET));
parameters.injected_errors.push(
InjectedError(211, net::ERR_CONNECTION_RESET));
parameters.injected_errors.push(
InjectedError(337, net::ERR_CONNECTION_RESET));
parameters.injected_errors.push(
InjectedError(400, net::ERR_CONNECTION_RESET));
parameters.injected_errors.push(
InjectedError(512, net::ERR_CONNECTION_RESET));
request_handler.StartServing(parameters);
// Start and watch for interrupt.
DownloadItem* download(StartDownloadAndReturnItem(
initiator_shell_for_resumption(), request_handler.url()));
WaitForInterrupt(download);
PrepareToResume();
download->Resume();
WaitForInterrupt(download);
download->Resume();
WaitForInterrupt(download);
download->Resume();
WaitForInterrupt(download);
download->Resume();
WaitForInterrupt(download);
download->Resume();
WaitForCompletion(download);
EXPECT_EQ(expected_hash, download->GetHash());
}
// An interrupted download should remove the intermediate file when it is // An interrupted download should remove the intermediate file when it is
// cancelled. // cancelled.
IN_PROC_BROWSER_TEST_P(DownloadResumptionContentTest, IN_PROC_BROWSER_TEST_P(DownloadResumptionContentTest,
......
...@@ -280,7 +280,10 @@ void DownloadFileImpl::StreamActive() { ...@@ -280,7 +280,10 @@ void DownloadFileImpl::StreamActive() {
stream_reader_->GetStatus()); stream_reader_->GetStatus());
SendUpdate(); SendUpdate();
base::TimeTicks close_start(base::TimeTicks::Now()); base::TimeTicks close_start(base::TimeTicks::Now());
file_.Finish(); if (reason == DOWNLOAD_INTERRUPT_REASON_NONE)
file_.Finish();
else
file_.FinishWithError();
base::TimeTicks now(base::TimeTicks::Now()); base::TimeTicks now(base::TimeTicks::Now());
disk_writes_time_ += (now - close_start); disk_writes_time_ += (now - close_start);
RecordFileBandwidth( RecordFileBandwidth(
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment