[Downloads] Rework how hashes are calculated for download files.

content/browser/download/download_file_unittest.cc

Index: content/browser/download/download_file_unittest.cc
diff --git a/content/browser/download/download_file_unittest.cc b/content/browser/download/download_file_unittest.cc
index 245fdcd078312e9f26fbd6356760999eef5d75da..d24eeb81392016f001ecf5499debde63a09f43d7 100644
--- a/content/browser/download/download_file_unittest.cc
+++ b/content/browser/download/download_file_unittest.cc
@@ -4,7 +4,9 @@
 
 #include <stddef.h>
 #include <stdint.h>
+
 #include <utility>
+#include <vector>
 
 #include "base/files/file.h"
 #include "base/files/file_util.h"
@@ -18,9 +20,9 @@
 #include "content/browser/browser_thread_impl.h"
 #include "content/browser/byte_stream.h"
 #include "content/browser/download/download_create_info.h"
+#include "content/browser/download/download_destination_observer.h"
 #include "content/browser/download/download_file_impl.h"
 #include "content/browser/download/download_request_handle.h"
-#include "content/public/browser/download_destination_observer.h"
 #include "content/public/browser/download_interrupt_reasons.h"
 #include "content/public/browser/download_manager.h"
 #include "content/public/test/mock_download_manager.h"
