[DNR] Refactor: Add extension id to CompositeMatcher

Since a CompositeMatcher can only be associated with exactly one
extension, add the extension id field to the class, so that in
RequestParams, the allow_rule_max_priority map can be keyed by
extension id instead of a pointer to a CompositeMatcher. This is safer
if the map will be used between request phases as currently the
CompositeMatcher gets deleted if the extension unloads, which will
invalidate the map's key.

This CL has no behavior change.

Bug: NONE
Change-Id: I013ef29d173ed1915458a10eee3cbc15c5e1a697
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5384193
Reviewed-by: Devlin Cronin <rdevlin.cronin@chromium.org>
Auto-Submit: Kelvin Jiang <kelvinjiang@chromium.org>
Commit-Queue: Devlin Cronin <rdevlin.cronin@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1277893}

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

@@ -106,7 +106,8 @@ class RulesetManagerTest : public DNRTestBase {
         sources[0].CreateVerifiedMatcher(expected_checksum, &matchers[0]));
 
     *matcher = std::make_unique<CompositeMatcher>(
-        std::move(matchers), HostPermissionsAlwaysRequired::kFalse);
+        std::move(matchers), last_loaded_extension_->id(),
+        HostPermissionsAlwaysRequired::kFalse);
   }
 
   void SetIncognitoEnabled(const Extension* extension, bool incognito_enabled) {

extensions/browser/api/declarative_net_request/composite_matcher.cc

@@ -83,8 +83,11 @@ ActionInfo::ActionInfo(ActionInfo&&) = default;
 ActionInfo& ActionInfo::operator=(ActionInfo&& other) = default;
 
 CompositeMatcher::CompositeMatcher(MatcherList matchers,
+                                   const ExtensionId& extension_id,
                                    HostPermissionsAlwaysRequired mode)
-    : matchers_(std::move(matchers)), host_permissions_always_required_(mode) {
+    : matchers_(std::move(matchers)),
+      extension_id_(extension_id),
+      host_permissions_always_required_(mode) {
   DCHECK(AreIDsUnique(matchers_));
 }
 
@@ -170,7 +173,7 @@ ActionInfo CompositeMatcher::GetAction(
         GetMaxPriorityAction(std::move(final_action), std::move(action));
   }
 
-  params.allow_rule_max_priority[this] = max_allow_rule_priority;
+  params.allow_rule_max_priority[extension_id_] = max_allow_rule_priority;
 
   if (!final_action)
     return ActionInfo();
@@ -192,12 +195,12 @@ ActionInfo CompositeMatcher::GetAction(
 std::vector<RequestAction> CompositeMatcher::GetModifyHeadersActions(
     const RequestParams& params) const {
   std::vector<RequestAction> modify_headers_actions;
-  DCHECK(params.allow_rule_max_priority.contains(this));
+  DCHECK(params.allow_rule_max_priority.contains(extension_id_));
 
   // The priority of the highest priority matching allow or allowAllRequests
   // rule within this matcher, or std::nullopt if no such rule exists.
   std::optional<uint64_t> max_allow_rule_priority =
-      params.allow_rule_max_priority[this];
+      params.allow_rule_max_priority[extension_id_];
 
   for (const auto& matcher : matchers_) {
     // Plumb |max_allow_rule_priority| into GetModifyHeadersActions so that

extensions/browser/api/declarative_net_request/composite_matcher.h

@@ -54,7 +54,9 @@ class CompositeMatcher {
   using MatcherList = std::vector<std::unique_ptr<RulesetMatcher>>;
 
   // Each RulesetMatcher should have a distinct RulesetID.
-  CompositeMatcher(MatcherList matchers, HostPermissionsAlwaysRequired mode);
+  CompositeMatcher(MatcherList matchers,
+                   const ExtensionId& extension_id,
+                   HostPermissionsAlwaysRequired mode);
 
   CompositeMatcher(const CompositeMatcher&) = delete;
   CompositeMatcher& operator=(const CompositeMatcher&) = delete;
@@ -120,6 +122,9 @@ class CompositeMatcher {
   // be taken to reset this as this object is modified.
   mutable std::optional<bool> has_any_extra_headers_matcher_;
 
+  // The id of the extension associated with this matcher.
+  const ExtensionId extension_id_;
+
   const HostPermissionsAlwaysRequired host_permissions_always_required_;
 };
 

extensions/browser/api/declarative_net_request/composite_matcher_unittest.cc

@@ -89,7 +89,8 @@ TEST_F(CompositeMatcherTest, SamePrioritySpace) {
   matchers.push_back(std::move(allow_matcher));
   matchers.push_back(std::move(block_matcher));
   auto composite_matcher = std::make_unique<CompositeMatcher>(
-      std::move(matchers), HostPermissionsAlwaysRequired::kFalse);
+      std::move(matchers), /*extension_id=*/"",
+      HostPermissionsAlwaysRequired::kFalse);
 
   GURL google_url("http://google.com");
   RequestParams params;
@@ -113,7 +114,8 @@ TEST_F(CompositeMatcherTest, SamePrioritySpace) {
   matchers.push_back(std::move(allow_matcher));
   matchers.push_back(std::move(block_matcher));
   composite_matcher = std::make_unique<CompositeMatcher>(
-      std::move(matchers), HostPermissionsAlwaysRequired::kFalse);
+      std::move(matchers), /*extension_id=*/"",
+      HostPermissionsAlwaysRequired::kFalse);
 
   // The allow rule should now have higher priority.
   action_info = composite_matcher->GetAction(
@@ -157,7 +159,8 @@ TEST_F(CompositeMatcherTest, GetModifyHeadersActions) {
   matchers.push_back(std::move(matcher_1));
   matchers.push_back(std::move(matcher_2));
   auto composite_matcher = std::make_unique<CompositeMatcher>(
-      std::move(matchers), HostPermissionsAlwaysRequired::kFalse);
+      std::move(matchers), /*extension_id=*/"",
+      HostPermissionsAlwaysRequired::kFalse);
 
   GURL google_url = GURL("http://google.com/path");
   RequestParams google_params;
@@ -214,7 +217,8 @@ TEST_F(CompositeMatcherTest, GetModifyHeadersActions) {
   matchers.push_back(std::move(matcher_1));
   matchers.push_back(std::move(matcher_2));
   composite_matcher = std::make_unique<CompositeMatcher>(
-      std::move(matchers), HostPermissionsAlwaysRequired::kFalse);
+      std::move(matchers), /*extension_id=*/"",
+      HostPermissionsAlwaysRequired::kFalse);
 
   // Call GetBeforeRequestAction first to ensure that test and production code
   // paths are consistent.
@@ -317,7 +321,8 @@ TEST_F(CompositeMatcherTest, GetModifyHeadersActions_Priority) {
   matchers.push_back(std::move(matcher_1));
   matchers.push_back(std::move(matcher_2));
   auto composite_matcher = std::make_unique<CompositeMatcher>(
-      std::move(matchers), HostPermissionsAlwaysRequired::kFalse);
+      std::move(matchers), /*extension_id=*/"",
+      HostPermissionsAlwaysRequired::kFalse);
 
   // Make a request to "http://google.com/1" which matches with all
   // modifyHeaders rules and |allow_rule|.
@@ -423,7 +428,8 @@ TEST_F(CompositeMatcherTest, NotifyWithholdFromPageAccess) {
   std::vector<std::unique_ptr<RulesetMatcher>> matchers;
   matchers.push_back(std::move(matcher_1));
   auto composite_matcher = std::make_unique<CompositeMatcher>(
-      std::move(matchers), HostPermissionsAlwaysRequired::kFalse);
+      std::move(matchers), /*extension_id=*/"",
+      HostPermissionsAlwaysRequired::kFalse);
 
   GURL google_url = GURL("http://google.com");
   GURL example_url = GURL("http://example.com");
@@ -509,7 +515,8 @@ TEST_F(CompositeMatcherTest, HostPermissionsAlwaysRequired) {
   CompositeMatcher::MatcherList matchers;
   matchers.push_back(std::move(matcher));
   auto composite_matcher = std::make_unique<CompositeMatcher>(
-      std::move(matchers), HostPermissionsAlwaysRequired::kTrue);
+      std::move(matchers), /*extension_id=*/"",
+      HostPermissionsAlwaysRequired::kTrue);
 
   struct TestCases {
     const char* url;
@@ -583,7 +590,8 @@ TEST_F(CompositeMatcherTest, GetRedirectUrlFromPriority) {
   std::vector<std::unique_ptr<RulesetMatcher>> matchers;
   matchers.push_back(std::move(matcher_1));
   auto composite_matcher = std::make_unique<CompositeMatcher>(
-      std::move(matchers), HostPermissionsAlwaysRequired::kFalse);
+      std::move(matchers), /*extension_id=*/"",
+      HostPermissionsAlwaysRequired::kFalse);
 
   struct {
     GURL request_url;
@@ -650,7 +658,8 @@ TEST_F(CompositeMatcherTest, RulePlacement) {
 
   auto test_matchers = [](CompositeMatcher::MatcherList matchers) {
     auto composite_matcher = std::make_unique<CompositeMatcher>(
-        std::move(matchers), HostPermissionsAlwaysRequired::kFalse);
+        std::move(matchers), /*extension_id=*/"",
+        HostPermissionsAlwaysRequired::kFalse);
 
     GURL url("http://example.com");
     RequestParams params;

extensions/browser/api/declarative_net_request/request_params.h

@@ -67,11 +67,11 @@ struct RequestParams {
   url_pattern_index::UrlPatternIndexMatcher::EmbedderConditionsMatcher
       embedder_conditions_matcher;
 
-  // A map from CompositeMatcher to the priority of its highest priority
-  // matching allow or allowAllRequests rule if there is one, or std::nullopt
-  // otherwise. Used as a cache to prevent additional calls to
+  // A map from an extension ID to the priority of its CompositeMatcher's
+  // highest priority matching allow or allowAllRequests rule if there is one,
+  // or std::nullopt otherwise. Used as a cache to prevent additional calls to
   // GetBeforeRequestAction.
-  mutable base::flat_map<const CompositeMatcher*, std::optional<uint64_t>>
+  mutable base::flat_map<ExtensionId, std::optional<uint64_t>>
       allow_rule_max_priority;
 
   // Lower cased url, used for regex matching. Cached for performance.

extensions/browser/api/declarative_net_request/rules_monitor_service.cc

@@ -1114,7 +1114,8 @@ void RulesMonitorService::AddCompositeMatcher(
     return;
 
   auto matcher = std::make_unique<CompositeMatcher>(
-      std::move(matchers), GetHostPermissionsAlwaysRequired(extension));
+      std::move(matchers), extension.id(),
+      GetHostPermissionsAlwaysRequired(extension));
   ruleset_manager_.AddRuleset(extension.id(), std::move(matcher));
 }
 

extensions/browser/api/declarative_net_request/ruleset_manager.cc

@@ -80,14 +80,11 @@ RulesetManager::~RulesetManager() {
 void RulesetManager::AddRuleset(const ExtensionId& extension_id,
                                 std::unique_ptr<CompositeMatcher> matcher) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  DCHECK(!GetMatcherForExtension(extension_id))
+      << "AddRuleset called twice in succession for " << extension_id;
 
-  bool inserted =
-      rulesets_
-          .emplace(extension_id, prefs_->GetLastUpdateTime(extension_id),
-                   std::move(matcher))
-          .second;
-  DCHECK(inserted) << "AddRuleset called twice in succession for "
-                   << extension_id;
+  rulesets_.emplace(extension_id, prefs_->GetLastUpdateTime(extension_id),
+                    std::move(matcher));
 
   if (test_observer_)
     test_observer_->OnRulesetCountChanged(rulesets_.size());