Fix a crash when resuming an in-progress download before history is loaded

If a download is interrupted in a terminal state, the information
will be written to both history DB and in-progress DB. And the in-progress
DB info will later be erased when download history is loaded. However,
if Chrome crashes during the write, history DB could contain more recent
information. If the same download is resumed in reduced mode later, loading
the history DB could delete that download, causing Chrome to delete the
DownloadFile object on UI thread while it is reading data on download
task runner.
This CL fixes the issue by:
1. Force in-progress DB to write download information immediately if
a download is in a terminal state.
2. If a download is in terminal state in history DB and an in-progress
DB version is already in-progress, ignore the history DB version and
request it to be deleted. This matches the existing behavior if only
history DB is used. (If only history DB is used, a crash before download
terminal state is written to disk will make the download to be able to
resume, even if a failed notification is already shown).

BUG=1009839

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

chrome/browser/download/download_browsertest.cc

@@ -22,6 +22,7 @@
 #include "base/files/file_path.h"
 #include "base/files/file_util.h"
 #include "base/files/scoped_temp_dir.h"
+#include "base/guid.h"
 #include "base/memory/ref_counted.h"
 #include "base/path_service.h"
 #include "base/run_loop.h"
@@ -49,17 +50,20 @@
 #include "chrome/browser/download/download_crx_util.h"
 #include "chrome/browser/download/download_history.h"
 #include "chrome/browser/download/download_item_model.h"
+#include "chrome/browser/download/download_manager_utils.h"
 #include "chrome/browser/download/download_prefs.h"
 #include "chrome/browser/download/download_request_limiter.h"
 #include "chrome/browser/download/download_shelf.h"
 #include "chrome/browser/download/download_target_determiner.h"
 #include "chrome/browser/download/download_test_file_activity_observer.h"
+#include "chrome/browser/download/simple_download_manager_coordinator_factory.h"
 #include "chrome/browser/extensions/install_verifier.h"
 #include "chrome/browser/history/history_service_factory.h"
 #include "chrome/browser/infobars/infobar_service.h"
 #include "chrome/browser/metrics/subprocess_metrics_provider.h"
 #include "chrome/browser/permissions/permission_request_manager.h"
 #include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/profiles/profile_key.h"
 #include "chrome/browser/renderer_context_menu/render_view_context_menu_browsertest_util.h"
 #include "chrome/browser/renderer_context_menu/render_view_context_menu_test_util.h"
 #include "chrome/browser/reputation/safety_tip_test_utils.h"
@@ -84,6 +88,7 @@
 #include "components/download/public/common/download_danger_type.h"
 #include "components/download/public/common/download_interrupt_reasons.h"
 #include "components/download/public/common/download_item.h"
+#include "components/download/public/common/in_progress_download_manager.h"
 #include "components/history/content/browser/download_conversions.h"
 #include "components/history/core/browser/download_constants.h"
 #include "components/history/core/browser/download_row.h"
@@ -396,6 +401,65 @@ void SetHiddenDownloadCallback(DownloadItem* item,
 }
 #endif
 
