Include high-fidelity metadata about a download in incident reports.

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 9aed0ae9f8e86c9fca16856e0f144d78327e58f4..b8fc84087ac89cd284a32e5fd82fa8c9ecd46c4c 100644
--- a/chrome/browser/safe_browsing/download_protection_service_unittest.cc
+++ b/chrome/browser/safe_browsing/download_protection_service_unittest.cc
@@ -217,6 +217,10 @@ class DownloadProtectionServiceTest : public testing::Test {
     download_service_ = sb_service_->download_protection_service();
     download_service_->binary_feature_extractor_ = binary_feature_extractor_;
     download_service_->SetEnabled(true);
+    client_download_request_subscription_ =
+        download_service_->RegisterClientDownloadRequestCallback(
+            base::Bind(&DownloadProtectionServiceTest::OnClientDownloadRequest,
+                       base::Unretained(this)));
     base::RunLoop().RunUntilIdle();
     has_result_ = false;
 
@@ -231,6 +235,7 @@ class DownloadProtectionServiceTest : public testing::Test {
   }
 
   virtual void TearDown() {
+    client_download_request_subscription_.reset();
     sb_service_->ShutDown();
     // Flush all of the thread message loops to ensure that there are no
     // tasks currently running.
@@ -300,6 +305,12 @@ class DownloadProtectionServiceTest : public testing::Test {
     return certs.empty() ? NULL : certs[0];
   }
 
+  bool HasClientDownloadRequest() const {
+    return last_client_download_request_.get() != NULL;
+  }
+
+  void ClearClientDownloadRequest() { last_client_download_request_.reset(); }
+
  private:
   // Helper functions for FlushThreadMessageLoops.
   void RunAllPendingAndQuitUI() {
@@ -332,6 +343,14 @@ class DownloadProtectionServiceTest : public testing::Test {
     MessageLoop::current()->Run();
   }
 
+  void OnClientDownloadRequest(content::DownloadItem* download,
+                               const ClientDownloadRequest* request) {
+    if (request)
+      last_client_download_request_.reset(new ClientDownloadRequest(*request));
+    else
+      last_client_download_request_.reset();
+  }
+
  public:
   void CheckDoneCallback(
       DownloadProtectionService::DownloadCheckResult result) {
@@ -373,6 +392,9 @@ class DownloadProtectionServiceTest : public testing::Test {
 #if defined(OS_MACOSX)
   scoped_ptr<base::FieldTrialList> field_trial_list_;
 #endif
+  DownloadProtectionService::ClientDownloadRequestSubscription
+      client_download_request_subscription_;
+  scoped_ptr<ClientDownloadRequest> last_client_download_request_;
 };
 
 TEST_F(DownloadProtectionServiceTest, CheckClientDownloadInvalidUrl) {
@@ -395,6 +417,7 @@ TEST_F(DownloadProtectionServiceTest, CheckClientDownloadInvalidUrl) {
                  base::Unretained(this)));
   MessageLoop::current()->Run();
   EXPECT_TRUE(IsResult(DownloadProtectionService::UNKNOWN));
+  EXPECT_FALSE(HasClientDownloadRequest());
   Mock::VerifyAndClearExpectations(&item);
 
   url_chain.push_back(GURL("file://www.google.com/"));
@@ -411,6 +434,7 @@ TEST_F(DownloadProtectionServiceTest, CheckClientDownloadInvalidUrl) {
                  base::Unretained(this)));
   MessageLoop::current()->Run();
   EXPECT_TRUE(IsResult(DownloadProtectionService::UNKNOWN));
+  EXPECT_FALSE(HasClientDownloadRequest());
 }
 
 TEST_F(DownloadProtectionServiceTest, CheckClientDownloadNotABinary) {
@@ -434,6 +458,7 @@ TEST_F(DownloadProtectionServiceTest, CheckClientDownloadNotABinary) {
                  base::Unretained(this)));
   MessageLoop::current()->Run();
   EXPECT_TRUE(IsResult(DownloadProtectionService::UNKNOWN));
+  EXPECT_FALSE(HasClientDownloadRequest());
 }
 
 TEST_F(DownloadProtectionServiceTest, CheckClientDownloadWhitelistedUrl) {
@@ -488,8 +513,17 @@ TEST_F(DownloadProtectionServiceTest, CheckClientDownloadWhitelistedUrl) {
   MessageLoop::current()->Run();
 #if defined(OS_WIN)
   EXPECT_TRUE(IsResult(DownloadProtectionService::DANGEROUS));
+  EXPECT_TRUE(HasClientDownloadRequest());
+  ClearClientDownloadRequest();
 #else
   EXPECT_TRUE(IsResult(DownloadProtectionService::UNKNOWN));
+#if defined(OS_MACOSX)

[mattm, 2014/10/24 02:11:10]

These nested/repetitious ifdefs are a little ick. Maybe a separate condition
like
if WIN || MAC
EXPECT_TRUE(Hasblah())
clearblah()
else
EXPECT_FALSE(hasblah())
endif

[grt (UTC plus 2), 2014/10/30 19:07:35]

Looks better. Done.

+  // OSX sends pings for evaluation purposes.
+  EXPECT_TRUE(HasClientDownloadRequest());
+  ClearClientDownloadRequest();
+#else
+  EXPECT_FALSE(HasClientDownloadRequest());
+#endif
 #endif
 
   // Check that the referrer is not matched against the whitelist.
@@ -501,8 +535,17 @@ TEST_F(DownloadProtectionServiceTest, CheckClientDownloadWhitelistedUrl) {
   MessageLoop::current()->Run();
 #if defined(OS_WIN)
   EXPECT_TRUE(IsResult(DownloadProtectionService::DANGEROUS));
+  EXPECT_TRUE(HasClientDownloadRequest());
+  ClearClientDownloadRequest();
 #else
   EXPECT_TRUE(IsResult(DownloadProtectionService::UNKNOWN));
+#if defined(OS_MACOSX)
+  // OSX sends pings for evaluation purposes.
+  EXPECT_TRUE(HasClientDownloadRequest());
+  ClearClientDownloadRequest();
+#else
+  EXPECT_FALSE(HasClientDownloadRequest());
+#endif
 #endif
 
   // Redirect from a site shouldn't be checked either.
@@ -514,8 +557,17 @@ TEST_F(DownloadProtectionServiceTest, CheckClientDownloadWhitelistedUrl) {
   MessageLoop::current()->Run();
 #if defined(OS_WIN)
   EXPECT_TRUE(IsResult(DownloadProtectionService::DANGEROUS));
+  EXPECT_TRUE(HasClientDownloadRequest());
+  ClearClientDownloadRequest();
 #else
   EXPECT_TRUE(IsResult(DownloadProtectionService::UNKNOWN));
+#if defined(OS_MACOSX)
+  // OSX sends pings for evaluation purposes.
+  EXPECT_TRUE(HasClientDownloadRequest());
+  ClearClientDownloadRequest();
+#else
+  EXPECT_FALSE(HasClientDownloadRequest());
+#endif
 #endif
 
   // Only if the final url is whitelisted should it be SAFE.
@@ -527,8 +579,19 @@ TEST_F(DownloadProtectionServiceTest, CheckClientDownloadWhitelistedUrl) {
   MessageLoop::current()->Run();
 #if defined(OS_MACOSX)
   EXPECT_TRUE(IsResult(DownloadProtectionService::UNKNOWN));
+  EXPECT_FALSE(HasClientDownloadRequest());
 #else
   EXPECT_TRUE(IsResult(DownloadProtectionService::SAFE));
+  EXPECT_FALSE(HasClientDownloadRequest());
+#if defined(OS_MACOSX)
+  // OSX sends pings for evaluation purposes.
+  EXPECT_TRUE(HasClientDownloadRequest());
+  ClearClientDownloadRequest();
+#else
+  // TODO(grt): Make the service produce the request even when the URL is
+  // whitelisted.
+  EXPECT_FALSE(HasClientDownloadRequest());
+#endif
 #endif
 }
 
@@ -618,10 +681,19 @@ TEST_F(DownloadProtectionServiceTest, CheckClientDownloadSuccess) {
   MessageLoop::current()->Run();
 #if defined(OS_WIN)
   EXPECT_TRUE(IsResult(DownloadProtectionService::SAFE));
+  EXPECT_TRUE(HasClientDownloadRequest());
+  ClearClientDownloadRequest();
 #else
-  // On !OS_WIN, no file types are currently supported. Hence all erquests to
+  // On !OS_WIN, no file types are currently supported. Hence all requests to
   // CheckClientDownload() result in a verdict of UNKNOWN.
   EXPECT_TRUE(IsResult(DownloadProtectionService::UNKNOWN));
+#if defined(OS_MACOSX)
+  // OSX sends pings for evaluation purposes.
+  EXPECT_TRUE(HasClientDownloadRequest());
+  ClearClientDownloadRequest();
+#else
+  EXPECT_FALSE(HasClientDownloadRequest());
+#endif
 #endif
 
   // Invalid response should result in UNKNOWN.
@@ -637,6 +709,12 @@ TEST_F(DownloadProtectionServiceTest, CheckClientDownloadSuccess) {
                  base::Unretained(this)));
   MessageLoop::current()->Run();
   EXPECT_TRUE(IsResult(DownloadProtectionService::UNKNOWN));
+#if defined(OS_WIN) || defined(OS_MACOSX)
+  EXPECT_TRUE(HasClientDownloadRequest());
+  ClearClientDownloadRequest();
+#else
+  EXPECT_FALSE(HasClientDownloadRequest());
+#endif
   std::string feedback_ping;
   std::string feedback_response;
   EXPECT_FALSE(DownloadFeedbackService::GetPingsForDownloadForTesting(
@@ -658,8 +736,17 @@ TEST_F(DownloadProtectionServiceTest, CheckClientDownloadSuccess) {
       item, &feedback_ping, &feedback_response));
 #if defined(OS_WIN)
   EXPECT_TRUE(IsResult(DownloadProtectionService::DANGEROUS));
+  EXPECT_TRUE(HasClientDownloadRequest());
+  ClearClientDownloadRequest();
 #else
   EXPECT_TRUE(IsResult(DownloadProtectionService::UNKNOWN));
+#if defined(OS_MACOSX)
+  // OSX sends pings for evaluation purposes.
+  EXPECT_TRUE(HasClientDownloadRequest());
+  ClearClientDownloadRequest();
+#else
+  EXPECT_FALSE(HasClientDownloadRequest());
+#endif
 #endif
 
   // If the response is uncommon the result should also be marked as uncommon.
@@ -682,6 +769,8 @@ TEST_F(DownloadProtectionServiceTest, CheckClientDownloadSuccess) {
   EXPECT_TRUE(decoded_request.ParseFromString(feedback_ping));
   EXPECT_EQ(url_chain.back().spec(), decoded_request.url());
   EXPECT_EQ(response.SerializeAsString(), feedback_response);
+  EXPECT_TRUE(HasClientDownloadRequest());
+  ClearClientDownloadRequest();
 #else
   EXPECT_TRUE(IsResult(DownloadProtectionService::UNKNOWN));
 #endif
@@ -704,6 +793,8 @@ TEST_F(DownloadProtectionServiceTest, CheckClientDownloadSuccess) {
   EXPECT_TRUE(DownloadFeedbackService::GetPingsForDownloadForTesting(
       item, &feedback_ping, &feedback_response));
   EXPECT_EQ(response.SerializeAsString(), feedback_response);
+  EXPECT_TRUE(HasClientDownloadRequest());
+  ClearClientDownloadRequest();
 #else
   EXPECT_TRUE(IsResult(DownloadProtectionService::UNKNOWN));
 #endif
@@ -723,8 +814,17 @@ TEST_F(DownloadProtectionServiceTest, CheckClientDownloadSuccess) {
   MessageLoop::current()->Run();
 #if defined(OS_WIN)
   EXPECT_TRUE(IsResult(DownloadProtectionService::POTENTIALLY_UNWANTED));
+  EXPECT_TRUE(HasClientDownloadRequest());
+  ClearClientDownloadRequest();
 #else
   EXPECT_TRUE(IsResult(DownloadProtectionService::UNKNOWN));
+#if defined(OS_MACOSX)
+  // OSX sends pings for evaluation purposes.
+  EXPECT_TRUE(HasClientDownloadRequest());
+  ClearClientDownloadRequest();
+#else
+  EXPECT_FALSE(HasClientDownloadRequest());
+#endif
 #endif
 }
 
@@ -772,8 +872,17 @@ TEST_F(DownloadProtectionServiceTest, CheckClientDownloadHTTPS) {
   MessageLoop::current()->Run();
 #if defined(OS_WIN)
   EXPECT_TRUE(IsResult(DownloadProtectionService::DANGEROUS));
+  EXPECT_TRUE(HasClientDownloadRequest());
+  ClearClientDownloadRequest();
 #else
   EXPECT_TRUE(IsResult(DownloadProtectionService::UNKNOWN));
+#if defined(OS_MACOSX)
+  // OSX sends pings for evaluation purposes.
+  EXPECT_TRUE(HasClientDownloadRequest());
+  ClearClientDownloadRequest();
+#else
+  EXPECT_FALSE(HasClientDownloadRequest());
+#endif
 #endif
 }
 
@@ -826,6 +935,7 @@ TEST_F(DownloadProtectionServiceTest, CheckClientDownloadZip) {
                  base::Unretained(this)));
   MessageLoop::current()->Run();
   EXPECT_TRUE(IsResult(DownloadProtectionService::UNKNOWN));
+  EXPECT_FALSE(HasClientDownloadRequest());
   Mock::VerifyAndClearExpectations(sb_service_.get());
   Mock::VerifyAndClearExpectations(binary_feature_extractor_.get());
 
@@ -846,10 +956,19 @@ TEST_F(DownloadProtectionServiceTest, CheckClientDownloadZip) {
   MessageLoop::current()->Run();
 #if defined(OS_WIN)
   EXPECT_TRUE(IsResult(DownloadProtectionService::SAFE));
+  EXPECT_TRUE(HasClientDownloadRequest());
+  ClearClientDownloadRequest();
 #else
   // For !OS_WIN, no file types are currently supported. Hence the resulting
   // verdict is UNKNOWN.
   EXPECT_TRUE(IsResult(DownloadProtectionService::UNKNOWN));
+#if defined(OS_MACOSX)
+  // OSX sends pings for evaluation purposes.
+  EXPECT_TRUE(HasClientDownloadRequest());
+  ClearClientDownloadRequest();
+#else
+  EXPECT_FALSE(HasClientDownloadRequest());
+#endif
 #endif
   Mock::VerifyAndClearExpectations(binary_feature_extractor_.get());
 
@@ -868,8 +987,17 @@ TEST_F(DownloadProtectionServiceTest, CheckClientDownloadZip) {
   MessageLoop::current()->Run();
 #if defined(OS_WIN)
   EXPECT_TRUE(IsResult(DownloadProtectionService::DANGEROUS));
+  EXPECT_TRUE(HasClientDownloadRequest());
+  ClearClientDownloadRequest();
 #else
   EXPECT_TRUE(IsResult(DownloadProtectionService::UNKNOWN));
+#if defined(OS_MACOSX)
+  // OSX sends pings for evaluation purposes.
+  EXPECT_TRUE(HasClientDownloadRequest());
+  ClearClientDownloadRequest();
+#else
+  EXPECT_FALSE(HasClientDownloadRequest());
+#endif
 #endif
   Mock::VerifyAndClearExpectations(binary_feature_extractor_.get());
 }
@@ -908,6 +1036,7 @@ TEST_F(DownloadProtectionServiceTest, CheckClientDownloadCorruptZip) {
                  base::Unretained(this)));
   MessageLoop::current()->Run();
   EXPECT_TRUE(IsResult(DownloadProtectionService::UNKNOWN));
+  EXPECT_FALSE(HasClientDownloadRequest());
   Mock::VerifyAndClearExpectations(sb_service_.get());
   Mock::VerifyAndClearExpectations(binary_feature_extractor_.get());
 }
@@ -1010,6 +1139,7 @@ TEST_F(DownloadProtectionServiceTest,
   MessageLoop::current()->Run();
   net::TestURLFetcher* fetcher = factory.GetFetcherByID(0);
   EXPECT_EQ(NULL, fetcher);
+  EXPECT_FALSE(HasClientDownloadRequest());
 }
 #endif
 
@@ -1056,9 +1186,12 @@ TEST_F(DownloadProtectionServiceTest, CheckClientDownloadValidateRequest) {
   MessageLoop::current()->Run();
   net::TestURLFetcher* fetcher = factory.GetFetcherByID(0);
   EXPECT_EQ(NULL, fetcher);
+  EXPECT_FALSE(HasClientDownloadRequest());
 #else
   // Run the message loop(s) until SendRequest is called.
   FlushThreadMessageLoops();
+  EXPECT_TRUE(HasClientDownloadRequest());
+  ClearClientDownloadRequest();
   net::TestURLFetcher* fetcher = factory.GetFetcherByID(0);
   ASSERT_TRUE(fetcher);
   ClientDownloadRequest request;
@@ -1141,9 +1274,12 @@ TEST_F(DownloadProtectionServiceTest,
   MessageLoop::current()->Run();
   net::TestURLFetcher* fetcher = factory.GetFetcherByID(0);
   EXPECT_EQ(NULL, fetcher);
+  EXPECT_FALSE(HasClientDownloadRequest());
 #else
   // Run the message loop(s) until SendRequest is called.
   FlushThreadMessageLoops();
+  EXPECT_TRUE(HasClientDownloadRequest());
+  ClearClientDownloadRequest();
   net::TestURLFetcher* fetcher = factory.GetFetcherByID(0);
   ASSERT_TRUE(fetcher);
   ClientDownloadRequest request;
@@ -1229,8 +1365,11 @@ TEST_F(DownloadProtectionServiceTest,
     MessageLoop::current()->Run();
     net::TestURLFetcher* fetcher = factory.GetFetcherByID(0);
     EXPECT_EQ(NULL, fetcher);
+    EXPECT_FALSE(HasClientDownloadRequest());
 #else
     EXPECT_EQ(0, fetcher_watcher.WaitForRequest());
+    EXPECT_TRUE(HasClientDownloadRequest());
+    ClearClientDownloadRequest();
     net::TestURLFetcher* fetcher = factory.GetFetcherByID(0);
     ASSERT_TRUE(fetcher);
     ClientDownloadRequest request;
@@ -1304,8 +1443,11 @@ TEST_F(DownloadProtectionServiceTest,
     MessageLoop::current()->Run();
     net::TestURLFetcher* fetcher = factory.GetFetcherByID(0);
     EXPECT_EQ(NULL, fetcher);
+    EXPECT_FALSE(HasClientDownloadRequest());
 #else
     EXPECT_EQ(0, fetcher_watcher.WaitForRequest());
+    EXPECT_TRUE(HasClientDownloadRequest());
+    ClearClientDownloadRequest();
     net::TestURLFetcher* fetcher = factory.GetFetcherByID(0);
     ASSERT_TRUE(fetcher);
     ClientDownloadRequest request;
@@ -1460,6 +1602,12 @@ TEST_F(DownloadProtectionServiceTest, TestDownloadRequestTimeout) {
   // anything yet.
   MessageLoop::current()->Run();
   EXPECT_TRUE(IsResult(DownloadProtectionService::UNKNOWN));
+#if defined(OS_WIN) || defined(OS_MACOSX)
+  EXPECT_TRUE(HasClientDownloadRequest());
+  ClearClientDownloadRequest();
+#else
+  EXPECT_FALSE(HasClientDownloadRequest());
+#endif
 }
 
 TEST_F(DownloadProtectionServiceTest, TestDownloadItemDestroyed) {
@@ -1503,6 +1651,7 @@ TEST_F(DownloadProtectionServiceTest, TestDownloadItemDestroyed) {
   }
 
   EXPECT_TRUE(IsResult(DownloadProtectionService::UNKNOWN));
+  EXPECT_FALSE(HasClientDownloadRequest());
 }
 
 TEST_F(DownloadProtectionServiceTest, GetCertificateWhitelistStrings) {