SafeBrowsingTriggeredPopupBlocker: Log the activation position

Now that we are triggering activation for potentially multiple URLs
in the redirect chain, we should be collecting data about which
part of the chain caused activation.

Bug: None
Change-Id: I835a64f6fdb714cd70cf22cee0e83c87d01d4e25
Reviewed-on: https://chromium-review.googlesource.com/1124766Reviewed-by: Robert Kaplow <rkaplow@chromium.org>
Reviewed-by: Avi Drissman <avi@chromium.org>
Commit-Queue: Charlie Harrison <csharrison@chromium.org>
Cr-Commit-Position: refs/heads/master@{#572338}

chrome/browser/ui/blocked_content/safe_browsing_triggered_popup_blocker.cc

@@ -14,6 +14,7 @@
 #include "components/prefs/pref_service.h"
 #include "components/safe_browsing/db/util.h"
 #include "components/safe_browsing/db/v4_protocol_manager_util.h"
+#include "components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle.h"
 #include "content/public/browser/navigation_handle.h"
 #include "content/public/browser/page_navigator.h"
 #include "content/public/browser/render_frame_host.h"
@@ -29,13 +30,17 @@ void LogAction(SafeBrowsingTriggeredPopupBlocker::Action action) {
                             SafeBrowsingTriggeredPopupBlocker::Action::kCount);
 }
 
-safe_browsing::SubresourceFilterLevel PickMostSevereLevel(
-    const base::Optional<safe_browsing::SubresourceFilterLevel>& previous,
-    const safe_browsing::SubresourceFilterLevel& current) {
-  if (!previous.has_value()) {
-    return current;
-  }
-  return std::max(current, previous.value());
+subresource_filter::ActivationPosition GetActivationPosition(
+    size_t match_index,
+    size_t num_checks) {
+  DCHECK_GT(num_checks, 0u);
+  if (num_checks == 1)
+    return subresource_filter::ActivationPosition::kOnly;
+  if (match_index == 0)
+    return subresource_filter::ActivationPosition::kFirst;
+  if (match_index == num_checks - 1)
+    return subresource_filter::ActivationPosition::kLast;
+  return subresource_filter::ActivationPosition::kMiddle;
 }
 
 }  // namespace
