Refrain from sending hit report when user is in incognito mode

chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc

Index: chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc
diff --git a/chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc b/chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc
index cfbc90427bca1257770040557be3e8c792b5bb6b..727c2fe7a93e3c5f0b16fe622f4d403fb8bd5702 100644
--- a/chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc
+++ b/chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc
@@ -188,6 +188,15 @@ class FakeSafeBrowsingUIManager : public TestSafeBrowsingUIManager {
     threat_details_done_ = true;
   }
 
+  void MaybeReportSafeBrowsingHit(const HitReport& hit_report,
+                                  WebContents* web_contents) override {
+    if (SafeBrowsingUIManager::ShouldSendHitReport(hit_report, web_contents)) {
+      hit_report_sent_ = true;
+    }
+  }
+
+  bool hit_report_sent() { return hit_report_sent_; }
+
   void set_threat_details_done_callback(const base::Closure& callback) {
     EXPECT_TRUE(BrowserThread::CurrentlyOn(BrowserThread::UI));
     EXPECT_TRUE(threat_details_done_callback_.is_null());
@@ -206,6 +215,7 @@ class FakeSafeBrowsingUIManager : public TestSafeBrowsingUIManager {
   std::string report_;
   base::Closure threat_details_done_callback_;
   bool threat_details_done_;
+  bool hit_report_sent_ = false;

[Jialiu Lin, 2017/05/18 22:32:12]

nit: since all the other members are initialized in constructor, you might want
to do the same thing for hit_report_sent_ too.

[mortonm, 2017/05/19 20:21:03]

Done.

 
   DISALLOW_COPY_AND_ASSIGN(FakeSafeBrowsingUIManager);
 };
@@ -379,10 +389,10 @@ class SafeBrowsingBlockingPageBrowserTest
         ->ClearBadURL(url);
   }
 
-  // The basic version of this method, which uses a HTTP test URL.
-  GURL SetupWarningAndNavigate() {
+  // The basic version of this method, which uses an HTTP test URL.
+  GURL SetupWarningAndNavigate(bool is_incognito) {
     return SetupWarningAndNavigateToURL(
-        net::URLRequestMockHTTPJob::GetMockUrl(kEmptyPage));
+        net::URLRequestMockHTTPJob::GetMockUrl(kEmptyPage), is_incognito);
   }
 
   // Navigates to a warning on a valid HTTPS website.
@@ -395,7 +405,7 @@ class SafeBrowsingBlockingPageBrowserTest
     verify_result.cert_status = 0;
     mock_cert_verifier()->AddResultForCert(cert.get(), verify_result, net::OK);
     GURL url = https_server_.GetURL(kHTTPSPage);
-    return SetupWarningAndNavigateToURL(url);
+    return SetupWarningAndNavigateToURL(url, false);
   }
 
   // Navigates through an HTTPS interstitial, then opens up a SB warning on that
@@ -407,7 +417,7 @@ class SafeBrowsingBlockingPageBrowserTest
 
     // Proceed through the HTTPS interstitial.
     ui_test_utils::NavigateToURL(browser(), url);
-    EXPECT_TRUE(WaitForReady());
+    EXPECT_TRUE(WaitForReady(browser()));
     InterstitialPage* https_warning = browser()
                                           ->tab_strip_model()
                                           ->GetActiveWebContents()
@@ -418,7 +428,7 @@ class SafeBrowsingBlockingPageBrowserTest
     content::WaitForInterstitialDetach(
         browser()->tab_strip_model()->GetActiveWebContents());
 
-    return SetupWarningAndNavigateToURL(url);
+    return SetupWarningAndNavigateToURL(url, false);
   }
 
   // Adds a safebrowsing threat results to the fake safebrowsing service,
@@ -430,7 +440,7 @@ class SafeBrowsingBlockingPageBrowserTest
     SetURLThreatType(iframe_url, testing::get<0>(GetParam()));
 
     ui_test_utils::NavigateToURL(browser(), url);
-    EXPECT_TRUE(WaitForReady());
+    EXPECT_TRUE(WaitForReady(browser()));
     return url;
   }
 
@@ -451,7 +461,7 @@ class SafeBrowsingBlockingPageBrowserTest
     SetURLThreatType(*iframe_url, testing::get<0>(GetParam()));
 
     ui_test_utils::NavigateToURL(browser(), url);
-    EXPECT_TRUE(WaitForReady());
+    EXPECT_TRUE(WaitForReady(browser()));
     return url;
   }
 
@@ -550,9 +560,9 @@ class SafeBrowsingBlockingPageBrowserTest
     return interstitial->GetMainFrame();
   }
 