+class SimpleDownloadManagerCoordinatorWaiter
+    : public download::SimpleDownloadManagerCoordinator::Observer {
+ public:
+  explicit SimpleDownloadManagerCoordinatorWaiter(
+      download::SimpleDownloadManagerCoordinator* coordinator)
+      : coordinator_(coordinator) {
+    coordinator_->AddObserver(this);
+  }
+
+  ~SimpleDownloadManagerCoordinatorWaiter() override {
+    if (coordinator_)
+      coordinator_->RemoveObserver(this);
+  }
+
+  void WaitForInitialization() {
+    if (coordinator_ && coordinator_->initialized())
+      return;
+    base::RunLoop run_loop;
+    completion_closure_ = run_loop.QuitClosure();
+    run_loop.Run();
+    return;
+  }
+
+ private:
+  void OnDownloadsInitialized(bool active_downloads_only) override {
+    if (completion_closure_)
+      std::move(completion_closure_).Run();
+  }
+
+  void OnManagerGoingDown(
+      download::SimpleDownloadManagerCoordinator* coordinator) override {
+    DCHECK_EQ(coordinator_, coordinator);
+    coordinator_->RemoveObserver(this);
+    coordinator_ = nullptr;
+  }
+
+  download::SimpleDownloadManagerCoordinator* coordinator_;
+  base::OnceClosure completion_closure_;
+};
+
+void CreateCompletedDownload(content::DownloadManager* download_manager,
+                             const std::string& guid,
+                             const base::FilePath target_path,
+                             std::vector<GURL> url_chain,
+                             int64_t file_size) {
+  base::Time current_time = base::Time::Now();
+  download_manager->CreateDownloadItem(
+      guid, 1 /* id */, target_path, target_path, url_chain,
+      GURL() /* referrer_url */, GURL() /* site_url */, GURL() /* tab_url */,
+      GURL() /* tab_referrer_url */, url::Origin() /* request_initiator */,
+      "" /* mime_type */, "" /* original_mime_type */, current_time,
+      current_time, "" /* etag */, "" /* last_modified */, file_size, file_size,
+      "" /* hash */, download::DownloadItem::COMPLETE,
+      download::DOWNLOAD_DANGER_TYPE_USER_VALIDATED,
+      download::DOWNLOAD_INTERRUPT_REASON_NONE, false /* opened */,
+      current_time, false /* transient */,
+      std::vector<download::DownloadItem::ReceivedSlice>());
+}
+
 }  // namespace
 
 DownloadTestObserverNotInProgress::DownloadTestObserverNotInProgress(
@@ -494,6 +558,7 @@ class DownloadTest : public InProcessBrowserTest {
   DownloadTest() {}
 
   void SetUpOnMainThread() override {
+    ASSERT_TRUE(CheckTestDir());
     ASSERT_TRUE(InitialSetup());
     host_resolver()->AddRule("www.a.com", "127.0.0.1");
     host_resolver()->AddRule("foo.com", "127.0.0.1");
@@ -508,15 +573,16 @@ class DownloadTest : public InProcessBrowserTest {
     file_activity_observer_.reset();
   }
 
-  // Returning false indicates a failure of the setup, and should be asserted
-  // in the caller.
-  bool InitialSetup() {
+  bool CheckTestDir() {
     bool have_test_dir =
         base::PathService::Get(chrome::DIR_TEST_DATA, &test_dir_);
     EXPECT_TRUE(have_test_dir);
-    if (!have_test_dir)
-      return false;
+    return have_test_dir;
+  }
 
+  // Returning false indicates a failure of the setup, and should be asserted
+  // in the caller.
+  bool InitialSetup() {
     // Sanity check default values for window and tab count.
     int window_count = chrome::GetTotalBrowserCount();
     EXPECT_EQ(1, window_count);
@@ -4094,6 +4160,86 @@ IN_PROC_BROWSER_TEST_F(DownloadTest, DownloadLargeDataURL) {
   ASSERT_EQ(downloaded_data, png_data);
 }
 
+// Testing the behavior of resuming with only in-progress download manager.
+class InProgressDownloadTest : public DownloadTest {
+ public:
+  void SetUpOnMainThread() override { EXPECT_TRUE(CheckTestDir()); }
+};
+
+// Check that if a download exists in both in-progress and history DB,
+// resuming the download after loading the in-progress DB and before
+// history initialization will continue downloading the item even if it
+// is in a terminal state in history DB.
+IN_PROC_BROWSER_TEST_F(InProgressDownloadTest,
+                       ResumeInProgressDownloadBeforeLoadingHistory) {
+  embedded_test_server()->ServeFilesFromDirectory(GetTestDataDirectory());
+  ASSERT_TRUE(embedded_test_server()->Start());
+  GURL url = embedded_test_server()->GetURL("/downloads/a_zip_file.zip");
+  base::FilePath origin(OriginFile(
+      base::FilePath(FILE_PATH_LITERAL("downloads/a_zip_file.zip"))));
+  base::ScopedAllowBlockingForTesting allow_blocking;
+  ASSERT_TRUE(base::PathExists(origin));
+  // Gets the file size.
+  int64_t origin_file_size = 0;
+  EXPECT_TRUE(base::GetFileSize(origin, &origin_file_size));
+  std::string guid = base::GenerateGUID();
+
+  // Wait for in-progress download manager to initialize.
+  download::InProgressDownloadManager* in_progress_manager =
+      DownloadManagerUtils::GetInProgressDownloadManager(
+          browser()->profile()->GetProfileKey());
+  download::SimpleDownloadManagerCoordinator* coordinator =
+      SimpleDownloadManagerCoordinatorFactory::GetForKey(
+          browser()->profile()->GetProfileKey());
+  SimpleDownloadManagerCoordinatorWaiter coordinator_waiter(coordinator);
+  coordinator_waiter.WaitForInitialization();
+
+  base::FilePath target_path;
+  ASSERT_TRUE(
+      base::PathService::Get(chrome::DIR_DEFAULT_DOWNLOADS, &target_path));
+  target_path =
+      target_path.Append(base::FilePath(FILE_PATH_LITERAL("a_zip_file.zip")));
+  std::vector<GURL> url_chain;
+  url_chain.emplace_back(url);
+  base::Time current_time = base::Time::Now();
+  in_progress_manager->AddInProgressDownloadForTest(
+      std::make_unique<download::DownloadItemImpl>(
+          in_progress_manager, guid, 1 /* id */,
+          target_path.AddExtensionASCII("crdownload"), target_path, url_chain,
+          GURL() /* referrer_url */, GURL() /* site_url */,
+          GURL() /* tab_url */, GURL() /* tab_referrer_url */,
+          url::Origin() /* request_initiator */, "" /* mime_type */,
+          "" /* original_mime_type */, current_time, current_time,
+          "" /* etag */, "" /* last_modified */, 0 /* received_bytes */,
+          origin_file_size, 0 /* auto_resume_count */, "" /* hash */,
+          download::DownloadItem::INTERRUPTED,
+          download::DOWNLOAD_DANGER_TYPE_USER_VALIDATED,
+          download::DOWNLOAD_INTERRUPT_REASON_CRASH, false /* paused */,
+          false /* allow_metered */, false /* opened */, current_time,
+          false /* transient */,
+          std::vector<download::DownloadItem::ReceivedSlice>(),
+          nullptr /* download_entry */));
+
+  download::DownloadItem* download = coordinator->GetDownloadByGuid(guid);
+  content::DownloadManager* manager = DownloadManagerForBrowser(browser());
+  DownloadCoreService* service =
+      DownloadCoreServiceFactory::GetForBrowserContext(browser()->profile());
+  service->SetDownloadHistoryForTesting(nullptr);
+
+  ASSERT_TRUE(download);
+  PercentWaiter waiter(download);
+  // Resume the download first, before download history loads.
+  download->Resume(true);
+  // Now simulate that history DB is loaded.
+  manager->OnHistoryQueryComplete(
+      base::Bind(CreateCompletedDownload, base::Unretained(manager), guid,
+                 target_path, std::move(url_chain), origin_file_size));
+  // Download should continue and complete.
+  ASSERT_TRUE(waiter.WaitForFinished());
+  download::DownloadItem* history_download = manager->GetDownloadByGuid(guid);
+  CHECK_EQ(download, history_download);
+}
+
 #if BUILDFLAG(FULL_SAFE_BROWSING)
 
 namespace {

chrome/browser/download/download_core_service.h

@@ -8,11 +8,11 @@
 #include <memory>
 
 #include "base/macros.h"
+#include "chrome/browser/download/download_history.h"
 #include "components/keyed_service/core/keyed_service.h"
 #include "extensions/buildflags/buildflags.h"
 
 class ChromeDownloadManagerDelegate;
-class DownloadHistory;
 class ExtensionDownloadsEventRouter;
 
 namespace content {
@@ -65,6 +65,12 @@ class DownloadCoreService : public KeyedService {
   virtual void SetDownloadManagerDelegateForTesting(
       std::unique_ptr<ChromeDownloadManagerDelegate> delegate) = 0;
 
+  // Sets the DownloadHistory associated with this object and
+  // its DownloadManager. Takes ownership of |download_history|, and destroys
+  // the previous delegate.  For testing.
+  virtual void SetDownloadHistoryForTesting(
+      std::unique_ptr<DownloadHistory> download_history) {}
+
   // Returns false if at least one extension has disabled the shelf, true
   // otherwise.
   virtual bool IsShelfEnabled() = 0;

chrome/browser/download/download_core_service_impl.cc

@@ -142,6 +142,11 @@ void DownloadCoreServiceImpl::SetDownloadManagerDelegateForTesting(
     new_delegate->Shutdown();
 }
 
+void DownloadCoreServiceImpl::SetDownloadHistoryForTesting(
+    std::unique_ptr<DownloadHistory> download_history) {
+  download_history_ = std::move(download_history);
+}
+
 bool DownloadCoreServiceImpl::IsShelfEnabled() {
 #if defined(OS_ANDROID)
   return true;

chrome/browser/download/download_core_service_impl.h

@@ -49,6 +49,8 @@ class DownloadCoreServiceImpl : public DownloadCoreService {
   void SetDownloadManagerDelegateForTesting(
       std::unique_ptr<ChromeDownloadManagerDelegate> delegate) override;
   bool IsShelfEnabled() override;
+  void SetDownloadHistoryForTesting(
+      std::unique_ptr<DownloadHistory> download_history) override;
 
   // KeyedService
   void Shutdown() override;

components/download/internal/common/download_db_cache.cc

@@ -43,10 +43,19 @@ ShouldUpdateDownloadDBResult ShouldUpdateDownloadDB(
   if (current.download_info)
     current_info = current.download_info->in_progress_info;
 
-  base::FilePath current_path =
-      current_info ? current_info->current_path : base::FilePath();
-
-  bool paused = current_info ? current_info->paused : false;
+  base::FilePath current_path;
+  bool paused = false;
+  GURL url;
+  DownloadItem::DownloadState state = DownloadItem::DownloadState::IN_PROGRESS;
+  DownloadInterruptReason interrupt_reason = DOWNLOAD_INTERRUPT_REASON_NONE;
+  if (current_info) {
+    base::FilePath current_path = current_info->current_path;
+    paused = current_info->paused;
+    if (!current_info->url_chain.empty())
+      url = current_info->url_chain.back();
+    state = current_info->state;
+    interrupt_reason = current_info->interrupt_reason;
+  }
 
   // When download path is determined, Chrome should commit the history
   // immediately. Otherwise the file will be left permanently on the external
@@ -57,6 +66,9 @@ ShouldUpdateDownloadDBResult ShouldUpdateDownloadDB(
   if (previous.value() == current)
     return ShouldUpdateDownloadDBResult::NO_UPDATE;
 
+  if (IsDownloadDone(url, state, interrupt_reason))
+    return ShouldUpdateDownloadDBResult::UPDATE_IMMEDIATELY;
+
   return ShouldUpdateDownloadDBResult::UPDATE;
 }
 

components/download/internal/common/in_progress_download_manager.cc

@@ -402,7 +402,12 @@ void InProgressDownloadManager::DetermineDownloadTarget(
           : DownloadPathReservationTracker::OVERWRITE,
       base::Bind(&OnPathReserved, callback, download->GetDangerType(),
                  intermediate_path_cb_, download->GetForcedFilePath()));
-#endif
+#else
+  callback.Run(download->GetTargetFilePath(),
+               DownloadItem::TARGET_DISPOSITION_OVERWRITE,
+               download->GetDangerType(), download->GetFullPath(),
+               DOWNLOAD_INTERRUPT_REASON_NONE);
+#endif  // defined(OS_ANDROID)
 }
 
 void InProgressDownloadManager::ResumeInterruptedDownload(

content/browser/download/download_manager_impl.cc

@@ -924,11 +924,23 @@ download::DownloadItem* DownloadManagerImpl::CreateDownloadItem(
       received_bytes, total_bytes, hash, state, danger_type, interrupt_reason,
       opened, last_access_time, transient, received_slices));
   if (in_progress_download) {
-    // If the download item from history db is already in terminal state,
-    // remove it from the in-progress db. Otherwise, use the in-progress db one.
-    if (item->IsDone()) {
+    // If a download is in both history DB and in-progress DB, we should
+    // be able to remove the in-progress entry if the following 2 conditions
+    // are both met:
+    // 1. The download state in the history DB is a terminal state.
+    // 2. The download is not currently in progress.
+    // The situation could happen when browser crashes when download just
+    // reaches a terminal state. If the download is already in progress, we
+    // should wait for it to complete so that both DBs will be updated
+    // afterwards.
+    if (item->IsDone() && in_progress_download->GetState() !=
+                              download::DownloadItem::IN_PROGRESS) {
       in_progress_manager_->RemoveInProgressDownload(guid);
     } else {
+      // If one of the conditions are not met, use the in-progress download
+      // entry.
+      // TODO(qinmin): return nullptr so that the history DB will delete
+      // the download.
       item = std::move(in_progress_download);
       item->SetDelegate(this);
     }