@@ -150,19 +155,28 @@ void SafeBrowsingTriggeredPopupBlocker::OnSafeBrowsingChecksComplete(
     content::NavigationHandle* navigation_handle,
     const SafeBrowsingCheckResults& results) {
   DCHECK(navigation_handle->IsInMainFrame());
-  base::Optional<safe_browsing::SubresourceFilterLevel> current_level;
-  for (const auto& result : results) {
-    if (result.threat_type ==
-        safe_browsing::SBThreatType::SB_THREAT_TYPE_SUBRESOURCE_FILTER) {
-      auto abusive = result.threat_metadata.subresource_filter_match.find(
-          safe_browsing::SubresourceFilterType::ABUSIVE);
-      if (abusive != result.threat_metadata.subresource_filter_match.end()) {
-        current_level = PickMostSevereLevel(current_level, abusive->second);
-      }
+  base::Optional<safe_browsing::SubresourceFilterLevel> match_level;
+  base::Optional<size_t> match_index;
+  for (size_t i = 0u; i < results.size(); ++i) {
+    const auto& result = results[i];
+    if (result.threat_type !=
+        safe_browsing::SBThreatType::SB_THREAT_TYPE_SUBRESOURCE_FILTER)
+      continue;
+
+    auto abusive = result.threat_metadata.subresource_filter_match.find(
+        safe_browsing::SubresourceFilterType::ABUSIVE);
+    if (abusive != result.threat_metadata.subresource_filter_match.end() &&
+        (!match_level.has_value() || match_level.value() < abusive->second)) {
+      match_level = abusive->second;
+      match_index = i;
     }
   }
-  if (current_level.has_value()) {
-    level_for_next_committed_navigation_ = current_level;
+
+  if (match_level.has_value()) {
+    level_for_next_committed_navigation_ = match_level;
+    UMA_HISTOGRAM_ENUMERATION(
+        "ContentSettings.Popups.StrongBlockerActivationPosition",
+        GetActivationPosition(match_index.value(), results.size()));
   }
 }
 

chrome/browser/ui/blocked_content/safe_browsing_triggered_popup_blocker_unittest.cc

@@ -19,6 +19,7 @@
 #include "chrome/test/base/testing_browser_process.h"
 #include "components/safe_browsing/db/v4_protocol_manager_util.h"
 #include "components/subresource_filter/content/browser/fake_safe_browsing_database_manager.h"
+#include "components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle.h"
 #include "components/subresource_filter/core/browser/subresource_filter_features.h"
 #include "content/public/test/navigation_simulator.h"
 #include "content/public/test/test_renderer_host.h"
@@ -401,3 +402,56 @@ TEST_F(SafeBrowsingTriggeredPopupBlockerTest, WarningMatch_OnlyLogs) {
 
   EXPECT_FALSE(popup_blocker()->ShouldApplyStrongPopupBlocker(nullptr));
 }
+
+TEST_F(SafeBrowsingTriggeredPopupBlockerTest, ActivationPosition) {
+  const GURL enforce_url("https://enforce.test/");
+  const GURL warn_url("https://warn.test/");
+  MarkUrlAsAbusiveEnforce(enforce_url);
+  MarkUrlAsAbusiveWarning(warn_url);
+
+  using subresource_filter::ActivationPosition;
+  struct {
+    std::vector<const char*> urls;
+    base::Optional<ActivationPosition> expected_position;
+  } kTestCases[] = {
+      {{"https://normal.test/"}, base::nullopt},
+      {{"https://enforce.test/"}, ActivationPosition::kOnly},
+      {{"https://warn.test/"}, ActivationPosition::kOnly},
+
+      {{"https://normal.test/", "https://warn.test/"},
+       ActivationPosition::kLast},
+      {{"https://normal.test/", "https://normal.test/",
+        "https://enforce.test/"},
+       ActivationPosition::kLast},
+
+      {{"https://enforce.test", "https://normal.test/", "https://warn.test/"},
+       ActivationPosition::kFirst},
+      {{"https://warn.test/", "https://normal.test/"},
+       ActivationPosition::kFirst},
+
+      {{"https://normal.test/", "https://enforce.test/",
+        "https://normal.test/"},
+       ActivationPosition::kMiddle},
+  };
+
+  for (const auto& test_case : kTestCases) {
+    base::HistogramTester histograms;
+    const GURL& first_url = GURL(test_case.urls.front());
+    auto navigation_simulator =
+        content::NavigationSimulator::CreateRendererInitiated(first_url,
+                                                              main_rfh());
+    for (size_t i = 1; i < test_case.urls.size(); ++i) {
+      navigation_simulator->Redirect(GURL(test_case.urls[i]));
+    }
+    navigation_simulator->Commit();
+
+    histograms.ExpectTotalCount(
+        "ContentSettings.Popups.StrongBlockerActivationPosition",
+        test_case.expected_position.has_value() ? 1 : 0);
+    if (test_case.expected_position.has_value()) {
+      histograms.ExpectUniqueSample(
+          "ContentSettings.Popups.StrongBlockerActivationPosition",
+          static_cast<int>(test_case.expected_position.value()), 1);
+    }
+  }
+}

tools/metrics/histograms/histograms.xml

@@ -13407,6 +13407,18 @@ uploading your change for review.
   </summary>
 </histogram>
 
+<histogram name="ContentSettings.Popups.StrongBlockerActivationPosition"
+    enum="SafeBrowsingActivationPosition">
+  <owner>csharrison@chromium.org</owner>
+  <owner>ericrobinson@chromium.org</owner>
+  <summary>
+    For pages that trigger Safe Browsing triggered popup blocker (in warn or
+    enforce modes), records the position in the redirect chain for the page the
+    activation was triggered by.  If SubresourceFilterConsiderRedirects is
+    disabled, then always returns &quot;Only navigation&quot;.
+  </summary>
+</histogram>
+
 <histogram name="ContentSuggestions.Feed.NetworkRequestStatusCode"
     enum="CombinedHttpResponseAndNetErrorCode">
   <owner>pnoland@chromium.org</owner>