DNR: Introduce RulesetSource struct.

Introduce RulesetSource struct to be used for indexing code. This should help
make the indexing code a bit more generic since it does not need to assume the
location of the json and indexed ruleset files.

BUG=930961

Change-Id: I9f6f63434adbe8d3fbc0e5148c466c578c3dd422
Reviewed-on: https://chromium-review.googlesource.com/c/1466281Reviewed-by: Istiaque Ahmed <lazyboy@chromium.org>
Commit-Queue: Karan Bhatia <karandeepb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#632371}

chrome/browser/extensions/unpacked_installer.cc

@@ -23,6 +23,7 @@
 #include "components/sync/model/string_ordinal.h"
 #include "content/public/browser/browser_task_traits.h"
 #include "content/public/browser/browser_thread.h"
+#include "extensions/browser/api/declarative_net_request/ruleset_source.h"
 #include "extensions/browser/api/declarative_net_request/utils.h"
 #include "extensions/browser/extension_file_task_runner.h"
 #include "extensions/browser/extension_prefs.h"
@@ -274,7 +275,8 @@ bool UnpackedInstaller::IndexAndPersistRulesIfNeeded(std::string* error) {
   // TODO(crbug.com/761107): Change this so that we don't need to parse JSON
   // in the browser process.
   declarative_net_request::IndexAndPersistRulesResult result =
-      declarative_net_request::IndexAndPersistRulesUnsafe(*extension());
+      declarative_net_request::IndexAndPersistRulesUnsafe(
+          declarative_net_request::RulesetSource::Create(*extension()));
   if (!result.success) {
     *error = std::move(result.error);
     return false;

extensions/browser/api/declarative_net_request/BUILD.gn

@@ -20,6 +20,8 @@ source_set("declarative_net_request") {
     "ruleset_manager.h",
     "ruleset_matcher.cc",
     "ruleset_matcher.h",
+    "ruleset_source.cc",
+    "ruleset_source.h",
     "utils.cc",
     "utils.h",
   ]

extensions/browser/api/declarative_net_request/rules_monitor_service.cc

@@ -19,6 +19,7 @@
 #include "content/public/common/service_manager_connection.h"
 #include "extensions/browser/api/declarative_net_request/ruleset_manager.h"
 #include "extensions/browser/api/declarative_net_request/ruleset_matcher.h"
+#include "extensions/browser/api/declarative_net_request/ruleset_source.h"
 #include "extensions/browser/api/declarative_net_request/utils.h"
 #include "extensions/browser/extension_file_task_runner.h"
 #include "extensions/browser/extension_prefs.h"
@@ -186,8 +187,10 @@ class RulesMonitorService::FileSequenceState {
     IndexAndPersistRulesCallback ruleset_reindexed_callback = base::BindOnce(
         &FileSequenceState::OnRulesetReindexed, weak_factory_.GetWeakPtr(),
         std::move(info), result, std::move(ui_callback));
-    IndexAndPersistRules(connector_.get(), base::nullopt /* decoder_batch_id */,
-                         *extension, std::move(ruleset_reindexed_callback));
+    IndexAndPersistRules(
+        connector_.get(), base::nullopt /* decoder_batch_id */,
+        declarative_net_request::RulesetSource::Create(*extension),
+        std::move(ruleset_reindexed_callback));
   }
 
   // Callback invoked when the JSON ruleset is reindexed.

extensions/browser/api/declarative_net_request/ruleset_source.cc

+// Copyright 2019 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "extensions/browser/api/declarative_net_request/ruleset_source.h"
+
+#include "extensions/common/api/declarative_net_request/dnr_manifest_data.h"
+#include "extensions/common/extension.h"
+#include "extensions/common/extension_resource.h"
+#include "extensions/common/file_util.h"
+
+namespace extensions {
+namespace declarative_net_request {
+
+// static
+RulesetSource RulesetSource::Create(const Extension& extension) {
+  RulesetSource source;
+
+  const ExtensionResource* extension_resource =
+      declarative_net_request::DNRManifestData::GetRulesetResource(&extension);
+  if (!extension_resource)
+    return source;
+
+  source.json_path = extension_resource->GetFilePath();
+  source.indexed_path = file_util::GetIndexedRulesetPath(extension.path());
+  return source;
+}
+
+RulesetSource::~RulesetSource() = default;
+RulesetSource::RulesetSource(RulesetSource&&) = default;
+RulesetSource& RulesetSource::operator=(RulesetSource&&) = default;
+
+RulesetSource RulesetSource::Clone() const {
+  RulesetSource clone;
+  clone.json_path = json_path;
+  clone.indexed_path = indexed_path;
+  return clone;
+}
+
+RulesetSource::RulesetSource() = default;
+
+}  // namespace declarative_net_request
+}  // namespace extensions

extensions/browser/api/declarative_net_request/ruleset_source.h

+// Copyright 2019 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef EXTENSIONS_BROWSER_API_DECLARATIVE_NET_REQUEST_RULESET_SOURCE_H_
+#define EXTENSIONS_BROWSER_API_DECLARATIVE_NET_REQUEST_RULESET_SOURCE_H_
+
+#include "base/files/file_path.h"
+
+namespace extensions {
+class Extension;
+
+namespace declarative_net_request {
+
+// Holds paths for an extension ruleset.
+struct RulesetSource {
+  // Creates RulesetSource for |extension|. This must be called on a sequence
+  // which supports file IO.
+  static RulesetSource Create(const Extension& extension);
+
+  ~RulesetSource();
+  RulesetSource(RulesetSource&&);
+  RulesetSource& operator=(RulesetSource&&);
+
+  RulesetSource Clone() const;
+
+  // Path to the JSON rules.
+  base::FilePath json_path;
+
+  // Path to the indexed flatbuffer rules.
+  base::FilePath indexed_path;
+
+ private:
+  RulesetSource();
+
+  DISALLOW_COPY_AND_ASSIGN(RulesetSource);
+};
+
+}  // namespace declarative_net_request
+}  // namespace extensions
+
+#endif  // EXTENSIONS_BROWSER_API_DECLARATIVE_NET_REQUEST_RULESET_SOURCE_H_

extensions/browser/api/declarative_net_request/utils.cc

@@ -18,6 +18,7 @@
 #include "base/json/json_file_value_serializer.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/strings/strcat.h"
+#include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/timer/elapsed_timer.h"
@@ -28,11 +29,10 @@
 #include "extensions/browser/api/declarative_net_request/flat_ruleset_indexer.h"
 #include "extensions/browser/api/declarative_net_request/indexed_rule.h"
 #include "extensions/browser/api/declarative_net_request/parse_info.h"
+#include "extensions/browser/api/declarative_net_request/ruleset_source.h"
 #include "extensions/common/api/declarative_net_request.h"
-#include "extensions/common/api/declarative_net_request/dnr_manifest_data.h"
 #include "extensions/common/api/declarative_net_request/utils.h"
 #include "extensions/common/error_utils.h"
-#include "extensions/common/file_util.h"
 #include "extensions/common/install_warning.h"
 #include "extensions/common/manifest_constants.h"
 #include "services/data_decoder/public/cpp/safe_json_parser.h"
@@ -88,18 +88,16 @@ int GetChecksum(base::span<const uint8_t> data) {
   return static_cast<int>(hash & 0x7fffffff);
 }
 
-// Helper function to persist the indexed ruleset |data| for |extension|. The
-// ruleset is composed of a version header corresponding to the current ruleset
-// format version, followed by the actual ruleset data. Note: The checksum only
-// corresponds to this ruleset data and does not include the version header.
-bool PersistRuleset(const Extension& extension,
+// Helper function to persist the indexed ruleset |data| at the given |path|.
+// The ruleset is composed of a version header corresponding to the current
+// ruleset format version, followed by the actual ruleset data. Note: The
+// checksum only corresponds to this ruleset data and does not include the
+// version header.
+bool PersistRuleset(const base::FilePath& path,
                     base::span<const uint8_t> data,
                     int* ruleset_checksum) {
   DCHECK(ruleset_checksum);
 
-  const base::FilePath path =
-      file_util::GetIndexedRulesetPath(extension.path());
-
   // Create the directory corresponding to |path| if it does not exist.
   if (!base::CreateDirectory(path.DirName()))
     return false;
@@ -130,15 +128,9 @@ bool PersistRuleset(const Extension& extension,
   return true;
 }
 
-// Helper to retrieve the ruleset ExtensionResource for |extension|.
-const ExtensionResource* GetRulesetResource(const Extension& extension) {
-  return declarative_net_request::DNRManifestData::GetRulesetResource(
-      &extension);
-}
-
-// Helper to retrieve the filename of the JSON ruleset provided by |extension|.
-std::string GetJSONRulesetFilename(const Extension& extension) {
-  return GetRulesetResource(extension)->GetFilePath().BaseName().AsUTF8Unsafe();
+// Helper to retrieve the filename for the given |file_path|.
+std::string GetFilename(const base::FilePath& file_path) {
+  return file_path.BaseName().AsUTF8Unsafe();
 }
 
 InstallWarning CreateInstallWarning(const std::string& message) {
@@ -146,10 +138,9 @@ InstallWarning CreateInstallWarning(const std::string& message) {
                         manifest_keys::kDeclarativeRuleResourcesKey);
 }
 
-// Helper function to index |rules| and persist them to the
-// |indexed_ruleset_path|.
+// Helper function to index |rules| and persist them to |indexed_path|.
 ParseInfo IndexAndPersistRulesImpl(const base::Value& rules,
-                                   const Extension& extension,
+                                   const base::FilePath& indexed_path,
                                    std::vector<InstallWarning>* warnings,
                                    int* ruleset_checksum) {
   DCHECK(warnings);
@@ -227,7 +218,7 @@ ParseInfo IndexAndPersistRulesImpl(const base::Value& rules,
   indexer.Finish();
   UMA_HISTOGRAM_TIMES(kIndexRulesTimeHistogram, timer.Elapsed());
 
-  if (!PersistRuleset(extension, indexer.GetData(), ruleset_checksum))
+  if (!PersistRuleset(indexed_path, indexer.GetData(), ruleset_checksum))
     return ParseInfo(ParseResult::ERROR_PERSISTING_RULESET);
 
   if (rule_count_exceeded)
@@ -247,23 +238,22 @@ ParseInfo IndexAndPersistRulesImpl(const base::Value& rules,
   return ParseInfo(ParseResult::SUCCESS);
 }
 
-void OnSafeJSONParserSuccess(const Extension* extension,
+void OnSafeJSONParserSuccess(const RulesetSource& source,
                              IndexAndPersistRulesCallback callback,
                              std::unique_ptr<base::Value> root) {
   DCHECK(root);
 
   std::vector<InstallWarning> warnings;
   int ruleset_checksum;
-  const ParseInfo info =
-      IndexAndPersistRulesImpl(*root, *extension, &warnings, &ruleset_checksum);
+  const ParseInfo info = IndexAndPersistRulesImpl(*root, source.indexed_path,
+                                                  &warnings, &ruleset_checksum);
   if (info.result() == ParseResult::SUCCESS) {
     std::move(callback).Run(IndexAndPersistRulesResult::CreateSuccessResult(
         ruleset_checksum, std::move(warnings)));
     return;
   }
 
-  std::string error =
-      info.GetErrorDescription(GetJSONRulesetFilename(*extension));
+  std::string error = info.GetErrorDescription(GetFilename(source.json_path));
   std::move(callback).Run(
       IndexAndPersistRulesResult::CreateErrorResult(std::move(error)));
 }
@@ -310,45 +300,39 @@ IndexAndPersistRulesResult& IndexAndPersistRulesResult::operator=(
 IndexAndPersistRulesResult::IndexAndPersistRulesResult() = default;
 
 IndexAndPersistRulesResult IndexAndPersistRulesUnsafe(
-    const Extension& extension) {
+    const RulesetSource& source) {
   DCHECK(IsAPIAvailable());
 
-  const ExtensionResource* resource = GetRulesetResource(extension);
-  DCHECK(resource);
-
-  JSONFileValueDeserializer deserializer(resource->GetFilePath());
+  JSONFileValueDeserializer deserializer(source.json_path);
   std::string error;
   std::unique_ptr<base::Value> root = deserializer.Deserialize(
       nullptr /*error_code*/, &error /*error_message*/);
   if (!root) {
     return IndexAndPersistRulesResult::CreateErrorResult(
-        GetJSONParseError(GetJSONRulesetFilename(extension), error));
+        GetJSONParseError(GetFilename(source.json_path), error));
   }
 
   std::vector<InstallWarning> warnings;
   int ruleset_checksum;
-  const ParseInfo info =
-      IndexAndPersistRulesImpl(*root, extension, &warnings, &ruleset_checksum);
+  const ParseInfo info = IndexAndPersistRulesImpl(*root, source.indexed_path,
+                                                  &warnings, &ruleset_checksum);
   if (info.result() == ParseResult::SUCCESS) {
     return IndexAndPersistRulesResult::CreateSuccessResult(ruleset_checksum,
                                                            std::move(warnings));
   }
 
-  error = info.GetErrorDescription(GetJSONRulesetFilename(extension));
+  error = info.GetErrorDescription(GetFilename(source.json_path));
   return IndexAndPersistRulesResult::CreateErrorResult(std::move(error));
 }
 
 void IndexAndPersistRules(service_manager::Connector* connector,
                           const base::Optional<base::Token>& decoder_batch_id,
-                          const Extension& extension,
+                          const RulesetSource& source,
                           IndexAndPersistRulesCallback callback) {
   DCHECK(IsAPIAvailable());
 
-  const ExtensionResource* resource = GetRulesetResource(extension);
-  DCHECK(resource);
-
   std::string json_contents;
-  if (!base::ReadFileToString(resource->GetFilePath(), &json_contents)) {
+  if (!base::ReadFileToString(source.json_path, &json_contents)) {
     std::move(callback).Run(IndexAndPersistRulesResult::CreateErrorResult(
         manifest_errors::kDeclarativeNetRequestJSONRulesFileReadError));
     return;
@@ -358,12 +342,11 @@ void IndexAndPersistRules(service_manager::Connector* connector,
   // the callee interface.
   auto repeating_callback =
       base::AdaptCallbackForRepeating(std::move(callback));
-  auto success_callback =
-      base::BindRepeating(&OnSafeJSONParserSuccess,
-                          base::RetainedRef(&extension), repeating_callback);
+  auto success_callback = base::BindRepeating(
+      &OnSafeJSONParserSuccess, source.Clone(), repeating_callback);
   auto error_callback =
       base::BindRepeating(&OnSafeJSONParserError, repeating_callback,
-                          GetJSONRulesetFilename(extension));
+                          GetFilename(source.json_path));
 
   if (decoder_batch_id) {
     data_decoder::SafeJsonParser::ParseBatch(connector, json_contents,

extensions/browser/api/declarative_net_request/utils.h

@@ -16,6 +16,7 @@
 #include "base/optional.h"
 
 namespace base {
+class FilePath;
 class ListValue;
 class Token;
 }  // namespace base
@@ -25,10 +26,10 @@ class Connector;
 }  // namespace service_manager
 
 namespace extensions {
-class Extension;
 struct InstallWarning;
 
 namespace declarative_net_request {
+struct RulesetSource;
 
 struct IndexAndPersistRulesResult {
  public:
@@ -58,12 +59,11 @@ struct IndexAndPersistRulesResult {
   DISALLOW_COPY_AND_ASSIGN(IndexAndPersistRulesResult);
 };
 
-// Indexes and persists the JSON ruleset for for |extension|. This is
-// potentially unsafe since the JSON rules file is parsed in-process. Should
-// only be called for an extension which provided a JSON ruleset.
-// Note: This must be called on a sequence where file IO is allowed.
+// Indexes and persists the JSON ruleset for |source|. This is potentially
+// unsafe since the JSON rules file is parsed in-process. Note: This must be
+// called on a sequence where file IO is allowed.
 IndexAndPersistRulesResult IndexAndPersistRulesUnsafe(
-    const Extension& extension);
+    const RulesetSource& source);
 
 using IndexAndPersistRulesCallback =
     base::OnceCallback<void(IndexAndPersistRulesResult)>;
@@ -76,7 +76,7 @@ using IndexAndPersistRulesCallback =
 // NOTE: This must be called on a sequence where file IO is allowed.
 void IndexAndPersistRules(service_manager::Connector* connector,
                           const base::Optional<base::Token>& decoder_batch_id,
-                          const Extension& extension,
+                          const RulesetSource& source,
                           IndexAndPersistRulesCallback callback);
 
 // Returns true if |data| represents a valid data buffer containing indexed

extensions/browser/sandboxed_unpacker.cc

@@ -29,6 +29,7 @@
 #include "components/services/unzip/public/cpp/unzip.h"
 #include "content/public/browser/browser_task_traits.h"
 #include "content/public/browser/browser_thread.h"
+#include "extensions/browser/api/declarative_net_request/ruleset_source.h"
 #include "extensions/browser/api/declarative_net_request/utils.h"
 #include "extensions/browser/extension_file_task_runner.h"
 #include "extensions/browser/install/crx_install_error.h"
@@ -692,7 +693,7 @@ void SandboxedUnpacker::IndexAndPersistJSONRulesetIfNeeded(
 
   declarative_net_request::IndexAndPersistRules(
       connector_.get(), *data_decoder_service_filter_.instance_id(),
-      *extension_,
+      declarative_net_request::RulesetSource::Create(*extension_),
       base::BindOnce(&SandboxedUnpacker::OnJSONRulesetIndexed, this,
                      std::move(manifest)));
 }