Don't cache any DownloadDBEntry when loading from DownloadDB

Currently we cache all the DownloadDBEntry when loading in-progress downloads.
However, this is not necessary and it wastes a lot of memory.
This CL no longer caches those entries, and will only cache them later
when OnDownloadUpdated() is called on an in-progress download.

BUG=893651

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

components/download/internal/common/download_db_cache.cc

@@ -78,6 +78,16 @@ void OnDownloadDBUpdated(bool success) {
     LOG(ERROR) << "Unable to update DB entries";
 }
 
+// Check if a DownloadDBEntry represents an in progress download.
+bool IsInProgressEntry(base::Optional<DownloadDBEntry> entry) {
+  if (!entry || !entry->download_info ||
+      !entry->download_info->in_progress_info)
+    return false;
+
+  return entry->download_info->in_progress_info->state ==
+         DownloadItem::DownloadState::IN_PROGRESS;
+}
+
 }  // namespace
 
 DownloadDBCache::DownloadDBCache(std::unique_ptr<DownloadDB> download_db)
@@ -90,26 +100,16 @@ DownloadDBCache::DownloadDBCache(std::unique_ptr<DownloadDB> download_db)
 DownloadDBCache::~DownloadDBCache() = default;
 
 void DownloadDBCache::Initialize(InitializeCallback callback) {
-  // TODO(qinmin): migrate all the data from InProgressCache into
-  // |download_db_|.
-  if (!initialized_) {
-    RecordInProgressDBCount(kInitializationCount);
-    download_db_->Initialize(
-        base::BindOnce(&DownloadDBCache::OnDownloadDBInitialized,
-                       weak_factory_.GetWeakPtr(), std::move(callback)));
-    return;
-  }
-
-  auto db_entries = std::make_unique<std::vector<DownloadDBEntry>>();
-  for (auto it = entries_.begin(); it != entries_.end(); ++it)
-    db_entries->emplace_back(it->second);
-  std::move(callback).Run(true, std::move(db_entries));
+  DCHECK(!initialized_);
+  download_db_->Initialize(
+      base::BindOnce(&DownloadDBCache::OnDownloadDBInitialized,
+                     weak_factory_.GetWeakPtr(), std::move(callback)));
 }
 
 base::Optional<DownloadDBEntry> DownloadDBCache::RetrieveEntry(
     const std::string& guid) {
-  auto iter = entries_.find(guid);
-  if (iter != entries_.end())
+  auto iter = cached_entries_.find(guid);
+  if (iter != cached_entries_.end())
     return iter->second;
   return base::nullopt;
 }
@@ -129,7 +129,7 @@ void DownloadDBCache::AddOrReplaceEntry(const DownloadDBEntry& entry) {
                         this, &DownloadDBCache::UpdateDownloadDB);
   }
 