-  bool WaitForReady() {
+  bool WaitForReady(Browser* browser) {
     InterstitialPage* interstitial = InterstitialPage::GetInterstitialPage(
-        browser()->tab_strip_model()->GetActiveWebContents());
+        browser->tab_strip_model()->GetActiveWebContents());
     if (!interstitial)
       return false;
     return content::WaitForRenderFrameReady(interstitial->GetMainFrame());
@@ -619,7 +629,7 @@ class SafeBrowsingBlockingPageBrowserTest
   void TestReportingDisabledAndDontProceed(const GURL& url) {
     SetURLThreatType(url, testing::get<0>(GetParam()));
     ui_test_utils::NavigateToURL(browser(), url);
-    ASSERT_TRUE(WaitForReady());
+    ASSERT_TRUE(WaitForReady(browser()));
 
     EXPECT_EQ(HIDDEN, GetVisibility("extended-reporting-opt-in"));
     EXPECT_EQ(HIDDEN, GetVisibility("opt-in-checkbox"));
@@ -698,6 +708,12 @@ class SafeBrowsingBlockingPageBrowserTest
               security_info.malicious_content_status);
   }
 
+  bool hit_report_sent() {
+    return static_cast<FakeSafeBrowsingUIManager*>(
+               factory_.test_safe_browsing_service()->ui_manager().get())
+        ->hit_report_sent();
+  }
+
  protected:
   TestThreatDetailsFactory details_factory_;
 
@@ -705,16 +721,23 @@ class SafeBrowsingBlockingPageBrowserTest
   // Adds a safebrowsing result of the current test threat to the fake
   // safebrowsing service, navigates to that page, and returns the url.
   // The various wrappers supply different URLs.
-  GURL SetupWarningAndNavigateToURL(GURL url) {
+  GURL SetupWarningAndNavigateToURL(GURL url, bool is_incognito) {
     SetURLThreatType(url, testing::get<0>(GetParam()));
-    ui_test_utils::NavigateToURL(browser(), url);
-    EXPECT_TRUE(WaitForReady());
+    if (is_incognito) {
+      Browser* incognito_browser = CreateIncognitoBrowser();
+      ui_test_utils::NavigateToURL(incognito_browser, url);
+      EXPECT_TRUE(WaitForReady(incognito_browser));
+    } else {
+      ui_test_utils::NavigateToURL(browser(), url);
+      EXPECT_TRUE(WaitForReady(browser()));
+    }
     return url;
   }
 
   TestSafeBrowsingServiceFactory factory_;
   TestSafeBrowsingBlockingPageFactory blocking_page_factory_;
   net::EmbeddedTestServer https_server_;
+  // Browser* incognito_browser;

[Jialiu Lin, 2017/05/18 22:32:12]

You probably need this one. See my comments below. 
And you need to initialize this member to nullptr in the constructor. 

[mortonm, 2017/05/19 20:21:03]

Done.

 
   DISALLOW_COPY_AND_ASSIGN(SafeBrowsingBlockingPageBrowserTest);
 };
