Convert TemplateURLParser's ParameterFilter to be a callback.

This removes a hurdle around object lifetimes for an upcoming change that
makes TemplateURLParser asynchronous.

Bug: 699342
Change-Id: I92d88f221d7c06f48d6f3e3a9e21f34514c70661
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1877188Reviewed-by: Kevin Bailey <krb@chromium.org>
Reviewed-by: Max Moroz <mmoroz@chromium.org>
Reviewed-by: Ilya Sherman <isherman@chromium.org>
Commit-Queue: Robert Sesek <rsesek@chromium.org>
Cr-Commit-Position: refs/heads/master@{#709248}

chrome/browser/importer/in_process_importer_bridge.cc

@@ -65,14 +65,8 @@ namespace {
 
 // FirefoxURLParameterFilter is used to remove parameter mentioning Firefox from
 // the search URL when importing search engines.
-class FirefoxURLParameterFilter : public TemplateURLParser::ParameterFilter {
- public:
-  FirefoxURLParameterFilter() {}
-  ~FirefoxURLParameterFilter() override {}
-
-  // TemplateURLParser::ParameterFilter method.
-  bool KeepParameter(const std::string& key,
-                     const std::string& value) override {
+bool FirefoxURLParameterFilter(const std::string& key,
+                               const std::string& value) {
   std::string low_value = base::ToLowerASCII(value);
   if (low_value.find("mozilla") != std::string::npos ||
       low_value.find("firefox") != std::string::npos ||
@@ -80,11 +74,7 @@ class FirefoxURLParameterFilter : public TemplateURLParser::ParameterFilter {
     return false;
   }
   return true;
-  }
-
- private:
-  DISALLOW_COPY_AND_ASSIGN(FirefoxURLParameterFilter);
-};
+}
 
 // Attempts to create a TemplateURL from the provided data. |title| is optional.
 // If TemplateURL creation fails, returns null.
@@ -110,15 +100,14 @@ void ParseSearchEnginesFromFirefoxXMLData(
   DCHECK(search_engines);
 
   std::map<std::string, std::unique_ptr<TemplateURL>> search_engine_for_url;
-  FirefoxURLParameterFilter param_filter;
   // The first XML file represents the default search engine in Firefox 3, so we
   // need to keep it on top of the list.
   auto default_turl = search_engine_for_url.end();
   for (auto xml_iter = xml_data.begin(); xml_iter != xml_data.end();
        ++xml_iter) {
-    std::unique_ptr<TemplateURL> template_url =
-        TemplateURLParser::Parse(UIThreadSearchTermsData(), xml_iter->data(),
-                                 xml_iter->length(), &param_filter);
+    std::unique_ptr<TemplateURL> template_url = TemplateURLParser::Parse(
+        UIThreadSearchTermsData(), xml_iter->data(), xml_iter->length(),
+        base::BindRepeating(&FirefoxURLParameterFilter));
     if (template_url) {
       auto iter = search_engine_for_url.find(template_url->url());
       if (iter == search_engine_for_url.end()) {

chrome/browser/search_engines/template_url_parser_unittest.cc

@@ -3,6 +3,8 @@
 // found in the LICENSE file.
 
 #include "components/search_engines/template_url_parser.h"
+
+#include "base/bind.h"
 #include "base/files/file_util.h"
 #include "base/logging.h"
 #include "base/path_service.h"
@@ -15,40 +17,16 @@
 
 using base::ASCIIToUTF16;
 
-// ParamFilterImpl ------------------------------------------------------------
-
-// Filters any param which as an occurrence of name_str_ in its name or an
-// occurrence of value_str_ in its value.
-class ParamFilterImpl : public TemplateURLParser::ParameterFilter {
- public:
-  ParamFilterImpl(const std::string& name_str, const std::string& value_str);
-  ~ParamFilterImpl() override;
-
-  bool KeepParameter(const std::string& key, const std::string& value) override;
-
- private:
-  std::string name_str_;
-  std::string value_str_;
-
-  DISALLOW_COPY_AND_ASSIGN(ParamFilterImpl);
-};
-
-ParamFilterImpl::ParamFilterImpl(const std::string& name_str,
-                                 const std::string& value_str)
-    : name_str_(name_str), value_str_(value_str) {}
-
-ParamFilterImpl::~ParamFilterImpl() {
-}
-
-bool ParamFilterImpl::KeepParameter(const std::string& key,
+// Filters any param which as an occurrence of |name_str| in its |key| or an
+// occurrence of |value_str| in its |value|.
+bool TestFilter(const std::string& name_str,
+                const std::string& value_str,
+                const std::string& key,
                 const std::string& value) {
-  return (name_str_.empty() || key.find(name_str_) == std::string::npos) &&
-         (value_str_.empty() || value.find(value_str_) == std::string::npos);
+  return (name_str.empty() || key.find(name_str) == std::string::npos) &&
+         (value_str.empty() || value.find(value_str) == std::string::npos);
 }
 
-
-// TemplateURLParserTest ------------------------------------------------------
-
 class TemplateURLParserTest : public testing::Test {
  protected:
   TemplateURLParserTest();
@@ -59,7 +37,7 @@ class TemplateURLParserTest : public testing::Test {
   // Parses the OpenSearch description document at file_name (relative to the
   // data dir). The TemplateURL is placed in |template_url_|.
   void ParseFile(const std::string& file_name,
-                 TemplateURLParser::ParameterFilter* filter);
+                 const TemplateURLParser::ParameterFilter& filter);
 
   // ParseFile parses the results into this template_url.
   std::unique_ptr<TemplateURL> template_url_;
@@ -82,7 +60,7 @@ void TemplateURLParserTest::SetUp() {
 
 void TemplateURLParserTest::ParseFile(
     const std::string& file_name,
-    TemplateURLParser::ParameterFilter* filter) {
+    const TemplateURLParser::ParameterFilter& filter) {
   base::FilePath full_path = osdd_dir_.AppendASCII(file_name);
   ASSERT_TRUE(base::PathExists(full_path));
 
@@ -95,22 +73,26 @@ void TemplateURLParserTest::ParseFile(
 // Actual tests ---------------------------------------------------------------
 
 TEST_F(TemplateURLParserTest, FailOnBogusURL) {
-  ASSERT_NO_FATAL_FAILURE(ParseFile("bogus.xml", nullptr));
+  ASSERT_NO_FATAL_FAILURE(
+      ParseFile("bogus.xml", TemplateURLParser::ParameterFilter()));
   EXPECT_FALSE(template_url_);
 }
 
 TEST_F(TemplateURLParserTest, PassOnHTTPS) {
-  ASSERT_NO_FATAL_FAILURE(ParseFile("https.xml", nullptr));
+  ASSERT_NO_FATAL_FAILURE(
+      ParseFile("https.xml", TemplateURLParser::ParameterFilter()));
   EXPECT_TRUE(template_url_);
 }
 
 TEST_F(TemplateURLParserTest, FailOnPost) {
-  ASSERT_NO_FATAL_FAILURE(ParseFile("post.xml", nullptr));
+  ASSERT_NO_FATAL_FAILURE(
+      ParseFile("post.xml", TemplateURLParser::ParameterFilter()));
   EXPECT_FALSE(template_url_);
 }
 
 TEST_F(TemplateURLParserTest, TestDictionary) {
-  ASSERT_NO_FATAL_FAILURE(ParseFile("dictionary.xml", nullptr));
+  ASSERT_NO_FATAL_FAILURE(
+      ParseFile("dictionary.xml", TemplateURLParser::ParameterFilter()));
   ASSERT_TRUE(template_url_);
   EXPECT_EQ(ASCIIToUTF16("Dictionary.com"), template_url_->short_name());
   EXPECT_EQ(GURL("http://cache.lexico.com/g/d/favicon.ico"),
@@ -121,7 +103,8 @@ TEST_F(TemplateURLParserTest, TestDictionary) {
 }
 
 TEST_F(TemplateURLParserTest, TestMSDN) {
-  ASSERT_NO_FATAL_FAILURE(ParseFile("msdn.xml", nullptr));
+  ASSERT_NO_FATAL_FAILURE(
+      ParseFile("msdn.xml", TemplateURLParser::ParameterFilter()));
   ASSERT_TRUE(template_url_);
   EXPECT_EQ(ASCIIToUTF16("Search \" MSDN"), template_url_->short_name());
   EXPECT_EQ(GURL("http://search.msdn.microsoft.com/search/favicon.ico"),
@@ -133,7 +116,8 @@ TEST_F(TemplateURLParserTest, TestMSDN) {
 }
 
 TEST_F(TemplateURLParserTest, TestWikipedia) {
-  ASSERT_NO_FATAL_FAILURE(ParseFile("wikipedia.xml", nullptr));
+  ASSERT_NO_FATAL_FAILURE(
+      ParseFile("wikipedia.xml", TemplateURLParser::ParameterFilter()));
   ASSERT_TRUE(template_url_);
   EXPECT_EQ(ASCIIToUTF16("Wikipedia (English)"), template_url_->short_name());
   EXPECT_EQ(GURL("http://en.wikipedia.org/favicon.ico"),
@@ -153,14 +137,16 @@ TEST_F(TemplateURLParserTest, TestWikipedia) {
 }
 
 TEST_F(TemplateURLParserTest, NoCrashOnEmptyAttributes) {
-  ASSERT_NO_FATAL_FAILURE(ParseFile("url_with_no_attributes.xml", nullptr));
+  ASSERT_NO_FATAL_FAILURE(ParseFile("url_with_no_attributes.xml",
+                                    TemplateURLParser::ParameterFilter()));
 }
 
 TEST_F(TemplateURLParserTest, TestFirefoxEbay) {
   // This file uses the Parameter extension
   // (see http://www.opensearch.org/Specifications/OpenSearch/Extensions/Parameter/1.0)
-  ParamFilterImpl filter("ebay", "ebay");
-  ASSERT_NO_FATAL_FAILURE(ParseFile("firefox_ebay.xml", &filter));
+  TemplateURLParser::ParameterFilter filter =
+      base::BindRepeating(&TestFilter, "ebay", "ebay");
+  ASSERT_NO_FATAL_FAILURE(ParseFile("firefox_ebay.xml", filter));
   ASSERT_TRUE(template_url_);
   EXPECT_EQ(ASCIIToUTF16("eBay"), template_url_->short_name());
   EXPECT_TRUE(template_url_->url_ref().SupportsReplacement(SearchTermsData()));
@@ -176,8 +162,9 @@ TEST_F(TemplateURLParserTest, TestFirefoxEbay) {
 
 TEST_F(TemplateURLParserTest, TestFirefoxWebster) {
   // This XML file uses a namespace.
-  ParamFilterImpl filter(std::string(), "Mozilla");
-  ASSERT_NO_FATAL_FAILURE(ParseFile("firefox_webster.xml", &filter));
+  TemplateURLParser::ParameterFilter filter =
+      base::BindRepeating(&TestFilter, std::string(), "Mozilla");
+  ASSERT_NO_FATAL_FAILURE(ParseFile("firefox_webster.xml", filter));
   ASSERT_TRUE(template_url_);
   EXPECT_EQ(ASCIIToUTF16("Webster"), template_url_->short_name());
   EXPECT_TRUE(template_url_->url_ref().SupportsReplacement(SearchTermsData()));
@@ -191,8 +178,9 @@ TEST_F(TemplateURLParserTest, TestFirefoxWebster) {
 
 TEST_F(TemplateURLParserTest, TestFirefoxYahoo) {
   // This XML file uses a namespace.
-  ParamFilterImpl filter(std::string(), "Mozilla");
-  ASSERT_NO_FATAL_FAILURE(ParseFile("firefox_yahoo.xml", &filter));
+  TemplateURLParser::ParameterFilter filter =
+      base::BindRepeating(&TestFilter, std::string(), "Mozilla");
+  ASSERT_NO_FATAL_FAILURE(ParseFile("firefox_yahoo.xml", filter));
   ASSERT_TRUE(template_url_);
   EXPECT_EQ(ASCIIToUTF16("Yahoo"), template_url_->short_name());
   EXPECT_TRUE(template_url_->url_ref().SupportsReplacement(SearchTermsData()));
@@ -211,8 +199,9 @@ TEST_F(TemplateURLParserTest, TestFirefoxYahoo) {
 // firefox_yahoo.xml, the suggestion method was just changed to POST).
 TEST_F(TemplateURLParserTest, TestPostSuggestion) {
   // This XML file uses a namespace.
-  ParamFilterImpl filter(std::string(), "Mozilla");
-  ASSERT_NO_FATAL_FAILURE(ParseFile("post_suggestion.xml", &filter));
+  TemplateURLParser::ParameterFilter filter =
+      base::BindRepeating(&TestFilter, std::string(), "Mozilla");
+  ASSERT_NO_FATAL_FAILURE(ParseFile("post_suggestion.xml", filter));
   ASSERT_TRUE(template_url_);
   EXPECT_EQ(ASCIIToUTF16("Yahoo"), template_url_->short_name());
   EXPECT_TRUE(template_url_->url_ref().SupportsReplacement(SearchTermsData()));
@@ -227,7 +216,8 @@ TEST_F(TemplateURLParserTest, TestPostSuggestion) {
 
 // <Alias> tags are parsed and used as keyword for the template URL.
 TEST_F(TemplateURLParserTest, TestKeyword) {
-  ASSERT_NO_FATAL_FAILURE(ParseFile("keyword.xml", nullptr));
+  ASSERT_NO_FATAL_FAILURE(
+      ParseFile("keyword.xml", TemplateURLParser::ParameterFilter()));
   ASSERT_TRUE(template_url_);
   EXPECT_EQ(ASCIIToUTF16("Example"), template_url_->short_name());
   EXPECT_EQ("https://www.example.com/search?q={searchTerms}",
@@ -238,7 +228,8 @@ TEST_F(TemplateURLParserTest, TestKeyword) {
 // Empty <Alias> tags are ignored and the default keyword is used instead
 // (because empty keywords are not allowed).
 TEST_F(TemplateURLParserTest, TestEmptyKeyword) {
-  ASSERT_NO_FATAL_FAILURE(ParseFile("empty_keyword.xml", nullptr));
+  ASSERT_NO_FATAL_FAILURE(
+      ParseFile("empty_keyword.xml", TemplateURLParser::ParameterFilter()));
   ASSERT_TRUE(template_url_);
   EXPECT_EQ(ASCIIToUTF16("Example"), template_url_->short_name());
   EXPECT_EQ("https://www.example.com/search?q={searchTerms}",
@@ -249,12 +240,8 @@ TEST_F(TemplateURLParserTest, TestEmptyKeyword) {
 // An invalid template URL should not crash the parser.
 // crbug.com/770734
 TEST_F(TemplateURLParserTest, InvalidInput) {
-  struct DumbFilter : TemplateURLParser::ParameterFilter {
-    bool KeepParameter(const std::string& key,
-                       const std::string& value) override {
-      return true;
-    }
-  } filter;
+  TemplateURLParser::ParameterFilter filter = base::BindRepeating(
+      [](const std::string&, const std::string&) -> bool { return true; });
   constexpr char char_data[] = R"(
     <OpenSearchDescription>
     <Url template=")R:RRR?>RRR0" type="application/x-suggestions+json">
@@ -263,5 +250,5 @@ TEST_F(TemplateURLParserTest, InvalidInput) {
     </OpenSearchDescription>
   )";
   TemplateURLParser::Parse(SearchTermsData(), char_data, base::size(char_data),
-                           &filter);
+                           filter);
 }

components/search_engines/template_url_fetcher.cc

@@ -160,7 +160,8 @@ void TemplateURLFetcher::RequestDelegate::OnSimpleLoaderComplete(
 
   template_url_ = TemplateURLParser::Parse(
       fetcher_->template_url_service_->search_terms_data(),
-      response_body->data(), response_body->length(), nullptr);
+      response_body->data(), response_body->length(),
+      TemplateURLParser::ParameterFilter());
   if (!template_url_ ||
       !template_url_->url_ref().SupportsReplacement(
           fetcher_->template_url_service_->search_terms_data())) {

components/search_engines/template_url_parser.cc

@@ -132,7 +132,7 @@ class TemplateURLParsingContext {
   typedef std::pair<std::string, std::string> Param;
 
   explicit TemplateURLParsingContext(
-      TemplateURLParser::ParameterFilter* parameter_filter);
+      const TemplateURLParser::ParameterFilter& parameter_filter);
 
   static void StartElementImpl(void* ctx,
                                const xmlChar* name,
@@ -173,7 +173,7 @@ class TemplateURLParsingContext {
   // Character content for the current element.
   base::string16 string_;
 
-  TemplateURLParser::ParameterFilter* parameter_filter_;
+  const TemplateURLParser::ParameterFilter& parameter_filter_;
 
   // The list of parameters parsed in the Param nodes of a Url node.
   std::vector<Param> extra_params_;
@@ -202,7 +202,7 @@ TemplateURLParsingContext::ElementNameToElementTypeMap*
     TemplateURLParsingContext::kElementNameToElementTypeMap = nullptr;
 
 TemplateURLParsingContext::TemplateURLParsingContext(
-    TemplateURLParser::ParameterFilter* parameter_filter)
+    const TemplateURLParser::ParameterFilter& parameter_filter)
     : image_is_valid_for_favicon_(false),
       parameter_filter_(parameter_filter),
       method_(GET),
@@ -428,12 +428,12 @@ void TemplateURLParsingContext::ParseParam(const xmlChar** atts) {
   }
 
   if (!key.empty() &&
-      (!parameter_filter_ || parameter_filter_->KeepParameter(key, value)))
+      (parameter_filter_.is_null() || parameter_filter_.Run(key, value)))
     extra_params_.push_back(Param(key, value));
 }
 
 void TemplateURLParsingContext::ProcessURLParams() {
-  if (!parameter_filter_ && extra_params_.empty())
+  if (parameter_filter_.is_null() && extra_params_.empty())
     return;
 
   GURL url(is_suggest_url_ ? data_.suggestions_url : data_.url());
@@ -444,14 +444,14 @@ void TemplateURLParsingContext::ProcessURLParams() {
   // unwanted parameter.
   std::string new_query;
   bool modified = false;
-  if (parameter_filter_) {
+  if (!parameter_filter_.is_null()) {
     url::Component query = url.parsed_for_possibly_invalid_spec().query;
     url::Component key, value;
     const char* url_spec = url.spec().c_str();
     while (url::ExtractQueryKeyValue(url_spec, &query, &key, &value)) {
       std::string key_str(url_spec, key.begin, key.len);
       std::string value_str(url_spec, value.begin, value.len);
-      if (parameter_filter_->KeepParameter(key_str, value_str)) {
+      if (parameter_filter_.Run(key_str, value_str)) {
         AppendParamToQuery(key_str, value_str, &new_query);
       } else {
         modified = true;
@@ -497,7 +497,7 @@ std::unique_ptr<TemplateURL> TemplateURLParser::Parse(
     const SearchTermsData& search_terms_data,
     const char* data,
     size_t length,
-    TemplateURLParser::ParameterFilter* param_filter) {
+    const TemplateURLParser::ParameterFilter& param_filter) {
   // xmlSubstituteEntitiesDefault(1) makes it so that &amp; isn't mapped to
   // &#38; . Unfortunately xmlSubstituteEntitiesDefault affects global state.
   // If this becomes problematic we'll need to provide our own entity

components/search_engines/template_url_parser.h

@@ -10,6 +10,7 @@
 #include <memory>
 #include <string>
 
+#include "base/callback.h"
 #include "base/macros.h"
 
 class SearchTermsData;
@@ -19,16 +20,12 @@ class TemplateURL;
 // from OpenSearch description documents.
 class TemplateURLParser {
  public:
-  class ParameterFilter {
-   public:
-    // Invoked for each parameter of the template URL while parsing.  If this
-    // methods returns false, the parameter is not included.
-    virtual bool KeepParameter(const std::string& key,
-                               const std::string& value) = 0;
-
-   protected:
-    virtual ~ParameterFilter() {}
-  };
+  // A ParameterFilter is called for every URL paramter encountered during
+  // Parse(). It passes the parameter key as the first argument and the value
+  // as the second. The callback should return true if the parameter should be
+  // kept, and false if it should be discarded.
+  using ParameterFilter =
+      base::RepeatingCallback<bool(const std::string&, const std::string&)>;
 
   // Decodes the chunk of data representing a TemplateURL, creates the
   // TemplateURL, and returns it.  Returns null if the data does not describe a
@@ -42,7 +39,7 @@ class TemplateURLParser {
       const SearchTermsData& search_terms_data,
       const char* data,
       size_t length,
-      ParameterFilter* parameter_filter);
+      const ParameterFilter& parameter_filter);
 
  private:
   // No one should create one of these.

testing/libfuzzer/fuzzers/template_url_parser_fuzzer.cc

@@ -11,28 +11,21 @@
 #include "libxml/parser.h"
 
 #include "base/at_exit.h"
+#include "base/bind.h"
 #include "base/command_line.h"
 #include "base/i18n/icu_util.h"
 #include "components/search_engines/search_terms_data.h"
 #include "components/search_engines/template_url.h"
 #include "components/search_engines/template_url_parser.h"
+#include "testing/libfuzzer/libfuzzer_exports.h"
 
-class PseudoRandomFilter : public TemplateURLParser::ParameterFilter {
- public:
-  explicit PseudoRandomFilter(uint32_t seed) : generator_(seed), pool_(0, 1) {}
-  ~PseudoRandomFilter() override = default;
-
-  bool KeepParameter(const std::string&, const std::string&) override {
-    // Return true 254/255 times, ie: as if pool_ only returned uint8_t.
-    return pool_(generator_) % (UINT8_MAX + 1);
-  }
-
- private:
-  std::mt19937 generator_;
-  // Use a uint16_t here instead of uint8_t because uniform_int_distribution
-  // does not support 8 bit types on Windows.
-  std::uniform_int_distribution<uint16_t> pool_;
-};
+bool PseudoRandomFilter(std::mt19937* generator,
+                        std::uniform_int_distribution<uint16_t>* pool,
+                        const std::string&,
+                        const std::string&) {
+  // Return true 254/255 times, ie: as if pool only returned uint8_t.
+  return (*pool)(*generator) % (UINT8_MAX + 1);
+}
 
 struct FuzzerFixedParams {
   uint32_t seed_;
@@ -60,11 +53,21 @@ extern "C" int LLVMFuzzerTestOneInput(const uint8_t* data, size_t size) {
   if (size < sizeof(FuzzerFixedParams)) {
     return 0;
   }
+
   const FuzzerFixedParams* params =
       reinterpret_cast<const FuzzerFixedParams*>(data);
   size -= sizeof(FuzzerFixedParams);
+
+  std::mt19937 generator(params->seed_);
+  // Use a uint16_t here instead of uint8_t because uniform_int_distribution
+  // does not support 8 bit types on Windows.
+  std::uniform_int_distribution<uint16_t> pool(0, 1);
+
+  TemplateURLParser::ParameterFilter filter =
+      base::BindRepeating(&PseudoRandomFilter, base::Unretained(&generator),
+                          base::Unretained(&pool));
+
   const char* char_data = reinterpret_cast<const char*>(params + 1);
-  PseudoRandomFilter filter(params->seed_);
-  TemplateURLParser::Parse(SearchTermsData(), char_data, size, &filter);
+  TemplateURLParser::Parse(SearchTermsData(), char_data, size, filter);
   return 0;
 }