[subresource_filter] Decide UNSUPPORTED_SCHEME only if we'd otherwise activate

Right now the subresource filter reports UNSUPPORTED_SCHEME even if the
feature is disabled and the navigation would never have activated. This
patch aligns things closer to how they were before [1] landed, so that
UNSUPPORTED_SCHEME is not logged if the matched configuration's
activation level is DISABLED or there is no matched configuration.

BUG=todo

Review-Url: https://codereview.chromium.org/2888163002
Cr-Commit-Position: refs/heads/master@{#473093}

Author: csharrison

chrome/browser/subresource_filter/subresource_filter_unittest.cc

@@ -33,6 +33,7 @@
 #include "components/subresource_filter/core/browser/ruleset_service.h"
 #include "components/subresource_filter/core/browser/subresource_filter_features.h"
 #include "components/subresource_filter/core/browser/subresource_filter_features_test_support.h"
+#include "components/subresource_filter/core/common/activation_decision.h"
 #include "components/subresource_filter/core/common/test_ruleset_creator.h"
 #include "content/public/browser/navigation_throttle.h"
 #include "content/public/test/navigation_simulator.h"
@@ -256,3 +257,42 @@ TEST_F(SubresourceFilterTest, ExplicitWhitelisting_ShouldNotClearMetadata) {
   // blacklist.
   EXPECT_NE(nullptr, settings_manager()->GetSiteMetadata(url));
 }
+
+TEST_F(SubresourceFilterTest,
+       NavigationToBadSchemeUrlWithNoActivation_DoesNotReportBadScheme) {
+  // Don't report UNSUPPORTED_SCHEME if the navigation has no matching
+  // configuration.
+  scoped_configuration().ResetConfiguration(subresource_filter::Configuration(
+      subresource_filter::ActivationLevel::DISABLED,
+      subresource_filter::ActivationScope::NO_SITES));
+
+  GURL url("data:text/html,hello world");
+  SimulateNavigateAndCommit(url, main_rfh());
+  EXPECT_TRUE(CreateAndNavigateDisallowedSubframe(main_rfh()));
+  auto* driver_factory = subresource_filter::
+      ContentSubresourceFilterDriverFactory::FromWebContents(web_contents());
+  EXPECT_EQ(
+      subresource_filter::ActivationDecision::ACTIVATION_CONDITIONS_NOT_MET,
+      driver_factory->GetActivationDecisionForLastCommittedPageLoad());
+
+  // Also don't report UNSUPPORTED_SCHEME if the navigation matches a
+  // configuration with DISABLED activation level.
+  scoped_configuration().ResetConfiguration(subresource_filter::Configuration(
+      subresource_filter::ActivationLevel::DISABLED,
+      subresource_filter::ActivationScope::ALL_SITES));
+
+  SimulateNavigateAndCommit(url, main_rfh());
+  EXPECT_TRUE(CreateAndNavigateDisallowedSubframe(main_rfh()));
+  EXPECT_EQ(subresource_filter::ActivationDecision::ACTIVATION_DISABLED,
+            driver_factory->GetActivationDecisionForLastCommittedPageLoad());
+
+  // Sanity check that UNSUPPORTED_SCHEME is reported if the navigation does
+  // activate.
+  scoped_configuration().ResetConfiguration(subresource_filter::Configuration(
+      subresource_filter::ActivationLevel::ENABLED,
+      subresource_filter::ActivationScope::ALL_SITES));
+  SimulateNavigateAndCommit(url, main_rfh());
+  EXPECT_TRUE(CreateAndNavigateDisallowedSubframe(main_rfh()));
+  EXPECT_EQ(subresource_filter::ActivationDecision::UNSUPPORTED_SCHEME,
+            driver_factory->GetActivationDecisionForLastCommittedPageLoad());
+}

components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc

@@ -127,25 +127,28 @@ void ContentSubresourceFilterDriverFactory::OnSafeBrowsingMatchComputed(
   throttle_manager_->NotifyPageActivationComputed(navigation_handle, state);
 }
 
+// Be careful when modifying this method, as the order in which the
+// activation_decision_ is decided is very important and corresponds to UMA
+// metrics. In general we want to follow the pattern that
+// ACTIVATION_CONDITIONS_NOT_MET will always be logged if no configuration
+// matches this navigation. We log other decisions only if a configuration
+// matches and also would have activated.
 void ContentSubresourceFilterDriverFactory::
     ComputeActivationForMainFrameNavigation(
         content::NavigationHandle* navigation_handle,
         ActivationList matched_list) {
   const GURL& url(navigation_handle->GetURL());
 
-  if (!url.SchemeIsHTTPOrHTTPS()) {
-    activation_decision_ = ActivationDecision::UNSUPPORTED_SCHEME;
-    activation_options_ = Configuration::ActivationOptions();
-    return;
-  }
-
+  bool scheme_is_http_or_https = url.SchemeIsHTTPOrHTTPS();
   const auto config_list = GetEnabledConfigurations();
   const auto highest_priority_activated_config =
       std::find_if(config_list->configs_by_decreasing_priority().begin(),
                    config_list->configs_by_decreasing_priority().end(),
-                   [&url, matched_list, this](const Configuration& config) {
+                   [&url, scheme_is_http_or_https, matched_list,
+                    this](const Configuration& config) {
                      return DoesMainFrameURLSatisfyActivationConditions(
-                         url, config.activation_conditions, matched_list);
+                         url, scheme_is_http_or_https,
+                         config.activation_conditions, matched_list);
                    });
 
   bool has_activated_config =
@@ -164,7 +167,18 @@ void ContentSubresourceFilterDriverFactory::
     return;
   }
 
-  activation_options_ = highest_priority_activated_config->activation_options;
+  const Configuration::ActivationOptions& activation_options =
+      highest_priority_activated_config->activation_options;
+
+  // Log UNSUPPORTED_SCHEME if we would have otherwise activated.
+  if (!scheme_is_http_or_https &&
+      activation_options.activation_level != ActivationLevel::DISABLED) {
+    activation_decision_ = ActivationDecision::UNSUPPORTED_SCHEME;
+    activation_options_ = Configuration::ActivationOptions();
+    return;
+  }
+
+  activation_options_ = activation_options;
   activation_decision_ =
       activation_options_.activation_level == ActivationLevel::DISABLED
           ? ActivationDecision::ACTIVATION_DISABLED
@@ -174,12 +188,18 @@ void ContentSubresourceFilterDriverFactory::
 bool ContentSubresourceFilterDriverFactory::
     DoesMainFrameURLSatisfyActivationConditions(
         const GURL& url,
+        bool scheme_is_http_or_https,
         const Configuration::ActivationConditions& conditions,
         ActivationList matched_list) const {
+  // scheme_is_http_or_https implies url.SchemeIsHTTPOrHTTPS().
+  DCHECK(!scheme_is_http_or_https || url.SchemeIsHTTPOrHTTPS());
   switch (conditions.activation_scope) {
     case ActivationScope::ALL_SITES:
       return true;
     case ActivationScope::ACTIVATION_LIST:
+      // ACTIVATION_LIST does not support non http/s URLs.
+      if (!scheme_is_http_or_https)
+        return false;
       if (conditions.activation_list == matched_list)
         return true;
       if (conditions.activation_list == ActivationList::PHISHING_INTERSTITIAL &&

components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h

@@ -101,8 +101,10 @@ class ContentSubresourceFilterDriverFactory
 
   // Returns whether a main-frame navigation to the given |url| satisfies the
   // activation |conditions| of a given configuration, except for |priority|.
+  // Pass |scheme_is_http_or_https| to avoid multiple string comparisons.
   bool DoesMainFrameURLSatisfyActivationConditions(
       const GURL& url,
+      bool scheme_is_http_or_https,
       const Configuration::ActivationConditions& conditions,
       ActivationList matched_list) const;
 

components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc

@@ -734,9 +734,17 @@ TEST_P(ContentSubresourceFilterDriverFactoryActivationScopeTest,
                                   "https://example.test"};
   for (auto* url : unsupported_urls) {
     SCOPED_TRACE(url);
+    ActivationDecision expected_decision =
+        ActivationDecision::UNSUPPORTED_SCHEME;
+    // We only log UNSUPPORTED_SCHEME if the navigation would have otherwise
+    // activated. Note that non http/s URLs will never match an activation list.
+    if (test_data.expected_activation_decision ==
+            ActivationDecision::ACTIVATION_CONDITIONS_NOT_MET ||
+        test_data.activation_scope == ActivationScope::ACTIVATION_LIST) {
+      expected_decision = ActivationDecision::ACTIVATION_CONDITIONS_NOT_MET;
+    }
     NavigateAndExpectActivation({test_data.url_matches_activation_list},
-                                {GURL(url)},
-                                ActivationDecision::UNSUPPORTED_SCHEME);
+                                {GURL(url)}, expected_decision);
   }
   for (auto* url : supported_urls) {
     SCOPED_TRACE(url);