DNR: Reduce work while evaluating query transform.

Reduce the work that's done while evaluating the query transform of a rule. This
is done by indexing escaped values for the query key value pairs. Also, sort and
remove duplicates before indexing the query keys to be removed.

BUG=983685

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

extensions/browser/api/declarative_net_request/flat/extension_ruleset.fbs

@@ -30,7 +30,12 @@ table UrlTransform {
    /// If valid, doesn't include the '?' separator.
    query : string;
 
+   /// Query params to be removed. These are already sorted and escaped using
+   /// net::EscapeQueryParamValue.
    remove_query_params : [string];
+
+   /// Query params to be added/replaced. These are already escaped using
+   /// net::EscapeQueryParamValue.
    add_or_replace_query_params : [QueryKeyValue];
 
    clear_fragment : bool = false;

extensions/browser/api/declarative_net_request/flat_ruleset_indexer.cc

@@ -9,6 +9,7 @@
 #include "base/logging.h"
 #include "base/numerics/safe_conversions.h"
 #include "extensions/browser/api/declarative_net_request/indexed_rule.h"
+#include "net/base/escape.h"
 
 namespace extensions {
 namespace declarative_net_request {
@@ -28,17 +29,18 @@ using FlatStringOffset = FlatOffset<flatbuffers::String>;
 using FlatStringListOffset = FlatVectorOffset<flatbuffers::String>;
 
 // Writes to |builder| a flatbuffer vector of shared strings corresponding to
-// |vec| and returns the offset to it. If |vec| is empty, returns an empty
-// offset.
+// |container| and returns the offset to it. If |container| is empty, returns an
+// empty offset.
+template <typename T>
 FlatStringListOffset BuildVectorOfSharedStrings(
     flatbuffers::FlatBufferBuilder* builder,
-    const std::vector<std::string>& vec) {
-  if (vec.empty())
+    const T& container) {
+  if (container.empty())
     return FlatStringListOffset();
 
   std::vector<FlatStringOffset> offsets;
-  offsets.reserve(vec.size());
-  for (const auto& str : vec)
+  offsets.reserve(container.size());
+  for (const std::string& str : container)
     offsets.push_back(builder->CreateSharedString(str));
   return builder->CreateVector(offsets);
 }
@@ -110,9 +112,18 @@ FlatOffset<flat::UrlTransform> BuildTransformOffset(
   FlatStringOffset password = create_string_offset(transform.password);
 
   FlatStringListOffset remove_query_params;
+  const bool use_plus = true;
   if (transform.query_transform && transform.query_transform->remove_params) {
-    remove_query_params = BuildVectorOfSharedStrings(
-        builder, *transform.query_transform->remove_params);
+    // Escape, sort and remove duplicates.
+    std::set<std::string> remove_params_escaped;
+    for (const std::string& remove_param :
+         *transform.query_transform->remove_params) {
+      remove_params_escaped.insert(
+          net::EscapeQueryParamValue(remove_param, use_plus));
+    }
+
+    remove_query_params =
+        BuildVectorOfSharedStrings(builder, remove_params_escaped);
   }
 
   FlatVectorOffset<flat::QueryKeyValue> add_or_replace_params;
@@ -124,9 +135,12 @@ FlatOffset<flat::UrlTransform> BuildTransformOffset(
         transform.query_transform->add_or_replace_params->size());
     for (const dnr_api::QueryKeyValue& query_pair :
          *transform.query_transform->add_or_replace_params) {
-      add_or_replace_queries.push_back(flat::CreateQueryKeyValue(
-          *builder, builder->CreateSharedString(query_pair.key),
-          builder->CreateSharedString(query_pair.value)));
+      FlatStringOffset key = builder->CreateSharedString(
+          net::EscapeQueryParamValue(query_pair.key, use_plus));
+      FlatStringOffset value = builder->CreateSharedString(
+          net::EscapeQueryParamValue(query_pair.value, use_plus));
+      add_or_replace_queries.push_back(
+          flat::CreateQueryKeyValue(*builder, key, value));
     }
     add_or_replace_params = builder->CreateVector(add_or_replace_queries);
   }

extensions/browser/api/declarative_net_request/indexed_ruleset_format_version_unittest.cc

@@ -125,7 +125,7 @@ TEST_F(IndexedRulesetFormatVersionTest, CheckVersionUpdated) {
   EXPECT_EQ(StripCommentsAndWhitespace(kFlatbufferSchemaExpected),
             StripCommentsAndWhitespace(flatbuffer_schema))
       << "Schema change detected; update this test and the schema version.";
-  EXPECT_EQ(10, GetIndexedRulesetFormatVersionForTesting())
+  EXPECT_EQ(11, GetIndexedRulesetFormatVersionForTesting())
       << "Update this test if you update the schema version.";
 }
 

extensions/browser/api/declarative_net_request/ruleset_matcher.cc

@@ -22,7 +22,6 @@
 #include "extensions/browser/api/web_request/web_request_info.h"
 #include "extensions/common/api/declarative_net_request.h"
 #include "extensions/common/api/declarative_net_request/utils.h"
-#include "net/base/escape.h"
 #include "net/base/registry_controlled_domains/registry_controlled_domain.h"
 #include "net/base/url_util.h"
 #include "url/url_constants.h"
@@ -152,6 +151,12 @@ base::StringPiece CreateStringPiece(const ::flatbuffers::String& str) {
   return base::StringPiece(str.c_str(), str.size());
 }
 
+// Returns true if the given |vec| is nullptr or empty.
+template <typename T>
+bool IsEmpty(const flatbuffers::Vector<T>* vec) {
+  return !vec || vec->size() == 0;
+}
+
 // Performs any required query transformations on the |url|. Returns true if the
 // query should be modified and populates |modified_query|.
 bool GetModifiedQuery(const GURL& url,
@@ -159,41 +164,44 @@ bool GetModifiedQuery(const GURL& url,
                       std::string* modified_query) {
   DCHECK(modified_query);
 
-  // TODO(crbug.com/983685): We should be able to reduce the work here by
-  // storing the escaped query params in the indexed format.
-  const bool use_plus = true;
-  std::set<std::string> escaped_remove_query_params;
-  if (transform.remove_query_params()) {
-    for (const ::flatbuffers::String* str : *transform.remove_query_params()) {
-      escaped_remove_query_params.insert(
-          net::EscapeQueryParamValue(CreateStringPiece(*str), use_plus));
-    }
+  // |remove_query_params| should always be sorted.
+  DCHECK(
+      IsEmpty(transform.remove_query_params()) ||
+      std::is_sorted(transform.remove_query_params()->begin(),
+                     transform.remove_query_params()->end(),
+                     [](const flatbuffers::String* x1,
+                        const flatbuffers::String* x2) { return *x1 < *x2; }));
+
+  // Return early if there's nothing to modify.
+  if (IsEmpty(transform.remove_query_params()) &&
+      IsEmpty(transform.add_or_replace_query_params())) {
+    return false;
+  }
+
+  std::vector<base::StringPiece> remove_query_params;
+  if (!IsEmpty(transform.remove_query_params())) {
+    remove_query_params.reserve(transform.remove_query_params()->size());
+    for (const ::flatbuffers::String* str : *transform.remove_query_params())
+      remove_query_params.push_back(CreateStringPiece(*str));
   }
 
   // We don't use a map from keys to vector of values to ensure the relative
-  // order of different params specified by the extension is respected. We use
-  // a std::list to support fast removal from middle of the list.
-  std::list<std::pair<std::string, std::string>>
-      escaped_add_or_replace_query_params;
-  if (transform.add_or_replace_query_params()) {
+  // order of different params specified by the extension is respected. We use a
+  // std::list to support fast removal from middle of the list. Note that the
+  // key value pairs should already be escaped.
+  std::list<std::pair<base::StringPiece, base::StringPiece>>
+      add_or_replace_query_params;
+  if (!IsEmpty(transform.add_or_replace_query_params())) {
     for (const flat::QueryKeyValue* query_pair :
          *transform.add_or_replace_query_params()) {
       DCHECK(query_pair->key());
       DCHECK(query_pair->value());
-      std::string key = net::EscapeQueryParamValue(
-          CreateStringPiece(*query_pair->key()), use_plus);
-      std::string value = net::EscapeQueryParamValue(
-          CreateStringPiece(*query_pair->value()), use_plus);
-      escaped_add_or_replace_query_params.emplace_back(std::move(key),
-                                                       std::move(value));
+      add_or_replace_query_params.emplace_back(
+          CreateStringPiece(*query_pair->key()),
+          CreateStringPiece(*query_pair->value()));
     }
   }
 
-  if (escaped_add_or_replace_query_params.empty() &&
-      escaped_remove_query_params.empty()) {
-    return false;
-  }
-
   std::vector<std::string> query_parts;
 
   auto create_query_part = [](base::StringPiece key, base::StringPiece value) {
@@ -204,20 +212,20 @@ bool GetModifiedQuery(const GURL& url,
   for (net::QueryIterator it(url); !it.IsAtEnd(); it.Advance()) {
     std::string key = it.GetKey();
     // Remove query param.
-    if (base::Contains(escaped_remove_query_params, key)) {
+    if (std::binary_search(remove_query_params.begin(),
+                           remove_query_params.end(), key)) {
       query_changed = true;
       continue;
     }
 
-    auto replace_iterator =
-        std::find_if(escaped_add_or_replace_query_params.begin(),
-                     escaped_add_or_replace_query_params.end(),
-                     [&key](const std::pair<std::string, std::string>& param) {
-                       return param.first == key;
-                     });
+    auto replace_iterator = std::find_if(
+        add_or_replace_query_params.begin(), add_or_replace_query_params.end(),
+        [&key](const std::pair<base::StringPiece, base::StringPiece>& param) {
+          return param.first == key;
+        });
 
     // Nothing to do.
-    if (replace_iterator == escaped_add_or_replace_query_params.end()) {
+    if (replace_iterator == add_or_replace_query_params.end()) {
       query_parts.push_back(create_query_part(key, it.GetValue()));
       continue;
     }
@@ -225,11 +233,11 @@ bool GetModifiedQuery(const GURL& url,
     // Replace query param.
     query_changed = true;
     query_parts.push_back(create_query_part(key, replace_iterator->second));
-    escaped_add_or_replace_query_params.erase(replace_iterator);
+    add_or_replace_query_params.erase(replace_iterator);
   }
 
   // Append any remaining query params.
-  for (const auto& params : escaped_add_or_replace_query_params) {
+  for (const auto& params : add_or_replace_query_params) {
     query_changed = true;
     query_parts.push_back(create_query_part(params.first, params.second));
   }

extensions/browser/api/declarative_net_request/utils.cc

@@ -33,7 +33,7 @@ namespace {
 // url_pattern_index.fbs. Whenever an extension with an indexed ruleset format
 // version different from the one currently used by Chrome is loaded, the
 // extension ruleset will be reindexed.
-constexpr int kIndexedRulesetFormatVersion = 10;
+constexpr int kIndexedRulesetFormatVersion = 11;
 
 // This static assert is meant to catch cases where
 // url_pattern_index::kUrlPatternIndexFormatVersion is incremented without