Revert "Enable DownloadDBForNewDownloads by default"

This reverts commit b1ef7206.

Reason for revert: The is causing crashes on Linux CFI bot:
https://ci.chromium.org/p/chromium/builders/ci/Linux%20CFI/13126


Original change's description:
> Enable DownloadDBForNewDownloads by default
> 
> BUG=842245
> 
> Change-Id: Ieea0d3bcaef8f8f47a32c74845c056c452ca7f8c
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1532922
> Commit-Queue: Min Qin <qinmin@chromium.org>
> Reviewed-by: Xing Liu <xingliu@chromium.org>
> Reviewed-by: Shakti Sahu <shaktisahu@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#644406}

TBR=qinmin@chromium.org,shaktisahu@chromium.org,xingliu@chromium.org

Change-Id: I8a9550ab957e439b3a40b9caf38205d902354075
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 842245
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1541660Reviewed-by: Brian White <bcwhite@chromium.org>
Commit-Queue: Brian White <bcwhite@chromium.org>
Cr-Commit-Position: refs/heads/master@{#644784}

chrome/browser/download/download_browsertest.cc

@@ -384,6 +384,11 @@ void SetHiddenDownloadCallback(DownloadItem* item,
 }
 #endif
 
+// Callback for HistoryObserver; used in DownloadHistoryCheck
+bool HasDataAndName(const history::DownloadRow& row) {
+  return row.received_bytes > 0 && !row.target_path.empty();
+}
+
 }  // namespace
 
 DownloadTestObserverNotInProgress::DownloadTestObserverNotInProgress(
@@ -423,8 +428,15 @@ class HistoryObserver : public DownloadHistory::Observer {
       service->GetDownloadHistory()->RemoveObserver(this);
   }
 
+  void SetFilterCallback(const FilterCallback& callback) {
+    callback_ = callback;
+  }
+
   void OnDownloadStored(download::DownloadItem* item,
                         const history::DownloadRow& info) override {
+    if (!callback_.is_null() && (!callback_.Run(info)))
+        return;
+
     seen_stored_ = true;
     if (waiting_)
       base::RunLoop::QuitCurrentWhenIdleDeprecated();
@@ -448,6 +460,7 @@ class HistoryObserver : public DownloadHistory::Observer {
   Profile* profile_;
   bool waiting_ = false;
   bool seen_stored_ = false;
+  FilterCallback callback_;
 
   DISALLOW_COPY_AND_ASSIGN(HistoryObserver);
 };
@@ -1921,7 +1934,7 @@ IN_PROC_BROWSER_TEST_F(DownloadTest, MAYBE_DownloadHistoryCheck) {
       TestFileErrorInjector::Create(DownloadManagerForBrowser(browser())));
   TestFileErrorInjector::FileErrorInfo error_info = {
       TestFileErrorInjector::FILE_OPERATION_STREAM_COMPLETE, 0,
-      download::DOWNLOAD_INTERRUPT_REASON_SERVER_BAD_CONTENT};
+      download::DOWNLOAD_INTERRUPT_REASON_NETWORK_FAILED};
   error_info.stream_offset = 0;
   error_info.stream_bytes_written = 1024;
   injector->InjectError(error_info);
@@ -1936,11 +1949,36 @@ IN_PROC_BROWSER_TEST_F(DownloadTest, MAYBE_DownloadHistoryCheck) {
   // Download the url and wait until the object has been stored.
   base::Time start(base::Time::Now());
   HistoryObserver observer(browser()->profile());
+  observer.SetFilterCallback(base::Bind(&HasDataAndName));
   ui_test_utils::NavigateToURL(browser(), redirect_url);
+  observer.WaitForStored();
 
   // Get the details on what was stored into the history.
   std::unique_ptr<std::vector<history::DownloadRow>> downloads_in_database;
-  ASSERT_EQ(0u, downloads_in_database->size());
+  ASSERT_TRUE(DownloadsHistoryDataCollector(
+      browser()->profile()).WaitForDownloadInfo(&downloads_in_database));
+  ASSERT_EQ(1u, downloads_in_database->size());
+
+  // Confirm history storage is what you expect for a partially completed
+  // slow download job.
+  history::DownloadRow& row(downloads_in_database->at(0));
+  EXPECT_EQ(DestinationFile(browser(), file), row.target_path);
+  EXPECT_EQ(FILE_PATH_LITERAL("Unconfirmed"),
+            row.current_path.BaseName().value().substr(0, 11));
+  EXPECT_EQ(FILE_PATH_LITERAL(".crdownload"), row.current_path.Extension());
+  ASSERT_EQ(2u, row.url_chain.size());
+  EXPECT_EQ(redirect_url.spec(), row.url_chain[0].spec());
+  EXPECT_EQ(download_url.spec(), row.url_chain[1].spec());
+  EXPECT_EQ(history::DownloadDangerType::MAYBE_DANGEROUS_CONTENT,
+            row.danger_type);
+  EXPECT_LE(start, row.start_time);
+  EXPECT_EQ(content::SlowDownloadHttpResponse::kFirstDownloadSize,
+            row.received_bytes);
+  EXPECT_EQ(content::SlowDownloadHttpResponse::kFirstDownloadSize +
+                content::SlowDownloadHttpResponse::kSecondDownloadSize,
+            row.total_bytes);
+  EXPECT_EQ(history::DownloadState::IN_PROGRESS, row.state);
+  EXPECT_FALSE(row.opened);
 
   // Finish the download.  We're ok relying on the history to be flushed
   // at this point as our queries will be behind the history updates
@@ -1961,7 +1999,6 @@ IN_PROC_BROWSER_TEST_F(DownloadTest, MAYBE_DownloadHistoryCheck) {
   base::Time end(base::Time::Now());
 
   // Get what was stored in the history.
-  observer.WaitForStored();
   ASSERT_TRUE(DownloadsHistoryDataCollector(
       browser()->profile()).WaitForDownloadInfo(&downloads_in_database));
   ASSERT_EQ(1u, downloads_in_database->size());
@@ -1985,7 +2022,7 @@ IN_PROC_BROWSER_TEST_F(DownloadTest, MAYBE_DownloadHistoryCheck) {
             row1.total_bytes);
   EXPECT_EQ(history::DownloadState::INTERRUPTED, row1.state);
   EXPECT_EQ(history::ToHistoryDownloadInterruptReason(
-                download::DOWNLOAD_INTERRUPT_REASON_SERVER_BAD_CONTENT),
+                download::DOWNLOAD_INTERRUPT_REASON_NETWORK_FAILED),
             row1.interrupt_reason);
   EXPECT_FALSE(row1.opened);
 }
@@ -2003,35 +2040,48 @@ IN_PROC_BROWSER_TEST_F(DownloadTest, DownloadHistoryDangerCheck) {
       embedded_test_server()->GetURL("/downloads/dangerous/dangerous.swf");
 
   // Download the url and wait until the object has been stored.
-  auto completion_observer =
-      std::make_unique<content::DownloadTestObserverTerminal>(
-          DownloadManagerForBrowser(browser()), 1,
-          content::DownloadTestObserver::ON_DANGEROUS_DOWNLOAD_IGNORE);
-  auto dangerous_observer =
-      std::make_unique<content::DownloadTestObserverTerminal>(
+  std::unique_ptr<content::DownloadTestObserver> download_observer(
+      new content::DownloadTestObserverTerminal(
           DownloadManagerForBrowser(browser()), 1,
-          content::DownloadTestObserver::ON_DANGEROUS_DOWNLOAD_QUIT);
+          content::DownloadTestObserver::ON_DANGEROUS_DOWNLOAD_IGNORE));
   base::Time start(base::Time::Now());
   HistoryObserver observer(browser()->profile());
+  observer.SetFilterCallback(base::Bind(&HasDataAndName));
   ui_test_utils::NavigateToURL(browser(), download_url);
+  observer.WaitForStored();
+
+  // Get the details on what was stored into the history.
+  std::unique_ptr<std::vector<history::DownloadRow>> downloads_in_database;
+  ASSERT_TRUE(DownloadsHistoryDataCollector(
+      browser()->profile()).WaitForDownloadInfo(&downloads_in_database));
+  ASSERT_EQ(1u, downloads_in_database->size());
+
+  // Confirm history storage is what you expect for an unvalidated
+  // dangerous file.
+  base::FilePath file(FILE_PATH_LITERAL("downloads/dangerous/dangerous.swf"));
+  history::DownloadRow& row(downloads_in_database->at(0));
+  EXPECT_EQ(DestinationFile(browser(), file), row.target_path);
+  EXPECT_NE(DownloadTargetDeterminer::GetCrDownloadPath(
+                DestinationFile(browser(), file)),
+            row.current_path);
+  EXPECT_EQ(history::DownloadDangerType::DANGEROUS_FILE, row.danger_type);
+  EXPECT_LE(start, row.start_time);
+  EXPECT_EQ(history::DownloadState::IN_PROGRESS, row.state);
+  EXPECT_FALSE(row.opened);
 
   // Validate the download and wait for it to finish.
   std::vector<DownloadItem*> downloads;
   DownloadManagerForBrowser(browser())->GetAllDownloads(&downloads);
   ASSERT_EQ(1u, downloads.size());
-  dangerous_observer->WaitForFinished();
   downloads[0]->ValidateDangerousDownload();
-  completion_observer->WaitForFinished();
-  EXPECT_EQ(1u, completion_observer->NumDangerousDownloadsSeen());
+  download_observer->WaitForFinished();
 
   // Get history details and confirm it's what you expect.
-  observer.WaitForStored();
-  std::unique_ptr<std::vector<history::DownloadRow>> downloads_in_database;
+  downloads_in_database->clear();
   ASSERT_TRUE(DownloadsHistoryDataCollector(
       browser()->profile()).WaitForDownloadInfo(&downloads_in_database));
   ASSERT_EQ(1u, downloads_in_database->size());
   history::DownloadRow& row1(downloads_in_database->at(0));
-  base::FilePath file(FILE_PATH_LITERAL("downloads/dangerous/dangerous.swf"));
   EXPECT_EQ(DestinationFile(browser(), file), row1.target_path);
   EXPECT_EQ(DestinationFile(browser(), file), row1.current_path);
   EXPECT_EQ(history::DownloadDangerType::USER_VALIDATED, row1.danger_type);

chrome/browser/download/download_history_unittest.cc

@@ -441,16 +441,6 @@ class DownloadHistoryTest : public testing::Test {
         .WillRepeatedly(SetArgPointee<0>(items));
   }
 
-  void SetDownloadDBEnabled(bool enabled) {
-    if (enabled) {
-      feature_list_.InitAndEnableFeature(
-          download::features::kDownloadDBForNewDownloads);
-    } else {
-      feature_list_.InitAndDisableFeature(
-          download::features::kDownloadDBForNewDownloads);
-    }
-  }
-
  private:
   content::TestBrowserThreadBundle test_browser_thread_bundle_;
   std::vector<std::unique_ptr<StrictMockDownloadItem>> items_;
@@ -460,16 +450,13 @@ class DownloadHistoryTest : public testing::Test {
   std::unique_ptr<DownloadHistory> download_history_;
   content::DownloadManager::Observer* manager_observer_ = nullptr;
   size_t download_created_index_ = 0;
-  base::test::ScopedFeatureList feature_list_;
 
   DISALLOW_COPY_AND_ASSIGN(DownloadHistoryTest);
 };
 
 // Test loading an item from the database, changing it, saving it back, removing
 // it.
-TEST_F(DownloadHistoryTest, DownloadHistoryTest_LoadWithoutDownloadDB) {
-  SetDownloadDBEnabled(false);
-
+TEST_F(DownloadHistoryTest, DownloadHistoryTest_Load) {
   // Load a download from history, create the item, OnDownloadCreated,
   // OnDownloadUpdated, OnDownloadRemoved.
   history::DownloadRow info;
@@ -497,37 +484,6 @@ TEST_F(DownloadHistoryTest, DownloadHistoryTest_LoadWithoutDownloadDB) {
   ExpectDownloadsRemoved(ids);
 }
 
-// Test loading an item from the database, changing it and removing it.
-TEST_F(DownloadHistoryTest, DownloadHistoryTest_LoadWithDownloadDB) {
-  SetDownloadDBEnabled(true);
-
-  // Load a download from history, create the item, OnDownloadCreated,
-  // OnDownloadUpdated, OnDownloadRemoved.
-  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));
-    ExpectNoDownloadCreated();
-  }
-  EXPECT_TRUE(DownloadHistory::IsPersisted(&item(0)));
-
-  // Pretend that something changed on the item, the update will not be
-  // persisted.
-  EXPECT_CALL(item(0), GetOpened()).WillRepeatedly(Return(true));
-  item(0).NotifyObserversDownloadUpdated();
-  ExpectNoDownloadUpdated();
-
-  // Pretend that the user removed the item.
-  IdSet ids;
-  ids.insert(info.id);
-  item(0).NotifyObserversDownloadRemoved();
-  ExpectDownloadsRemoved(ids);
-}
-
 // Test that the OnHistoryQueryComplete() observer method is invoked for an
 // observer that was added before the initial history query completing.
 TEST_F(DownloadHistoryTest, DownloadHistoryTest_OnHistoryQueryComplete_Pre) {
@@ -580,9 +536,6 @@ TEST_F(DownloadHistoryTest, DownloadHistoryTest_OnHistoryQueryComplete_Post) {
 // Test creating an item, saving it to the database, changing it, saving it
 // back, removing it.
 TEST_F(DownloadHistoryTest, DownloadHistoryTest_Create) {
-  // Disable download DB.
-  SetDownloadDBEnabled(false);
-
   // Create a fresh item not from history, OnDownloadCreated, OnDownloadUpdated,
   // OnDownloadRemoved.
   CreateDownloadHistory(std::unique_ptr<InfoVector>(new InfoVector()));
@@ -613,7 +566,6 @@ TEST_F(DownloadHistoryTest, DownloadHistoryTest_Create) {
 // Test that changes to persisted fields in a DownloadItem triggers database
 // updates.
 TEST_F(DownloadHistoryTest, DownloadHistoryTest_Update) {
-  SetDownloadDBEnabled(false);
   CreateDownloadHistory(std::unique_ptr<InfoVector>(new InfoVector()));
 
   history::DownloadRow info;
@@ -727,8 +679,6 @@ TEST_F(DownloadHistoryTest, DownloadHistoryTest_Update) {
 // IsTemporary, saving it back, changing it, saving it back because it isn't
 // Temporary anymore.
 TEST_F(DownloadHistoryTest, DownloadHistoryTest_Temporary) {
-  SetDownloadDBEnabled(false);
-
   // Create a fresh item not from history, OnDownloadCreated, OnDownloadUpdated,
   // OnDownloadRemoved.
   CreateDownloadHistory(std::unique_ptr<InfoVector>(new InfoVector()));
@@ -779,7 +729,7 @@ TEST_F(DownloadHistoryTest, DownloadHistoryTest_RemoveWhileAdding) {
   InitBasicItem(FILE_PATH_LITERAL("/foo/bar.pdf"), "http://example.com/bar.pdf",
                 "http://example.com/referrer.html",
                 download::DownloadItem::COMPLETE, &info);
-  EXPECT_CALL(item(0), IsDone()).WillRepeatedly(Return(true));
+
   // Instruct CreateDownload() to not callback to DownloadHistory immediately,
   // but to wait for FinishCreateDownload().
   set_slow_create_download(true);
@@ -841,9 +791,6 @@ TEST_F(DownloadHistoryTest, DownloadHistoryTest_Multiple) {
 
 // Test what happens when HistoryService/CreateDownload::CreateDownload() fails.
 TEST_F(DownloadHistoryTest, DownloadHistoryTest_CreateFailed) {
-  // Disable download DB.
-  SetDownloadDBEnabled(false);
-
   // Create a fresh item not from history, OnDownloadCreated, OnDownloadUpdated,
   // OnDownloadRemoved.
   CreateDownloadHistory(std::unique_ptr<InfoVector>(new InfoVector()));
@@ -874,8 +821,8 @@ TEST_F(DownloadHistoryTest, DownloadHistoryTest_UpdateWhileAdding) {
   history::DownloadRow info;
   InitBasicItem(FILE_PATH_LITERAL("/foo/bar.pdf"), "http://example.com/bar.pdf",
                 "http://example.com/referrer.html",
-                download::DownloadItem::COMPLETE, &info);
-  EXPECT_CALL(item(0), IsDone()).WillRepeatedly(Return(true));
+                download::DownloadItem::IN_PROGRESS, &info);
+
   // Instruct CreateDownload() to not callback to DownloadHistory immediately,
   // but to wait for FinishCreateDownload().
   set_slow_create_download(true);
@@ -895,13 +842,15 @@ TEST_F(DownloadHistoryTest, DownloadHistoryTest_UpdateWhileAdding) {
   // ItemAdded should call OnDownloadUpdated, which should detect that the item
   // changed while it was being added and call UpdateDownload immediately.
   info.opened = true;
-  ExpectDownloadUpdated(info, true);
+  ExpectDownloadUpdated(info, false);
 }
 
 // Test creating and updating an item with DownloadDB enabled.
 TEST_F(DownloadHistoryTest, CreateWithDownloadDB) {
   // Enable download DB.
-  SetDownloadDBEnabled(true);
+  base::test::ScopedFeatureList feature_list;
+  feature_list.InitAndEnableFeature(
+      download::features::kDownloadDBForNewDownloads);
 
   // Create a fresh item not from download DB
   CreateDownloadHistory(std::unique_ptr<InfoVector>(new InfoVector()));
@@ -927,7 +876,9 @@ TEST_F(DownloadHistoryTest, CreateWithDownloadDB) {
 // Test creating history download item that exists in DownloadDB.
 TEST_F(DownloadHistoryTest, CreateHistoryItemInDownloadDB) {
   // Enable download DB.
-  SetDownloadDBEnabled(true);
+  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",
@@ -962,7 +913,9 @@ TEST_F(DownloadHistoryTest, CreateHistoryItemInDownloadDB) {
 TEST_F(DownloadHistoryTest,
        CreateInProgressHistoryItemNonResumableInDownloadDB) {
   // Enable download DB.
-  SetDownloadDBEnabled(true);
+  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",
@@ -990,7 +943,9 @@ TEST_F(DownloadHistoryTest,
 // Test loading history download item that will be cleared by |manager_|
 TEST_F(DownloadHistoryTest, RemoveClearedItemFromHistory) {
   // Enable download DB.
-  SetDownloadDBEnabled(true);
+  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",

chrome/browser/download/download_ui_controller_unittest.cc

@@ -144,9 +144,6 @@ DownloadUIControllerTest::CreateMockInProgressDownload() {
   EXPECT_CALL(*item, IsTemporary()).WillRepeatedly(Return(false));
   EXPECT_CALL(*item, GetDownloadCreationType())
       .WillRepeatedly(Return(download::DownloadItem::TYPE_ACTIVE_DOWNLOAD));
-  EXPECT_CALL(*item, IsSavePackageDownload()).WillRepeatedly(Return(false));
-  EXPECT_CALL(*item, GetOriginalMimeType())
-      .WillRepeatedly(Return(std::string()));
   content::DownloadItemUtils::AttachInfo(item.get(), browser_context(),
                                          nullptr);
 
@@ -228,6 +225,7 @@ TEST_F(DownloadUIControllerTest, DownloadUIController_HistoryDownload) {
   EXPECT_FALSE(notified_item());
 
   // Resume the download, and it should update the UI.
+  EXPECT_CALL(*item, GetOriginalMimeType());
   EXPECT_CALL(*item, GetState())
       .WillRepeatedly(Return(download::DownloadItem::IN_PROGRESS));
   item->NotifyObserversDownloadUpdated();

components/download/public/common/download_features.cc

@@ -31,7 +31,7 @@ const base::Feature kParallelDownloading {
 };
 
 const base::Feature kDownloadDBForNewDownloads{
-    "DownloadDBForNewDownloads", base::FEATURE_ENABLED_BY_DEFAULT};
+    "DownloadDBForNewDownloads", base::FEATURE_DISABLED_BY_DEFAULT};
 
 }  // namespace features
 }  // namespace download