Clear cancelled or non-resumable interrupted download from Download DB

We already started removing cancelled and non-resumable download from history
DB. And we should do the same for download DB.
This CL moves all the logic of clearing those download into DownloadManagerImpl.
Here is the work flow:
1. when in-progress download manager is loaded, clear all the non-resumable
downloads.
2. When DownloadHistory loads all the history items, a nullptr will be
returned if the history item should be cleaned, and history DB will remove
the item.
3. After DownloadHistory finished loading, in-progress download manager will
remove all the non-resumable downloads, so that we are guaranteed that changes
in Download DB are propagated to DownloadHistory

This CL also fixes issues that cancelled download in Download DB are not
propagated to history DB.

BUG=851650

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

chrome/browser/download/download_history.cc

@@ -48,10 +48,6 @@
 #include "content/public/browser/download_manager.h"
 #include "extensions/buildflags/buildflags.h"
 
-#if defined(OS_ANDROID)
-#include "components/download/public/common/download_utils.h"
-#endif  // defined(OS_ANDROID)
-
 #if BUILDFLAG(ENABLE_EXTENSIONS)
 #include "chrome/browser/extensions/api/downloads/downloads_api.h"
 #endif
@@ -299,30 +295,11 @@ void DownloadHistory::LoadHistoryDownloads(std::unique_ptr<InfoVector> infos) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   DCHECK(notifier_.GetManager());
 
