Import in-progress downloads when history DB is loaded

The logic in loading the download history is quite complicated today.
When DownloadManagerImpl is created, it first loads the download from
in-progress manager. And then it starts to load the downloads from
History DB. That's causing several issues:
1. Due to the mismatch between history DB and in-progress DB, DownloadManagerImpl
need to resolve the conflicts when history DB is loaded. Normally it just
takes the information from in-progress DB. However, due to the latency during
database commit, history DB might contain fresher information than the
in-progress DB if the download is in a terminating state (Cancelled/Completed).
And that's causing crbug/898859 as an extra OnDownloadCreated() call is called
for those terminating downloads.

2. DownloadHistory used to call CreateDownloadItem() and expect a reentrant
OnDownloadCreated() call when loading history downloads. However, this may
no longer happen as OnDownloadCreated() may have already been called when
loading the in-progress DB. So DownloadHistory have to handle the case that
there is no reentrant call, which makes the code hard to understand.

3. DownloadHistory might get a OnDownloadCreated() call even before history
is loaded. And some of the downloads might have already existed in the
history DB. This will cause a failed sql insert LOG in the console. Although
the behavior is expected, it is not quite clean.

This CL fixes all above issue by loading the in-progress DB records and the
history DB records at the same time. Once CreateDownloadItem() is called,
DownloadManagerImpl will find the corresponding in-progress download and return
it to DownloadHistory, and calling OnDownloadCreated() at the same time.

BUG=898859

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

chrome/browser/android/download/download_manager_service.cc

