Don't collect safebrowsing DOM details if the warning was main page load blocking.

BUG=564409

Review URL: https://codereview.chromium.org/1488423002

Cr-Commit-Position: refs/heads/master@{#363074}

chrome/browser/safe_browsing/safe_browsing_blocking_page.cc

@@ -514,15 +514,10 @@ void SafeBrowsingBlockingPage::ShowBlockingPage(
 // static
 bool SafeBrowsingBlockingPage::IsMainPageLoadBlocked(
     const UnsafeResourceList& unsafe_resources) {
-  // Client-side phishing detection interstitials never block the main frame
-  // load, since they happen after the page is finished loading.
-  if (unsafe_resources[0].threat_type ==
-      SB_THREAT_TYPE_CLIENT_SIDE_PHISHING_URL) {
-    return false;
-  }
-
-  // Otherwise, check the threat type.
-  return unsafe_resources.size() == 1 && !unsafe_resources[0].is_subresource;
+  // If there is more than one unsafe resource, the main page load must not be
+  // blocked. Otherwise, check if the one resource is.
+  return unsafe_resources.size() == 1 &&
+         unsafe_resources[0].IsMainPageLoadBlocked();
 }
 
 std::string SafeBrowsingBlockingPage::GetMetricPrefix() const {

chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc

@@ -6,6 +6,8 @@
 // threat urls.  It then uses a real browser to go to these urls, and sends
 // "goback" or "proceed" commands and verifies they work.
 
+#include <algorithm>
+
 #include "base/bind.h"
 #include "base/command_line.h"
 #include "base/prefs/pref_service.h"
@@ -59,6 +61,7 @@ namespace {
 
 const char kEmptyPage[] = "empty.html";
 const char kMalwarePage[] = "safe_browsing/malware.html";
+const char kMalwarePage2[] = "safe_browsing/malware2.html";
 const char kMalwareIframe[] = "safe_browsing/malware_iframe.html";
 const char kUnrelatedUrl[] = "https://www.google.com";
 
@@ -420,7 +423,7 @@ class SafeBrowsingBlockingPageBrowserTest
   // navigates to a page with an iframe containing the threat site, and returns
   // the url of the parent page.
   GURL SetupThreatIframeWarningAndNavigate() {
-    GURL url = net::URLRequestMockHTTPJob::GetMockUrl(kMalwarePage);
+    GURL url = net::URLRequestMockHTTPJob::GetMockUrl(kMalwarePage2);
     GURL iframe_url = net::URLRequestMockHTTPJob::GetMockUrl(kMalwareIframe);
     SetURLThreatType(iframe_url, GetParam());
 
@@ -448,24 +451,6 @@ class SafeBrowsingBlockingPageBrowserTest
     interstitial_page->CommandReceived(base::IntToString(command));
   }
 
-  void DontProceedThroughInterstitial() {
-    WebContents* contents =
-        browser()->tab_strip_model()->GetActiveWebContents();
-    InterstitialPage* interstitial_page = InterstitialPage::GetInterstitialPage(
-        contents);
-    ASSERT_TRUE(interstitial_page);
-    interstitial_page->DontProceed();
-  }
-
-  void ProceedThroughInterstitial() {
-    WebContents* contents =
-        browser()->tab_strip_model()->GetActiveWebContents();
-    InterstitialPage* interstitial_page = InterstitialPage::GetInterstitialPage(
-        contents);
-    ASSERT_TRUE(interstitial_page);
-    interstitial_page->Proceed();
-  }
-
   void AssertNoInterstitial(bool wait_for_delete) {
     WebContents* contents =
         browser()->tab_strip_model()->GetActiveWebContents();
@@ -756,6 +741,120 @@ IN_PROC_BROWSER_TEST_P(SafeBrowsingBlockingPageBrowserTest,
     ASSERT_TRUE(report.ParseFromString(serialized));
     // Verify the report is complete.
     EXPECT_TRUE(report.complete());
+    // Do some basic verification of report contents.
+    EXPECT_EQ(url.spec(), report.page_url());
+    EXPECT_EQ(net::URLRequestMockHTTPJob::GetMockUrl(kMalwareIframe).spec(),
+              report.url());
+    std::vector<std::string> report_urls;
+    for (int i = 0; i < report.resources_size(); ++i)
+      report_urls.push_back(report.resources(i).url());
+    ASSERT_EQ(3U, report_urls.size());
+    std::sort(report_urls.begin(), report_urls.end());
+    EXPECT_EQ("http://example.com/cross_site_iframe.html", report_urls[0]);
+    EXPECT_EQ(url.spec(), report_urls[1]);
+    EXPECT_EQ(net::URLRequestMockHTTPJob::GetMockUrl(kMalwareIframe).spec(),
+              report_urls[2]);
+  }
+}
+
+IN_PROC_BROWSER_TEST_P(SafeBrowsingBlockingPageBrowserTest,
+                       MainFrameBlockedShouldHaveNoDOMDetailsWhenDontProceed) {
+  const bool expect_threat_details =
+      SafeBrowsingBlockingPage::ShouldReportThreatDetails(GetParam());
+
+  scoped_refptr<content::MessageLoopRunner> threat_report_sent_runner(
+      new content::MessageLoopRunner);
+  if (expect_threat_details)
+    SetReportSentCallback(threat_report_sent_runner->QuitClosure());
+
+  // Navigate to a safe page which contains multiple potential DOM details.
+  // (Despite the name, kMalwarePage is not the page flagged as malware in this
+  // test.)
+  GURL safe_url(net::URLRequestMockHTTPJob::GetMockUrl(kMalwarePage));
+  ui_test_utils::NavigateToURL(browser(), safe_url);
+
+  EXPECT_EQ(nullptr, details_factory_.get_details());
+
+  // Start navigation to bad page (kEmptyPage), which will be blocked before it
+  // is committed.
+  GURL url = SetupWarningAndNavigate();
+
+  FakeThreatDetails* fake_threat_details = details_factory_.get_details();
+  EXPECT_EQ(expect_threat_details, fake_threat_details != nullptr);
+
+  // Go back.
+  EXPECT_EQ(VISIBLE, GetVisibility("extended-reporting-opt-in"));
+  EXPECT_TRUE(Click("opt-in-checkbox"));
+  EXPECT_TRUE(ClickAndWaitForDetach("primary-button"));
+  AssertNoInterstitial(true);  // Assert the interstitial is gone
+
+  EXPECT_TRUE(browser()->profile()->GetPrefs()->GetBoolean(
+              prefs::kSafeBrowsingExtendedReportingEnabled));
+  EXPECT_EQ(safe_url,
+            browser()->tab_strip_model()->GetActiveWebContents()->GetURL());
+
+  if (expect_threat_details) {
+    threat_report_sent_runner->Run();
+    std::string serialized = GetReportSent();
+    ClientSafeBrowsingReportRequest report;
+    ASSERT_TRUE(report.ParseFromString(serialized));
+    // Verify the report is complete.
+    EXPECT_TRUE(report.complete());
+    EXPECT_EQ(url.spec(), report.page_url());
+    EXPECT_EQ(url.spec(), report.url());
+    ASSERT_EQ(1, report.resources_size());
+    EXPECT_EQ(url.spec(), report.resources(0).url());
+  }
+}
+
+IN_PROC_BROWSER_TEST_P(
+    SafeBrowsingBlockingPageBrowserTest,
+    MainFrameBlockedShouldHaveNoDOMDetailsWhenProceeding) {
+  const bool expect_threat_details =
+      SafeBrowsingBlockingPage::ShouldReportThreatDetails(GetParam());
+
+  scoped_refptr<content::MessageLoopRunner> threat_report_sent_runner(
+      new content::MessageLoopRunner);
+  if (expect_threat_details)
+    SetReportSentCallback(threat_report_sent_runner->QuitClosure());
+
+  // Navigate to a safe page which contains multiple potential DOM details.
+  // (Despite the name, kMalwarePage is not the page flagged as malware in this
+  // test.)
+  ui_test_utils::NavigateToURL(
+      browser(), net::URLRequestMockHTTPJob::GetMockUrl(kMalwarePage));
+
+  EXPECT_EQ(nullptr, details_factory_.get_details());
+
+  // Start navigation to bad page (kEmptyPage), which will be blocked before it
+  // is committed.
+  GURL url = SetupWarningAndNavigate();
+
+  FakeThreatDetails* fake_threat_details = details_factory_.get_details();
+  EXPECT_EQ(expect_threat_details, fake_threat_details != nullptr);
+
+  // Proceed through the warning.
+  EXPECT_EQ(VISIBLE, GetVisibility("extended-reporting-opt-in"));
+  EXPECT_TRUE(Click("opt-in-checkbox"));
+  EXPECT_TRUE(ClickAndWaitForDetach("proceed-link"));
+  AssertNoInterstitial(true);  // Assert the interstitial is gone
+
+  EXPECT_TRUE(browser()->profile()->GetPrefs()->GetBoolean(
+              prefs::kSafeBrowsingExtendedReportingEnabled));
+  EXPECT_EQ(url,
+            browser()->tab_strip_model()->GetActiveWebContents()->GetURL());
+
+  if (expect_threat_details) {
+    threat_report_sent_runner->Run();
+    std::string serialized = GetReportSent();
+    ClientSafeBrowsingReportRequest report;
+    ASSERT_TRUE(report.ParseFromString(serialized));
+    // Verify the report is complete.
+    EXPECT_TRUE(report.complete());
+    EXPECT_EQ(url.spec(), report.page_url());
+    EXPECT_EQ(url.spec(), report.url());
+    ASSERT_EQ(1, report.resources_size());
+    EXPECT_EQ(url.spec(), report.resources(0).url());
   }
 }
 

chrome/browser/safe_browsing/threat_details.cc

@@ -227,10 +227,14 @@ void ThreatDetails::StartCollection() {
   if (nav_entry && !referrer_url.is_empty())
     AddUrl(referrer_url, GURL(), std::string(), NULL);
 
-  // Get URLs of frames, scripts etc from the DOM.
-  // OnReceivedThreatDOMDetails will be called when the renderer replies.
-  content::RenderViewHost* view = web_contents()->GetRenderViewHost();
-  view->Send(new SafeBrowsingMsg_GetThreatDOMDetails(view->GetRoutingID()));
+  if (!resource_.IsMainPageLoadBlocked()) {
+    // Get URLs of frames, scripts etc from the DOM.
+    // OnReceivedThreatDOMDetails will be called when the renderer replies.
+    // TODO(mattm): In theory, if the user proceeds through the warning DOM
+    // detail collection could be started once the page loads.
+    content::RenderViewHost* view = web_contents()->GetRenderViewHost();
+    view->Send(new SafeBrowsingMsg_GetThreatDOMDetails(view->GetRoutingID()));
+  }
 }
 
 // When the renderer is done, this is called.

chrome/browser/safe_browsing/ui_manager.cc

@@ -73,6 +73,21 @@ SafeBrowsingUIManager::UnsafeResource::UnsafeResource()
 
 SafeBrowsingUIManager::UnsafeResource::~UnsafeResource() { }
 
+bool SafeBrowsingUIManager::UnsafeResource::IsMainPageLoadBlocked() const {
+  // Subresource hits cannot happen until after main page load is committed.
+  if (is_subresource)
+    return false;
+
+  // Client-side phishing detection interstitials never block the main frame
+  // load, since they happen after the page is finished loading.
+  if (threat_type == SB_THREAT_TYPE_CLIENT_SIDE_PHISHING_URL ||
+      threat_type == SB_THREAT_TYPE_CLIENT_SIDE_MALWARE_URL) {
+    return false;
+  }
+
+  return true;
+}
+
 // SafeBrowsingUIManager -------------------------------------------------------
 
 SafeBrowsingUIManager::SafeBrowsingUIManager(

chrome/browser/safe_browsing/ui_manager.h

@@ -47,6 +47,8 @@ class SafeBrowsingUIManager
     UnsafeResource();
     ~UnsafeResource();
 
+    bool IsMainPageLoadBlocked() const;
+
     GURL url;
     GURL original_url;
     std::vector<GURL> redirect_urls;

chrome/test/data/safe_browsing/malware2.html

+<html>
+<body>
+<iframe src="malware_iframe.html"></iframe>
+<iframe src="http://example.com/cross_site_iframe.html"></iframe>
+<img src="malware_image.png">
+<img src="http://example.com/image.png">
+</body>
+</html>