Break ties in DNR priorities based on action type

The order of priority is: allow > allowAllRequests > block >
upgradeScheme > redirect.

This is implemented by including the action in the lower 8 bits of the
priority that is persisted in the flatbuffer ruleset. To make room for
this, the peristed priority is expanded from uint to uint64.

Bug: 1026733
Change-Id: I3207175bd3a92a2bd945ca428f493d1af3252739
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1952284
Commit-Queue: Theodore Dubois <tbodt@google.com>
Reviewed-by: Karan Bhatia <karandeepb@chromium.org>
Reviewed-by: Charlie Harrison <csharrison@chromium.org>
Cr-Commit-Position: refs/heads/master@{#734713}

chrome/browser/extensions/api/declarative_net_request/ruleset_manager_unittest.cc

@@ -164,8 +164,9 @@ TEST_P(RulesetManagerTest, MultipleRulesets) {
     manager()->EvaluateRequest(request, false /*is_incognito_context*/);
     return !request.dnr_actions->empty() &&
            ((*request.dnr_actions)[0] ==
-            RequestAction(RequestActionType::BLOCK, rule_id, kDefaultPriority,
-                          dnr_api::SOURCE_TYPE_MANIFEST, extension_id));
+            CreateRequestActionForTesting(
+                RequestActionType::BLOCK, rule_id, kDefaultPriority,
+                dnr_api::SOURCE_TYPE_MANIFEST, extension_id));
   };
 
   for (int mask = 0; mask < 4; mask++) {
@@ -245,9 +246,9 @@ TEST_P(RulesetManagerTest, IncognitoRequests) {
 
   manager()->EvaluateRequest(request_info, false /*is_incognito_context*/);
   ASSERT_EQ(1u, request_info.dnr_actions->size());
-  EXPECT_EQ(RequestAction(RequestActionType::BLOCK, *rule_one.id,
-                          kDefaultPriority, dnr_api::SOURCE_TYPE_MANIFEST,
-                          last_loaded_extension()->id()),
+  EXPECT_EQ(CreateRequestActionForTesting(
+                RequestActionType::BLOCK, *rule_one.id, kDefaultPriority,
+                dnr_api::SOURCE_TYPE_MANIFEST, last_loaded_extension()->id()),
             (*request_info.dnr_actions)[0]);
   request_info.dnr_actions.reset();
 
@@ -259,17 +260,17 @@ TEST_P(RulesetManagerTest, IncognitoRequests) {
 
   manager()->EvaluateRequest(request_info, true /*is_incognito_context*/);
   ASSERT_EQ(1u, request_info.dnr_actions->size());
-  EXPECT_EQ(RequestAction(RequestActionType::BLOCK, *rule_one.id,
-                          kDefaultPriority, dnr_api::SOURCE_TYPE_MANIFEST,
-                          last_loaded_extension()->id()),
+  EXPECT_EQ(CreateRequestActionForTesting(
+                RequestActionType::BLOCK, *rule_one.id, kDefaultPriority,
+                dnr_api::SOURCE_TYPE_MANIFEST, last_loaded_extension()->id()),
             (*request_info.dnr_actions)[0]);
   request_info.dnr_actions.reset();
 
   manager()->EvaluateRequest(request_info, false /*is_incognito_context*/);
   ASSERT_EQ(1u, request_info.dnr_actions->size());
-  EXPECT_EQ(RequestAction(RequestActionType::BLOCK, *rule_one.id,
-                          kDefaultPriority, dnr_api::SOURCE_TYPE_MANIFEST,
-                          last_loaded_extension()->id()),
+  EXPECT_EQ(CreateRequestActionForTesting(
+                RequestActionType::BLOCK, *rule_one.id, kDefaultPriority,
+                dnr_api::SOURCE_TYPE_MANIFEST, last_loaded_extension()->id()),
             (*request_info.dnr_actions)[0]);
   request_info.dnr_actions.reset();
 }
@@ -313,9 +314,9 @@ TEST_P(RulesetManagerTest, TotalEvaluationTimeHistogram) {
 
     manager()->EvaluateRequest(example_com_request, is_incognito_context);
     ASSERT_EQ(1u, example_com_request.dnr_actions->size());
-    EXPECT_EQ(RequestAction(RequestActionType::BLOCK, *rule.id,
-                            kDefaultPriority, dnr_api::SOURCE_TYPE_MANIFEST,
-                            last_loaded_extension()->id()),
+    EXPECT_EQ(CreateRequestActionForTesting(
+                  RequestActionType::BLOCK, *rule.id, kDefaultPriority,
+                  dnr_api::SOURCE_TYPE_MANIFEST, last_loaded_extension()->id()),
               (*example_com_request.dnr_actions)[0]);
 
     tester.ExpectTotalCount(kHistogramName, 1);
@@ -349,7 +350,7 @@ TEST_P(RulesetManagerTest, Redirect) {
   // redirected to "google.com".
   const bool is_incognito_context = false;
   const char* kExampleURL = "http://example.com";
-  RequestAction expected_redirect_action(
+  RequestAction expected_redirect_action = CreateRequestActionForTesting(
       RequestActionType::REDIRECT, *rule.id, *rule.priority,
       dnr_api::SOURCE_TYPE_MANIFEST, last_loaded_extension()->id());
   expected_redirect_action.redirect_url = GURL("http://google.com");
@@ -425,10 +426,10 @@ TEST_P(RulesetManagerTest, ExtensionScheme) {
   WebRequestInfo request_1(GetRequestParamsForURL("http://example.com"));
   manager()->EvaluateRequest(request_1, false /*is_incognito_context*/);
   ASSERT_EQ(1u, request_1.dnr_actions->size());
-  EXPECT_EQ(
-      RequestAction(RequestActionType::BLOCK, kMinValidID, kDefaultPriority,
-                    dnr_api::SOURCE_TYPE_MANIFEST, extension_1->id()),
-      (*request_1.dnr_actions)[0]);
+  EXPECT_EQ(CreateRequestActionForTesting(
+                RequestActionType::BLOCK, kMinValidID, kDefaultPriority,
+                dnr_api::SOURCE_TYPE_MANIFEST, extension_1->id()),
+            (*request_1.dnr_actions)[0]);
 
   // Ensure that the background page for |extension_1| won't be blocked or
   // redirected.
@@ -512,7 +513,7 @@ TEST_P(RulesetManagerTest, RemoveHeaders) {
 
   // Removal of the cookie header should be attributed to |extension_2| because
   // it was installed later than |extension_1| and thus has more priority.
-  RequestAction expected_action_1(
+  RequestAction expected_action_1 = CreateRequestActionForTesting(
       RequestActionType::REMOVE_HEADERS, kMinValidID, kDefaultPriority,
       dnr_api::SOURCE_TYPE_MANIFEST, extension_2->id());
   expected_action_1.request_headers_to_remove.push_back(
@@ -522,7 +523,7 @@ TEST_P(RulesetManagerTest, RemoveHeaders) {
   expected_action_1.request_headers_to_remove.push_back(
       net::HttpRequestHeaders::kReferer);
 
-  RequestAction expected_action_2(
+  RequestAction expected_action_2 = CreateRequestActionForTesting(
       RequestActionType::REMOVE_HEADERS, kMinValidID, kDefaultPriority,
       dnr_api::SOURCE_TYPE_MANIFEST, extension_1->id());
   expected_action_2.response_headers_to_remove.push_back("set-cookie");
@@ -689,11 +690,11 @@ TEST_P(RulesetManagerTest, PageAllowingAPI) {
 
     if (test_case.expect_blocked_with_allowed_pages) {
       ASSERT_EQ(1u, actions.size());
-      EXPECT_EQ(
-          RequestAction(RequestActionType::BLOCK, *test_case.matched_rule_id,
-                        kDefaultPriority, dnr_api::SOURCE_TYPE_MANIFEST,
-                        last_loaded_extension()->id()),
-          actions[0]);
+      EXPECT_EQ(CreateRequestActionForTesting(
+                    RequestActionType::BLOCK, *test_case.matched_rule_id,
+                    kDefaultPriority, dnr_api::SOURCE_TYPE_MANIFEST,
+                    last_loaded_extension()->id()),
+                actions[0]);
     } else {
       EXPECT_TRUE(actions.empty());
     }
@@ -783,7 +784,7 @@ TEST_P(RulesetManagerTest, HostPermissionForInitiator) {
     manager()->AddRuleset(redirect_extension_id, std::move(redirect_matcher),
                           URLPatternSet());
     for (const auto& test : cases) {
-      RequestAction redirect_action(
+      RequestAction redirect_action = CreateRequestActionForTesting(
           RequestActionType::REDIRECT, kMinValidID, kMinValidPriority,
           dnr_api::SOURCE_TYPE_MANIFEST, redirect_extension_id);
       redirect_action.redirect_url = GURL("https://foo.com/");
@@ -801,7 +802,7 @@ TEST_P(RulesetManagerTest, HostPermissionForInitiator) {
     manager()->AddRuleset(blocking_extension_id, std::move(blocking_matcher),
                           URLPatternSet());
     for (const auto& test : cases) {
-      RequestAction block_action(
+      RequestAction block_action = CreateRequestActionForTesting(
           RequestActionType::BLOCK, kMinValidID, kDefaultPriority,
           dnr_api::SOURCE_TYPE_MANIFEST, blocking_extension_id);
 

components/subresource_filter/core/common/indexed_ruleset.cc

@@ -52,12 +52,12 @@ VerifyStatus GetVerifyStatus(const uint8_t* buffer,
 
 // RulesetIndexer --------------------------------------------------------------
 
-const int RulesetIndexer::kIndexedFormatVersion = 26;
+const int RulesetIndexer::kIndexedFormatVersion = 27;
 
 // This static assert is meant to catch cases where
 // url_pattern_index::kUrlPatternIndexFormatVersion is incremented without
 // updating RulesetIndexer::kIndexedFormatVersion.
-static_assert(url_pattern_index::kUrlPatternIndexFormatVersion == 5,
+static_assert(url_pattern_index::kUrlPatternIndexFormatVersion == 6,
               "kUrlPatternIndexFormatVersion has changed, make sure you've "
               "also updated RulesetIndexer::kIndexedFormatVersion above.");
 

components/url_pattern_index/flat/url_pattern_index.fbs

@@ -104,7 +104,7 @@ table UrlRule {
   id : uint;
 
   // Priority of the rule. Larger the value, greater the priority.
-  priority : uint;
+  priority : uint64;
 }
 
 // Contains an N-gram (acting as a key in a hash table) and a list of URL rules

components/url_pattern_index/url_pattern_index.h

@@ -83,7 +83,7 @@ int CompareDomains(base::StringPiece lhs_domain, base::StringPiece rhs_domain);
 // Increase this value when introducing an incompatible change to the
 // UrlPatternIndex schema (flat/url_pattern_index.fbs). url_pattern_index
 // clients can use this as a signal to rebuild rulesets.
-constexpr int kUrlPatternIndexFormatVersion = 5;
+constexpr int kUrlPatternIndexFormatVersion = 6;
 
 // The class used to construct an index over the URL patterns of a set of URL
 // rules. The rules themselves need to be converted to FlatBuffers format by the

extensions/browser/api/declarative_net_request/indexed_rule.cc

@@ -382,25 +382,42 @@ ParseResult ParseRedirect(dnr_api::Redirect redirect,
 
 bool DoesActionSupportPriority(dnr_api::RuleActionType type) {
   switch (type) {
-    case dnr_api::RULE_ACTION_TYPE_NONE:
-      break;
     case dnr_api::RULE_ACTION_TYPE_BLOCK:
-      return true;
     case dnr_api::RULE_ACTION_TYPE_REDIRECT:
-      return true;
     case dnr_api::RULE_ACTION_TYPE_ALLOW:
-      return true;
     case dnr_api::RULE_ACTION_TYPE_UPGRADESCHEME:
+    case dnr_api::RULE_ACTION_TYPE_ALLOWALLREQUESTS:
       return true;
     case dnr_api::RULE_ACTION_TYPE_REMOVEHEADERS:
       return false;
-    case dnr_api::RULE_ACTION_TYPE_ALLOWALLREQUESTS:
-      return true;
+    case dnr_api::RULE_ACTION_TYPE_NONE:
+      break;
   }
   NOTREACHED();
   return false;
 }
 
+uint8_t GetActionTypePriority(dnr_api::RuleActionType action_type) {
+  switch (action_type) {
+    case dnr_api::RULE_ACTION_TYPE_ALLOW:
+      return 5;
+    case dnr_api::RULE_ACTION_TYPE_ALLOWALLREQUESTS:
+      return 4;
+    case dnr_api::RULE_ACTION_TYPE_BLOCK:
+      return 3;
+    case dnr_api::RULE_ACTION_TYPE_UPGRADESCHEME:
+      return 2;
+    case dnr_api::RULE_ACTION_TYPE_REDIRECT:
+      return 1;
+    case dnr_api::RULE_ACTION_TYPE_REMOVEHEADERS:
+      return 0;
+    case dnr_api::RULE_ACTION_TYPE_NONE:
+      break;
+  }
+  NOTREACHED();
+  return 0;
+}
+
 }  // namespace
 
 IndexedRule::IndexedRule() = default;
@@ -493,8 +510,10 @@ ParseResult IndexedRule::CreateIndexedRule(dnr_api::Rule parsed_rule,
 
   indexed_rule->action_type = parsed_rule.action.type;
   indexed_rule->id = base::checked_cast<uint32_t>(parsed_rule.id);
-  indexed_rule->priority = base::checked_cast<uint32_t>(
-      is_priority_supported ? *parsed_rule.priority : kDefaultPriority);
+  indexed_rule->priority = parsed_rule.priority ? ComputeIndexedRulePriority(
+                                                      *parsed_rule.priority,
+                                                      indexed_rule->action_type)
+                                                : kDefaultPriority;
   indexed_rule->options = GetOptionsMask(parsed_rule);
   indexed_rule->activation_types = GetActivationTypes(parsed_rule);
 
@@ -560,5 +579,16 @@ ParseResult IndexedRule::CreateIndexedRule(dnr_api::Rule parsed_rule,
   return ParseResult::SUCCESS;
 }
 
+uint64_t ComputeIndexedRulePriority(int parsed_rule_priority,
+                                    dnr_api::RuleActionType action_type) {
+  if (!DoesActionSupportPriority(action_type))
+    return kDefaultPriority;
+  // Incorporate the action's priority into the rule priority, so e.g. allow
+  // rules will be given a higher priority than block rules with the same
+  // priority specified in the rule JSON.
+  return (base::checked_cast<uint32_t>(parsed_rule_priority) << 8) |
+         GetActionTypePriority(action_type);
+}
+
 }  // namespace declarative_net_request
 }  // namespace extensions

extensions/browser/api/declarative_net_request/indexed_rule.h

@@ -71,6 +71,12 @@ struct IndexedRule {
   DISALLOW_COPY_AND_ASSIGN(IndexedRule);
 };
 
+// Compute the rule priority for indexing, by combining the priority from
+// the JSON rule and the priority of the action type. Exposed for testing.
+uint64_t ComputeIndexedRulePriority(
+    int parsed_rule_priority,
+    api::declarative_net_request::RuleActionType action_type);
+
 }  // namespace declarative_net_request
 }  // namespace extensions
 

extensions/browser/api/declarative_net_request/indexed_rule_unittest.cc

@@ -134,7 +134,9 @@ TEST_F(IndexedRuleTest, PriorityParsing) {
 
     EXPECT_EQ(cases[i].expected_result, result);
     if (result == ParseResult::SUCCESS)
-      EXPECT_EQ(cases[i].expected_priority, indexed_rule.priority);
+      EXPECT_EQ(ComputeIndexedRulePriority(cases[i].expected_priority,
+                                           cases[i].action_type),
+                indexed_rule.priority);
   }
 
   // Ensure priority is ignored for non-before-request rules.

extensions/browser/api/declarative_net_request/request_action.cc

@@ -10,12 +10,12 @@ namespace declarative_net_request {
 RequestAction::RequestAction(
     RequestAction::Type type,
     uint32_t rule_id,
-    uint32_t rule_priority,
+    uint64_t index_priority,
     api::declarative_net_request::SourceType source_type,
     const ExtensionId& extension_id)
     : type(type),
       rule_id(rule_id),
-      rule_priority(rule_priority),
+      index_priority(index_priority),
       source_type(source_type),
       extension_id(extension_id) {}
 RequestAction::~RequestAction() = default;
@@ -36,8 +36,8 @@ base::Optional<RequestAction> GetMaxPriorityAction(
     return rhs;
   if (!rhs)
     return lhs;
-  return lhs->rule_priority > rhs->rule_priority ? std::move(lhs)
-                                                 : std::move(rhs);
+  return lhs->index_priority > rhs->index_priority ? std::move(lhs)
+                                                   : std::move(rhs);
 }
 
 }  // namespace declarative_net_request

extensions/browser/api/declarative_net_request/request_action.h

@@ -41,7 +41,7 @@ struct RequestAction {
 
   RequestAction(Type type,
                 uint32_t rule_id,
-                uint32_t rule_priority,
+                uint64_t index_priority,
                 api::declarative_net_request::SourceType source_type,
                 const ExtensionId& extension_id);
   ~RequestAction();
@@ -59,8 +59,9 @@ struct RequestAction {
   // The ID of the matching rule for this action.
   uint32_t rule_id;
 
-  // The priority of the matching rule for this action.
-  uint32_t rule_priority;
+  // The priority of this action in the index. This is a combination of the
+  // rule's priority and the rule's action's priority.
+  uint64_t index_priority;
 
   // The source type of the matching rule for this action.
   api::declarative_net_request::SourceType source_type;

extensions/browser/api/declarative_net_request/test_utils.cc

@@ -12,6 +12,7 @@
 #include "base/files/file_util.h"
 #include "base/json/json_file_value_serializer.h"
 #include "base/logging.h"
+#include "extensions/browser/api/declarative_net_request/indexed_rule.h"
 #include "extensions/browser/api/declarative_net_request/ruleset_matcher.h"
 #include "extensions/browser/api/declarative_net_request/ruleset_source.h"
 #include "extensions/browser/extension_prefs.h"
@@ -30,7 +31,26 @@ RequestAction CreateRequestActionForTesting(RequestAction::Type type,
                                             uint32_t rule_priority,
                                             dnr_api::SourceType source_type,
                                             const ExtensionId& extension_id) {
-  return RequestAction(type, rule_id, rule_priority, source_type, extension_id);
+  dnr_api::RuleActionType action = [type] {
+    switch (type) {
+      case RequestAction::Type::BLOCK:
+      case RequestAction::Type::COLLAPSE:
+        return dnr_api::RULE_ACTION_TYPE_BLOCK;
+      case RequestAction::Type::ALLOW:
+        return dnr_api::RULE_ACTION_TYPE_ALLOW;
+      case RequestAction::Type::REDIRECT:
+        return dnr_api::RULE_ACTION_TYPE_REDIRECT;
+      case RequestAction::Type::UPGRADE:
+        return dnr_api::RULE_ACTION_TYPE_UPGRADESCHEME;
+      case RequestAction::Type::REMOVE_HEADERS:
+        return dnr_api::RULE_ACTION_TYPE_REMOVEHEADERS;
+      case RequestAction::Type::ALLOW_ALL_REQUESTS:
+        return dnr_api::RULE_ACTION_TYPE_ALLOWALLREQUESTS;
+    }
+  }();
+  return RequestAction(type, rule_id,
+                       ComputeIndexedRulePriority(rule_priority, action),
+                       source_type, extension_id);
 }
 
 // Note: This is not declared in the anonymous namespace so that we can use it
@@ -49,7 +69,7 @@ bool operator==(const RequestAction& lhs, const RequestAction& rhs) {
 
   auto get_members_tuple = [](const RequestAction& action) {
     return std::tie(action.type, action.redirect_url, action.rule_id,
-                    action.rule_priority, action.source_type,
+                    action.index_priority, action.source_type,
                     action.extension_id);
   };
 
@@ -95,7 +115,7 @@ std::ostream& operator<<(std::ostream& output, const RequestAction& action) {
                                  : std::string("nullopt"))
          << "\n";
   output << "|rule_id| " << action.rule_id << "\n";
-  output << "|rule_priority| " << action.rule_priority << "\n";
+  output << "|index_priority| " << action.index_priority << "\n";
   output << "|source_type| "
          << api::declarative_net_request::ToString(action.source_type) << "\n";
   output << "|extension_id| " << action.extension_id << "\n";

extensions/browser/api/declarative_net_request/utils.cc

@@ -42,7 +42,7 @@ constexpr int kIndexedRulesetFormatVersion = 15;
 // This static assert is meant to catch cases where
 // url_pattern_index::kUrlPatternIndexFormatVersion is incremented without
 // updating kIndexedRulesetFormatVersion.
-static_assert(url_pattern_index::kUrlPatternIndexFormatVersion == 5,
+static_assert(url_pattern_index::kUrlPatternIndexFormatVersion == 6,
               "kUrlPatternIndexFormatVersion has changed, make sure you've "
               "also updated kIndexedRulesetFormatVersion above.");