[SB Delayed Warnings] Only handle phishing pages

The delayed warnings experiment should only handle phishing pages. It
doesn't make sense to run it for other threat types, as the goal is to
understand the effect of different URL display modes to phishing
threats.

This CL also fixes a DCHECK when there are multiple unsafe resources in
the current WebContents. The old code assumed that the method to create
the user interaction observer could only be called once per WebContents.
In reality, it's called for each unsafe resource, so this CL ignores
subsequent calls.

Bug: 1057157
Change-Id: Ifb87dae060c2a08c37aeef55835c318ba7bcdfec
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2130626
Commit-Queue: Mustafa Emre Acer <meacer@chromium.org>
Reviewed-by: Varun Khaneja <vakh@chromium.org>
Reviewed-by: Xinghui Lu <xinghuilu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#756470}

chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc

@@ -1968,9 +1968,14 @@ class SafeBrowsingBlockingPageDelayedWarningBrowserTest
 
  protected:
   void NavigateAndAssertNoInterstitial() {
-    const GURL url = embedded_test_server()->GetURL("/empty.html");
-    SetURLThreatType(url, SB_THREAT_TYPE_URL_PHISHING);
-    ui_test_utils::NavigateToURL(browser(), url);
+    // Use a page that contains an iframe so that we can test both top frame
+    // and subresource warnings.
+    const GURL top_frame = embedded_test_server()->GetURL("/iframe.html");
+    SetURLThreatType(top_frame, SB_THREAT_TYPE_URL_PHISHING);
+    const GURL iframe = embedded_test_server()->GetURL("/title1.html");
+    SetURLThreatType(iframe, SB_THREAT_TYPE_URL_PHISHING);
+
+    ui_test_utils::NavigateToURL(browser(), top_frame);
     AssertNoInterstitial(browser(), true);
   }
 
@@ -1981,7 +1986,6 @@ class SafeBrowsingBlockingPageDelayedWarningBrowserTest
     return testing::get<2>(GetParam());
   }
 
- private:
   void SetURLThreatType(const GURL& url, SBThreatType threat_type) {
     TestSafeBrowsingService* service = factory_.test_safe_browsing_service();
     ASSERT_TRUE(service);
@@ -1991,6 +1995,7 @@ class SafeBrowsingBlockingPageDelayedWarningBrowserTest
         ->SetURLThreatType(url, threat_type);
   }
 
+ private:
   base::test::ScopedFeatureList scoped_feature_list_;
   TestSafeBrowsingServiceFactory factory_;
   TestSafeBrowsingBlockingPageFactory blocking_page_factory_;
@@ -2014,6 +2019,22 @@ IN_PROC_BROWSER_TEST_P(SafeBrowsingBlockingPageDelayedWarningBrowserTest,
                                DelayedWarningEvent::kWarningNotShown, 1);
 }
 
+IN_PROC_BROWSER_TEST_P(SafeBrowsingBlockingPageDelayedWarningBrowserTest,
+                       NotPhishing_WarningNotDelayed) {
+  base::HistogramTester histograms;
+
+  // Navigate to a non-phishing page. The warning should not be delayed.
+  const GURL url = embedded_test_server()->GetURL("/empty.html");
+  SetURLThreatType(url, SB_THREAT_TYPE_URL_MALWARE);
+  ui_test_utils::NavigateToURL(browser(), url);
+  EXPECT_TRUE(WaitForReady(browser()));
+
+  // Navigate to about:blank to "flush" metrics, if any.
+  ui_test_utils::NavigateToURL(browser(), GURL(url::kAboutBlankURL));
+
+  histograms.ExpectTotalCount(kDelayedWarningsHistogram, 0);
+}
+
 IN_PROC_BROWSER_TEST_P(SafeBrowsingBlockingPageDelayedWarningBrowserTest,
                        KeyPress_WarningShown) {
   base::HistogramTester histograms;

chrome/browser/safe_browsing/user_interaction_observer.cc

@@ -78,7 +78,13 @@ void SafeBrowsingUserInteractionObserver::CreateForWebContents(
     const security_interstitials::UnsafeResource& resource,
     bool is_main_frame,
     scoped_refptr<SafeBrowsingUIManager> ui_manager) {
-  DCHECK(!FromWebContents(web_contents));
+  // This method is called for all unsafe resources on |web_contents|. Only
+  // create an observer if there isn't one.
+  // TODO(crbug.com/1057157): The observer should observe all unsafe resources
+  // instead of the first one only.
+  if (FromWebContents(web_contents)) {
+    return;
+  }
   auto observer = std::make_unique<SafeBrowsingUserInteractionObserver>(
       web_contents, resource, is_main_frame, ui_manager);
   web_contents->SetUserData(kWebContentsUserDataKey, std::move(observer));

components/safe_browsing/core/browser/safe_browsing_url_checker_impl.cc

@@ -183,23 +183,25 @@ void SafeBrowsingUrlCheckerImpl::OnUrlResult(const GURL& url,
 
   TRACE_EVENT_ASYNC_END1("safe_browsing", "CheckUrl", this, "url", url.spec());
 
-  if (base::FeatureList::IsEnabled(kDelayedWarnings)) {
-    // Delayed warnings experiment delays the warning until a user interaction
-    // happens. Create an interaction observer and continue like there wasn't
-    // a warning. The observer will create the interstitial when necessary.
-    security_interstitials::UnsafeResource unsafe_resource =
-        MakeUnsafeResource(url, threat_type, metadata);
-    unsafe_resource.is_delayed_warning = true;
-    url_checker_delegate_
-        ->StartObservingInteractionsForDelayedBlockingPageHelper(
-            unsafe_resource, resource_type_ == ResourceType::kMainFrame);
-
-    // Let the navigation continue.
-    threat_type = SB_THREAT_TYPE_SAFE;
-    state_ = STATE_DELAYED_BLOCKING_PAGE;
-    if (!RunNextCallback(true, false))
-      return;
-    // No need to call ProcessUrls, it'll return early.
+  // Handle main frame and subresources. We do this to catch resources flagged
+  // as phishing even if the top frame isn't flagged.
+  if (threat_type == SB_THREAT_TYPE_URL_PHISHING &&
+      base::FeatureList::IsEnabled(kDelayedWarnings)) {
+    if (state_ != STATE_DELAYED_BLOCKING_PAGE) {
+      // Delayed warnings experiment delays the warning until a user interaction
+      // happens. Create an interaction observer and continue like there wasn't
+      // a warning. The observer will create the interstitial when necessary.
+      security_interstitials::UnsafeResource unsafe_resource =
+          MakeUnsafeResource(url, threat_type, metadata);
+      unsafe_resource.is_delayed_warning = true;
+      url_checker_delegate_
+          ->StartObservingInteractionsForDelayedBlockingPageHelper(
+              unsafe_resource, resource_type_ == ResourceType::kMainFrame);
+      state_ = STATE_DELAYED_BLOCKING_PAGE;
+    }
+    // Let the navigation continue in case of delayed warnings.
+    // No need to call ProcessUrls here, it'll return early.
+    RunNextCallback(true, false);
     return;
   }