-  entries_[guid] = entry;
+  cached_entries_[guid] = entry;
   updated_guids_.emplace(guid);
   if (result == ShouldUpdateDownloadDBResult::UPDATE_IMMEDIATELY) {
     UpdateDownloadDB();
@@ -138,7 +138,7 @@ void DownloadDBCache::AddOrReplaceEntry(const DownloadDBEntry& entry) {
 }
 
 void DownloadDBCache::RemoveEntry(const std::string& guid) {
-  entries_.erase(guid);
+  cached_entries_.erase(guid);
   updated_guids_.erase(guid);
   if (initialized_)
     download_db_->Remove(guid);
@@ -149,11 +149,16 @@ void DownloadDBCache::UpdateDownloadDB() {
     return;
 
   std::vector<DownloadDBEntry> entries;
-  for (auto guid : updated_guids_) {
+  for (const auto& guid : updated_guids_) {
     base::Optional<DownloadDBEntry> entry = RetrieveEntry(guid);
     DCHECK(entry);
     entries.emplace_back(entry.value());
+    // If the entry is no longer in-progress, remove it from the cache as it may
+    // not update again soon.
+    if (!IsInProgressEntry(entry))
+      cached_entries_.erase(guid);
   }
+  updated_guids_.clear();
   if (initialized_) {
     download_db_->AddOrReplaceEntries(entries,
                                       base::BindOnce(&OnDownloadDBUpdated));
@@ -207,12 +212,10 @@ void DownloadDBCache::OnDownloadDBEntriesLoaded(
   for (auto& entry : *entries) {
     // If the entry is from the metadata cache migration, just remove it from
     // DB as the data is not being cleaned up properly.
-    if (entry.download_info->id < 0) {
+    if (entry.download_info->id < 0)
       RemoveEntry(entry.download_info->guid);
-    } else {
+    else
       CleanUpInProgressEntry(&entry);
-      entries_[entry.download_info->guid] = entry;
-    }
   }
   std::move(callback).Run(success, std::move(entries));
 }

components/download/internal/common/download_db_cache.h

@@ -70,8 +70,9 @@ class COMPONENTS_DOWNLOAD_EXPORT DownloadDBCache
   std::unique_ptr<DownloadDB> download_db_;
 
   using DownloadDBEntryMap = std::map<std::string, DownloadDBEntry>;
-  // All in progress downloads stored in |download_db_|.
-  DownloadDBEntryMap entries_;
+  // Entries that are currently being updated will be cached to reduce disk
+  // writing.
+  DownloadDBEntryMap cached_entries_;
 
   // GUIDs of updated entries.
   std::set<std::string> updated_guids_;

components/download/internal/common/download_db_cache_unittest.cc

@@ -33,6 +33,7 @@ namespace {
 DownloadDBEntry CreateDownloadDBEntry() {
   DownloadDBEntry entry;
   DownloadInfo download_info;
+  download_info.in_progress_info = InProgressInfo();
   download_info.guid = base::GenerateGUID();
   static int id = 0;
   download_info.id = ++id;
@@ -46,6 +47,14 @@ std::string GetKey(const std::string& guid) {
          "," + guid;
 }
 
+// Clean up an in-progress entry that's loaded from the download DB, since
+// newly loaded entries should be in an interrupted state.
+void CleanUpInProgressEntry(DownloadDBEntry* entry) {
+  entry->download_info->in_progress_info->state = DownloadItem::INTERRUPTED;
+  entry->download_info->in_progress_info->interrupt_reason =
+      DOWNLOAD_INTERRUPT_REASON_CRASH;
+}
+
 }  // namespace
 
 class DownloadDBCacheTest : public testing::Test {
@@ -114,11 +123,13 @@ TEST_F(DownloadDBCacheTest, InitializeAndRetrieve) {
   db_->LoadCallback(true);
   ASSERT_EQ(loaded_entries.size(), 2u);
 
-  for (auto db_entry : loaded_entries) {
-    ASSERT_EQ(db_entry, db_cache_->RetrieveEntry(db_entry.GetGuid()));
-    ASSERT_EQ(db_entry,
-              DownloadDBConversions::DownloadDBEntryFromProto(
-                  db_entries_.find(GetKey(db_entry.GetGuid()))->second));
+  for (auto& db_entry : loaded_entries) {
+    DownloadDBEntry entry = DownloadDBConversions::DownloadDBEntryFromProto(
+        db_entries_.find(GetKey(db_entry.GetGuid()))->second);
+    // Newly loaded entries should be in an interrupted state.
+    CleanUpInProgressEntry(&entry);
+    ASSERT_EQ(db_entry, entry);
+    EXPECT_FALSE(db_cache_->RetrieveEntry(db_entry.GetGuid()));
   }
 }
 
@@ -159,10 +170,20 @@ TEST_F(DownloadDBCacheTest, ModifyExistingEntry) {
   db_->LoadCallback(true);
   ASSERT_EQ(loaded_entries.size(), 2u);
 
-  loaded_entries[0].download_info->id = 100;
-  loaded_entries[1].download_info->id = 100;
+  // Let the DBCache to cache the entries first.
+  loaded_entries[0].download_info->in_progress_info->state =
+      DownloadItem::IN_PROGRESS;
+  loaded_entries[1].download_info->id = 101;
   db_cache_->AddOrReplaceEntry(loaded_entries[0]);
+  db_->UpdateCallback(true);
   db_cache_->AddOrReplaceEntry(loaded_entries[1]);
+  db_->UpdateCallback(true);
+  // Only the first entry is cached, as the second entry is still interrupted.
+  EXPECT_TRUE(db_cache_->RetrieveEntry(loaded_entries[0].GetGuid()));
+  EXPECT_FALSE(db_cache_->RetrieveEntry(loaded_entries[1].GetGuid()));
+
+  loaded_entries[0].download_info->id = 100;
+  db_cache_->AddOrReplaceEntry(loaded_entries[0]);
 
   ASSERT_EQ(task_runner_->GetPendingTaskCount(), 1u);
   ASSERT_GT(task_runner_->NextPendingTaskDelay(), base::TimeDelta());
@@ -177,7 +198,7 @@ TEST_F(DownloadDBCacheTest, ModifyExistingEntry) {
   db_->LoadCallback(true);
   ASSERT_EQ(loaded_entries.size(), 2u);
   ASSERT_EQ(loaded_entries[0].download_info->id, 100);
-  ASSERT_EQ(loaded_entries[1].download_info->id, 100);
+  ASSERT_EQ(loaded_entries[1].download_info->id, 101);
 }
 
 // Test that modifying current path will immediately update the DB.
@@ -232,8 +253,6 @@ TEST_F(DownloadDBCacheTest, RemoveEntry) {
   std::string guid2 = loaded_entries[1].GetGuid();
   db_cache_->RemoveEntry(loaded_entries[0].GetGuid());
   db_->UpdateCallback(true);
-  ASSERT_FALSE(db_cache_->RetrieveEntry(guid));
-  ASSERT_TRUE(db_cache_->RetrieveEntry(guid2));
 
   loaded_entries.clear();
   DownloadDB* download_db = GetDownloadDB();
@@ -256,7 +275,14 @@ TEST_F(DownloadDBCacheTest, RemoveWhileModifyExistingEntry) {
   db_->InitCallback(true);
   db_->LoadCallback(true);
   ASSERT_EQ(loaded_entries.size(), 2u);
+  // Let the DBCache to cache the entry first.
+  loaded_entries[0].download_info->in_progress_info->state =
+      DownloadItem::IN_PROGRESS;
+  db_cache_->AddOrReplaceEntry(loaded_entries[0]);
+  ASSERT_EQ(task_runner_->GetPendingTaskCount(), 0u);
+  db_->UpdateCallback(true);
 
+  // Update the cached entry. A task will be posted to update the DB.
   loaded_entries[0].download_info->id = 100;
   db_cache_->AddOrReplaceEntry(loaded_entries[0]);
 
@@ -273,6 +299,7 @@ TEST_F(DownloadDBCacheTest, RemoveWhileModifyExistingEntry) {
                                           &loaded_entries));
   db_->LoadCallback(true);
   ASSERT_EQ(loaded_entries.size(), 1u);
+  CleanUpInProgressEntry(&loaded_entries[0]);
   ASSERT_EQ(remaining, loaded_entries[0]);
 }
 

components/download/internal/common/in_progress_download_manager.cc

@@ -262,9 +262,9 @@ base::Optional<DownloadEntry> InProgressDownloadManager::GetInProgressEntry(
     DownloadItemImpl* download) {
   if (!download)
     return base::Optional<DownloadEntry>();
+  if (base::ContainsKey(download_entries_, download->GetGuid()))
+    return download_entries_[download->GetGuid()];
 
-  return CreateDownloadEntryFromDownloadDBEntry(
-      download_db_cache_->RetrieveEntry(download->GetGuid()));
   return base::Optional<DownloadEntry>();
 }
 
@@ -350,14 +350,9 @@ void InProgressDownloadManager::StartDownloadWithItem(
   if (info->is_new_download && !should_persist_new_download)
     non_persistent_download_guids_.insert(download->GetGuid());
   // If the download is not persisted, don't notify |download_db_cache_|.
-  if (non_persistent_download_guids_.find(download->GetGuid()) ==
-      non_persistent_download_guids_.end()) {
-    base::Optional<DownloadDBEntry> entry_opt =
-        download_db_cache_->RetrieveEntry(download->GetGuid());
-    if (!entry_opt.has_value()) {
-      download_db_cache_->AddOrReplaceEntry(
-          CreateDownloadDBEntryFromItem(*download));
-    }
+  if (!base::ContainsKey(non_persistent_download_guids_, download->GetGuid())) {
+    download_db_cache_->AddOrReplaceEntry(
+        CreateDownloadDBEntryFromItem(*download));
     download->RemoveObserver(download_db_cache_.get());
     download->AddObserver(download_db_cache_.get());
   }
@@ -386,9 +381,12 @@ void InProgressDownloadManager::StartDownloadWithItem(
 void InProgressDownloadManager::OnInitialized(
     bool success,
     std::unique_ptr<std::vector<DownloadDBEntry>> entries) {
-  // Destroy the in-progress cache as it is no longer needed on success.
-  if (base::FeatureList::IsEnabled(features::kDownloadDBForNewDownloads)) {
-    for (const auto& entry : *entries) {
+  for (const auto& entry : *entries) {
+    base::Optional<DownloadEntry> download_entry =
+        CreateDownloadEntryFromDownloadDBEntry(entry);
+    if (download_entry)
+      download_entries_[download_entry->guid] = download_entry.value();
+    if (base::FeatureList::IsEnabled(features::kDownloadDBForNewDownloads)) {
       auto item = CreateDownloadItemImpl(this, entry);
       if (!item)
         continue;
@@ -396,6 +394,8 @@ void InProgressDownloadManager::OnInitialized(
       in_progress_downloads_.emplace_back(std::move(item));
     }
   }
+  if (base::FeatureList::IsEnabled(features::kDownloadDBForNewDownloads))
+    OnAllInprogressDownloadsLoaded();
   is_initialized_ = true;
   for (auto& callback : on_initialized_callbacks_)
     std::move(*callback).Run();
@@ -427,4 +427,8 @@ InProgressDownloadManager::TakeInProgressDownloads() {
   return std::move(in_progress_downloads_);
 }
 
+void InProgressDownloadManager::OnAllInprogressDownloadsLoaded() {
+  download_entries_.clear();
+}
+
 }  // namespace download

components/download/public/common/in_progress_download_manager.h

@@ -138,6 +138,11 @@ class COMPONENTS_DOWNLOAD_EXPORT InProgressDownloadManager
   virtual std::vector<std::unique_ptr<download::DownloadItemImpl>>
   TakeInProgressDownloads();
 
+  // Called when all the DownloadItem is loaded.
+  // TODO(qinmin): remove this once features::kDownloadDBForNewDownloads is
+  // enabled by default.
+  void OnAllInprogressDownloadsLoaded();
+
   void set_file_factory(std::unique_ptr<DownloadFileFactory> file_factory) {
     file_factory_ = std::move(file_factory);
   }
@@ -196,6 +201,13 @@ class COMPONENTS_DOWNLOAD_EXPORT InProgressDownloadManager
   // Cache for DownloadDB.
   std::unique_ptr<DownloadDBCache> download_db_cache_;
 
+  using DownloadEntryMap = std::map<std::string, DownloadEntry>;
+  // DownloadEntries to provide persistent information when creating download
+  // item.
+  // TODO(qinmin): remove this once features::kDownloadDBForNewDownloads is
+  // enabled by default.
+  DownloadEntryMap download_entries_;
+
   // listens to information about in-progress download items.
   std::unique_ptr<DownloadItem::Observer> in_progress_download_observer_;
 

content/browser/download/download_manager_impl.cc

@@ -1064,6 +1064,7 @@ void DownloadManagerImpl::PostInitialization(
     }
     in_progress_downloads_.clear();
 
+    in_progress_manager_->OnAllInprogressDownloadsLoaded();
     for (auto& observer : observers_)
       observer.OnManagerInitialized();
 }