DNR: Introduce updateEnabledRulesets (2/2).

This CL completes the implementation of the updateEnabledRulesets
extension function by persisting the set of enabled static rulesets to
prefs. This ensures that the setting is persisted across extension
loads and sessions. Note that this setting is cleared on extension
update.

BUG=754526

Change-Id: I2e87d658f97abb33fb69355c16c6c7587f37a5db
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2191731
Commit-Queue: Karan Bhatia <karandeepb@chromium.org>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#768563}

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

@@ -411,6 +411,33 @@ class DeclarativeNetRequestBrowserTest
     ASSERT_EQ("success", ExecuteScriptInBackgroundPage(extension_id, script));
   }
 
+  void UpdateEnabledRulesets(
+      const ExtensionId& extension_id,
+      const std::vector<std::string>& ruleset_ids_to_remove,
+      const std::vector<std::string>& ruleset_ids_to_add) {
+    static constexpr char kScript[] = R"(
+      chrome.declarativeNetRequest.updateEnabledRulesets($1, $2, () => {
+        window.domAutomationController.send(chrome.runtime.lastError ?
+            chrome.runtime.lastError.message : 'success');
+      });
+    )";
+
+    std::unique_ptr<base::Value> ids_to_remove =
+        ListBuilder()
+            .Append(ruleset_ids_to_remove.begin(), ruleset_ids_to_remove.end())
+            .Build();
+    std::unique_ptr<base::Value> ids_to_add =
+        ListBuilder()
+            .Append(ruleset_ids_to_add.begin(), ruleset_ids_to_add.end())
+            .Build();
+
+    // A cast is necessary from ListValue to Value, else this fails to compile.
+    const std::string script = content::JsReplace(
+        kScript, static_cast<const base::Value&>(*ids_to_remove),
+        static_cast<const base::Value&>(*ids_to_add));
+    ASSERT_EQ("success", ExecuteScriptInBackgroundPage(extension_id, script));
+  }
+
   void SetActionsAsBadgeText(const ExtensionId& extension_id, bool pref) {
     const char* pref_string = pref ? "true" : "false";
     static constexpr char kSetActionCountAsBadgeTextScript[] = R"(
@@ -3989,11 +4016,15 @@ IN_PROC_BROWSER_TEST_P(DeclarativeNetRequestBrowserTest_Packed,
   ASSERT_NO_FATAL_FAILURE(
       AddDynamicRules(extension_id, {CreateMainFrameBlockRule("dynamic")}));
 
+  // Also update the set of enabled static rulesets.
+  ASSERT_NO_FATAL_FAILURE(
+      UpdateEnabledRulesets(extension_id, {"id1"}, {"id2", "id3"}));
+
   CompositeMatcher* composite_matcher =
       ruleset_manager()->GetMatcherForExtension(extension_id);
   ASSERT_TRUE(composite_matcher);
   EXPECT_THAT(GetPublicRulesetIDs(*extension, *composite_matcher),
-              UnorderedElementsAre("id1", "id3", dnr_api::DYNAMIC_RULESET_ID));
+              UnorderedElementsAre("id2", "id3", dnr_api::DYNAMIC_RULESET_ID));
 
   // Also sanity check the extension prefs entry for the rulesets.
   ExtensionPrefs* prefs = ExtensionPrefs::Get(profile());
@@ -4013,6 +4044,13 @@ IN_PROC_BROWSER_TEST_P(DeclarativeNetRequestBrowserTest_Packed,
       &checksum));
   EXPECT_TRUE(
       prefs->GetDNRDynamicRulesetChecksum(extension_id, &dynamic_checksum_1));
