Download Feedback Service should upload all eligible downloads

chrome/browser/download/download_browsertest.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/download/download_browsertest.h"
 
 #include <stdint.h>
 #include <sstream>
 #include <utility>
 
@@ skipping 57 matching lines @@
 #include "chrome/grit/generated_resources.h"
 #include "chrome/test/base/in_process_browser_test.h"
 #include "chrome/test/base/ui_test_utils.h"
 #include "components/history/content/browser/download_constants_utils.h"
 #include "components/history/core/browser/download_constants.h"
 #include "components/history/core/browser/download_row.h"
 #include "components/history/core/browser/history_service.h"
 #include "components/infobars/core/confirm_infobar_delegate.h"
 #include "components/infobars/core/infobar.h"
 #include "components/prefs/pref_service.h"
+#include "components/safe_browsing_db/safe_browsing_prefs.h"
 #include "content/public/browser/download_danger_type.h"
 #include "content/public/browser/download_interrupt_reasons.h"
 #include "content/public/browser/download_item.h"
 #include "content/public/browser/download_manager.h"
 #include "content/public/browser/download_save_info.h"
 #include "content/public/browser/download_url_parameters.h"
 #include "content/public/browser/notification_source.h"
 #include "content/public/browser/render_frame_host.h"
 #include "content/public/browser/render_view_host.h"
 #include "content/public/browser/render_widget_host.h"
@@ skipping 3215 matching lines @@
   ASSERT_EQ(1u, downloads.size());
 
   DownloadItem* download = downloads[0];
   EXPECT_FALSE(download->IsDangerous());
   EXPECT_EQ(content::DOWNLOAD_DANGER_TYPE_MAYBE_DANGEROUS_CONTENT,
             download->GetDangerType());
 
   download->Cancel(true);
 }
 
-IN_PROC_BROWSER_TEST_F(DownloadTest, FeedbackService) {
+IN_PROC_BROWSER_TEST_F(DownloadTest, FeedbackServiceDiscardDownload) {
+  PrefService* prefs = browser()->profile()->GetPrefs();
+  prefs->SetBoolean(prefs::kSafeBrowsingEnabled, true);
+  safe_browsing::SetExtendedReportingPref(prefs, true);
+
   // Make a dangerous file.
   GURL download_url(net::URLRequestMockHTTPJob::GetMockUrl(
       "downloads/dangerous/dangerous.swf"));
   std::unique_ptr<content::DownloadTestObserverInterrupted> observer(
       new content::DownloadTestObserverInterrupted(
           DownloadManagerForBrowser(browser()), 1,
           content::DownloadTestObserver::ON_DANGEROUS_DOWNLOAD_QUIT));
   ui_test_utils::NavigateToURLWithDisposition(
       browser(), GURL(download_url), WindowOpenDisposition::NEW_BACKGROUND_TAB,
       ui_test_utils::BROWSER_TEST_NONE);
@@ skipping 23 matching lines @@
   download_protection_service->feedback_service()->MaybeStorePingsForDownload(
       safe_browsing::DownloadProtectionService::UNCOMMON,
       downloads[0],
       ping_request,
       ping_response);
   ASSERT_TRUE(safe_browsing::DownloadFeedbackService::IsEnabledForDownload(
       *(downloads[0])));
 
   // Begin feedback and check that the file is "stolen".
   download_protection_service->feedback_service()->BeginFeedbackForDownload(
-      downloads[0]);
+      downloads[0], DownloadCommands::DISCARD);
   std::vector<DownloadItem*> updated_downloads;
   GetDownloads(browser(), &updated_downloads);
   ASSERT_TRUE(updated_downloads.empty());
 }
 
