Retry initializing download db on failure

also fixes an issue that DownloadDBImpl could be null for incognito downloads

BUG=842245

Change-Id: I1688b6ad15c400a27d6877b60f149c7eee181794
Reviewed-on: https://chromium-review.googlesource.com/1147604
Commit-Queue: Min Qin <qinmin@chromium.org>
Reviewed-by: Xing Liu <xingliu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#577992}

components/download/database/BUILD.gn

@@ -9,6 +9,7 @@ if (is_android) {
 
 source_set("database") {
   sources = [
+    "download_db.cc",
     "download_db.h",
     "download_db_conversions.cc",
     "download_db_conversions.h",

components/download/database/download_db.cc

+// Copyright 2018 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "components/download/database/download_db.h"
+
+#include "base/callback.h"
+#include "components/download/database/download_db_entry.h"
+
+namespace download {
+
+DownloadDB::DownloadDB() = default;
+
+DownloadDB::~DownloadDB() = default;
+
+void DownloadDB::Initialize(InitializeCallback callback) {
+  std::move(callback).Run(true);
+}
+
+void DownloadDB::AddOrReplace(const DownloadDBEntry& entry) {}
+
+void DownloadDB::AddOrReplaceEntries(
+    const std::vector<DownloadDBEntry>& entry) {}
+
+void DownloadDB::LoadEntries(LoadEntriesCallback callback) {
+  std::move(callback).Run(true,
+                          std::make_unique<std::vector<DownloadDBEntry>>());
+}
+
+void DownloadDB::Remove(const std::string& guid) {}
+
+}  // namespace download

components/download/database/download_db.h

@@ -23,27 +23,23 @@ class DownloadDB {
       std::unique_ptr<std::vector<DownloadDBEntry>> entries)>;
   using InitializeCallback = base::OnceCallback<void(bool success)>;
 
-  virtual ~DownloadDB() = default;
-
-  // Returns whether or not this object is initialized and can be interracted
-  // with.
-  virtual bool IsInitialized() = 0;
+  DownloadDB();
+  virtual ~DownloadDB();
 
   // Initializes this db asynchronously, callback will be run on completion.
-  virtual void Initialize(InitializeCallback callback) = 0;
+  virtual void Initialize(InitializeCallback callback);
 
   // Adds or updates |entry| in the storage.
-  virtual void AddOrReplace(const DownloadDBEntry& entry) = 0;
+  virtual void AddOrReplace(const DownloadDBEntry& entry);
 
   // Adds or updates multiple entries in the storage.
-  virtual void AddOrReplaceEntries(
-      const std::vector<DownloadDBEntry>& entry) = 0;
+  virtual void AddOrReplaceEntries(const std::vector<DownloadDBEntry>& entry);
 
   // Retrieves all entries with the given |download_namespace|.
-  virtual void LoadEntries(LoadEntriesCallback callback) = 0;
+  virtual void LoadEntries(LoadEntriesCallback callback);
 
   // Removes the Entry associated with |guid| from the storage.
-  virtual void Remove(const std::string& guid) = 0;
+  virtual void Remove(const std::string& guid);
 };
 
 }  // namespace download

components/download/database/download_db_impl.cc

@@ -18,6 +18,8 @@ namespace download {
 
 namespace {
 
+const int kMaxNumInitializeAttempts = 3;
+
 const char kDatabaseClientName[] = "DownloadDB";
 using ProtoKeyVector = std::vector<std::string>;
 using ProtoEntryVector = std::vector<download_pb::DownloadDBEntry>;
@@ -58,6 +60,7 @@ DownloadDBImpl::DownloadDBImpl(
       db_(std::move(db)),
       is_initialized_(false),
       download_namespace_(download_namespace),
+      num_initialize_attempts_(0),
       weak_factory_(this) {}
 
 DownloadDBImpl::~DownloadDBImpl() = default;
@@ -68,6 +71,7 @@ bool DownloadDBImpl::IsInitialized() {
 
 void DownloadDBImpl::Initialize(InitializeCallback callback) {
   DCHECK(!IsInitialized());
+
   // These options reduce memory consumption.
   leveldb_env::Options options = leveldb_proto::CreateSimpleOptions();
   options.reuse_logs = false;
@@ -144,6 +148,10 @@ void DownloadDBImpl::OnAllEntriesLoaded(
 
 void DownloadDBImpl::OnDatabaseInitialized(InitializeCallback callback,
                                            bool success) {
+  if (!success) {
+    DestroyAndReinitialize(std::move(callback));
+    return;
+  }
   is_initialized_ = success;
   std::move(callback).Run(success);
 }
@@ -155,7 +163,11 @@ void DownloadDBImpl::OnDatabaseDestroyed(InitializeCallback callback,
     return;
   }
 
-  Initialize(std::move(callback));
+  num_initialize_attempts_++;
+  if (num_initialize_attempts_ >= kMaxNumInitializeAttempts)
+    std::move(callback).Run(false);
+  else
+    Initialize(std::move(callback));
 }
 
 void DownloadDBImpl::OnUpdateDone(bool success) {

components/download/database/download_db_impl.h

@@ -33,7 +33,6 @@ class DownloadDBImpl : public DownloadDB {
   ~DownloadDBImpl() override;
 
   // DownloadDB implementation.
-  bool IsInitialized() override;
   void Initialize(InitializeCallback callback) override;
   void AddOrReplace(const DownloadDBEntry& entry) override;
   void AddOrReplaceEntries(
@@ -44,6 +43,8 @@ class DownloadDBImpl : public DownloadDB {
  private:
   friend class DownloadDBTest;
 
+  bool IsInitialized();
+
   void DestroyAndReinitialize(InitializeCallback callback);
 
   // Returns the key of the db entry.
@@ -80,6 +81,9 @@ class DownloadDBImpl : public DownloadDB {
   // Namespace of this db.
   DownloadNamespace download_namespace_;
 
+  // Number of initialize attempts.
+  int num_initialize_attempts_;
+
   base::WeakPtrFactory<DownloadDBImpl> weak_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(DownloadDBImpl);

components/download/database/download_db_impl_unittest.cc

@@ -62,6 +62,8 @@ class DownloadDBTest : public testing::Test {
     loaded_entries->swap(*entries);
   }
 
+  bool IsInitialized() { return download_db_->IsInitialized(); }
+
   void PrepopulateSampleEntries() {
     DownloadDBEntry first = CreateDownloadDBEntry();
     DownloadDBEntry second = CreateDownloadDBEntry();
@@ -93,25 +95,25 @@ class DownloadDBTest : public testing::Test {
 
 TEST_F(DownloadDBTest, InitializeSucceeded) {
   CreateDatabase();
-  ASSERT_FALSE(download_db_->IsInitialized());
+  ASSERT_FALSE(IsInitialized());
 
   download_db_->Initialize(
       base::BindOnce(&DownloadDBTest::InitCallback, base::Unretained(this)));
   db_->InitCallback(true);
 
-  ASSERT_TRUE(download_db_->IsInitialized());
+  ASSERT_TRUE(IsInitialized());
   ASSERT_TRUE(init_success_);
 }
 
 TEST_F(DownloadDBTest, InitializeFailed) {
   CreateDatabase();
-  ASSERT_FALSE(download_db_->IsInitialized());
+  ASSERT_FALSE(IsInitialized());
 
   download_db_->Initialize(
       base::BindOnce(&DownloadDBTest::InitCallback, base::Unretained(this)));
   db_->InitCallback(false);
 
-  ASSERT_FALSE(download_db_->IsInitialized());
+  ASSERT_FALSE(IsInitialized());
   ASSERT_FALSE(init_success_);
 }
 
@@ -121,7 +123,7 @@ TEST_F(DownloadDBTest, LoadEntries) {
   download_db_->Initialize(
       base::BindOnce(&DownloadDBTest::InitCallback, base::Unretained(this)));
   db_->InitCallback(true);
-  ASSERT_TRUE(download_db_->IsInitialized());
+  ASSERT_TRUE(IsInitialized());
 
   std::vector<DownloadDBEntry> loaded_entries;
   download_db_->LoadEntries(base::BindOnce(
@@ -141,7 +143,7 @@ TEST_F(DownloadDBTest, AddEntry) {
   download_db_->Initialize(
       base::BindOnce(&DownloadDBTest::InitCallback, base::Unretained(this)));
   db_->InitCallback(true);
-  ASSERT_TRUE(download_db_->IsInitialized());
+  ASSERT_TRUE(IsInitialized());
 
   DownloadDBEntry entry = CreateDownloadDBEntry();
   download_db_->AddOrReplace(entry);
@@ -172,7 +174,7 @@ TEST_F(DownloadDBTest, ReplaceEntry) {
   download_db_->Initialize(
       base::BindOnce(&DownloadDBTest::InitCallback, base::Unretained(this)));
   db_->InitCallback(true);
-  ASSERT_TRUE(download_db_->IsInitialized());
+  ASSERT_TRUE(IsInitialized());
 
   InProgressInfo in_progress_info;
   in_progress_info.current_path =
@@ -209,7 +211,7 @@ TEST_F(DownloadDBTest, Remove) {
   download_db_->Initialize(
       base::BindOnce(&DownloadDBTest::InitCallback, base::Unretained(this)));
   db_->InitCallback(true);
-  ASSERT_TRUE(download_db_->IsInitialized());
+  ASSERT_TRUE(IsInitialized());
 
   download_db_->Remove(first.GetGuid());
   db_->UpdateCallback(true);
@@ -230,7 +232,7 @@ TEST_F(DownloadDBTest, DestroyAndReinitialize) {
   download_db_->Initialize(
       base::BindOnce(&DownloadDBTest::InitCallback, base::Unretained(this)));
   db_->InitCallback(true);
-  ASSERT_TRUE(download_db_->IsInitialized());
+  ASSERT_TRUE(IsInitialized());
 
   std::vector<DownloadDBEntry> loaded_entries;
   download_db_->LoadEntries(base::BindOnce(

components/download/internal/common/download_db_cache.cc

@@ -120,7 +120,9 @@ void CleanUpInProgressEntry(DownloadDBEntry& entry) {
 DownloadDBCache::DownloadDBCache(std::unique_ptr<DownloadDB> download_db)
     : initialized_(false),
       download_db_(std::move(download_db)),
-      weak_factory_(this) {}
+      weak_factory_(this) {
+  DCHECK(download_db_);
+}
 
 DownloadDBCache::~DownloadDBCache() = default;
 
@@ -175,7 +177,7 @@ void DownloadDBCache::AddOrReplaceEntry(const DownloadDBEntry& entry) {
 void DownloadDBCache::RemoveEntry(const std::string& guid) {
   entries_.erase(guid);
   updated_guids_.erase(guid);
-  if (download_db_)
+  if (initialized_)
     download_db_->Remove(guid);
 }
 
@@ -188,15 +190,13 @@ void DownloadDBCache::UpdateDownloadDB() {
     DCHECK(entry);
     entries.emplace_back(entry.value());
   }
-  download_db_->AddOrReplaceEntries(entries);
+  if (initialized_)
+    download_db_->AddOrReplaceEntries(entries);
 }
 
 void DownloadDBCache::OnDownloadUpdated(DownloadItem* download) {
   // TODO(crbug.com/778425): Properly handle fail/resume/retry for downloads
   // that are in the INTERRUPTED state for a long time.
-  if (!download_db_)
-    return;
-
   base::Optional<DownloadDBEntry> current = RetrieveEntry(download->GetGuid());
   bool fetch_error_body = GetFetchErrorBody(current);
   DownloadUrlParameters::RequestHeadersType request_header_type =
@@ -217,25 +217,22 @@ void DownloadDBCache::OnDownloadDBInitialized(InitializeCallback callback,
     download_db_->LoadEntries(
         base::BindOnce(&DownloadDBCache::OnDownloadDBEntriesLoaded,
                        weak_factory_.GetWeakPtr(), std::move(callback)));
+  } else {
+    OnDownloadDBEntriesLoaded(std::move(callback), false,
+                              std::make_unique<std::vector<DownloadDBEntry>>());
   }
-  // TODO(qinmin): Recreate the database if |success| is false.
-  // http://crbug.com/847661.
 }
 
 void DownloadDBCache::OnDownloadDBEntriesLoaded(
     InitializeCallback callback,
     bool success,
     std::unique_ptr<std::vector<DownloadDBEntry>> entries) {
-  if (success) {
-    initialized_ = true;
-    for (auto& entry : *entries) {
-      CleanUpInProgressEntry(entry);
-      entries_[entry.download_info->guid] = entry;
-    }
-    std::move(callback).Run(std::move(entries));
+  initialized_ = success;
+  for (auto& entry : *entries) {
+    CleanUpInProgressEntry(entry);
+    entries_[entry.download_info->guid] = entry;
   }
-  // TODO(qinmin): Recreate the database if |success| is false.
-  // http://crbug.com/847661.
+  std::move(callback).Run(std::move(entries));
 }
 
 void DownloadDBCache::SetTimerTaskRunnerForTesting(

components/download/internal/common/in_progress_download_manager.cc

@@ -279,9 +279,12 @@ void InProgressDownloadManager::Initialize(
           switches::kEnableDownloadDB)) {
     // TODO(qinmin): migrate all the data from InProgressCache into
     // |download_db_|.
-    download_db_cache_ =
-        std::make_unique<DownloadDBCache>(std::make_unique<DownloadDBImpl>(
-            DownloadNamespace::NAMESPACE_BROWSER_DOWNLOAD, metadata_cache_dir));
+    download_db_cache_ = std::make_unique<DownloadDBCache>(
+        metadata_cache_dir.empty()
+            ? std::make_unique<DownloadDB>()
+            : std::make_unique<DownloadDBImpl>(
+                  DownloadNamespace::NAMESPACE_BROWSER_DOWNLOAD,
+                  metadata_cache_dir));
     download_db_cache_->Initialize(base::BindOnce(
         &InProgressDownloadManager::OnInitialized, weak_factory_.GetWeakPtr()));
   } else {