+  base::Optional<std::set<RulesetID>> enabled_static_rulesets =
+      prefs->GetDNREnabledStaticRulesets(extension_id);
+  ASSERT_TRUE(enabled_static_rulesets);
+  EXPECT_THAT(
+      *enabled_static_rulesets,
+      UnorderedElementsAre(RulesetID(kMinValidStaticRulesetID.value() + 1),
+                           RulesetID(kMinValidStaticRulesetID.value() + 2)));
 
   std::vector<TestRulesetInfo> new_rulesets = {
       create_single_rule_ruleset("id1", true, "yahoo"),
@@ -4044,6 +4082,10 @@ IN_PROC_BROWSER_TEST_P(DeclarativeNetRequestBrowserTest_Packed,
   EXPECT_TRUE(
       prefs->GetDNRDynamicRulesetChecksum(extension_id, &dynamic_checksum_2));
   EXPECT_EQ(dynamic_checksum_2, dynamic_checksum_1);
+
+  // Ensure the preference for enabled static rulesets is cleared on extension
+  // update.
+  EXPECT_FALSE(prefs->GetDNREnabledStaticRulesets(extension_id));
 }
 
 // Tests extension update for an extension using declarativeNetRequest.

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

@@ -821,8 +821,7 @@ TEST_P(SingleRulesetTest, DynamicRulesetRace) {
   LoadAndExpectSuccess();
   ruleset_waiter.WaitForExtensionsWithRulesetsCount(1);
 
-  const std::string extension_id = extension()->id();
-
+  const ExtensionId extension_id = extension()->id();
   service()->DisableExtension(extension_id,
                               disable_reason::DISABLE_USER_ACTION);
   ruleset_waiter.WaitForExtensionsWithRulesetsCount(0);
@@ -1303,6 +1302,21 @@ TEST_P(MultipleRulesetsTest, UpdateAndGetEnabledRulesets_Success) {
   VerifyPublicRulesetIDs(*extension(),
                          {kId1, kId2, kId3, dnr_api::DYNAMIC_RULESET_ID});
   VerifyGetEnabledRulesetsFunction(*extension(), {kId1, kId2, kId3});
+
+  // Ensure the set of enabled rulesets persists across extension reloads.
+  const ExtensionId extension_id = extension()->id();
+  service()->DisableExtension(extension_id,
+                              disable_reason::DISABLE_USER_ACTION);
+  ruleset_waiter.WaitForExtensionsWithRulesetsCount(0);
+
+  service()->EnableExtension(extension_id);
+  ruleset_waiter.WaitForExtensionsWithRulesetsCount(1);
+  const Extension* extension =
+      registry()->GetExtensionById(extension_id, ExtensionRegistry::ENABLED);
+  ASSERT_TRUE(extension);
+  VerifyPublicRulesetIDs(*extension,
+                         {kId1, kId2, kId3, dnr_api::DYNAMIC_RULESET_ID});
+  VerifyGetEnabledRulesetsFunction(*extension, {kId1, kId2, kId3});
 }
 
 INSTANTIATE_TEST_SUITE_P(All,

extensions/browser/api/declarative_net_request/composite_matcher.cc

@@ -17,6 +17,7 @@
 #include "extensions/browser/api/declarative_net_request/request_action.h"
 #include "extensions/browser/api/declarative_net_request/request_params.h"
 #include "extensions/browser/api/declarative_net_request/utils.h"
+#include "extensions/common/api/declarative_net_request/constants.h"
 
 namespace extensions {
 namespace declarative_net_request {
@@ -97,6 +98,18 @@ void CompositeMatcher::AddOrUpdateRuleset(
   OnMatchersModified();
 }
 
+std::set<RulesetID> CompositeMatcher::ComputeStaticRulesetIDs() const {
+  std::set<RulesetID> result;
+  for (const std::unique_ptr<RulesetMatcher>& matcher : matchers_) {
+    if (matcher->id() == kDynamicRulesetID)
+      continue;
+
+    result.insert(matcher->id());
+  }
+
+  return result;
+}
+
 ActionInfo CompositeMatcher::GetBeforeRequestAction(
     const RequestParams& params,
     PageAccess page_access) const {

extensions/browser/api/declarative_net_request/composite_matcher.h

@@ -7,6 +7,7 @@
 
 #include <cstdint>
 #include <memory>
+#include <set>
 #include <vector>
 
 #include "base/macros.h"
@@ -63,6 +64,10 @@ class CompositeMatcher {
   // corresponding ID is already present, updates the matcher.
   void AddOrUpdateRuleset(std::unique_ptr<RulesetMatcher> new_matcher);
 
+  // Computes and returns the set of static RulesetIDs corresponding to
+  // |matchers_|.
+  std::set<RulesetID> ComputeStaticRulesetIDs() const;
+
   // Returns a RequestAction for the network request specified by |params|, or
   // base::nullopt if there is no matching rule.
   ActionInfo GetBeforeRequestAction(

extensions/browser/api/declarative_net_request/index_helper.cc

@@ -64,7 +64,7 @@ IndexHelper::Result CombineResults(
     total_index_and_persist_time += index_result.index_and_persist_time;
     total_rules_count += index_result.rules_count;
 
-    if (source->enabled()) {
+    if (source->enabled_by_default()) {
       enabled_rules_count += index_result.rules_count;
       enabled_regex_rules_count += index_result.regex_rules_count;
     }

extensions/browser/api/declarative_net_request/rules_monitor_service.cc

@@ -263,9 +263,16 @@ void RulesMonitorService::OnExtensionLoaded(
   {
     std::vector<RulesetSource> sources =
         RulesetSource::CreateStatic(*extension);
+
+    base::Optional<std::set<RulesetID>> prefs_enabled_rulesets =
+        prefs_->GetDNREnabledStaticRulesets(extension->id());
+
     bool ruleset_failed_to_load = false;
     for (auto& source : sources) {
-      if (!source.enabled())
+      bool enabled = prefs_enabled_rulesets
+                         ? base::Contains(*prefs_enabled_rulesets, source.id())
+                         : source.enabled_by_default();
+      if (!enabled)
         continue;
 
       if (!prefs_->GetDNRStaticRulesetChecksum(extension->id(), source.id(),
@@ -387,9 +394,7 @@ void RulesMonitorService::UpdateDynamicRulesInternal(
 
 void RulesMonitorService::OnInitialRulesetsLoaded(LoadRequestData load_data) {
   DCHECK(!load_data.rulesets.empty());
-  DCHECK(std::all_of(
-      load_data.rulesets.begin(), load_data.rulesets.end(),
-      [](const RulesetInfo& ruleset) { return ruleset.source().enabled(); }));
+
   // Perform pending dynamic rule updates.
   {
     auto it = pending_dynamic_rule_updates_.find(load_data.extension_id);
@@ -585,11 +590,11 @@ void RulesMonitorService::OnNewStaticRulesetsLoaded(
                         std::make_move_iterator(old_matchers.end()));
   }
 
-  // TODO(crbug.com/754526): IDs for the new set of enabled static rulesets
-  // should be persisted to prefs so that they are persisted across extension
-  // loads.
   matcher->SetMatchers(std::move(new_matchers));
 
+  prefs_->SetDNREnabledStaticRulesets(load_data.extension_id,
+                                      matcher->ComputeStaticRulesetIDs());
+
   AdjustExtraHeaderListenerCountIfNeeded(had_extra_headers_matcher);
 
   std::move(callback).Run(base::nullopt);

extensions/browser/api/declarative_net_request/ruleset_source.cc

@@ -329,7 +329,7 @@ RulesetSource RulesetSource::CreateDynamic(content::BrowserContext* context,
       dynamic_ruleset_directory.AppendASCII(kDynamicRulesJSONFilename),
       dynamic_ruleset_directory.AppendASCII(kDynamicIndexedRulesFilename),
       kDynamicRulesetID, dnr_api::MAX_NUMBER_OF_DYNAMIC_RULES, extension_id,
-      true /* enabled */);
+      true /* enabled_by_default */);
 }
 
 // static
@@ -347,7 +347,8 @@ std::unique_ptr<RulesetSource> RulesetSource::CreateTemporarySource(
   // Use WrapUnique since RulesetSource constructor is private.
   return base::WrapUnique(new RulesetSource(
       std::move(temporary_file_json), std::move(temporary_file_indexed), id,
-      rule_count_limit, std::move(extension_id), true /* enabled */));
+      rule_count_limit, std::move(extension_id),
+      true /* enabled_by_default */));
 }
 
 RulesetSource::~RulesetSource() = default;
@@ -356,7 +357,7 @@ RulesetSource& RulesetSource::operator=(RulesetSource&&) = default;
 
 RulesetSource RulesetSource::Clone() const {
   return RulesetSource(json_path_, indexed_path_, id_, rule_count_limit_,
-                       extension_id_, enabled_);
+                       extension_id_, enabled_by_default_);
 }
 
 IndexAndPersistJSONRulesetResult
@@ -498,13 +499,13 @@ RulesetSource::RulesetSource(base::FilePath json_path,
                              RulesetID id,
                              size_t rule_count_limit,
                              ExtensionId extension_id,
-                             bool enabled)
+                             bool enabled_by_default)
     : json_path_(std::move(json_path)),
       indexed_path_(std::move(indexed_path)),
       id_(id),
       rule_count_limit_(rule_count_limit),
       extension_id_(std::move(extension_id)),
-      enabled_(enabled) {}
+      enabled_by_default_(enabled_by_default) {}
 
 }  // namespace declarative_net_request
 }  // namespace extensions

extensions/browser/api/declarative_net_request/ruleset_source.h

@@ -168,8 +168,8 @@ class RulesetSource {
   const ExtensionId& extension_id() const { return extension_id_; }
 
   // Whether the ruleset is enabled by default (as specified in the extension
-  // manifest).
-  bool enabled() const { return enabled_; }
+  // manifest for a static ruleset). Always true for a dynamic ruleset.
+  bool enabled_by_default() const { return enabled_by_default_; }
 
   // Indexes and persists the JSON ruleset. This is potentially unsafe since the
   // JSON rules file is parsed in-process. Note: This must be called on a
@@ -213,7 +213,7 @@ class RulesetSource {
   RulesetID id_;
   size_t rule_count_limit_;
   ExtensionId extension_id_;
-  bool enabled_;
+  bool enabled_by_default_;
 
   DISALLOW_COPY_AND_ASSIGN(RulesetSource);
 };

extensions/browser/extension_prefs.cc

@@ -227,6 +227,10 @@ constexpr const char kDNRDynamicRulesetPref[] = "dnr_dynamic_ruleset";
 // Net Request API.
 constexpr const char kDNRChecksumKey[] = "checksum";
 
+// Key corresponding to the list of enabled static ruleset IDs for an extension.
+// Used for the Declarative Net Request API.
+constexpr const char kDNREnabledStaticRulesetIDs[] = "dnr_enabled_ruleset_ids";
+
 // A boolean preference that indicates whether the extension's icon should be
 // automatically badged to the matched action count for a tab. False by default.
 constexpr const char kPrefDNRUseActionCountAsBadgeText[] =
@@ -1869,6 +1873,36 @@ void ExtensionPrefs::SetDNRDynamicRulesetChecksum(
                       std::make_unique<base::Value>(checksum));
 }
 
+base::Optional<std::set<declarative_net_request::RulesetID>>
+ExtensionPrefs::GetDNREnabledStaticRulesets(
+    const ExtensionId& extension_id) const {
+  std::set<declarative_net_request::RulesetID> ids;
+  const base::ListValue* ids_value = nullptr;
+  if (!ReadPrefAsList(extension_id, kDNREnabledStaticRulesetIDs, &ids_value))
+    return base::nullopt;
+
+  DCHECK(ids_value);
+  for (const base::Value& id_value : ids_value->GetList()) {
+    if (!id_value.is_int())
+      return base::nullopt;
+
+    ids.insert(declarative_net_request::RulesetID(id_value.GetInt()));
+  }
+
+  return ids;
+}
+
+void ExtensionPrefs::SetDNREnabledStaticRulesets(
+    const ExtensionId& extension_id,
+    const std::set<declarative_net_request::RulesetID>& ids) {
+  std::vector<base::Value> ids_list;
+  for (const auto& id : ids)
+    ids_list.push_back(base::Value(id.value()));
+
+  UpdateExtensionPref(extension_id, kDNREnabledStaticRulesetIDs,
+                      std::make_unique<base::Value>(ids_list));
+}
+
 bool ExtensionPrefs::GetDNRUseActionCountAsBadgeText(
     const ExtensionId& extension_id) const {
   return ReadPrefAsBooleanAndReturn(extension_id,
@@ -2065,6 +2099,10 @@ void ExtensionPrefs::PopulateExtensionInfoPrefs(
                                   std::move(ruleset_prefs));
   }
 
+  // Clear the list of enabled static rulesets for the extension since it
+  // shouldn't persist across extension updates.
+  extension_dict->Remove(kDNREnabledStaticRulesetIDs, nullptr /* out_value */);
+
   if (util::CanWithholdPermissionsFromExtension(*extension)) {
     // If the withhold permission creation flag is present it takes precedence
     // over any previous stored value.

extensions/browser/extension_prefs.h

@@ -608,6 +608,16 @@ class ExtensionPrefs : public KeyedService {
   void SetDNRDynamicRulesetChecksum(const ExtensionId& extension_id,
                                     int checksum);
 
+  // Returns the set of enabled static ruleset IDs or base::nullopt if the
+  // extension hasn't updated the set of enabled static rulesets.
+  base::Optional<std::set<declarative_net_request::RulesetID>>
+  GetDNREnabledStaticRulesets(const ExtensionId& extension_id) const;
+  // Updates the set of enabled static rulesets for the |extension_id|. This
+  // preference gets cleared on extension update.
+  void SetDNREnabledStaticRulesets(
+      const ExtensionId& extension_id,
+      const std::set<declarative_net_request::RulesetID>& ids);
+
   // Whether the extension with the given |extension_id| is using its ruleset's
   // matched action count for the badge text. This is set via the
   // setActionCountAsBadgeText API call.