Track CTR of uncommon download warning.

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 14 matching lines @@
 #include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/sys_info.h"
 #include "base/test/test_file_util.h"
 #include "build/build_config.h"
 #include "chrome/app/chrome_command_ids.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/chrome_notification_types.h"
 #include "chrome/browser/download/chrome_download_manager_delegate.h"
+#include "chrome/browser/download/download_commands.h"
 #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_prefs.h"
 #include "chrome/browser/download/download_request_limiter.h"
 #include "chrome/browser/download/download_service.h"
 #include "chrome/browser/download/download_service_factory.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"
@@ skipping 23 matching lines @@
 #include "chrome/test/base/in_process_browser_test.h"
 #include "chrome/test/base/test_switches.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 "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"
 #include "content/public/browser/resource_context.h"
@@ skipping 293 matching lines @@
 
  private:
   Profile* profile_;
   bool waiting_;
   bool seen_stored_;
   FilterCallback callback_;
 
   DISALLOW_COPY_AND_ASSIGN(HistoryObserver);
 };
 
+class FakeSafeBrowsingService : public safe_browsing::SafeBrowsingService {

[asanka, 2016/03/14 18:13:46]

This the 7th FakeSafeBrowsingService :-(. Not necessarily for this CL, but we
really should make this a test fixture under chrome/browser/safe_browsing/.

[Jialiu Lin, 2016/03/14 23:54:04]

Totally agree. Bug created at https://crbug.com/594683

+ public:
+  FakeSafeBrowsingService() {}
+
+  std::string GetDownloadReport() const { return report_; }
+
+ protected:
+  ~FakeSafeBrowsingService() override {}
+
+  void SendSerializedDownloadReport(const std::string& report) override {
+    // Disable download protection to prevent flackiness.

[asanka, 2016/03/14 18:13:46]

*flakiness.

Also, you'll need to explain how disabling the download protection service at
this point prevents flakiness.

[Jialiu Lin, 2016/03/14 23:54:04]

No need of this line since FakeDownloadProtectionService is used. 

+    download_protection_service()->SetEnabled(false);
+    report_ = report;
+  }
+
+  std::string report_;
+};
+
+// Factory that creates FakeSafeBrowsingService instances.
+class TestSafeBrowsingServiceFactory
+    : public safe_browsing::SafeBrowsingServiceFactory {
+ public:
+  TestSafeBrowsingServiceFactory() : fake_safe_browsing_service_(nullptr) {}
+  ~TestSafeBrowsingServiceFactory() override {}
+
+  safe_browsing::SafeBrowsingService* CreateSafeBrowsingService() override {
+    if (!fake_safe_browsing_service_) {

[asanka, 2016/03/14 18:13:46]

This function should be getting called at most once per browser session. It's
acceptable to verify it and rely on it. I.e.
DCHECK(!fake_safe_browsing_service_) and always create one. Otherwise it appears
from this test that there's uncertainty about whether multiple safe browsing
service instances could be created.

[Jialiu Lin, 2016/03/14 23:54:04]

Done.

+      fake_safe_browsing_service_ = new FakeSafeBrowsingService();
+    }
+    return fake_safe_browsing_service_.get();
+  }
+
+  scoped_refptr<FakeSafeBrowsingService> fake_safe_browsing_service() {
+    return fake_safe_browsing_service_;
+  }
+
+ private:
+  scoped_refptr<FakeSafeBrowsingService> fake_safe_browsing_service_;
+};
+
 class DownloadTest : public InProcessBrowserTest {
  public:
   // Choice of navigation or direct fetch.  Used by |DownloadFileCheckErrors()|.
   enum DownloadMethod {
     DOWNLOAD_NAVIGATE,
     DOWNLOAD_DIRECT
   };
 
   // Information passed in to |DownloadFileCheckErrors()|.
   struct DownloadInfo {
     const char* starting_url;           // URL for initiating the download.
     const char* expected_download_url;  // Expected value of DI::GetURL(). Can
                                         // be different if |starting_url|
                                         // initiates a download from another
                                         // URL.
     DownloadMethod download_method;     // Navigation or Direct.
     // Download interrupt reason (NONE is OK).
     content::DownloadInterruptReason reason;
     bool show_download_item;  // True if the download item appears on the shelf.
     bool should_redirect_to_documents;  // True if we save it in "My Documents".
   };
 
   struct FileErrorInjectInfo {
     DownloadInfo download_info;
     content::TestFileErrorInjector::FileErrorInfo error_info;
   };
 
-  DownloadTest() {}
+  DownloadTest()
+      : test_safe_browsing_factory_(new TestSafeBrowsingServiceFactory()) {}
+
+  void SetUp() override {
+    safe_browsing::SafeBrowsingService::RegisterFactory(
+        test_safe_browsing_factory_.get());
+    InProcessBrowserTest::SetUp();
+  }
+
+  void TearDown() override {
+    safe_browsing::SafeBrowsingService::RegisterFactory(nullptr);
+    InProcessBrowserTest::TearDown();
+  }
 
   void SetUpOnMainThread() override {
     base::FeatureList::ClearInstanceForTesting();
     scoped_ptr<base::FeatureList> feature_list(new base::FeatureList);
     feature_list->InitializeFromCommandLine(
         features::kDownloadResumption.name, std::string());
     base::FeatureList::SetInstance(std::move(feature_list));
     BrowserThread::PostTask(
         BrowserThread::IO, FROM_HERE,
         base::Bind(&chrome_browser_net::SetUrlRequestMocksEnabled, true));
@@ skipping 623 matching lines @@
     if (downloads.size() != 1)
       return NULL;
 
     error_injector->ClearError();
     DownloadItem* download = downloads[0];
     EXPECT_EQ(DownloadItem::INTERRUPTED, download->GetState());
     EXPECT_EQ(error, download->GetLastReason());
     return download;
   }
 
+ protected:
+  scoped_ptr<TestSafeBrowsingServiceFactory> test_safe_browsing_factory_;
+
  private:
   static void EnsureNoPendingDownloadJobsOnIO(bool* result) {
     if (net::URLRequestSlowDownloadJob::NumberOutstandingRequests())
       *result = false;
     BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
                             base::MessageLoop::QuitWhenIdleClosure());
   }
 
   // Location of the test data.
   base::FilePath test_dir_;
@@ skipping 2191 matching lines @@
   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]);
   std::vector<DownloadItem*> updated_downloads;
   GetDownloads(browser(), &updated_downloads);
   ASSERT_TRUE(updated_downloads.empty());
 }
