Adding metrics for safe browsing activation on redirects.

Add a metric to indicate where in the redirect chain the safe browsing
activation occurred, either at the start, middle, or end, or in the
case where the navigation is direct, in the only position.

Bug: 851545
Change-Id: I39d0848aee825db287171a2c63862bdb434373a1
Reviewed-on: https://chromium-review.googlesource.com/1099647Reviewed-by: Alexei Svitkine <asvitkine@chromium.org>
Reviewed-by: Charlie Harrison <csharrison@chromium.org>
Commit-Queue: Eric Robinson <ericrobinson@chromium.org>
Cr-Commit-Position: refs/heads/master@{#568162}

components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle.cc

@@ -261,35 +261,57 @@ SubresourceFilterSafeBrowsingActivationThrottle::
 
 ActivationDecision
 SubresourceFilterSafeBrowsingActivationThrottle::GetActivationDecision(
-    const std::vector<ConfigResult>& configurations,
+    const std::vector<ConfigResult>& configs,
     ConfigResult* selected_config) {
-  auto selected_itr = configurations.begin();
-  for (auto current_itr = configurations.begin();
-       current_itr != configurations.end(); current_itr++) {
+  size_t selected_index = 0;
+  for (size_t current_index = 0; current_index < configs.size();
+       current_index++) {
     // Prefer later configs when there's a tie.
     // Rank no matching config slightly below priority zero.
-    const auto selected_priority =
-        selected_itr->matched_valid_configuration
-            ? selected_itr->config.activation_conditions.priority
+    const int selected_priority =
+        configs[selected_index].matched_valid_configuration
+            ? configs[selected_index].config.activation_conditions.priority
             : -1;
-    const auto current_priority =
-        current_itr->matched_valid_configuration
-            ? current_itr->config.activation_conditions.priority
+    const int current_priority =
+        configs[current_index].matched_valid_configuration
+            ? configs[current_index].config.activation_conditions.priority
             : -1;
     if (current_priority >= selected_priority) {
-      selected_itr = current_itr;
+      selected_index = current_index;
     }
   }
   // Ensure that the list was not empty, and assign the configuration.
-  DCHECK(selected_itr != configurations.end());
-  *selected_config = *selected_itr;
+  DCHECK(selected_index != configs.size());
+  *selected_config = configs[selected_index];
 
-  if (!selected_itr->matched_valid_configuration) {
+  if (!selected_config->matched_valid_configuration) {
     return ActivationDecision::ACTIVATION_CONDITIONS_NOT_MET;
   }
 
+  // Get the activation level for the matching configuration.
   auto activation_level =
       selected_config->config.activation_options.activation_level;
+
+  // If there is an activation triggered by the activation list (not a dry run),
+  // report where in the redirect chain it was triggered.
+  if (selected_config->config.activation_conditions.activation_scope ==
+          ActivationScope::ACTIVATION_LIST &&
+      activation_level == ActivationLevel::ENABLED) {
+    ActivationPosition position;
+    if (configs.size() == 1) {
+      position = ActivationPosition::kOnly;
+    } else if (selected_index == 0) {
+      position = ActivationPosition::kFirst;
+    } else if (selected_index == configs.size() - 1) {
+      position = ActivationPosition::kLast;
+    } else {
+      position = ActivationPosition::kMiddle;
+    }
+    UMA_HISTOGRAM_ENUMERATION(
+        "SubresourceFilter.PageLoad.Activation.RedirectPosition", position);
+  }
+
+  // Compute and return the activation decision.
   return activation_level == ActivationLevel::DISABLED
              ? ActivationDecision::ACTIVATION_DISABLED
              : ActivationDecision::ACTIVATED;

components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle.h

@@ -28,6 +28,14 @@ namespace subresource_filter {
 
 class SubresourceFilterClient;
 
+enum class ActivationPosition {
+  kOnly = 0,
+  kFirst = 1,
+  kMiddle = 2,
+  kLast = 3,
+  kMaxValue = kLast,
+};
+
 // Navigation throttle responsible for activating subresource filtering on page
 // loads that match the SUBRESOURCE_FILTER Safe Browsing list.
 class SubresourceFilterSafeBrowsingActivationThrottle
@@ -84,7 +92,7 @@ class SubresourceFilterSafeBrowsingActivationThrottle
   // Gets the ActivationDecision for the given Configuration.
   // Returns it, or ACTIVATION_CONDITIONS_NOT_MET if no Configuration.
   ActivationDecision GetActivationDecision(
-      const std::vector<ConfigResult>& configurations,
+      const std::vector<ConfigResult>& configs,
       ConfigResult* selected_config);
 
   // Returns whether a main-frame navigation satisfies the activation

components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle_unittest.cc

@@ -15,6 +15,7 @@
 #include "base/metrics/field_trial.h"
 #include "base/run_loop.h"
 #include "base/test/metrics/histogram_tester.h"
+#include "base/test/scoped_feature_list.h"
 #include "base/test/test_mock_time_task_runner.h"
 #include "build/build_config.h"
 #include "components/safe_browsing/db/test_database_manager.h"
@@ -894,6 +895,65 @@ TEST_P(SubresourceFilterSafeBrowsingActivationThrottleParamTest,
   tester().ExpectTotalCount(kSafeBrowsingCheckTime, 2);
 }
 
+struct RedirectSamplesAndResults {
+  std::vector<GURL> urls;
+  bool expected_activation;
+  ActivationPosition expected_position;
+};
+
+TEST_F(SubresourceFilterSafeBrowsingActivationThrottleTest,
+       ActivationTriggeredOnRedirect) {
+  // Turn on the feature to perform safebrowsing on redirects.
+  base::test::ScopedFeatureList scoped_feature_list;
+  scoped_feature_list.InitAndEnableFeature(
+      kSafeBrowsingSubresourceFilterConsiderRedirects);
+  std::string histogram_string =
+      "SubresourceFilter.PageLoad.Activation.RedirectPosition";
+
+  // Set up the urls for enforcement.
+  GURL enforce_url("https://example.enforce");
+  GURL warn_url("https://example.warning");
+  GURL normal_url("https://example.regular");
+  safe_browsing::SubresourceFilterType type =
+      safe_browsing::SubresourceFilterType::ABUSIVE;
+  safe_browsing::ThreatMetadata metadata;
+  metadata.subresource_filter_match[type] =
+      safe_browsing::SubresourceFilterLevel::WARN;
+  ConfigureForMatch(enforce_url,
+                    safe_browsing::SB_THREAT_TYPE_SUBRESOURCE_FILTER);
+  ConfigureForMatch(warn_url, safe_browsing::SB_THREAT_TYPE_SUBRESOURCE_FILTER,
+                    metadata);
+
+  // Check cases where there are multiple redirection.
+  const RedirectSamplesAndResults kTestCases[] = {
+      {{enforce_url, normal_url, normal_url}, true, ActivationPosition::kFirst},
+      {{warn_url, normal_url, enforce_url}, true, ActivationPosition::kLast},
+      {{enforce_url, normal_url, warn_url}, true, ActivationPosition::kFirst},
+      {{normal_url, enforce_url, warn_url}, true, ActivationPosition::kMiddle},
+      {{normal_url, normal_url}, false, ActivationPosition::kMaxValue},
+      {{enforce_url}, true, ActivationPosition::kOnly},
+  };
+  for (const auto& test_case : kTestCases) {
+    const base::HistogramTester histograms;
+    SimulateStartAndExpectProceed(test_case.urls[0]);
+    for (size_t index = 1; index < test_case.urls.size(); index++) {
+      SimulateRedirectAndExpectProceed(test_case.urls[index]);
+    }
+    RunUntilIdle();
+    SimulateCommitAndExpectProceed();
+    if (test_case.expected_activation) {
+      EXPECT_EQ(ActivationLevel::ENABLED,
+                *observer()->GetPageActivationForLastCommittedLoad());
+      histograms.ExpectUniqueSample(histogram_string,
+                                    test_case.expected_position, 1);
+    } else {
+      EXPECT_EQ(ActivationLevel::DISABLED,
+                *observer()->GetPageActivationForLastCommittedLoad());
+      histograms.ExpectTotalCount(histogram_string, 0);
+    }
+  }
+}
+
 // Disabled due to flaky failures: https://crbug.com/753669.
 TEST_P(SubresourceFilterSafeBrowsingActivationThrottleParamTest,
        DISABLED_ListMatchedOnStartWithRedirect_NoActivation) {

tools/metrics/histograms/enums.xml

@@ -40246,6 +40246,13 @@ Called by update_net_trust_anchors.py.-->
   <int value="1" label="Kill (He's dead, Jim!)"/>
 </enum>
 
+<enum name="SafeBrowsingActivationPosition">
+  <int value="0" label="Only URL in redirect chain"/>
+  <int value="1" label="First URL in redirect chain"/>
+  <int value="2" label="Any middle URL in redirect chain"/>
+  <int value="3" label="Last redirect in URL chain"/>
+</enum>
+
 <enum name="SafeBrowsingAppOptIn">
   <int value="0" label="No opt-in preference in manifest"/>
   <int value="1" label="Explicit opt-in via manifest"/>

tools/metrics/histograms/histograms.xml

@@ -96007,6 +96007,17 @@ uploading your change for review.
   </summary>
 </histogram>
 
+<histogram name="SubresourceFilter.PageLoad.Activation.RedirectPosition"
+    enum="SafeBrowsingActivationPosition">
+  <owner>ericrobinson@chromium.org</owner>
+  <summary>
+    For pages that trigger Safe Browsing activations (not including dry runs),
+    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="SubresourceFilter.PageLoad.Activation.WallDuration"
     units="microseconds">
   <owner>csharrison@chromium.org</owner>