[Autofill][States] Prevent LoadStatesData from sending callback for

every task posted

Currently, |AlternativeStateNameMap::LoadStatesData()| takes a
|done_callback| as a parameter and inserts this callback to
|pending_init_done_callbacks_| for every task posted which results in
an error since |done_callback| is of |base::OnceClosure| type and can
be fired only once.

With this change, |done_callback| is added to
|pending_init_done_callbacks_| before the tasks are posted.

Bug: 1111960
Change-Id: If6fdf4a6af2a3ee9eaa0f5ac21075a7ca7ead754
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2536630
Commit-Queue: Vidhan Jain <vidhanj@google.com>
Reviewed-by: Matthias Körber <koerber@google.com>
Reviewed-by: Dominic Battré <battre@chromium.org>
Cr-Commit-Position: refs/heads/master@{#827824}

components/autofill/core/browser/geo/alternative_state_name_map_updater.cc

@@ -74,27 +74,42 @@ bool AlternativeStateNameMapUpdater::ContainsState(
 }
 
 void AlternativeStateNameMapUpdater::LoadStatesData(
-    const CountryToStateNamesListMapping& country_to_state_names_map,
+    CountryToStateNamesListMapping country_to_state_names_map,
     PrefService* pref_service,
     base::OnceClosure done_callback) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-  // Get the states data installation path from |pref_service|.
-  const std::string data_download_path =
-      pref_service->GetString(prefs::kAutofillStatesDataDir);
+  DCHECK(pref_service);
+
+  // Get the states data installation path from |pref_service| which is set by
+  // the component updater once it downloads the states data and should be safe
+  // to use.
+  const base::FilePath data_download_path =
+      pref_service->GetFilePath(prefs::kAutofillStatesDataDir);
+
+  const std::vector<std::string>& country_codes =
+      CountryDataMap::GetInstance()->country_codes();
+
+  // Remove all invalid country names.
+  base::EraseIf(country_to_state_names_map,
+                [&country_codes](
+                    const CountryToStateNamesListMapping::value_type& entry) {
+                  return !base::Contains(country_codes, entry.first.value());
+                });
 
   // If the installed directory path is empty, it means that the component is
   // not ready for use yet.
-  if (data_download_path.empty()) {
+  if (data_download_path.empty() || country_to_state_names_map.empty()) {
     std::move(done_callback).Run();
     return;
   }
 
-  const std::vector<std::string>& country_codes =
-      CountryDataMap::GetInstance()->country_codes();
+  pending_init_done_callbacks_.push_back(std::move(done_callback));
 
   // The |country_to_state_names_map| maps country_code names to a vector of
   // state names that are associated with this corresponding country.
   for (const auto& entry : country_to_state_names_map) {
+    // country_code is used as the filename.
+    // Example -> File "DE" contains the geographical states data of Germany.
     const AlternativeStateNameMap::CountryCode& country_code = entry.first;
     const std::vector<AlternativeStateNameMap::StateName>& states =
         entry.second;
@@ -104,20 +119,12 @@ void AlternativeStateNameMapUpdater::LoadStatesData(
     if (!base::Contains(country_codes, country_code.value()))
       continue;
 
-    // country_code is used as the filename.
-    // Example -> File "DE" contains the geographical states data of Germany.
-    // |data_download_path| is set by the component updater once it downloads
-    // the states data and should be safe to use.
-    const base::FilePath file_path =
-        base::FilePath::FromUTF8Unsafe(data_download_path)
-            .AppendASCII(country_code.value());
-
     ++number_pending_init_tasks_;
-    pending_init_done_callbacks_.push_back(std::move(done_callback));
 
     base::PostTaskAndReplyWithResult(
         task_runner_.get(), FROM_HERE,
-        base::BindOnce(&LoadDataFromFile, file_path),
+        base::BindOnce(&LoadDataFromFile,
+                       data_download_path.AppendASCII(country_code.value())),
         base::BindOnce(
             &AlternativeStateNameMapUpdater::ProcessLoadedStateFileContent,
             weak_ptr_factory_.GetWeakPtr(), states));

components/autofill/core/browser/geo/alternative_state_name_map_updater.h

@@ -42,10 +42,9 @@ class AlternativeStateNameMapUpdater {
   // countries to load.
   // Each call to LoadStatesData triggers loading state data files, so requests
   // should be batched up.
-  void LoadStatesData(
-      const CountryToStateNamesListMapping& country_to_state_names_map,
-      PrefService* pref_service,
-      base::OnceClosure done_callback);
+  void LoadStatesData(CountryToStateNamesListMapping country_to_state_names_map,
+                      PrefService* pref_service,
+                      base::OnceClosure done_callback);
 
 #if defined(UNIT_TEST)
   // A wrapper around |ProcessLoadedStateFileContent| used for testing purposes.

components/autofill/core/browser/geo/alternative_state_name_map_updater_unittest.cc

@@ -84,12 +84,13 @@ TEST_F(AlternativeStateNameMapUpdaterTest, TestLoadStatesData) {
   base::WriteFile(GetPath().AppendASCII("DE"),
                   test::CreateStatesProtoAsString());
   WritePathToPref(GetPath());
-
+  CountryToStateNamesListMapping country_to_state_names_list_mapping = {
+      {AlternativeStateNameMap::CountryCode("DE"),
+       {AlternativeStateNameMap::StateName(ASCIIToUTF16("Bavaria"))}}};
   base::RunLoop run_loop;
   alternative_state_name_map_updater.LoadStatesData(
-      {{AlternativeStateNameMap::CountryCode("DE"),
-        {AlternativeStateNameMap::StateName(ASCIIToUTF16("Bavaria"))}}},
-      pref_service_.get(), run_loop.QuitClosure());
+      country_to_state_names_list_mapping, pref_service_.get(),
+      run_loop.QuitClosure());
   run_loop.Run();
 
   EXPECT_NE(
@@ -99,6 +100,29 @@ TEST_F(AlternativeStateNameMapUpdaterTest, TestLoadStatesData) {
       base::nullopt);
 }
 
+// Tests that there is no insertion in the AlternativeStateNameMap when a
+// garbage country code is supplied to the LoadStatesData for which the states
+// data file does not exist.
+TEST_F(AlternativeStateNameMapUpdaterTest, NoTaskIsPosted) {
+  test::ClearAlternativeStateNameMapForTesting();
+
+  base::WriteFile(GetPath().AppendASCII("DE"),
+                  test::CreateStatesProtoAsString());
+  WritePathToPref(GetPath());
+
+  CountryToStateNamesListMapping country_to_state_names_list_mapping = {
+      {AlternativeStateNameMap::CountryCode("DEE"),
+       {AlternativeStateNameMap::StateName(ASCIIToUTF16("Bavaria"))}}};
+  base::RunLoop run_loop;
+  alternative_state_name_map_updater.LoadStatesData(
+      country_to_state_names_list_mapping, pref_service_.get(),
+      run_loop.QuitClosure());
+  run_loop.Run();
+
+  EXPECT_TRUE(
+      AlternativeStateNameMap::GetInstance()->IsLocalisedStateNamesMapEmpty());
+}
+
 // Tests that the AlternativeStateNameMap is populated when
 // |StateNameMapUpdater::LoadStatesData()| is called and there are UTF8 strings.
 TEST_F(AlternativeStateNameMapUpdaterTest, TestLoadStatesDataUTF8) {
@@ -112,11 +136,14 @@ TEST_F(AlternativeStateNameMapUpdaterTest, TestLoadStatesDataUTF8) {
                  .alternative_names = {"Parana", "State of Parana"}}));
   WritePathToPref(GetPath());
 
+  CountryToStateNamesListMapping country_to_state_names_list_mapping = {
+      {AlternativeStateNameMap::CountryCode("ES"),
+       {AlternativeStateNameMap::StateName(ASCIIToUTF16("Parana"))}}};
+
   base::RunLoop run_loop;
   alternative_state_name_map_updater.LoadStatesData(
-      {{AlternativeStateNameMap::CountryCode("ES"),
-        {AlternativeStateNameMap::StateName(ASCIIToUTF16("Parana"))}}},
-      pref_service_.get(), run_loop.QuitClosure());
+      country_to_state_names_list_mapping, pref_service_.get(),
+      run_loop.QuitClosure());
   run_loop.Run();
 
   base::Optional<StateEntry> entry1 =
@@ -142,6 +169,57 @@ TEST_F(AlternativeStateNameMapUpdaterTest, TestLoadStatesDataUTF8) {
                                                {"Parana", "State of Parana"}));
 }
 
+// Tests that the AlternativeStateNameMap is populated when
+// |StateNameMapUpdater::LoadStatesData()| is called for states data of
+// multiple countries simultaneously.
+TEST_F(AlternativeStateNameMapUpdaterTest,
+       TestLoadStatesDataOfMultipleCountriesSimultaneously) {
+  test::ClearAlternativeStateNameMapForTesting();
+
+  base::WriteFile(GetPath().AppendASCII("DE"),
+                  test::CreateStatesProtoAsString());
+  base::WriteFile(
+      GetPath().AppendASCII("ES"),
+      test::CreateStatesProtoAsString(
+          "ES", {.canonical_name = "Paraná",
+                 .abbreviations = {"PR"},
+                 .alternative_names = {"Parana", "State of Parana"}}));
+  WritePathToPref(GetPath());
+
+  CountryToStateNamesListMapping country_to_state_names = {
+      {AlternativeStateNameMap::CountryCode("ES"),
+       {AlternativeStateNameMap::StateName(ASCIIToUTF16("Parana"))}},
+      {AlternativeStateNameMap::CountryCode("DE"),
+       {AlternativeStateNameMap::StateName(ASCIIToUTF16("Bavaria"))}}};
+
+  base::RunLoop run_loop;
+  alternative_state_name_map_updater.LoadStatesData(
+      country_to_state_names, pref_service_.get(), run_loop.QuitClosure());
+  run_loop.Run();
+
+  base::Optional<StateEntry> entry1 =
+      AlternativeStateNameMap::GetInstance()->GetEntry(
+          AlternativeStateNameMap::CountryCode("ES"),
+          AlternativeStateNameMap::StateName(base::UTF8ToUTF16("Paraná")));
+  EXPECT_NE(entry1, base::nullopt);
+  EXPECT_EQ(entry1->canonical_name(), "Paraná");
+  EXPECT_THAT(entry1->abbreviations(),
+              testing::UnorderedElementsAreArray({"PR"}));
+  EXPECT_THAT(entry1->alternative_names(), testing::UnorderedElementsAreArray(
+                                               {"Parana", "State of Parana"}));
+
+  base::Optional<StateEntry> entry2 =
+      AlternativeStateNameMap::GetInstance()->GetEntry(
+          AlternativeStateNameMap::CountryCode("DE"),
+          AlternativeStateNameMap::StateName(base::UTF8ToUTF16("Bavaria")));
+  EXPECT_NE(entry2, base::nullopt);
+  EXPECT_EQ(entry2->canonical_name(), "Bavaria");
+  EXPECT_THAT(entry2->abbreviations(),
+              testing::UnorderedElementsAreArray({"BY"}));
+  EXPECT_THAT(entry2->alternative_names(),
+              testing::UnorderedElementsAreArray({"Bayern"}));
+}
+
 // Tests the |StateNameMapUpdater::ContainsState()| functionality.
 TEST_F(AlternativeStateNameMapUpdaterTest, ContainsState) {
   EXPECT_TRUE(AlternativeStateNameMapUpdater::ContainsStateForTesting(