@@ -229,6 +229,9 @@ DownloadManagerService::RetriveInProgressDownloadManager(
     content::BrowserContext* context) {
   if (in_progress_manager_) {
     DCHECK(!context->IsOffTheRecord());
+    // Set |is_pending_downloads_loaded_| to false so that we need to wait for
+    // download history to initialize before performing new download actions.
+    is_pending_downloads_loaded_ = false;
     return in_progress_manager_.release();
   }
   return nullptr;
@@ -562,6 +565,10 @@ void DownloadManagerService::CreateInProgressDownloadManager() {
 }
 
 void DownloadManagerService::OnPendingDownloadsLoaded() {
+  // If |in_progress_manager_| is null, wait for DownloadHistory to initialize
+  // before performing any pending actions.
+  if (!in_progress_manager_ && !is_history_query_complete_)
+    return;
   is_pending_downloads_loaded_ = true;
   for (auto iter = pending_actions_.begin(); iter != pending_actions_.end();
        ++iter) {

chrome/browser/download/download_history.cc

@@ -319,8 +319,7 @@ void DownloadHistory::LoadHistoryDownloads(std::unique_ptr<InfoVector> infos) {
       ScheduleRemoveDownload(it->id);
       continue;
     }
-    if (item->GetId() == loading_id_)
-      OnDownloadRestoredFromHistory(item);
+    DCHECK_EQ(download::DownloadItem::kInvalidId, loading_id_);
 
     // The download might have been completed or cancelled without informing
     // history DB. If this is the case, populate the new state back to history

chrome/browser/download/download_history_unittest.cc

@@ -209,12 +209,9 @@ class DownloadHistoryTest : public testing::Test {
     return manager_observer_;
   }
 
-  // Creates the DownloadHistory. If |call_on_download_created| is false,
-  // DownloadHistory::OnDownloadCreated() will not be called by |manager_|.
-  // If |return_null_item| is true, |manager_| will return nullptr on
-  // CreateDownloadItem() call,
+  // Creates the DownloadHistory. If |return_null_item| is true, |manager_|
+  // will return nullptr on CreateDownloadItem() call,
   void CreateDownloadHistory(std::unique_ptr<InfoVector> infos,
-                             bool call_on_download_created = true,
                              bool return_null_item = false) {
     DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
     CHECK(infos.get());
@@ -235,28 +232,20 @@ class DownloadHistoryTest : public testing::Test {
           history::ToContentDownloadInterruptReason(row.interrupt_reason),
           row.opened, row.last_access_time, row.transient,
           history::ToContentReceivedSlices(row.download_slice_info));
-      if (call_on_download_created) {
+      if (return_null_item) {
+        EXPECT_CALL(manager(), MockCreateDownloadItem(adapter))
+            .WillOnce(Return(nullptr));
+      } else {
         EXPECT_CALL(manager(), MockCreateDownloadItem(adapter))
             .WillOnce(DoAll(
                 InvokeWithoutArgs(
                     this, &DownloadHistoryTest::CallOnDownloadCreatedInOrder),
                 Return(&item(index))));
-      } else {
-        download::DownloadItem* download =
-            return_null_item ? nullptr : &item(index);
-        EXPECT_CALL(manager(), MockCreateDownloadItem(adapter))
-            .WillOnce(Return(download));
       }
     }
     history_ = new FakeHistoryAdapter();
     history_->ExpectWillQueryDownloads(std::move(infos));
-    if (call_on_download_created) {
-      EXPECT_CALL(manager(), GetAllDownloads(_)).WillRepeatedly(Return());
-    } else {
-      EXPECT_CALL(manager(), GetAllDownloads(_))
-          .WillRepeatedly(
-              WithArg<0>(Invoke(this, &DownloadHistoryTest::AddAllDownloads)));
-    }
+    EXPECT_CALL(manager(), GetAllDownloads(_)).WillRepeatedly(Return());
     download_history_.reset(new DownloadHistory(
         &manager(),
         std::unique_ptr<DownloadHistory::HistoryAdapter>(history_)));
@@ -892,7 +881,7 @@ TEST_F(DownloadHistoryTest, CreateHistoryItemInDownloadDB) {
   EXPECT_CALL(item(0), GetReceivedBytes()).WillRepeatedly(Return(50));
   std::unique_ptr<InfoVector> infos(new InfoVector());
   infos->push_back(info);
-  CreateDownloadHistory(std::move(infos), false);
+  CreateDownloadHistory(std::move(infos));
   EXPECT_TRUE(DownloadHistory::IsPersisted(&item(0)));
 
   // Modify the item, it should not trigger any updates.
@@ -924,7 +913,7 @@ TEST_F(DownloadHistoryTest, RemoveClearedItemFromHistory) {
 
   std::unique_ptr<InfoVector> infos(new InfoVector());
   infos->push_back(info);
-  CreateDownloadHistory(std::move(infos), false, true);
+  CreateDownloadHistory(std::move(infos), true);
 
   // The download should be removed from history afterwards.
   IdSet ids;

content/browser/download/download_manager_impl.cc

@@ -548,15 +548,11 @@ void DownloadManagerImpl::OnInProgressDownloadManagerInitialized() {
       in_progress_downloads = in_progress_manager_->TakeInProgressDownloads();
   uint32_t max_id = download::DownloadItem::kInvalidId;
   for (auto& download : in_progress_downloads) {
-    DCHECK(!base::ContainsKey(downloads_by_guid_, download->GetGuid()));
-    // If this id is not unique, drop this download, this may happen due to a
-    // previous history DB failure. See http://crbug.com/898859.
-    // TODO(qinmin): remove the downloaded files too if wasn't completed.
-    if (base::ContainsKey(downloads_, download->GetId())) {
+    uint32_t id = download->GetId();
+    if (base::ContainsKey(in_progress_downloads_, id)) {
       in_progress_manager_->RemoveInProgressDownload(download->GetGuid());
       continue;
     }
-    uint32_t id = download->GetId();
     if (id > max_id)
       max_id = id;
 #if defined(OS_ANDROID)
@@ -568,14 +564,7 @@ void DownloadManagerImpl::OnInProgressDownloadManagerInitialized() {
       continue;
     }
 #endif  // defined(OS_ANDROID)
-    DownloadItemUtils::AttachInfo(download.get(), GetBrowserContext(), nullptr);
-    download::DownloadItemImpl* item = download.get();
-    item->SetDelegate(this);
-    downloads_by_guid_[download->GetGuid()] = item;
-    downloads_[id] = std::move(download);
-    for (auto& observer : observers_)
-      observer.OnDownloadCreated(this, item);
-    DVLOG(20) << __func__ << "() download = " << item->DebugString(true);
+    in_progress_downloads_[id] = std::move(download);
   }
   PostInitialization(DOWNLOAD_INITIALIZATION_DEPENDENCY_IN_PROGRESS_CACHE);
   SetNextId(max_id + 1);
@@ -724,11 +713,7 @@ void DownloadManagerImpl::CreateSavePackageDownloadItemWithId(
   DownloadItemUtils::AttachInfo(download_item, GetBrowserContext(),
                                 WebContentsImpl::FromRenderFrameHostID(
                                     render_process_id, render_frame_id));
-  downloads_[download_item->GetId()] = base::WrapUnique(download_item);
-  DCHECK(!base::ContainsKey(downloads_by_guid_, download_item->GetGuid()));
-  downloads_by_guid_[download_item->GetGuid()] = download_item;
-  for (auto& observer : observers_)
-    observer.OnDownloadCreated(this, download_item);
+  OnDownloadCreated(base::WrapUnique(download_item));
   if (!item_created.is_null())
     item_created.Run(download_item);
 }
@@ -964,6 +949,9 @@ download::DownloadItem* DownloadManagerImpl::CreateDownloadItem(
     base::Time last_access_time,
     bool transient,
     const std::vector<download::DownloadItem::ReceivedSlice>& received_slices) {
+  // Retrive the in-progress download if it exists. Notice that this also
+  // removes it from |in_progress_downloads_|.
+  auto download = RetrieveInProgressDownload(id);
 #if defined(OS_ANDROID)
   // On Android, there is no way to interact with cancelled or non-resumable
   // download. Simply returning null and don't store them in this class to
@@ -977,29 +965,44 @@ download::DownloadItem* DownloadManagerImpl::CreateDownloadItem(
     return nullptr;
   }
 #endif
-  if (base::ContainsKey(downloads_, id)) {
+  if (download) {
     // If a completed or cancelled download item is already in the history db,
     // remove it from the in-progress db.
     if (state == download::DownloadItem::COMPLETE ||
         state == download::DownloadItem::CANCELLED) {
       in_progress_manager_->RemoveInProgressDownload(guid);
-    } else {
-      return downloads_[id].get();
+      // Destroy |download| as we can create from the history instead.
+      download.reset();
     }
   }
-  download::DownloadItemImpl* item = item_factory_->CreatePersistedItem(
-      this, guid, id, current_path, target_path, url_chain, referrer_url,
-      site_url, tab_url, tab_refererr_url, mime_type, original_mime_type,
-      start_time, end_time, etag, last_modified, received_bytes, total_bytes,
-      hash, state, danger_type, interrupt_reason, opened, last_access_time,
-      transient, received_slices);
-  DownloadItemUtils::AttachInfo(item, GetBrowserContext(), nullptr);
-  downloads_[id] = base::WrapUnique(item);
-  downloads_by_guid_[guid] = item;
+  download::DownloadItemImpl* item = download.get();
+  if (item) {
+    item->SetDelegate(this);
+    DownloadItemUtils::AttachInfo(item, GetBrowserContext(), nullptr);
+    OnDownloadCreated(std::move(download));
+  } else {
+    item = item_factory_->CreatePersistedItem(
+        this, guid, id, current_path, target_path, url_chain, referrer_url,
+        site_url, tab_url, tab_refererr_url, mime_type, original_mime_type,
+        start_time, end_time, etag, last_modified, received_bytes, total_bytes,
+        hash, state, danger_type, interrupt_reason, opened, last_access_time,
+        transient, received_slices);
+    DownloadItemUtils::AttachInfo(item, GetBrowserContext(), nullptr);
+    OnDownloadCreated(base::WrapUnique(item));
+  }
+  return item;
+}
+
+void DownloadManagerImpl::OnDownloadCreated(
+    std::unique_ptr<download::DownloadItemImpl> download) {
+  DCHECK(!base::ContainsKey(downloads_, download->GetId()));
+  DCHECK(!base::ContainsKey(downloads_by_guid_, download->GetGuid()));
+  download::DownloadItemImpl* item = download.get();
+  downloads_[item->GetId()] = std::move(download);
+  downloads_by_guid_[item->GetGuid()] = item;
   for (auto& observer : observers_)
     observer.OnDownloadCreated(this, item);
   DVLOG(20) << __func__ << "() download = " << item->DebugString(true);
-  return item;
 }
 
 void DownloadManagerImpl::PostInitialization(
@@ -1030,7 +1033,9 @@ void DownloadManagerImpl::PostInitialization(
   // cache are initialized.
   initialized_ = history_db_initialized_ && in_progress_cache_initialized_;
 
-  if (initialized_) {
+  if (!initialized_)
+    return;
+
 #if defined(OS_ANDROID)
     for (const auto& guid : cleared_download_guids_on_startup_)
       in_progress_manager_->RemoveInProgressDownload(guid);
@@ -1046,9 +1051,19 @@ void DownloadManagerImpl::PostInitialization(
           interrupted_download_cleared_from_history_);
     }
 #endif
+
+    // If there are still downloads in |in_progress_downloads_|, import them
+    // now.
+    for (auto& download : in_progress_downloads_) {
+      auto item = std::move(download.second);
+      item->SetDelegate(this);
+      DownloadItemUtils::AttachInfo(item.get(), GetBrowserContext(), nullptr);
+      OnDownloadCreated(std::move(item));
+    }
+    in_progress_downloads_.clear();
+
     for (auto& observer : observers_)
       observer.OnManagerInitialized();
-  }
 }
 
 bool DownloadManagerImpl::IsManagerInitialized() const {
@@ -1344,4 +1359,14 @@ bool DownloadManagerImpl::ShouldClearDownloadFromDB(
 }
 #endif  // defined(OS_ANDROID)
 
+std::unique_ptr<download::DownloadItemImpl>
+DownloadManagerImpl::RetrieveInProgressDownload(uint32_t id) {
+  if (base::ContainsKey(in_progress_downloads_, id)) {
+    auto download = std::move(in_progress_downloads_[id]);
+    in_progress_downloads_.erase(id);
+    return download;
+  }
+  return nullptr;
+}
+
 }  // namespace content

content/browser/download/download_manager_impl.h

@@ -292,6 +292,13 @@ class CONTENT_EXPORT DownloadManagerImpl
   // Whether |next_download_id_| is initialized.
   bool IsNextIdInitialized() const;
 
+  // Called when a new download is created.
+  void OnDownloadCreated(std::unique_ptr<download::DownloadItemImpl> download);
+
+  // Retrieves a download from |in_progress_downloads_|.
+  std::unique_ptr<download::DownloadItemImpl> RetrieveInProgressDownload(
+      uint32_t id);
+
 #if defined(OS_ANDROID)
   // Check whether a download should be cleared from history. On Android,
   // cancelled and non-resumable interrupted download will be cleaned up to
@@ -367,6 +374,11 @@ class CONTENT_EXPORT DownloadManagerImpl
   int cancelled_download_cleared_from_history_;
   int interrupted_download_cleared_from_history_;
 
+  // In progress downloads returned by |in_progress_manager_| that are not yet
+  // added to |downloads_|.
+  std::unordered_map<uint32_t, std::unique_ptr<download::DownloadItemImpl>>
+      in_progress_downloads_;
+
   // Callbacks to run once download ID is determined.
   using IdCallbackVector = std::vector<std::unique_ptr<GetNextIdCallback>>;
   IdCallbackVector id_callbacks_;

content/browser/download/download_manager_impl_unittest.cc

@@ -521,6 +521,11 @@ class DownloadManagerTest : public testing::Test {
     download_manager_->OnInProgressDownloadManagerInitialized();
   }
 
+  void OnHistoryDBInitialized() {
+    download_manager_->PostInitialization(
+        DownloadManager::DOWNLOAD_INITIALIZATION_DEPENDENCY_HISTORY_DB);
+  }
+
   void SetInProgressDownloadManager(
       std::unique_ptr<download::InProgressDownloadManager> manager) {
     download_manager_->in_progress_manager_ = std::move(manager);
@@ -746,8 +751,10 @@ TEST_F(DownloadManagerTest, OnInProgressDownloadsLoaded) {
   EXPECT_CALL(GetMockObserver(), OnDownloadCreated(download_manager_.get(), _))
       .WillOnce(Return());
   OnInProgressDownloadManagerInitialized();
-  ASSERT_TRUE(download_manager_->GetDownloadByGuid(kGuid));
+  ASSERT_FALSE(download_manager_->GetDownloadByGuid(kGuid));
 
+  OnHistoryDBInitialized();
+  ASSERT_TRUE(download_manager_->GetDownloadByGuid(kGuid));
   download::DownloadItem* download =
       download_manager_->GetDownloadByGuid(kGuid);
   download->Remove();