Disable CSD pings for interstitial click throughs

This CL removes the functionality for sending a CSD ping when an
interstitial was previously shown on the page. This is the minimal CL
to do so, and is intended to be merged.

Future CLs will clean up useless code introduced by this CL.

Bug: 1132060
Change-Id: I9f3e163268254cf79eee60b30b2d3da025293abc
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2429809Reviewed-by: Bettina Dea <bdea@chromium.org>
Reviewed-by: Varun Khaneja <vakh@chromium.org>
Commit-Queue: Daniel Rubery <drubery@chromium.org>
Cr-Commit-Position: refs/heads/master@{#812289}

chrome/browser/safe_browsing/client_side_detection_host.cc

@@ -507,11 +507,8 @@ void ClientSideDetectionHost::PhishingDetectionDone(
     base::UmaHistogramBoolean("SBClientPhishing.LocalModelDetectsPhishing",
                               verdict->is_phishing());
 
-    // We only send phishing verdict to the server if the verdict is phishing or
-    // if a SafeBrowsing interstitial was already shown for this site.  E.g., a
-    // phishing interstitial was shown but the user clicked
-    // through.
-    if (verdict->is_phishing() || DidShowSBInterstitial()) {
+    // We only send phishing verdict to the server if the verdict is phishing.
+    if (verdict->is_phishing()) {
       if (DidShowSBInterstitial()) {
         browse_info_->unsafe_resource = std::move(unsafe_resource_);
       }

chrome/browser/safe_browsing/client_side_detection_host_unittest.cc

@@ -106,11 +106,6 @@ MATCHER(CallbackIsNull, "") {
   return arg.is_null();
 }
 
-ACTION(QuitUIMessageLoop) {
-  EXPECT_TRUE(BrowserThread::CurrentlyOn(BrowserThread::UI));
-  base::RunLoop::QuitCurrentWhenIdleDeprecated();
-}
-
 class MockModelLoader : public ModelLoader {
  public:
   MockModelLoader() : ModelLoader(base::Closure(), nullptr, false) {}
@@ -716,16 +711,10 @@ TEST_F(ClientSideDetectionHostTest,
   WaitAndCheckPreClassificationChecks();
   SetUnsafeSubResourceForCurrent(true /* expect_unsafe_resource */);
 
-  EXPECT_CALL(*csd_service_,
-              SendClientReportPhishingRequest(PartiallyEqualVerdict(verdict), _,
-                                              _, CallbackIsNull()))
-      .WillOnce(QuitUIMessageLoop());
   std::vector<GURL> redirect_chain;
   redirect_chain.push_back(url);
   SetRedirectChain(redirect_chain);
   PhishingDetectionDone(verdict.SerializeAsString());
-  base::RunLoop().Run();
-  EXPECT_TRUE(Mock::VerifyAndClear(csd_host_.get()));
 }
 
 TEST_F(ClientSideDetectionHostTest,
@@ -756,22 +745,10 @@ TEST_F(ClientSideDetectionHostTest,
   NavigateWithSBHitAndCommit(url);
   WaitAndCheckPreClassificationChecks();
 
-  EXPECT_CALL(*csd_service_,
-              SendClientReportPhishingRequest(PartiallyEqualVerdict(verdict), _,
-                                              _, CallbackIsNull()))
-      .WillOnce(QuitUIMessageLoop());
   std::vector<GURL> redirect_chain;
   redirect_chain.push_back(url);
   SetRedirectChain(redirect_chain);
   PhishingDetectionDone(verdict.SerializeAsString());
-  base::RunLoop().Run();
-  EXPECT_TRUE(Mock::VerifyAndClear(csd_host_.get()));
-
-  EXPECT_CALL(*csd_service_, GetModelStr()).WillRepeatedly(Return("model_str"));
-  ExpectPreClassificationChecks(start_url, &kFalse, &kFalse, &kFalse, &kFalse,
-                                &kFalse);
-  NavigateWithoutSBHitAndCommit(start_url);
-  WaitAndCheckPreClassificationChecks();
 }
 
 TEST_F(
@@ -812,19 +789,10 @@ TEST_F(
 
   WaitAndCheckPreClassificationChecks();
 
-  // Even though we have a pending navigation, the DidShowSBInterstitial check
-  // should apply to the committed navigation, so we should get a report even
-  // though the verdict has is_phishing = false.
-  EXPECT_CALL(*csd_service_,
-              SendClientReportPhishingRequest(PartiallyEqualVerdict(verdict), _,
-                                              _, CallbackIsNull()))
-      .WillOnce(QuitUIMessageLoop());
   std::vector<GURL> redirect_chain;
   redirect_chain.push_back(url);
   SetRedirectChain(redirect_chain);
   PhishingDetectionDone(verdict.SerializeAsString());
-  base::RunLoop().Run();
-  EXPECT_TRUE(Mock::VerifyAndClear(csd_host_.get()));
 }
 
 TEST_F(ClientSideDetectionHostTest, SafeBrowsingHitOnFreshTab) {
@@ -1145,8 +1113,7 @@ TEST_F(ClientSideDetectionHostTest, RecordsPhishingDetectionDuration) {
   histogram_tester.ExpectTotalCount(
       "SBClientPhishing.PhishingDetectionDuration", 1);
 
-  // Navigate to a different host which will have a malware hit.
-  GURL url("http://malware-but-not-phishing.com/");
+  GURL url("http://phishing.example.com/");
   ClientPhishingRequest verdict;
   verdict.set_url(url.spec());
   verdict.set_client_score(0.1f);
@@ -1155,24 +1122,18 @@ TEST_F(ClientSideDetectionHostTest, RecordsPhishingDetectionDuration) {
   EXPECT_CALL(*csd_service_, GetModelStr()).WillRepeatedly(Return("model_str"));
   ExpectPreClassificationChecks(url, &kFalse, &kFalse, &kFalse, &kFalse,
                                 &kFalse);
-  NavigateWithSBHitAndCommit(url);
+  NavigateAndCommit(url);
   WaitAndCheckPreClassificationChecks();
   const base::TimeDelta duration = base::TimeDelta::FromMilliseconds(10);
   AdvanceTimeTickClock(duration);
 
-  EXPECT_CALL(*csd_service_,
-              SendClientReportPhishingRequest(PartiallyEqualVerdict(verdict), _,
-                                              _, CallbackIsNull()))
-      .WillOnce(QuitUIMessageLoop());
   std::vector<GURL> redirect_chain;
   redirect_chain.push_back(url);
   SetRedirectChain(redirect_chain);
   PhishingDetectionDone(verdict.SerializeAsString());
-  base::RunLoop().Run();
-  EXPECT_TRUE(Mock::VerifyAndClear(csd_host_.get()));
 
   histogram_tester.ExpectTotalCount(
-      "SBClientPhishing.PhishingDetectionDuration", 2);
+      "SBClientPhishing.PhishingDetectionDuration", 3);
   EXPECT_LE(duration.InMilliseconds(),
             histogram_tester
                 .GetAllSamples("SBClientPhishing.PhishingDetectionDuration")