-  int cancelled_download_cleared_from_history = 0;
-  int interrupted_download_cleared_from_history = 0;
   for (InfoVector::const_iterator it = infos->begin();
        it != infos->end(); ++it) {
     loading_id_ = history::ToContentDownloadId(it->id);
     download::DownloadItem::DownloadState history_download_state =
         history::ToContentDownloadState(it->state);
-#if defined(OS_ANDROID)
-    // Clean up cancelled download on Android.
-    if (history_download_state == download::DownloadItem::CANCELLED) {
-      ScheduleRemoveDownload(it->id);
-      ++cancelled_download_cleared_from_history;
-      continue;
-    }
-    download::DownloadInterruptReason reason =
-        history::ToContentDownloadInterruptReason(it->interrupt_reason);
-    if (reason != download::DOWNLOAD_INTERRUPT_REASON_NONE &&
-        download::GetDownloadResumeMode(reason, false, false) ==
-            download::ResumeMode::INVALID) {
-      ScheduleRemoveDownload(it->id);
-      ++interrupted_download_cleared_from_history;
-      continue;
-    }
-#endif  // defined(OS_ANDROID)
     download::DownloadItem* item = notifier_.GetManager()->CreateDownloadItem(
         it->guid, loading_id_, it->current_path, it->target_path, it->url_chain,
         it->referrer_url, it->site_url, it->tab_url, it->tab_referrer_url,
@@ -336,12 +313,22 @@ void DownloadHistory::LoadHistoryDownloads(std::unique_ptr<InfoVector> infos) {
         history::ToContentDownloadInterruptReason(it->interrupt_reason),
         it->opened, it->last_access_time, it->transient,
         history::ToContentReceivedSlices(it->download_slice_info));
+    // DownloadManager returns a nullptr if it decides to remove the download
+    // permanently.
+    if (item == nullptr) {
+      ScheduleRemoveDownload(it->id);
+      continue;
+    }
     if (item->GetId() == loading_id_)
       OnDownloadRestoredFromHistory(item);
 
-    // Update the history DB if download is completed.
-    if (history_download_state != download::DownloadItem::COMPLETE &&
-        item->GetState() == download::DownloadItem::COMPLETE) {
+    // The download might have been completed or cancelled without informing
+    // history DB. If this is the case, populate the new state back to history
+    // DB.
+    if ((history_download_state != download::DownloadItem::COMPLETE &&
+         item->GetState() == download::DownloadItem::COMPLETE) ||
+        (history_download_state != download::DownloadItem::CANCELLED &&
+         item->GetState() == download::DownloadItem::CANCELLED)) {
       OnDownloadUpdated(notifier_.GetManager(), item);
     }
 #if BUILDFLAG(ENABLE_EXTENSIONS)
@@ -356,18 +343,6 @@ void DownloadHistory::LoadHistoryDownloads(std::unique_ptr<InfoVector> infos) {
     ++history_size_;
   }
 
-  if (cancelled_download_cleared_from_history > 0) {
-    UMA_HISTOGRAM_COUNTS_1000(
-        "MobileDownload.CancelledDownloadRemovedFromHistory",
-        cancelled_download_cleared_from_history);
-  }
-
-  if (interrupted_download_cleared_from_history > 0) {
-    UMA_HISTOGRAM_COUNTS_1000(
-        "MobileDownload.InterruptedDownloadsRemovedFromHistory",
-        interrupted_download_cleared_from_history);
-  }
-
   // Indicate that the history db is initialized.
   notifier_.GetManager()->PostInitialization(
       content::DownloadManager::DOWNLOAD_INITIALIZATION_DEPENDENCY_HISTORY_DB);
@@ -581,10 +556,12 @@ bool DownloadHistory::NeedToUpdateDownloadHistory(
   }
 #endif
 
-  // When download DB is enabled, only completed download should be added to or
-  // updated in history DB.
+  // When download DB is enabled, only completed or cancelled download should be
+  // added to or updated in history DB. In-progress and interrupted download
+  // will be stored in the in-progress DB.
   return !base::FeatureList::IsEnabled(
              download::features::kDownloadDBForNewDownloads) ||
          item->IsSavePackageDownload() ||
-         item->GetState() == download::DownloadItem::COMPLETE;
+         item->GetState() == download::DownloadItem::COMPLETE ||
+         item->GetState() == download::DownloadItem::CANCELLED;
 }

chrome/browser/download/download_history_unittest.cc

@@ -209,8 +209,13 @@ 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,
   void CreateDownloadHistory(std::unique_ptr<InfoVector> infos,
-                             bool call_on_download_created = true) {
+                             bool call_on_download_created = true,
+                             bool return_null_item = false) {
     DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
     CHECK(infos.get());
     EXPECT_CALL(manager(), AddObserver(_)).WillOnce(WithArg<0>(Invoke(
@@ -237,8 +242,10 @@ class DownloadHistoryTest : public testing::Test {
                     this, &DownloadHistoryTest::CallOnDownloadCreatedInOrder),
                 Return(&item(index))));
       } else {
+        download::DownloadItem* download =
+            return_null_item ? nullptr : &item(index);
         EXPECT_CALL(manager(), MockCreateDownloadItem(adapter))
-            .WillOnce(Return(&item(index)));
+            .WillOnce(Return(download));
       }
     }
     history_ = new FakeHistoryAdapter();
@@ -902,4 +909,26 @@ TEST_F(DownloadHistoryTest, CreateHistoryItemInDownloadDB) {
   ExpectDownloadUpdated(info, false);
 }
 
+// Test loading history download item that will be cleared by |manager_|
+TEST_F(DownloadHistoryTest, RemoveClearedItemFromHistory) {
+  // Enable download DB.
+  base::test::ScopedFeatureList feature_list;
+  feature_list.InitAndEnableFeature(
+      download::features::kDownloadDBForNewDownloads);
+
+  history::DownloadRow info;
+  InitBasicItem(FILE_PATH_LITERAL("/foo/bar.pdf"), "http://example.com/bar.pdf",
+                "http://example.com/referrer.html",
+                download::DownloadItem::IN_PROGRESS, &info);
+
+  std::unique_ptr<InfoVector> infos(new InfoVector());
+  infos->push_back(info);
+  CreateDownloadHistory(std::move(infos), false, true);
+
+  // The download should be removed from history afterwards.
+  IdSet ids;
+  ids.insert(info.id);
+  ExpectDownloadsRemoved(ids);
+}
+
 }  // anonymous namespace

content/browser/download/download_manager_impl.cc

@@ -304,6 +304,8 @@ DownloadManagerImpl::DownloadManagerImpl(BrowserContext* browser_context)
           browser_context_->RetriveInProgressDownloadManager()),
       next_download_id_(download::DownloadItem::kInvalidId),
       is_history_download_id_retrieved_(false),
