Sample 1% url whitelisted PPAPI downloads to ping safe browsing server

chrome/browser/safe_browsing/download_protection_service_unittest.cc

Index: chrome/browser/safe_browsing/download_protection_service_unittest.cc
diff --git a/chrome/browser/safe_browsing/download_protection_service_unittest.cc b/chrome/browser/safe_browsing/download_protection_service_unittest.cc
index 5f2c85cb21706d07094e90281e49b574ff1df2c1..fd130088645804a484ca3a109f58aeef2e2a67a4 100644
--- a/chrome/browser/safe_browsing/download_protection_service_unittest.cc
+++ b/chrome/browser/safe_browsing/download_protection_service_unittest.cc
@@ -556,8 +556,7 @@ void DownloadProtectionServiceTest::CheckClientDownloadReportCorruptArchive(
       &factory, ClientDownloadResponse::SAFE, net::HTTP_OK,
       net::URLRequestStatus::SUCCESS);
 
-  profile_->GetPrefs()->SetBoolean(prefs::kSafeBrowsingExtendedReportingEnabled,
-                                   is_extended_reporting);
+  SetExtendedReportingPreference(is_extended_reporting);
   content::MockDownloadItem item;
   if (type == ZIP) {
     PrepareBasicDownloadItem(&item, {"http://www.evil.com/a.zip"},  // url_chain
@@ -1987,8 +1986,10 @@ TEST_F(DownloadProtectionServiceTest, PPAPIDownloadRequest_Unsupported) {
   base::FilePath default_file_path(FILE_PATH_LITERAL("/foo/bar/test.txt"));
   std::vector<base::FilePath::StringType> alternate_extensions{
       FILE_PATH_LITERAL(".tmp"), FILE_PATH_LITERAL(".asdfasdf")};
+  SetExtendedReportingPreference(false);
   download_service_->CheckPPAPIDownloadRequest(
       GURL("http://example.com/foo"), default_file_path, alternate_extensions,
+      profile_.get(),
       base::Bind(&DownloadProtectionServiceTest::SyncCheckDoneCallback,
                  base::Unretained(this)));
   ASSERT_TRUE(IsResult(DownloadProtectionService::SAFE));
@@ -2019,8 +2020,10 @@ TEST_F(DownloadProtectionServiceTest, PPAPIDownloadRequest_SupportedDefault) {
     factory.ClearFakeResponses();
     PrepareResponse(&factory, test_case.verdict, net::HTTP_OK,
                     net::URLRequestStatus::SUCCESS);
+    SetExtendedReportingPreference(false);
     download_service_->CheckPPAPIDownloadRequest(
         GURL("http://example.com/foo"), default_file_path, alternate_extensions,
+        profile_.get(),
         base::Bind(&DownloadProtectionServiceTest::CheckDoneCallback,
                    base::Unretained(this)));
     base::RunLoop().Run();
@@ -2038,8 +2041,10 @@ TEST_F(DownloadProtectionServiceTest, PPAPIDownloadRequest_SupportedAlternate) {
   EXPECT_CALL(*sb_service_->mock_database_manager(),
               MatchDownloadWhitelistUrl(_))
       .WillRepeatedly(Return(false));
+  SetExtendedReportingPreference(false);
   download_service_->CheckPPAPIDownloadRequest(
       GURL("http://example.com/foo"), default_file_path, alternate_extensions,
+      profile_.get(),
       base::Bind(&DownloadProtectionServiceTest::CheckDoneCallback,
                  base::Unretained(this)));
   base::RunLoop().Run();
@@ -2047,22 +2052,82 @@ TEST_F(DownloadProtectionServiceTest, PPAPIDownloadRequest_SupportedAlternate) {
   ASSERT_TRUE(IsResult(DownloadProtectionService::DANGEROUS));
 }
 
-TEST_F(DownloadProtectionServiceTest, PPAPIDownloadRequest_WhitelistedURL) {
+TEST_F(DownloadProtectionServiceTest,
+       PPAPIDownloadRequest_WhitelistedURLNotExtendedReporting) {
   net::FakeURLFetcherFactory factory(nullptr);
   base::FilePath default_file_path(FILE_PATH_LITERAL("/foo/bar/test.crx"));
   std::vector<base::FilePath::StringType> alternate_extensions;
+  // Configs "server" side response to DANGEROUS, in case a request is send out.
+  PrepareResponse(&factory, ClientDownloadResponse::DANGEROUS, net::HTTP_OK,
+                  net::URLRequestStatus::SUCCESS);
+  EXPECT_CALL(*sb_service_->mock_database_manager(),
+              MatchDownloadWhitelistUrl(_))
+      .WillRepeatedly(Return(true));
+  SetExtendedReportingPreference(false);
+  // Sets the sample rate to 1.00.
+  SetWhitelistedDownloadSampleRate(1.00);
+  download_service_->CheckPPAPIDownloadRequest(
+      GURL("http://example.com/foo"), default_file_path, alternate_extensions,
+      profile_.get(),
+      base::Bind(&DownloadProtectionServiceTest::CheckDoneCallback,
+                 base::Unretained(this)));
+  base::RunLoop().Run();

[asanka, 2016/07/15 14:59:52]

Perhaps not for this CL since it's going to involve changing all the tests that
use DownloadProtectionService::CheckDoneCallback().

The recommended pattern for running message loops during tests is to:

1. Pick a point in the test where whatever the test was waiting for has
happened, and make the test invoke base::RunLoop().QuitClosure() at that point.
2. Invoke base::RunLoop().Run().

This way, the test doesn't need to worry about some other test method invoking a
nested run loop. In addition the test won't be sensitive to any stray tasks that
may also be scheduled on the same message loop.

The call site would look like:

   base::RunLoop run_loop;
   download_service_->CheckPPAPI...(base::Bind(&CheckDoneCallback,
base:Unretained(this), run_loop.QuitClosure());
   run_loop.Run();

CheckDoneCallback would take a base::Closure parameter that can be invoked to
break out of the correct run loop instead of invoking
MessageLoop::current()->QuitWhenIdle().

[Jialiu Lin, 2016/07/15 22:10:56]

Thanks for your detailed explanation on RunLoop. I have refactored all the tests
accordingly. No more MessageLoop needed.

+
+  // Result should be SAFE, since download matches whitelist but user is not in
+  // extended reporting group (a.k.a no request sents out).
+  ASSERT_TRUE(IsResult(DownloadProtectionService::SAFE));
+}
+
+TEST_F(DownloadProtectionServiceTest,
+       PPAPIDownloadRequest_WhitelistedURLIncognito) {
+  net::FakeURLFetcherFactory factory(nullptr);
+  base::FilePath default_file_path(FILE_PATH_LITERAL("/foo/bar/test.crx"));
+  std::vector<base::FilePath::StringType> alternate_extensions;
+  // Configs "server" side response to DANGEROUS, in case a request is send out.
+  PrepareResponse(&factory, ClientDownloadResponse::DANGEROUS, net::HTTP_OK,
+                  net::URLRequestStatus::SUCCESS);
   EXPECT_CALL(*sb_service_->mock_database_manager(),
               MatchDownloadWhitelistUrl(_))
       .WillRepeatedly(Return(true));
+  SetExtendedReportingPreference(true);
   download_service_->CheckPPAPIDownloadRequest(
       GURL("http://example.com/foo"), default_file_path, alternate_extensions,
+      profile_->GetOffTheRecordProfile(),
       base::Bind(&DownloadProtectionServiceTest::CheckDoneCallback,
                  base::Unretained(this)));
   base::RunLoop().Run();
 
+  // Result should still be SAFE, since download matches whitelist and user is
+  // in incognito mode.
   ASSERT_TRUE(IsResult(DownloadProtectionService::SAFE));
 }
 
+TEST_F(DownloadProtectionServiceTest,
+       PPAPIDownloadRequest_SampleWhitelistedURL) {
+  net::FakeURLFetcherFactory factory(nullptr);
+  base::FilePath default_file_path(FILE_PATH_LITERAL("/foo/bar/test.crx"));
+  std::vector<base::FilePath::StringType> alternate_extensions;
+  // Configs "server" side response to DANGEROUS.
+  PrepareResponse(&factory, ClientDownloadResponse::DANGEROUS, net::HTTP_OK,
+                  net::URLRequestStatus::SUCCESS);
+  EXPECT_CALL(*sb_service_->mock_database_manager(),
+              MatchDownloadWhitelistUrl(_))
+      .WillRepeatedly(Return(true));
+  // Sets the sample rate to 1.00 to trigger ClientDownloadRequest.
+  SetWhitelistedDownloadSampleRate(1.00);
+  SetExtendedReportingPreference(true);
+  download_service_->CheckPPAPIDownloadRequest(
+      GURL("http://example.com/foo"), default_file_path, alternate_extensions,
+      profile_.get(),
+      base::Bind(&DownloadProtectionServiceTest::CheckDoneCallback,
+                 base::Unretained(this)));
+  base::RunLoop().Run();
+
+  // Result should be Dangerous, since we will sample this download and ping SB
+  // Server (server returns DANGEROUS verdict).
+  ASSERT_TRUE(IsResult(DownloadProtectionService::DANGEROUS));
+}
+
 TEST_F(DownloadProtectionServiceTest, PPAPIDownloadRequest_FetchFailed) {
   net::FakeURLFetcherFactory factory(nullptr);
   base::FilePath default_file_path(FILE_PATH_LITERAL("/foo/bar/test.crx"));
@@ -2072,8 +2137,10 @@ TEST_F(DownloadProtectionServiceTest, PPAPIDownloadRequest_FetchFailed) {
   EXPECT_CALL(*sb_service_->mock_database_manager(),
               MatchDownloadWhitelistUrl(_))
       .WillRepeatedly(Return(false));
+  SetExtendedReportingPreference(false);
   download_service_->CheckPPAPIDownloadRequest(
       GURL("http://example.com/foo"), default_file_path, alternate_extensions,
+      profile_.get(),
       base::Bind(&DownloadProtectionServiceTest::CheckDoneCallback,
                  base::Unretained(this)));
   base::RunLoop().Run();
@@ -2091,8 +2158,10 @@ TEST_F(DownloadProtectionServiceTest, PPAPIDownloadRequest_InvalidResponse) {
   EXPECT_CALL(*sb_service_->mock_database_manager(),
               MatchDownloadWhitelistUrl(_))
       .WillRepeatedly(Return(false));
+  SetExtendedReportingPreference(false);
   download_service_->CheckPPAPIDownloadRequest(
       GURL("http://example.com/foo"), default_file_path, alternate_extensions,
+      profile_.get(),
       base::Bind(&DownloadProtectionServiceTest::CheckDoneCallback,
                  base::Unretained(this)));
   base::RunLoop().Run();
@@ -2110,8 +2179,10 @@ TEST_F(DownloadProtectionServiceTest, PPAPIDownloadRequest_Timeout) {
   PrepareResponse(&factory, ClientDownloadResponse::SAFE, net::HTTP_OK,
                   net::URLRequestStatus::SUCCESS);
   download_service_->download_request_timeout_ms_ = 0;
+  SetExtendedReportingPreference(false);
   download_service_->CheckPPAPIDownloadRequest(
       GURL("http://example.com/foo"), default_file_path, alternate_extensions,
+      profile_.get(),
       base::Bind(&DownloadProtectionServiceTest::CheckDoneCallback,
                  base::Unretained(this)));
   base::RunLoop().Run();
@@ -2170,8 +2241,9 @@ TEST_F(DownloadProtectionServiceTest, PPAPIDownloadRequest_Payload) {
   PrepareResponse(&factory, ClientDownloadResponse::SAFE, net::HTTP_OK,
                   net::URLRequestStatus::SUCCESS);
   const GURL kRequestorUrl("http://example.com/foo");
+  SetExtendedReportingPreference(false);
   download_service_->CheckPPAPIDownloadRequest(
-      kRequestorUrl, default_file_path, alternate_extensions,
+      kRequestorUrl, default_file_path, alternate_extensions, profile_.get(),
       base::Bind(&DownloadProtectionServiceTest::CheckDoneCallback,
                  base::Unretained(this)));
   base::RunLoop().Run();