+
+IN_PROC_BROWSER_TEST_F(DownloadTest, SendUncommonDownloadAcceptReport) {
+  browser()->profile()->GetPrefs()->SetBoolean(prefs::kSafeBrowsingEnabled,
+                                               true);
+  // Make a dangerous file.
+  GURL download_url(
+      net::URLRequestMockHTTPJob::GetMockUrl(kDangerousMockFilePath));
+  scoped_ptr<content::DownloadTestObserver> dangerous_observer(
+      DangerousDownloadWaiter(
+          browser(), 1,
+          content::DownloadTestObserver::ON_DANGEROUS_DOWNLOAD_QUIT));
+  scoped_ptr<content::DownloadTestObserver> in_progress_observer(
+      new content::DownloadTestObserverInProgress(
+          DownloadManagerForBrowser(browser()), 1));
+
+  ui_test_utils::NavigateToURL(browser(), download_url);
+  in_progress_observer->WaitForFinished();
+
+  std::vector<DownloadItem*> downloads;
+  DownloadManagerForBrowser(browser())->GetAllDownloads(&downloads);
+  ASSERT_EQ(1u, downloads.size());
+  DownloadItem* download = downloads[0];
+  // Simulates an uncommon download.
+  download->OnContentCheckCompleted(

[asanka, 2016/03/14 18:13:46]

OnContentCheckCompleted cannot be issued until all the data has been saved. The
test will crash if OnAllDataSaved() hasn't been called prior to this point.
Since the in_progress_observer doesn't enforce it, we may end up being flaky.
:-(

In decreasing order of preference, here's what I think should happen instead:

 *  Make DownloadProtectionService flag the download as UNCOMMON. One approach
would be to introduce a new FakeDownloadProtectionService that could be returned
by FakeSafeBrowsingService::download_protection_service(). In response to
FakeDownloadProtectionService::CheckClientDownload(), it can respond with a
verdict of UNCOMMON. Then you can wait for the dangerous_observer to complete
without calling OnContentCheckCompleted(). You can call
DownloadCommands::ExecuteCommand(KEEP) after this.

 *  Failing that, add a DownloadTestObserver derivative that waits for
DownloadItem::AllDataSaved() to be true. Since the download is expected to be
marked as dangerous, receiving all the data will not result in the download
being marked as complete. Once AllDataSaved() becomes true, you can safely call
OnContentCheckCompleted(). You only need the DownloadAllDataSavedObserver (which
you wait on before calling OnCOntentCheckCompleted) and a
DownloadTestObserverTerminal that you wait on at the end to make sure that the
download runs to completion.

[Jialiu Lin, 2016/03/14 23:54:04]

implemented based on option 1 here. Now , no need to create an
in_progress_observer. 

+      content::DownloadDangerType::DOWNLOAD_DANGER_TYPE_UNCOMMON_CONTENT);
+  ASSERT_EQ(download->GetDangerType(),
+            content::DownloadDangerType::DOWNLOAD_DANGER_TYPE_UNCOMMON_CONTENT);
+  DownloadCommands(download).ExecuteCommand(DownloadCommands::KEEP);
+  dangerous_observer->WaitForFinished();
+
+  safe_browsing::ClientSafeBrowsingReportRequest actual_report;
+  actual_report.ParseFromString(
+      test_safe_browsing_factory_->fake_safe_browsing_service()
+          ->GetDownloadReport());
+  EXPECT_EQ(safe_browsing::ClientSafeBrowsingReportRequest::
+                DANGEROUS_DOWNLOAD_WARNING,
+            actual_report.type());
+  EXPECT_EQ(safe_browsing::ClientDownloadResponse::UNCOMMON,
+            actual_report.download_verdict());
+  EXPECT_EQ(download->GetURL().spec(), actual_report.url());
+  EXPECT_TRUE(actual_report.did_proceed());
+
+  download->Cancel(true);
+}
 #endif  // FULL_SAFE_BROWSING
 
 class DownloadTestWithShelf : public DownloadTest {
   void SetUpCommandLine(base::CommandLine* command_line) override {
 #if defined(OS_CHROMEOS)
     command_line->AppendSwitch(switches::kDisableDownloadNotification);
 #endif
     DownloadTest::SetUpCommandLine(command_line);
   }
 };
@@ skipping 242 matching lines @@
 
   scoped_ptr<content::DownloadTestObserver> observer(DangerousDownloadWaiter(
       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());
 }