@@ -41,6 +43,14 @@ using ::testing::StrictMock;
 namespace content {
 namespace {
 
+std::string GetHexEncodedHashValue(crypto::SecureHash* hash_state) {
+  if (!hash_state)
+    return std::string();
+  std::vector<char> hash_value(hash_state->GetHashLength());
+  hash_state->Finish(&hash_value.front(), hash_value.size());
+  return base::HexEncode(&hash_value.front(), hash_value.size());
+}
+
 class MockByteStreamReader : public ByteStreamReader {
  public:
   MockByteStreamReader() {}
@@ -55,41 +65,45 @@ class MockByteStreamReader : public ByteStreamReader {
 
 class MockDownloadDestinationObserver : public DownloadDestinationObserver {
  public:
-  MOCK_METHOD3(DestinationUpdate, void(int64_t, int64_t, const std::string&));
-  MOCK_METHOD1(DestinationError, void(DownloadInterruptReason));
-  MOCK_METHOD1(DestinationCompleted, void(const std::string&));
+  MOCK_METHOD2(DestinationUpdate, void(int64_t, int64_t));
+  void DestinationError(DownloadInterruptReason reason,
+                        int64_t bytes_so_far,
+                        scoped_ptr<crypto::SecureHash> hash_state) override {
+    MockDestinationError(
+        reason, bytes_so_far, GetHexEncodedHashValue(hash_state.get()));
+  }
+  void DestinationCompleted(
+      int64_t total_bytes,
+      scoped_ptr<crypto::SecureHash> hash_state) override {
+    MockDestinationCompleted(total_bytes,
+                             GetHexEncodedHashValue(hash_state.get()));
+  }
+
+  MOCK_METHOD3(MockDestinationError,
+               void(DownloadInterruptReason, int64_t, const std::string&));
+  MOCK_METHOD2(MockDestinationCompleted, void(int64_t, const std::string&));
 
   // Doesn't override any methods in the base class.  Used to make sure
   // that the last DestinationUpdate before a Destination{Completed,Error}
   // had the right values.
-  MOCK_METHOD3(CurrentUpdateStatus, void(int64_t, int64_t, const std::string&));
+  MOCK_METHOD2(CurrentUpdateStatus, void(int64_t, int64_t));
 };
 
 MATCHER(IsNullCallback, "") { return (arg.is_null()); }
 
-typedef void (DownloadFile::*DownloadFileRenameMethodType)(
-    const base::FilePath&,
-    const DownloadFile::RenameCompletionCallback&);
+enum DownloadFileRenameMethodType { RENAME_AND_UNIQUIFY, RENAME_AND_ANNOTATE };
 
 // This is a test DownloadFileImpl that has no retry delay and, on Posix,
 // retries renames failed due to ACCESS_DENIED.
 class TestDownloadFileImpl : public DownloadFileImpl {
  public:
-  TestDownloadFileImpl(const DownloadSaveInfo& save_info,
+  TestDownloadFileImpl(scoped_ptr<DownloadSaveInfo> save_info,
                        const base::FilePath& default_downloads_directory,
-                       const GURL& url,
-                       const GURL& referrer_url,
-                       bool calculate_hash,
-                       base::File file,
                        scoped_ptr<ByteStreamReader> stream,
                        const net::BoundNetLog& bound_net_log,
                        base::WeakPtr<DownloadDestinationObserver> observer)
-      : DownloadFileImpl(save_info,
+      : DownloadFileImpl(std::move(save_info),
                          default_downloads_directory,
-                         url,
-                         referrer_url,
-                         calculate_hash,
-                         std::move(file),
                          std::move(stream),
                          bound_net_log,
                          observer) {}
@@ -113,11 +127,11 @@ class TestDownloadFileImpl : public DownloadFileImpl {
 
 class DownloadFileTest : public testing::Test {
  public:
-
-  static const char* kTestData1;
-  static const char* kTestData2;
-  static const char* kTestData3;
-  static const char* kDataHash;
+  static const char kTestData1[];
+  static const char kTestData2[];
+  static const char kTestData3[];
+  static const char kDataHash[];
+  static const char kEmptyHash[];
   static const uint32_t kDummyDownloadId;
   static const int kDummyChildId;
   static const int kDummyRequestId;
@@ -128,27 +142,23 @@ class DownloadFileTest : public testing::Test {
       input_stream_(NULL),
       bytes_(-1),
       bytes_per_sec_(-1),
-      hash_state_("xyzzy"),
       ui_thread_(BrowserThread::UI, &loop_),
       file_thread_(BrowserThread::FILE, &loop_) {
   }
 
   ~DownloadFileTest() override {}
 
-  void SetUpdateDownloadInfo(int64_t bytes,
-                             int64_t bytes_per_sec,
-                             const std::string& hash_state) {
+  void SetUpdateDownloadInfo(int64_t bytes, int64_t bytes_per_sec) {
     bytes_ = bytes;
     bytes_per_sec_ = bytes_per_sec;
-    hash_state_ = hash_state;
   }
 
   void ConfirmUpdateDownloadInfo() {
-    observer_->CurrentUpdateStatus(bytes_, bytes_per_sec_, hash_state_);
+    observer_->CurrentUpdateStatus(bytes_, bytes_per_sec_);
   }
 
   void SetUp() override {
-    EXPECT_CALL(*(observer_.get()), DestinationUpdate(_, _, _))
+    EXPECT_CALL(*(observer_.get()), DestinationUpdate(_, _))
         .Times(AnyNumber())
         .WillRepeatedly(Invoke(this, &DownloadFileTest::SetUpdateDownloadInfo));
   }
@@ -178,14 +188,12 @@ class DownloadFileTest : public testing::Test {
         .RetiresOnSaturation();
 
     scoped_ptr<DownloadSaveInfo> save_info(new DownloadSaveInfo());
-    download_file_.reset(new TestDownloadFileImpl(
-        *save_info, base::FilePath(),
-        GURL(),  // Source
-        GURL(),  // Referrer
-        calculate_hash, std::move(save_info->file),
-        scoped_ptr<ByteStreamReader>(input_stream_), net::BoundNetLog(),
-        observer_factory_.GetWeakPtr()));
-    download_file_->SetClientGuid("12345678-ABCD-1234-DCBA-123456789ABC");
+    download_file_.reset(
+        new TestDownloadFileImpl(std::move(save_info),
+                                 base::FilePath(),
+                                 scoped_ptr<ByteStreamReader>(input_stream_),
+                                 net::BoundNetLog(),
+                                 observer_factory_.GetWeakPtr()));
 
     EXPECT_CALL(*input_stream_, Read(_, _))
         .WillOnce(Return(ByteStreamReader::STREAM_EMPTY))
@@ -270,19 +278,21 @@ class DownloadFileTest : public testing::Test {
   }
 
   void FinishStream(DownloadInterruptReason interrupt_reason,
-                    bool check_observer) {
+                    bool check_observer,
+                    const std::string& expected_hash) {
     ::testing::Sequence s1;
     SetupFinishStream(interrupt_reason, s1);
     sink_callback_.Run();
     VerifyStreamAndSize();
     if (check_observer) {
-      EXPECT_CALL(*(observer_.get()), DestinationCompleted(_));
+      EXPECT_CALL(*(observer_.get()),
+                  MockDestinationCompleted(_, expected_hash));
       loop_.RunUntilIdle();
       ::testing::Mock::VerifyAndClearExpectations(observer_.get());
-      EXPECT_CALL(*(observer_.get()), DestinationUpdate(_, _, _))
+      EXPECT_CALL(*(observer_.get()), DestinationUpdate(_, _))
           .Times(AnyNumber())
-          .WillRepeatedly(Invoke(this,
-                                 &DownloadFileTest::SetUpdateDownloadInfo));
+          .WillRepeatedly(
+              Invoke(this, &DownloadFileTest::SetUpdateDownloadInfo));
     }
   }
 
@@ -290,14 +300,14 @@ class DownloadFileTest : public testing::Test {
       const base::FilePath& full_path,
       base::FilePath* result_path_p) {
     return InvokeRenameMethodAndWaitForCallback(
-        &DownloadFile::RenameAndUniquify, full_path, result_path_p);
+        RENAME_AND_UNIQUIFY, full_path, result_path_p);
   }
 
   DownloadInterruptReason RenameAndAnnotate(
       const base::FilePath& full_path,
       base::FilePath* result_path_p) {
     return InvokeRenameMethodAndWaitForCallback(
-        &DownloadFile::RenameAndAnnotate, full_path, result_path_p);
+        RENAME_AND_ANNOTATE, full_path, result_path_p);
   }
 
   void ExpectPermissionError(DownloadInterruptReason err) {
@@ -307,20 +317,40 @@ class DownloadFileTest : public testing::Test {
   }
 
  protected:
+  void InvokeRenameMethod(
+      DownloadFileRenameMethodType method,
+      const base::FilePath& full_path,
+      const DownloadFile::RenameCompletionCallback& completion_callback) {
+    switch (method) {
+      case RENAME_AND_UNIQUIFY:
+        download_file_->RenameAndUniquify(full_path, completion_callback);
+        break;
+
+      case RENAME_AND_ANNOTATE:
+        download_file_->RenameAndAnnotate(
+            full_path,
+            "12345678-ABCD-1234-DCBA-123456789ABC",
+            GURL(),
+            GURL(),
+            completion_callback);
+        break;
+    }
+  }
+
   DownloadInterruptReason InvokeRenameMethodAndWaitForCallback(
       DownloadFileRenameMethodType method,
       const base::FilePath& full_path,
       base::FilePath* result_path_p) {
     DownloadInterruptReason result_reason(DOWNLOAD_INTERRUPT_REASON_NONE);
     base::FilePath result_path;
-
     base::RunLoop loop_runner;
-    ((*download_file_).*method)(full_path,
-                                base::Bind(&DownloadFileTest::SetRenameResult,
-                                           base::Unretained(this),
-                                           loop_runner.QuitClosure(),
-                                           &result_reason,
-                                           result_path_p));
+    DownloadFile::RenameCompletionCallback completion_callback =
+        base::Bind(&DownloadFileTest::SetRenameResult,
+                   base::Unretained(this),
+                   loop_runner.QuitClosure(),
+                   &result_reason,
+                   result_path_p);
+    InvokeRenameMethod(method, full_path, completion_callback);
     loop_runner.Run();
     return result_reason;
   }
@@ -341,7 +371,6 @@ class DownloadFileTest : public testing::Test {
   // Latest update sent to the observer.
   int64_t bytes_;
   int64_t bytes_per_sec_;
-  std::string hash_state_;
 
   base::MessageLoop loop_;
 
@@ -391,15 +420,17 @@ class DownloadFileTestWithRename
 // the value parameter.
 INSTANTIATE_TEST_CASE_P(DownloadFile,
                         DownloadFileTestWithRename,
-                        ::testing::Values(&DownloadFile::RenameAndAnnotate,
-                                          &DownloadFile::RenameAndUniquify));
+                        ::testing::Values(RENAME_AND_ANNOTATE,
+                                          RENAME_AND_UNIQUIFY));
 
-const char* DownloadFileTest::kTestData1 =
+const char DownloadFileTest::kTestData1[] =
     "Let's write some data to the file!\n";
-const char* DownloadFileTest::kTestData2 = "Writing more data.\n";
-const char* DownloadFileTest::kTestData3 = "Final line.";
-const char* DownloadFileTest::kDataHash =
+const char DownloadFileTest::kTestData2[] = "Writing more data.\n";
+const char DownloadFileTest::kTestData3[] = "Final line.";
+const char DownloadFileTest::kDataHash[] =
     "CBF68BF10F8003DB86B31343AFAC8C7175BD03FB5FC905650F8C80AF087443A8";
+const char DownloadFileTest::kEmptyHash[] =
+    "E3B0C44298FC1C149AFBF4C8996FB92427AE41E4649B934CA495991B7852B855";
 
 const uint32_t DownloadFileTest::kDummyDownloadId = 23;
 const int DownloadFileTest::kDummyChildId = 3;
@@ -457,10 +488,7 @@ TEST_P(DownloadFileTestWithRename, RenameFileFinal) {
   EXPECT_FALSE(base::PathExists(path_2));
   EXPECT_TRUE(base::PathExists(path_3));
 
-  // Should not be able to get the hash until the file is closed.
-  std::string hash;
-  EXPECT_FALSE(download_file_->GetHash(&hash));
-  FinishStream(DOWNLOAD_INTERRUPT_REASON_NONE, true);
+  FinishStream(DOWNLOAD_INTERRUPT_REASON_NONE, true, kDataHash);
   loop_.RunUntilIdle();
 
   // Rename the file after downloading all the data and closing the file.
@@ -474,10 +502,6 @@ TEST_P(DownloadFileTestWithRename, RenameFileFinal) {
   EXPECT_FALSE(base::PathExists(path_3));
   EXPECT_TRUE(base::PathExists(path_4));
 
-  // Check the hash.
-  EXPECT_TRUE(download_file_->GetHash(&hash));
-  EXPECT_EQ(kDataHash, base::HexEncode(hash.data(), hash.size()));
-
   DestroyDownloadFile(0);
 }
 
@@ -505,7 +529,7 @@ TEST_F(DownloadFileTest, RenameOverwrites) {
   ASSERT_TRUE(base::ReadFileToString(new_path, &file_contents));
   EXPECT_NE(std::string(file_data), file_contents);
 
-  FinishStream(DOWNLOAD_INTERRUPT_REASON_NONE, true);
+  FinishStream(DOWNLOAD_INTERRUPT_REASON_NONE, true, kEmptyHash);
   loop_.RunUntilIdle();
   DestroyDownloadFile(0);
 }
@@ -530,7 +554,7 @@ TEST_F(DownloadFileTest, RenameUniquifies) {
   EXPECT_EQ(DOWNLOAD_INTERRUPT_REASON_NONE, RenameAndUniquify(path_1, NULL));
   EXPECT_TRUE(base::PathExists(path_1_suffixed));
 
-  FinishStream(DOWNLOAD_INTERRUPT_REASON_NONE, true);
+  FinishStream(DOWNLOAD_INTERRUPT_REASON_NONE, true, kEmptyHash);
   loop_.RunUntilIdle();
   DestroyDownloadFile(0);
 }
@@ -547,7 +571,7 @@ TEST_F(DownloadFileTest, RenameRecognizesSelfConflict) {
             RenameAndUniquify(initial_path, &new_path));
   EXPECT_TRUE(base::PathExists(initial_path));
 
-  FinishStream(DOWNLOAD_INTERRUPT_REASON_NONE, true);
+  FinishStream(DOWNLOAD_INTERRUPT_REASON_NONE, true, kEmptyHash);
   loop_.RunUntilIdle();
   DestroyDownloadFile(0);
   EXPECT_EQ(initial_path.value(), new_path.value());
@@ -582,7 +606,7 @@ TEST_P(DownloadFileTestWithRename, RenameError) {
     EXPECT_FALSE(base::PathExists(target_path_suffixed));
   }
 
-  FinishStream(DOWNLOAD_INTERRUPT_REASON_NONE, true);
+  FinishStream(DOWNLOAD_INTERRUPT_REASON_NONE, true, kEmptyHash);
   loop_.RunUntilIdle();
   DestroyDownloadFile(0);
 }
@@ -645,10 +669,11 @@ TEST_P(DownloadFileTestWithRename, RenameWithErrorRetry) {
 
     // The Rename() should fail here and enqueue a retry task without invoking
     // the completion callback.
-    ((*download_file_).*GetParam())(target_path,
-                                    base::Bind(&TestRenameCompletionCallback,
-                                               succeeding_run.QuitClosure(),
-                                               &did_run_callback));
+    InvokeRenameMethod(GetParam(),
+                       target_path,
+                       base::Bind(&TestRenameCompletionCallback,
+                                  succeeding_run.QuitClosure(),
+                                  &did_run_callback));
     EXPECT_FALSE(did_run_callback);
 
     base::RunLoop first_failing_run;
@@ -674,7 +699,7 @@ TEST_P(DownloadFileTestWithRename, RenameWithErrorRetry) {
   succeeding_run.Run();
   EXPECT_TRUE(did_run_callback);
 
-  FinishStream(DOWNLOAD_INTERRUPT_REASON_NONE, true);
+  FinishStream(DOWNLOAD_INTERRUPT_REASON_NONE, true, kEmptyHash);
   loop_.RunUntilIdle();
   DestroyDownloadFile(0);
 }
@@ -689,10 +714,8 @@ TEST_F(DownloadFileTest, StreamEmptySuccess) {
   // do anything.
   AppendDataToFile(NULL, 0);
 
-  // Finish the download this way and make sure we see it on the
-  // observer.
-  EXPECT_CALL(*(observer_.get()), DestinationCompleted(_));
-  FinishStream(DOWNLOAD_INTERRUPT_REASON_NONE, false);
+  // Finish the download this way and make sure we see it on the observer.
+  FinishStream(DOWNLOAD_INTERRUPT_REASON_NONE, true, kEmptyHash);
   loop_.RunUntilIdle();
 
   DestroyDownloadFile(0);
@@ -705,9 +728,10 @@ TEST_F(DownloadFileTest, StreamEmptyError) {
 
   // Finish the download in error and make sure we see it on the
   // observer.
-  EXPECT_CALL(*(observer_.get()),
-              DestinationError(
-                  DOWNLOAD_INTERRUPT_REASON_NETWORK_DISCONNECTED))
+  EXPECT_CALL(
+      *(observer_.get()),
+      MockDestinationError(
+          DOWNLOAD_INTERRUPT_REASON_NETWORK_DISCONNECTED, 0, kEmptyHash))
       .WillOnce(InvokeWithoutArgs(
           this, &DownloadFileTest::ConfirmUpdateDownloadInfo));
 
@@ -716,9 +740,10 @@ TEST_F(DownloadFileTest, StreamEmptyError) {
   // the last one may have the correct information even if the failure
   // doesn't produce an update, as the timer update may have triggered at the
   // same time.
-  EXPECT_CALL(*(observer_.get()), CurrentUpdateStatus(0, _, _));
+  EXPECT_CALL(*(observer_.get()), CurrentUpdateStatus(0, _));
 
-  FinishStream(DOWNLOAD_INTERRUPT_REASON_NETWORK_DISCONNECTED, false);
+  FinishStream(
+      DOWNLOAD_INTERRUPT_REASON_NETWORK_DISCONNECTED, false, kEmptyHash);
 
   loop_.RunUntilIdle();
 
@@ -734,7 +759,7 @@ TEST_F(DownloadFileTest, StreamNonEmptySuccess) {
   ::testing::Sequence s1;
   SetupDataAppend(chunks1, 2, s1);
   SetupFinishStream(DOWNLOAD_INTERRUPT_REASON_NONE, s1);
-  EXPECT_CALL(*(observer_.get()), DestinationCompleted(_));
+  EXPECT_CALL(*(observer_.get()), MockDestinationCompleted(_, _));
   sink_callback_.Run();
   VerifyStreamAndSize();
   loop_.RunUntilIdle();
@@ -752,8 +777,8 @@ TEST_F(DownloadFileTest, StreamNonEmptyError) {
   SetupFinishStream(DOWNLOAD_INTERRUPT_REASON_NETWORK_DISCONNECTED, s1);
 
   EXPECT_CALL(*(observer_.get()),
-              DestinationError(
-                  DOWNLOAD_INTERRUPT_REASON_NETWORK_DISCONNECTED))
+              MockDestinationError(
+                  DOWNLOAD_INTERRUPT_REASON_NETWORK_DISCONNECTED, _, _))
       .WillOnce(InvokeWithoutArgs(
           this, &DownloadFileTest::ConfirmUpdateDownloadInfo));
 
@@ -763,8 +788,7 @@ TEST_F(DownloadFileTest, StreamNonEmptyError) {
   // doesn't produce an update, as the timer update may have triggered at the
   // same time.
   EXPECT_CALL(*(observer_.get()),
-              CurrentUpdateStatus(strlen(kTestData1) + strlen(kTestData2),
-                                  _, _));
+              CurrentUpdateStatus(strlen(kTestData1) + strlen(kTestData2), _));
 
   sink_callback_.Run();
   loop_.RunUntilIdle();
@@ -772,26 +796,4 @@ TEST_F(DownloadFileTest, StreamNonEmptyError) {
   DestroyDownloadFile(0);
 }
 
-// Send some data, wait 3/4s of a second, run the message loop, and
-// confirm the values the observer received are correct.
-TEST_F(DownloadFileTest, ConfirmUpdate) {
-  CreateDownloadFile(0, true);
-
-  const char* chunks1[] = { kTestData1, kTestData2 };
-  AppendDataToFile(chunks1, 2);
-
-  // Run the message loops for 750ms and check for results.
-  loop_.task_runner()->PostDelayedTask(FROM_HERE,
-                                       base::MessageLoop::QuitWhenIdleClosure(),
-                                       base::TimeDelta::FromMilliseconds(750));
-  loop_.Run();
-
-  EXPECT_EQ(static_cast<int64_t>(strlen(kTestData1) + strlen(kTestData2)),
-            bytes_);
-  EXPECT_EQ(download_file_->GetHashState(), hash_state_);
-
-  FinishStream(DOWNLOAD_INTERRUPT_REASON_NONE, true);
-  DestroyDownloadFile(0);
-}
-
 }  // namespace content