+      cancelled_download_cleared_from_history_(0),
+      interrupted_download_cleared_from_history_(0),
       weak_factory_(this) {
   DCHECK(browser_context);
   download::SetIOTaskRunner(
@@ -545,14 +547,22 @@ void DownloadManagerImpl::OnInProgressDownloadManagerInitialized() {
   for (auto& download : in_progress_downloads) {
     DCHECK(!base::ContainsKey(downloads_by_guid_, download->GetGuid()));
     DCHECK(!base::ContainsKey(downloads_, download->GetId()));
+    uint32_t id = download->GetId();
+    if (id > max_id)
+      max_id = id;
+#if defined(OS_ANDROID)
+    // On android, clean up cancelled and non resumable interrupted downloads.
+    if (ShouldClearDownloadFromDB(download->GetState(),
+                                  download->GetLastReason())) {
+      cleared_download_guids_on_startup_.insert(download->GetGuid());
+      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;
-    uint32_t id = download->GetId();
     downloads_[id] = std::move(download);
-    if (id > max_id)
-      max_id = id;
     for (auto& observer : observers_)
       observer.OnDownloadCreated(this, item);
     DVLOG(20) << __func__ << "() download = " << item->DebugString(true);
@@ -942,6 +952,16 @@ download::DownloadItem* DownloadManagerImpl::CreateDownloadItem(
     base::Time last_access_time,
     bool transient,
     const std::vector<download::DownloadItem::ReceivedSlice>& received_slices) {
+#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
+  // reduce memory usage.
+  if (cleared_download_guids_on_startup_.find(guid) !=
+          cleared_download_guids_on_startup_.end() ||
+      ShouldClearDownloadFromDB(state, interrupt_reason)) {
+    return nullptr;
+  }
+#endif
   if (base::ContainsKey(downloads_, id)) {
     // If a completed or cancelled download item is already in the history db,
     // remove it from the in-progress db.
@@ -993,6 +1013,21 @@ void DownloadManagerImpl::PostInitialization(
   initialized_ = history_db_initialized_ && in_progress_cache_initialized_;
 
   if (initialized_) {
+#if defined(OS_ANDROID)
+    for (const auto& guid : cleared_download_guids_on_startup_)
+      in_progress_manager_->RemoveInProgressDownload(guid);
+    if (cancelled_download_cleared_from_history_ > 0) {
+      UMA_HISTOGRAM_COUNTS_1000(
+          "MobileDownload.CancelledDownloadRemovedFromHistory",
+          cancelled_download_cleared_from_history_);
+    }
+
+    if (interrupted_download_cleared_from_history_ > 0) {
+      UMA_HISTOGRAM_COUNTS_1000(
+          "MobileDownload.InterruptedDownloadsRemovedFromHistory",
+          interrupted_download_cleared_from_history_);
+    }
+#endif
     for (auto& observer : observers_)
       observer.OnManagerInitialized();
   }
@@ -1273,4 +1308,23 @@ bool DownloadManagerImpl::IsNextIdInitialized() const {
   return is_history_download_id_retrieved_ && in_progress_cache_initialized_;
 }
 
+#if defined(OS_ANDROID)
+bool DownloadManagerImpl::ShouldClearDownloadFromDB(
+    download::DownloadItem::DownloadState state,
+    download::DownloadInterruptReason reason) {
+  if (state == download::DownloadItem::CANCELLED) {
+    ++cancelled_download_cleared_from_history_;
+    return true;
+  }
+  if (reason != download::DOWNLOAD_INTERRUPT_REASON_NONE &&
+      download::GetDownloadResumeMode(reason, false /* restart_required */,
+                                      false /* user_action_required */) ==
+          download::ResumeMode::INVALID) {
+    ++interrupted_download_cleared_from_history_;
+    return true;
+  }
+  return false;
+}
+#endif  // defined(OS_ANDROID)
+
 }  // namespace content

content/browser/download/download_manager_impl.h

@@ -20,6 +20,7 @@
 #include "base/observer_list.h"
 #include "base/sequenced_task_runner_helpers.h"
 #include "base/synchronization/lock.h"
+#include "build/build_config.h"
 #include "components/download/public/common/download_item_impl_delegate.h"
 #include "components/download/public/common/download_url_parameters.h"
 #include "components/download/public/common/in_progress_download_manager.h"
@@ -291,6 +292,14 @@ class CONTENT_EXPORT DownloadManagerImpl
   // Whether |next_download_id_| is initialized.
   bool IsNextIdInitialized() const;
 
+#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
+  // save memory.
+  bool ShouldClearDownloadFromDB(download::DownloadItem::DownloadState state,
+                                 download::DownloadInterruptReason reason);
+#endif  // defined(OS_ANDROID)
+
   // Factory for creation of downloads items.
   std::unique_ptr<download::DownloadItemFactory> item_factory_;
 
@@ -349,6 +358,11 @@ class CONTENT_EXPORT DownloadManagerImpl
   // Whether next download ID from history DB is being retrieved.
   bool is_history_download_id_retrieved_;
 
+  // The download GUIDs that are cleared up on startup.
+  std::set<std::string> cleared_download_guids_on_startup_;
+  int cancelled_download_cleared_from_history_;
+  int interrupted_download_cleared_from_history_;
+
   // Callbacks to run once download ID is determined.
   using IdCallbackVector = std::vector<std::unique_ptr<GetNextIdCallback>>;
   IdCallbackVector id_callbacks_;