Use the data_decoder service in TemplateURLParser.

This requires making the TemplateURLParser be asynchronous rather than
directly returning the result. That has a ripple effect of changing the
lifetimes of some of the parameters to Parse(), such as the
SearchTermsData.

The Firefox importer also uses the TemplateURLParser (although it may
be entirely broken, per https://crbug.com/868768). The importer assumes
that all operations are synchronous, so this adds an internal helper
class to manage the now-asynchronous state for TemplateURL parsing.

Bug: 699342
Change-Id: I311d9e29dbbca34a4f5696b251a0fbaaadcc506b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1879973
Commit-Queue: Robert Sesek <rsesek@chromium.org>
Reviewed-by: Ilya Sherman <isherman@chromium.org>
Reviewed-by: Martin Barbella <mbarbella@chromium.org>
Reviewed-by: Nico Weber <thakis@chromium.org>
Reviewed-by: Kevin Bailey <krb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#712773}

chrome/browser/BUILD.gn

@@ -5580,6 +5580,7 @@ static_library("test_support") {
     "//content/test:test_support",
     "//google_apis:test_support",
     "//net:test_support",
+    "//services/data_decoder/public/cpp:test_support",
     "//services/preferences/public/cpp/tracked:test_support",
     "//skia",
     "//testing/gmock",

chrome/browser/importer/in_process_importer_bridge.cc

@@ -21,6 +21,7 @@
 #include "components/search_engines/template_url.h"
 #include "components/search_engines/template_url_parser.h"
 #include "components/search_engines/template_url_prepopulate_data.h"
+#include "services/data_decoder/public/cpp/data_decoder.h"
 #include "ui/base/l10n/l10n_util.h"
 
 #include <iterator>
@@ -92,53 +93,95 @@ std::unique_ptr<TemplateURL> CreateTemplateURL(const base::string16& url,
   return std::make_unique<TemplateURL>(data);
 }
 
-// Parses the OpenSearch XML files in |xml_files| and populates |search_engines|
-// with the resulting TemplateURLs.
-void ParseSearchEnginesFromFirefoxXMLData(
-    const std::vector<std::string>& xml_data,
-    TemplateURLService::OwnedTemplateURLVector* search_engines) {
-  DCHECK(search_engines);
+}  // namespace
 
-  std::map<std::string, std::unique_ptr<TemplateURL>> search_engine_for_url;
-  // 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(),
-        base::BindRepeating(&FirefoxURLParameterFilter));
-    if (template_url) {
-      auto iter = search_engine_for_url.find(template_url->url());
-      if (iter == search_engine_for_url.end()) {
-        iter = search_engine_for_url
-                   .insert(std::make_pair(template_url->url(),
-                                          std::move(template_url)))
-                   .first;
-      } else {
-        // We have already found a search engine with the same URL.  We give
-        // priority to the latest one found, as GetSearchEnginesXMLFiles()
-        // returns a vector with first Firefox default search engines and then
-        // the user's ones.  We want to give priority to the user ones.
-        iter->second = std::move(template_url);
+// When the Bridge receives the search engines XML data via
+// SetFirefoxSearchEnginesXMLData(), this class is responsible for managing the
+// asynchronous TemplateURL parsing operations. The Bridge generally operates
+// synchronously, so this class manages the state and notifies the bridge when
+// parsing is done.
+class InProcessImporterBridge::SearchEnginesParser {
+ public:
+  // Starts parsing the |search_engines_xml_data| and will notify |bridge|
+  // upon completion.
+  SearchEnginesParser(const std::vector<std::string>& search_engines_xml_data,
+                      InProcessImporterBridge* bridge)
+      : bridge_(bridge), data_decoder_(new data_decoder::DataDecoder()) {
+    DCHECK(!search_engines_xml_data.empty());
+    StartParse(search_engines_xml_data);
   }
-      if (default_turl == search_engine_for_url.end())
-        default_turl = iter;
+
+  // Returns true if all the data have been parsed, false if the operation
+  // is still ongoing.
+  bool is_done() const { return is_done_; }
+
+  // If InProcessImporterBridge::NotifyEnded() is called before is_done()
+  // returns true, NotifyEnded() sets this flag so that it can be called back
+  // to complete the import.
+  void set_notify_ended_on_completion() { notify_ended_on_completion_ = true; }
+
+ private:
+  void StartParse(const std::vector<std::string>& search_engines_xml_data) {
+    const auto& last_item = search_engines_xml_data.end() - 1;
+    TemplateURLParser::ParameterFilter param_filter =
+        base::BindRepeating(&FirefoxURLParameterFilter);
+
+    for (auto it = search_engines_xml_data.begin();
+         it != search_engines_xml_data.end(); ++it) {
+      // Because all TemplateURLParser are handled by the same data_decoder_
+      // instance, the results will be returned FIFO.
+      // The SearchEnginesParser is owned by the InProcessImporterBridge,
+      // which is not deleted until NotifyEnded() is called, so using Unretained
+      // is safe.
+      TemplateURLParser::ParseWithDataDecoder(
+          data_decoder_.get(), &search_terms_data_, *it, param_filter,
+          base::BindOnce(&SearchEnginesParser::OnURLParsed,
+                         base::Unretained(this), it == last_item));
     }
   }
 
-  // Put the results in the |search_engines| vector.
-  for (auto t_iter = search_engine_for_url.begin();
-       t_iter != search_engine_for_url.end(); ++t_iter) {
-    if (t_iter == default_turl)
-      search_engines->insert(search_engines->begin(),
-                             std::move(default_turl->second));
-    else
-      search_engines->push_back(std::move(t_iter->second));
+  void OnURLParsed(bool is_last_item, std::unique_ptr<TemplateURL> url) {
+    if (url)
+      parsed_urls_.push_back(std::move(url));
+
+    if (is_last_item)
+      FinishParsing();
   }
-}
 
-}  // namespace
+  void FinishParsing() {
+    is_done_ = true;
+
+    // Shut down the DataDecoder.
+    data_decoder_.reset();
+
+    bridge_->WriteSearchEngines(std::move(parsed_urls_));
+
+    if (notify_ended_on_completion_)
+      bridge_->NotifyEnded();
+  }
+
+  // Storage for the URLs. These are stored in the same order as the original
+  // |search_engines_xml_data|.
+  TemplateURLService::OwnedTemplateURLVector parsed_urls_;
+
+  InProcessImporterBridge* bridge_;  // Weak, owns this.
+
+  // Set to true if the last search engine has been parsed.
+  bool is_done_ = false;
+
+  // Set to true if the ImporterBridge has been NotifyEnded() already but was
+  // waiting on this class to finish the import.
+  bool notify_ended_on_completion_ = false;
+
+  // Parameter for TemplateURLParser.
+  UIThreadSearchTermsData search_terms_data_;
+
+  // The DataDecoder instance that is shared amongst all the TemplateURLs being
+  // parsed.
+  std::unique_ptr<data_decoder::DataDecoder> data_decoder_;
+
+  DISALLOW_COPY_AND_ASSIGN(SearchEnginesParser);
+};
 
 InProcessImporterBridge::InProcessImporterBridge(
     ProfileWriter* writer,
@@ -186,10 +229,10 @@ void InProcessImporterBridge::SetKeywords(
 
 void InProcessImporterBridge::SetFirefoxSearchEnginesXMLData(
     const std::vector<std::string>& search_engine_data) {
-  TemplateURLService::OwnedTemplateURLVector search_engines;
-  ParseSearchEnginesFromFirefoxXMLData(search_engine_data, &search_engines);
-
-  writer_->AddKeywords(std::move(search_engines), true);
+  if (!search_engine_data.empty()) {
+    // SearchEnginesParser will call back the Bridge back when it is done.
+    search_engines_.reset(new SearchEnginesParser(search_engine_data, this));
+  }
 }
 
 void InProcessImporterBridge::SetPasswordForm(
@@ -228,6 +271,13 @@ void InProcessImporterBridge::NotifyItemEnded(importer::ImportItem item) {
 }
 
 void InProcessImporterBridge::NotifyEnded() {
+  // If there are search engines to parse but parsing them is not yet complete,
+  // arrange to be called back when they are done.
+  if (search_engines_ && !search_engines_->is_done()) {
+    search_engines_->set_notify_ended_on_completion();
+    return;
+  }
+
   host_->NotifyImportEnded();
 }
 
@@ -236,3 +286,35 @@ base::string16 InProcessImporterBridge::GetLocalizedString(int message_id) {
 }
 
 InProcessImporterBridge::~InProcessImporterBridge() {}
+
+void InProcessImporterBridge::WriteSearchEngines(
+    TemplateURL::OwnedTemplateURLVector template_urls) {
+  std::map<std::string, std::unique_ptr<TemplateURL>> search_engine_for_url;
+  for (auto& template_url : template_urls) {
+    std::string key = template_url->url();
+    // Give priority to the latest template URL that is found, as
+    // GetSearchEnginesXMLFiles() returns a vector with first Firefox default
+    // search engines and then the user's ones. The user ones should take
+    // precedence.
+    search_engine_for_url[key] = std::move(template_url);
+  }
+  // The first URL 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();
+  if (!template_urls.empty())
+    default_turl = search_engine_for_url.find(template_urls[0]->url());
+
+  // Put the results in the |search_engines| vector.
+  TemplateURLService::OwnedTemplateURLVector search_engines;
+  for (auto it = search_engine_for_url.begin();
+       it != search_engine_for_url.end(); ++it) {
+    if (it == default_turl) {
+      search_engines.insert(search_engines.begin(),
+                            std::move(default_turl->second));
+    } else {
+      search_engines.push_back(std::move(it->second));
+    }
+  }
+
+  writer_->AddKeywords(std::move(search_engines), true);
+}

chrome/browser/importer/in_process_importer_bridge.h

@@ -60,10 +60,19 @@ class InProcessImporterBridge : public ImporterBridge {
   // End ImporterBridge implementation.
 
  private:
+  class SearchEnginesParser;
+  friend class SearchEnginesParser;
+
   ~InProcessImporterBridge() override;
 
+  // Called by the SearchEnginesParser when all the search engines have been
+  // parsed. The |template_urls| vector is in the same sort order that was
+  // passed to SetFirefoxSearchEnginesXMLData().
+  void WriteSearchEngines(TemplateURL::OwnedTemplateURLVector template_urls);
+
   ProfileWriter* const writer_;  // weak
   const base::WeakPtr<ExternalProcessImporterHost> host_;
+  std::unique_ptr<SearchEnginesParser> search_engines_;
 
   DISALLOW_COPY_AND_ASSIGN(InProcessImporterBridge);
 };

chrome/browser/search_engines/template_url_parser_unittest.cc

@@ -10,9 +10,11 @@
 #include "base/path_service.h"
 #include "base/stl_util.h"
 #include "base/strings/utf_string_conversions.h"
+#include "base/test/task_environment.h"
 #include "chrome/common/chrome_paths.h"
 #include "components/search_engines/search_terms_data.h"
 #include "components/search_engines/template_url.h"
+#include "services/data_decoder/public/cpp/test_support/in_process_data_decoder.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 using base::ASCIIToUTF16;
@@ -39,15 +41,25 @@ class TemplateURLParserTest : public testing::Test {
   void ParseFile(const std::string& file_name,
                  const TemplateURLParser::ParameterFilter& filter);
 
+  void ParseString(const std::string& data,
+                   const TemplateURLParser::ParameterFilter& filter);
+
   // ParseFile parses the results into this template_url.
   std::unique_ptr<TemplateURL> template_url_;
 
  private:
+  void OnTemplateURLParsed(base::OnceClosure quit_closure,
+                           std::unique_ptr<TemplateURL> template_url) {
+    template_url_ = std::move(template_url);
+    std::move(quit_closure).Run();
+  }
+
   base::FilePath osdd_dir_;
+  base::test::TaskEnvironment task_environment_;
+  data_decoder::test::InProcessDataDecoder data_decoder_;
 };
 
-TemplateURLParserTest::TemplateURLParserTest() {
-}
+TemplateURLParserTest::TemplateURLParserTest() {}
 
 TemplateURLParserTest::~TemplateURLParserTest() {
 }
@@ -66,8 +78,19 @@ void TemplateURLParserTest::ParseFile(
 
   std::string contents;
   ASSERT_TRUE(base::ReadFileToString(full_path, &contents));
-  template_url_ = TemplateURLParser::Parse(SearchTermsData(), contents.data(),
-                                           contents.length(), filter);
+  ParseString(contents, filter);
+}
+
+void TemplateURLParserTest::ParseString(
+    const std::string& data,
+    const TemplateURLParser::ParameterFilter& filter) {
+  base::RunLoop run_loop;
+  SearchTermsData search_terms_data;
+  TemplateURLParser::Parse(
+      &search_terms_data, data, filter,
+      base::BindOnce(&TemplateURLParserTest::OnTemplateURLParsed,
+                     base::Unretained(this), run_loop.QuitClosure()));
+  run_loop.Run();
 }
 
 // Actual tests ---------------------------------------------------------------
@@ -249,6 +272,5 @@ TEST_F(TemplateURLParserTest, InvalidInput) {
     </Url>
     </OpenSearchDescription>
   )";
-  TemplateURLParser::Parse(SearchTermsData(), char_data, base::size(char_data),
-                           filter);
+  ParseString(char_data, filter);
 }

chrome/browser/search_engines/template_url_service_test_util.h

@@ -15,6 +15,7 @@
 #include "components/search_engines/template_url.h"
 #include "components/search_engines/template_url_data.h"
 #include "components/search_engines/template_url_service_observer.h"
+#include "services/data_decoder/public/cpp/test_support/in_process_data_decoder.h"
 
 class KeywordWebDataService;
 class TemplateURLService;
@@ -83,6 +84,7 @@ class TemplateURLServiceTestUtil : public TemplateURLServiceObserver {
   base::string16 search_term_;
   scoped_refptr<KeywordWebDataService> web_data_service_;
   std::unique_ptr<TemplateURLService> model_;
+  data_decoder::test::InProcessDataDecoder data_decoder_;
 
   DISALLOW_COPY_AND_ASSIGN(TemplateURLServiceTestUtil);
 };

chrome/utility/BUILD.gn

@@ -48,6 +48,7 @@ static_library("utility") {
     "//media",
     "//net:net_with_v8",
     "//printing/buildflags",
+    "//services/data_decoder:lib",
     "//services/network:network_service",
     "//services/service_manager/public/cpp",
     "//skia",
@@ -100,7 +101,6 @@ static_library("utility") {
       "//chrome/common:mojo_bindings",
       "//chrome/common/importer:interfaces",
       "//components/autofill/core/common",
-      "//services/data_decoder:lib",
       "//services/proxy_resolver:lib",
     ]
   }

components/search_engines/BUILD.gn

@@ -70,9 +70,9 @@ static_library("search_engines") {
     "//components/variations",
     "//google_apis",
     "//net",
+    "//services/data_decoder/public/cpp",
     "//services/network/public/cpp",
     "//sql",
-    "//third_party/libxml",  # https://crbug.com/699342
     "//third_party/metrics_proto",
     "//ui/base",
     "//ui/gfx",

components/search_engines/DEPS

@@ -15,8 +15,8 @@ include_rules = [
   "+components/variations",
   "+components/webdata",
   "+google_apis",
-  "+libxml",
   "+net",
+  "+services/data_decoder/public",
   "+services/network/public/cpp",
   "+services/network/test",
   "+sql",

components/search_engines/template_url_fetcher.cc

@@ -80,6 +80,7 @@ class TemplateURLFetcher::RequestDelegate {
   base::string16 keyword() const { return keyword_; }
 
  private:
+  void OnTemplateURLParsed(std::unique_ptr<TemplateURL> template_url);
   void OnLoaded();
   void AddSearchProvider();
 
@@ -140,6 +141,25 @@ TemplateURLFetcher::RequestDelegate::RequestDelegate(
       50000 /* max_body_size */);
 }
 
+void TemplateURLFetcher::RequestDelegate::OnTemplateURLParsed(
+    std::unique_ptr<TemplateURL> template_url) {
+  template_url_ = std::move(template_url);
+
+  if (!template_url_ ||
+      !template_url_->url_ref().SupportsReplacement(
+          fetcher_->template_url_service_->search_terms_data())) {
+    fetcher_->RequestCompleted(this);
+    // WARNING: RequestCompleted deletes us.
+    return;
+  }
+
+  // Wait for the model to be loaded before adding the provider.
+  if (!fetcher_->template_url_service_->loaded())
+    return;
+  AddSearchProvider();
+  // WARNING: AddSearchProvider deletes us.
+}
+
 void TemplateURLFetcher::RequestDelegate::OnLoaded() {
   template_url_subscription_.reset();
   if (!template_url_)
@@ -158,23 +178,11 @@ void TemplateURLFetcher::RequestDelegate::OnSimpleLoaderComplete(
     return;
   }
 
-  template_url_ = TemplateURLParser::Parse(
-      fetcher_->template_url_service_->search_terms_data(),
-      response_body->data(), response_body->length(),
-      TemplateURLParser::ParameterFilter());
-  if (!template_url_ ||
-      !template_url_->url_ref().SupportsReplacement(
-          fetcher_->template_url_service_->search_terms_data())) {
-    fetcher_->RequestCompleted(this);
-    // WARNING: RequestCompleted deletes us.
-    return;
-  }
-
-  // Wait for the model to be loaded before adding the provider.
-  if (!fetcher_->template_url_service_->loaded())
-    return;
-  AddSearchProvider();
-  // WARNING: AddSearchProvider deletes us.
+  TemplateURLParser::Parse(
+      &fetcher_->template_url_service_->search_terms_data(),
+      *response_body.get(), TemplateURLParser::ParameterFilter(),
+      base::BindOnce(&RequestDelegate::OnTemplateURLParsed,
+                     base::Unretained(this)));
 }
 
 void TemplateURLFetcher::RequestDelegate::AddSearchProvider() {

components/search_engines/template_url_parser.h

@@ -16,6 +16,10 @@
 class SearchTermsData;
 class TemplateURL;
 
+namespace data_decoder {
+class DataDecoder;
+}
+
 // TemplateURLParser, as the name implies, handling reading of TemplateURLs
 // from OpenSearch description documents.
 class TemplateURLParser {
@@ -27,19 +31,30 @@ class TemplateURLParser {
   using ParameterFilter =
       base::RepeatingCallback<bool(const std::string&, const std::string&)>;
 
+  using ParseCallback = base::OnceCallback<void(std::unique_ptr<TemplateURL>)>;
+
   // Decodes the chunk of data representing a TemplateURL, creates the
-  // TemplateURL, and returns it.  Returns null if the data does not describe a
-  // valid TemplateURL, the URLs referenced do not point to valid http/https
-  // resources, or for some other reason we do not support the described
-  // TemplateURL.  |parameter_filter| can be used if you want to filter some
-  // parameters out of the URL.  For example, when importing from another
-  // browser, we remove any parameter identifying that browser.  If set to null,
-  // the URL is not modified.
-  static std::unique_ptr<TemplateURL> Parse(
-      const SearchTermsData& search_terms_data,
-      const char* data,
-      size_t length,
-      const ParameterFilter& parameter_filter);
+  // TemplateURL, and calls the |completion_callback| with the result. A null
+  // value is provided if the data does not describe a valid TemplateURL, the
+  // URLs referenced do not point to valid http/https resources, or for some
+  // other reason we do not support the described TemplateURL.
+  // |parameter_filter| can be used if you want to filter some parameters out
+  // of the URL. For example, when importing from another browser, we remove
+  // any parameter identifying that browser. If set to null, the URL is not
+  // modified.
+  static void Parse(const SearchTermsData* search_terms_data,
+                    const std::string& data,
+                    const ParameterFilter& parameter_filter,
+                    ParseCallback completion_callback);
+
+  // The same as Parse(), but it allows the caller to manage the lifetime of
+  // the DataDecoder service. The |data_decoder| must be kept alive until the
+  // |completion_callback| is called.
+  static void ParseWithDataDecoder(data_decoder::DataDecoder* data_decoder,
+                                   const SearchTermsData* search_terms_data,
+                                   const std::string& data,
+                                   const ParameterFilter& parameter_filter,
+                                   ParseCallback completion_callback);
 
  private:
   // No one should create one of these.

services/data_decoder/public/cpp/BUILD.gn

@@ -31,11 +31,13 @@ source_set("cpp") {
   public = [
     "data_decoder.h",
     "json_sanitizer.h",
+    "safe_xml_parser.h",
   ]
 
   sources = [
     "data_decoder.cc",
     "json_sanitizer.cc",
+    "safe_xml_parser.cc",
   ]
 
   configs += [ "//build/config/compiler:wexit_time_destructors" ]
@@ -68,12 +70,10 @@ source_set("cpp") {
     public += [
       "decode_image.h",
       "safe_bundled_exchanges_parser.h",
-      "safe_xml_parser.h",
     ]
     sources += [
       "decode_image.cc",
       "safe_bundled_exchanges_parser.cc",
-      "safe_xml_parser.cc",
     ]
   }
 

testing/libfuzzer/fuzzers/BUILD.gn

@@ -67,6 +67,8 @@ fuzzer_test("template_url_parser_fuzzer") {
     "//base",
     "//base:i18n",
     "//components/search_engines:search_engines",
+    "//services/data_decoder/public/cpp",
+    "//services/data_decoder/public/cpp:test_support",
     "//third_party/libxml:libxml",
   ]
   dict = "//third_party/libxml/fuzz/xml.dict"

testing/libfuzzer/fuzzers/template_url_parser_fuzzer.cc

@@ -14,9 +14,12 @@
 #include "base/bind.h"
 #include "base/command_line.h"
 #include "base/i18n/icu_util.h"
+#include "base/run_loop.h"
+#include "base/task/single_thread_task_executor.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 "services/data_decoder/public/cpp/test_support/in_process_data_decoder.h"
 #include "testing/libfuzzer/libfuzzer_exports.h"
 
 bool PseudoRandomFilter(std::mt19937* generator,
@@ -45,7 +48,11 @@ void ignore(void* ctx, const char* msg, ...) {
 
 class Env {
  public:
-  Env() { xmlSetGenericErrorFunc(NULL, &ignore); }
+  Env() { xmlSetGenericErrorFunc(nullptr, &ignore); }
+
+ private:
+  base::SingleThreadTaskExecutor executor_;
+  data_decoder::test::InProcessDataDecoder data_decoder_;
 };
 
 extern "C" int LLVMFuzzerTestOneInput(const uint8_t* data, size_t size) {
@@ -63,11 +70,22 @@ extern "C" int LLVMFuzzerTestOneInput(const uint8_t* data, size_t size) {
   // does not support 8 bit types on Windows.
   std::uniform_int_distribution<uint16_t> pool(0, 1);
 
+  base::RunLoop run_loop;
+
+  SearchTermsData search_terms_data;
+  std::string string_data(reinterpret_cast<const char*>(params + 1), size);
   TemplateURLParser::ParameterFilter filter =
       base::BindRepeating(&PseudoRandomFilter, base::Unretained(&generator),
                           base::Unretained(&pool));
+  TemplateURLParser::Parse(&search_terms_data, string_data, filter,
+                           base::BindOnce(
+                               [](base::OnceClosure quit_closure,
+                                  std::unique_ptr<TemplateURL> ignored) {
+                                 std::move(quit_closure).Run();
+                               },
+                               run_loop.QuitClosure()));
+
+  run_loop.Run();
 
-  const char* char_data = reinterpret_cast<const char*>(params + 1);
-  TemplateURLParser::Parse(SearchTermsData(), char_data, size, filter);
   return 0;
 }

third_party/libxml/BUILD.gn

@@ -141,7 +141,6 @@ static_library("libxml") {
     ":xml_reader",
     ":xml_writer",
     ":libxml_utils",
-    "//components/search_engines",
     "//testing/libfuzzer/*",
     "//third_party/blink/renderer/*",
     "//third_party/fontconfig",