DNR: Load ruleset directly in memory instead of mmap-ing it.

This CL changes the ruleset file data to no longer be memory mapped. This was
not ideal since mmap-ing a file on the IO thread can cause it to get de-
scheduled in case of a page fault, which we do not want. This CL changes the
ruleset file data to be copied and available directly in memory.

BUG=774271, 696822

Change-Id: I1c8aae2c1d48a7bf68a2c878a227d15344588039
Reviewed-on: https://chromium-review.googlesource.com/874595Reviewed-by: Istiaque Ahmed <lazyboy@chromium.org>
Reviewed-by: Ilya Sherman <isherman@chromium.org>
Commit-Queue: Karan Bhatia <karandeepb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#530703}

extensions/browser/api/declarative_net_request/ruleset_matcher.cc

@@ -10,7 +10,6 @@
 #include "base/logging.h"
 #include "base/memory/ptr_util.h"
 #include "base/metrics/histogram_macros.h"
-#include "base/task_scheduler/post_task.h"
 #include "base/threading/thread_restrictions.h"
 #include "base/timer/elapsed_timer.h"
 #include "extensions/browser/api/declarative_net_request/flat/extension_ruleset_generated.h"
@@ -23,9 +22,6 @@ namespace declarative_net_request {
 namespace flat_rule = url_pattern_index::flat;
 
 namespace {
-void DeleteRulesetHelper(std::unique_ptr<base::MemoryMappedFile> ruleset) {
-  base::AssertBlockingAllowed();
-}
 
 using FindRuleStrategy =
     url_pattern_index::UrlPatternIndexMatcher::FindRuleStrategy;
@@ -46,16 +42,13 @@ RulesetMatcher::LoadRulesetResult RulesetMatcher::CreateVerifiedMatcher(
   if (!base::PathExists(indexed_ruleset_path))
     return kLoadErrorInvalidPath;
 
-  // TODO(crbug.com/774271): Revisit mmap-ing the file.
-  auto ruleset = std::make_unique<base::MemoryMappedFile>();
-  if (!ruleset->Initialize(indexed_ruleset_path,
-                           base::MemoryMappedFile::READ_ONLY)) {
-    return kLoadErrorMemoryMap;
-  }
+  std::string ruleset_data;
+  if (!base::ReadFileToString(indexed_ruleset_path, &ruleset_data))
+    return kLoadErrorFileRead;
 
   // This guarantees that no memory access will end up outside the buffer.
-  if (!IsValidRulesetData(ruleset->data(), ruleset->length(),
-                          expected_ruleset_checksum)) {
+  if (!IsValidRulesetData(reinterpret_cast<const uint8_t*>(ruleset_data.data()),
+                          ruleset_data.size(), expected_ruleset_checksum)) {
     return kLoadErrorRulesetVerification;
   }
 
@@ -65,18 +58,11 @@ RulesetMatcher::LoadRulesetResult RulesetMatcher::CreateVerifiedMatcher(
 
   // Using WrapUnique instead of make_unique since this class has a private
   // constructor.
-  *matcher = base::WrapUnique(new RulesetMatcher(std::move(ruleset)));
+  *matcher = base::WrapUnique(new RulesetMatcher(std::move(ruleset_data)));
   return kLoadSuccess;
 }
 
-RulesetMatcher::~RulesetMatcher() {
-  // |ruleset_| must be destroyed on a sequence which supports file IO.
-  // TODO(crbug.com/696822): Revisit this to ensure that this is safe and causes
-  // no resource leak even if this task fails.
-  base::PostTaskWithTraits(
-      FROM_HERE, {base::MayBlock(), base::TaskPriority::BACKGROUND},
-      base::BindOnce(&DeleteRulesetHelper, std::move(ruleset_)));
-}
+RulesetMatcher::~RulesetMatcher() = default;
 
 bool RulesetMatcher::ShouldBlockRequest(const GURL& url,
                                         const url::Origin& first_party_origin,
@@ -141,9 +127,9 @@ bool RulesetMatcher::ShouldRedirectRequest(
   return true;
 }
 
-RulesetMatcher::RulesetMatcher(std::unique_ptr<base::MemoryMappedFile> ruleset)
-    : ruleset_(std::move(ruleset)),
-      root_(flat::GetExtensionIndexedRuleset(ruleset_->data())),
+RulesetMatcher::RulesetMatcher(std::string ruleset_data)
+    : ruleset_data_(std::move(ruleset_data)),
+      root_(flat::GetExtensionIndexedRuleset(ruleset_data_.data())),
       blacklist_matcher_(root_->blacklist_index()),
       whitelist_matcher_(root_->whitelist_index()),
       redirect_matcher_(root_->redirect_index()),

extensions/browser/api/declarative_net_request/ruleset_matcher.h

@@ -6,8 +6,8 @@
 #define EXTENSIONS_BROWSER_API_DECLARATIVE_NET_REQUEST_RULESET_MATCHER_H_
 
 #include <memory>
+#include <string>
 
-#include "base/files/memory_mapped_file.h"
 #include "components/url_pattern_index/url_pattern_index.h"
 
 namespace base {
@@ -29,11 +29,10 @@ struct UrlRuleMetadata;
 }  // namespace flat
 
 // RulesetMatcher encapsulates the Declarative Net Request API ruleset
-// corresponding to a single extension. The ruleset file is memory mapped. This
-// uses the url_pattern_index component to achieve fast matching of network
-// requests against declarative rules. Since this class is immutable, it is
-// thread-safe. In practice it is accessed on the IO thread but created on a
-// sequence where file IO is allowed.
+// corresponding to a single extension. This uses the url_pattern_index
+// component to achieve fast matching of network requests against declarative
+// rules. Since this class is immutable, it is thread-safe. In practice it is
+// accessed on the IO thread but created on a sequence where file IO is allowed.
 class RulesetMatcher {
  public:
   // Describes the result of creating a RulesetMatcher instance.
@@ -42,7 +41,7 @@ class RulesetMatcher {
   enum LoadRulesetResult {
     kLoadSuccess = 0,
     kLoadErrorInvalidPath = 1,
-    kLoadErrorMemoryMap = 2,
+    kLoadErrorFileRead = 2,
     kLoadErrorRulesetVerification = 3,
     kLoadResultMax
   };
@@ -79,10 +78,9 @@ class RulesetMatcher {
   using ExtensionMetadataList =
       flatbuffers::Vector<flatbuffers::Offset<flat::UrlRuleMetadata>>;
 
-  explicit RulesetMatcher(std::unique_ptr<base::MemoryMappedFile> ruleset_file);
+  explicit RulesetMatcher(std::string ruleset_data);
 
-  // The memory mapped ruleset file.
-  std::unique_ptr<base::MemoryMappedFile> ruleset_;
+  const std::string ruleset_data_;
 
   const flat::ExtensionIndexedRuleset* const root_;
   const UrlPatternIndexMatcher blacklist_matcher_;

tools/metrics/histograms/enums.xml

@@ -25030,7 +25030,7 @@ Called by update_gpu_driver_bug_workaround_entries.py.-->
 <enum name="LoadRulesetResult">
   <int value="0" label="Load succeeded"/>
   <int value="1" label="Load failed - Invalid path"/>
-  <int value="2" label="Load failed - Memory Map error"/>
+  <int value="2" label="Load failed - File read error"/>
   <int value="3" label="Load failed - Ruleset verification error"/>
 </enum>