+IN_PROC_BROWSER_TEST_F(DownloadTest, FeedbackServiceKeepDownload) {
+  PrefService* prefs = browser()->profile()->GetPrefs();
+  prefs->SetBoolean(prefs::kSafeBrowsingEnabled, true);
+  safe_browsing::SetExtendedReportingPref(prefs, true);
+  EnableFileChooser(true);
+  // Make a dangerous file.
+  GURL download_url(net::URLRequestMockHTTPJob::GetMockUrl(
+      "downloads/dangerous/dangerous.swf"));
+  std::unique_ptr<content::DownloadTestObserverInterrupted>
+  interruption_observer(
+      new content::DownloadTestObserverInterrupted(
+          DownloadManagerForBrowser(browser()), 1,
+          content::DownloadTestObserver::ON_DANGEROUS_DOWNLOAD_QUIT));
+  std::unique_ptr<content::DownloadTestObserver> completion_observer(
+      new content::DownloadTestObserverTerminal(
+          DownloadManagerForBrowser(browser()), 1,
+          content::DownloadTestObserver::ON_DANGEROUS_DOWNLOAD_QUIT));
+  ui_test_utils::NavigateToURLWithDisposition(
+      browser(), GURL(download_url), WindowOpenDisposition::NEW_BACKGROUND_TAB,
+      ui_test_utils::BROWSER_TEST_NONE);
+  interruption_observer->WaitForFinished();
+
+  // Get the download from the DownloadManager.
+  std::vector<DownloadItem*> downloads;
+  DownloadManagerForBrowser(browser())->GetAllDownloads(&downloads);
+  ASSERT_EQ(1u, downloads.size());
+  EXPECT_TRUE(downloads[0]->IsDangerous());
+
+  // Save fake pings for the download.
+  safe_browsing::ClientDownloadReport fake_metadata;
+  fake_metadata.mutable_download_request()->set_url("http://test");
+  fake_metadata.mutable_download_request()->set_length(1);
+  fake_metadata.mutable_download_request()->mutable_digests()->set_sha1("hi");
+  fake_metadata.mutable_download_response()->set_verdict(
+      safe_browsing::ClientDownloadResponse::UNCOMMON);
+  std::string ping_request(
+      fake_metadata.download_request().SerializeAsString());
+  std::string ping_response(
+      fake_metadata.download_response().SerializeAsString());
+  safe_browsing::SafeBrowsingService* sb_service =
+      g_browser_process->safe_browsing_service();
+  safe_browsing::DownloadProtectionService* download_protection_service =
+      sb_service->download_protection_service();
+  download_protection_service->feedback_service()->MaybeStorePingsForDownload(
+      safe_browsing::DownloadProtectionService::UNCOMMON, downloads[0],
+      ping_request, ping_response);
+  ASSERT_TRUE(safe_browsing::DownloadFeedbackService::IsEnabledForDownload(
+      *(downloads[0])));
+
+  // Begin feedback and check that file is still there.
+  download_protection_service->feedback_service()->BeginFeedbackForDownload(
+      downloads[0], DownloadCommands::KEEP);
+  // Wait for download to complete.
+  completion_observer->WaitForFinished();
+
+  std::vector<DownloadItem*> updated_downloads;
+  GetDownloads(browser(), &updated_downloads);
+  ASSERT_FALSE(updated_downloads.empty());

[asanka, 2016/11/07 16:01:55]

Should assert that there's exactly one download. Otherwise the property
assertions that follow won't be reliable.

[Jialiu Lin, 2016/11/14 20:57:39]

Done.

+  ASSERT_FALSE(updated_downloads[0]->IsDangerous());
+  ASSERT_TRUE(PathExists(updated_downloads[0]->GetFullPath()));
+}

[asanka, 2016/11/07 16:01:54]

I'm okay with the tests as is. But ideally we'd test against a throttled upload
server and verify that we can safely delete the file at download->GetFullPath()
without the upload process getting interrupted. :-)

I don't know if the reliability of the upload mechanism is tested elsewhere, but
its resilience against the download target file getting removed is a new
requirement. Perhaps we should add such tests as followups? I'm worried that we
can regress on this aspect without any tests to catch it.

[Jialiu Lin, 2016/11/14 20:57:39]

acknowledged.
I agree. We need more tests to verify the reliability. Since I'm trying to get
this change in M56 (branch on Nov 17), I'll probably defer this to a later CL.
And also as pkasting@ suggested, if this temp file solution works out fine, we
can also use this mechanism for discarded downloads, thus unify the logic of
handling file uploading. I created crbug.com/665100 as a reminder of these two
follow-ups. 

+
 IN_PROC_BROWSER_TEST_F(
-     DownloadTestWithFakeSafeBrowsing,
-     SendUncommonDownloadReportIfUserProceed) {
+    DownloadTestWithFakeSafeBrowsing,
+    SendUncommonDownloadReportIfUserProceed) {
   browser()->profile()->GetPrefs()->SetBoolean(prefs::kSafeBrowsingEnabled,
                                                true);
   // Make a dangerous file.
   GURL download_url(
       net::URLRequestMockHTTPJob::GetMockUrl(kDangerousMockFilePath));
   std::unique_ptr<content::DownloadTestObserver> dangerous_observer(
       DangerousDownloadWaiter(
           browser(), 1,
           content::DownloadTestObserver::ON_DANGEROUS_DOWNLOAD_QUIT));
   ui_test_utils::NavigateToURL(browser(), download_url);
@@ skipping 290 matching lines @@
           browser(), 1,
           content::DownloadTestObserver::ON_DANGEROUS_DOWNLOAD_DENY));
   ui_test_utils::NavigateToURL(browser(), extension_url);
 
   observer->WaitForFinished();
 
   // Download shelf should close.
   EXPECT_FALSE(browser()->window()->IsDownloadShelfVisible());
 }
 #endif  // defined(OS_CHROMEOS)