@@ -744,7 +767,7 @@ IN_PROC_BROWSER_TEST_P(SafeBrowsingBlockingPageBrowserTest, RedirectCanceled) {
 }
 
 IN_PROC_BROWSER_TEST_P(SafeBrowsingBlockingPageBrowserTest, DontProceed) {
-  SetupWarningAndNavigate();
+  SetupWarningAndNavigate(false);
 
   EXPECT_EQ(VISIBLE, GetVisibility("primary-button"));
   EXPECT_EQ(HIDDEN, GetVisibility("details"));
@@ -762,7 +785,7 @@ IN_PROC_BROWSER_TEST_P(SafeBrowsingBlockingPageBrowserTest, DontProceed) {
 }
 
 IN_PROC_BROWSER_TEST_P(SafeBrowsingBlockingPageBrowserTest, Proceed) {
-  GURL url = SetupWarningAndNavigate();
+  GURL url = SetupWarningAndNavigate(false);
 
   EXPECT_TRUE(ClickAndWaitForDetach("proceed-link"));
   AssertNoInterstitial(true);  // Assert the interstitial is gone.
@@ -908,7 +931,7 @@ IN_PROC_BROWSER_TEST_P(SafeBrowsingBlockingPageBrowserTest,
 
   // Start navigation to bad page (kEmptyPage), which will be blocked before it
   // is committed.
-  GURL url = SetupWarningAndNavigate();
+  GURL url = SetupWarningAndNavigate(false);
 
   ThreatDetails* threat_details = details_factory_.get_details();
   EXPECT_EQ(expect_threat_details, threat_details != nullptr);
@@ -959,7 +982,7 @@ IN_PROC_BROWSER_TEST_P(
 
   // Start navigation to bad page (kEmptyPage), which will be blocked before it
   // is committed.
-  GURL url = SetupWarningAndNavigate();
+  GURL url = SetupWarningAndNavigate(false);
 
   ThreatDetails* threat_details = details_factory_.get_details();
   EXPECT_EQ(expect_threat_details, threat_details != nullptr);
@@ -996,7 +1019,7 @@ IN_PROC_BROWSER_TEST_P(SafeBrowsingBlockingPageBrowserTest, ProceedDisabled) {
   browser()->profile()->GetPrefs()->SetBoolean(
       prefs::kSafeBrowsingProceedAnywayDisabled, true);
 
-  SetupWarningAndNavigate();
+  SetupWarningAndNavigate(false);
 
   EXPECT_EQ(VISIBLE, GetVisibility("primary-button"));
   EXPECT_EQ(HIDDEN, GetVisibility("details"));
@@ -1031,7 +1054,7 @@ IN_PROC_BROWSER_TEST_P(SafeBrowsingBlockingPageBrowserTest,
                        ReloadWhileInterstitialShowing) {
   // Start navigation to bad page (kEmptyPage), which will be blocked before it
   // is committed.
-  const GURL url = SetupWarningAndNavigate();
+  const GURL url = SetupWarningAndNavigate(false);
 
   // Checkbox should be showing.
   EXPECT_EQ(VISIBLE, GetVisibility("extended-reporting-opt-in"));
@@ -1049,7 +1072,7 @@ IN_PROC_BROWSER_TEST_P(SafeBrowsingBlockingPageBrowserTest,
   EXPECT_EQ(url, controller.GetPendingEntry()->GetURL());
 
   // "Reload" the tab.
-  SetupWarningAndNavigate();
+  SetupWarningAndNavigate(false);
 
   // Checkbox should be showing.
   EXPECT_EQ(VISIBLE, GetVisibility("extended-reporting-opt-in"));
@@ -1066,7 +1089,7 @@ IN_PROC_BROWSER_TEST_P(SafeBrowsingBlockingPageBrowserTest,
 }
 
 IN_PROC_BROWSER_TEST_P(SafeBrowsingBlockingPageBrowserTest, LearnMore) {
-  SetupWarningAndNavigate();
+  SetupWarningAndNavigate(false);
   EXPECT_TRUE(ClickAndWaitForDetach("learn-more-link"));
   AssertNoInterstitial(false);  // Assert the interstitial is gone
 
@@ -1104,7 +1127,7 @@ IN_PROC_BROWSER_TEST_P(SafeBrowsingBlockingPageBrowserTest,
   histograms.ExpectTotalCount(interaction_histogram, 0);
 
   // After navigating to the page, the totals should be set.
-  SetupWarningAndNavigate();
+  SetupWarningAndNavigate(false);
   histograms.ExpectTotalCount(decision_histogram, 1);
   histograms.ExpectBucketCount(decision_histogram,
                                security_interstitials::MetricsHelper::SHOW, 1);
@@ -1148,7 +1171,7 @@ IN_PROC_BROWSER_TEST_P(SafeBrowsingBlockingPageBrowserTest,
   histograms.ExpectTotalCount(interaction_histogram, 0);
 
   // After navigating to the page, the totals should be set.
-  GURL url = SetupWarningAndNavigate();
+  GURL url = SetupWarningAndNavigate(false);
   histograms.ExpectTotalCount(decision_histogram, 1);
   histograms.ExpectBucketCount(decision_histogram,
                                security_interstitials::MetricsHelper::SHOW, 1);
@@ -1170,7 +1193,7 @@ IN_PROC_BROWSER_TEST_P(SafeBrowsingBlockingPageBrowserTest,
 }
 
 IN_PROC_BROWSER_TEST_P(SafeBrowsingBlockingPageBrowserTest, WhitelistRevisit) {
-  GURL url = SetupWarningAndNavigate();
+  GURL url = SetupWarningAndNavigate(false);
 
   EXPECT_TRUE(ClickAndWaitForDetach("proceed-link"));
   AssertNoInterstitial(true);  // Assert the interstitial is gone.
@@ -1205,7 +1228,7 @@ IN_PROC_BROWSER_TEST_P(SafeBrowsingBlockingPageBrowserTest,
 }
 
 IN_PROC_BROWSER_TEST_P(SafeBrowsingBlockingPageBrowserTest, WhitelistUnsaved) {
-  GURL url = SetupWarningAndNavigate();
+  GURL url = SetupWarningAndNavigate(false);
 
   // Navigate without making a decision.
   ui_test_utils::NavigateToURL(browser(), GURL(kUnrelatedUrl));
@@ -1213,11 +1236,68 @@ IN_PROC_BROWSER_TEST_P(SafeBrowsingBlockingPageBrowserTest, WhitelistUnsaved) {
 
   // The non-whitelisted page should now show an interstitial.
   ui_test_utils::NavigateToURL(browser(), url);
-  EXPECT_TRUE(WaitForReady());
+  EXPECT_TRUE(WaitForReady(browser()));
   EXPECT_TRUE(ClickAndWaitForDetach("proceed-link"));
   AssertNoInterstitial(true);
 }
 
+IN_PROC_BROWSER_TEST_P(SafeBrowsingBlockingPageBrowserTest,
+                       VerifyHitReportSentOnSBERAndNotIncognito) {
+  // The extended reporting opt-in is presented in the interstitial for malware,
+  // phishing, and UwS threats.
+  const bool expect_threat_details =
+      SafeBrowsingBlockingPage::ShouldReportThreatDetails(
+          testing::get<0>(GetParam()));
+
+  scoped_refptr<content::MessageLoopRunner> threat_report_sent_runner(
+      new content::MessageLoopRunner);
+  if (expect_threat_details)
+    SetReportSentCallback(threat_report_sent_runner->QuitClosure());
+
+  browser()->profile()->GetPrefs()->SetBoolean(
+      prefs::kSafeBrowsingExtendedReportingEnabled, true);  // set up SBER
+  GURL url = SetupWarningAndNavigate(false);                // not incognito
+  EXPECT_TRUE(hit_report_sent());
+}
+
+IN_PROC_BROWSER_TEST_P(SafeBrowsingBlockingPageBrowserTest,
+                       VerifyHitReportNotSentOnIncognito) {
+  // The extended reporting opt-in is presented in the interstitial for malware,
+  // phishing, and UwS threats.
+  const bool expect_threat_details =
+      SafeBrowsingBlockingPage::ShouldReportThreatDetails(
+          testing::get<0>(GetParam()));
+
+  scoped_refptr<content::MessageLoopRunner> threat_report_sent_runner(
+      new content::MessageLoopRunner);
+  if (expect_threat_details)
+    SetReportSentCallback(threat_report_sent_runner->QuitClosure());
+
+  browser()->profile()->GetPrefs()->SetBoolean(

[Jialiu Lin, 2017/05/18 22:32:12]

You're configuring the extended reporting prefs on a irrelevant browser
instance. Shouldn't it be the incognito browser?

You probably need a incognito_browser private member.  

[mortonm, 2017/05/19 20:21:03]

Done.

+      prefs::kSafeBrowsingExtendedReportingEnabled, true);  // set up SBER
+  GURL url = SetupWarningAndNavigate(true);                 // incognito
+  EXPECT_FALSE(hit_report_sent());
+}
+
+IN_PROC_BROWSER_TEST_P(SafeBrowsingBlockingPageBrowserTest,
+                       VerifyHitReportNotSentWithoutSBER) {
+  // The extended reporting opt-in is presented in the interstitial for malware,
+  // phishing, and UwS threats.
+  const bool expect_threat_details =
+      SafeBrowsingBlockingPage::ShouldReportThreatDetails(
+          testing::get<0>(GetParam()));
+
+  scoped_refptr<content::MessageLoopRunner> threat_report_sent_runner(
+      new content::MessageLoopRunner);
+  if (expect_threat_details)
+    SetReportSentCallback(threat_report_sent_runner->QuitClosure());
+
+  browser()->profile()->GetPrefs()->SetBoolean(
+      prefs::kSafeBrowsingExtendedReportingEnabled, false);  // no SBER
+  GURL url = SetupWarningAndNavigate(false);                 // not incognito
+  EXPECT_FALSE(hit_report_sent());
+}
+
 namespace {
 
 class SecurityStyleTestObserver : public content::WebContentsObserver {
@@ -1286,7 +1366,7 @@ IN_PROC_BROWSER_TEST_P(SafeBrowsingBlockingPageBrowserTest,
 
   // The security indicator should be downgraded while the interstitial shows.
   GURL bad_url = net::URLRequestMockHTTPJob::GetMockUrl(kEmptyPage);
-  SetupWarningAndNavigate();
+  SetupWarningAndNavigate(false);
   WebContents* error_tab = browser()->tab_strip_model()->GetActiveWebContents();
   ASSERT_TRUE(error_tab);
   ExpectSecurityIndicatorDowngrade(error_tab, 0u);
@@ -1377,7 +1457,7 @@ IN_PROC_BROWSER_TEST_P(SafeBrowsingBlockingPageBrowserTest,
 IN_PROC_BROWSER_TEST_P(SafeBrowsingBlockingPageBrowserTest,
                        SecurityState_HTTP) {
   // The security indicator should be downgraded while the interstitial shows.
-  SetupWarningAndNavigate();
+  SetupWarningAndNavigate(false);
   WebContents* error_tab = browser()->tab_strip_model()->GetActiveWebContents();
   ASSERT_TRUE(error_tab);
   ExpectSecurityIndicatorDowngrade(error_tab, 0u);