[Autofill] Remove self-destruct on timeout for RegionDataLoaderImpl

The self-destruction timer is not necessary because the RegionDataLoader
is guaranteed to be destructed once the query to libaddressinput
terminates.

Change-Id: Iac364183aa9d456b91ddc4d5f6f29b4c0ffe816d
Bug: 1148749
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2544945Reviewed-by: Dominic Battré <battre@chromium.org>
Reviewed-by: Rouslan Solomakhin <rouslan@chromium.org>
Reviewed-by: Christoph Schwering <schwering@google.com>
Commit-Queue: Matthias Körber <koerber@google.com>
Cr-Commit-Position: refs/heads/master@{#829628}

chrome/browser/ui/views/payments/shipping_address_editor_view_controller.cc

@@ -151,8 +151,7 @@ ShippingAddressEditorViewController::GetComboboxModelForType(
       region_model_ = model.get();
       if (chosen_country_index_ < countries_.size()) {
         model->LoadRegionData(countries_[chosen_country_index_].first,
-                              state()->GetRegionDataLoader(),
-                              /*timeout_ms=*/5000);
+                              state()->GetRegionDataLoader());
         if (!model->IsPendingRegionDataLoad()) {
           // If the data was already pre-loaded, the observer won't get notified
           // so we have to check for failure here.

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

@@ -28,12 +28,11 @@ class RegionDataLoader {
       const std::vector<const ::i18n::addressinput::RegionData*>& regions)>
       RegionDataLoaded;
 
-  virtual ~RegionDataLoader() {}
-  // Calls |loaded_callback| when the region data for |country_code| is ready or
-  // when |timeout_ms| miliseconds have passed. This may happen synchronously.
+  virtual ~RegionDataLoader() = default;
+  // Calls |loaded_callback| when the region data for |country_code| is ready.
+  // This may happen synchronously.
   virtual void LoadRegionData(const std::string& country_code,
-                              RegionDataLoaded callback,
-                              int64_t timeout_ms) = 0;
+                              RegionDataLoaded callback) = 0;
   // To forget about the |callback| givent to LoadRegionData, in cases where
   // callback owner is destroyed before loader.
   virtual void ClearCallback() = 0;

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

@@ -24,19 +24,20 @@ RegionDataLoaderImpl::RegionDataLoaderImpl(
       this, &RegionDataLoaderImpl::OnRegionDataLoaded));
 }
 
-RegionDataLoaderImpl::~RegionDataLoaderImpl() {}
+RegionDataLoaderImpl::~RegionDataLoaderImpl() = default;
 
 void RegionDataLoaderImpl::LoadRegionData(
     const std::string& country_code,
-    RegionDataLoaderImpl::RegionDataLoaded callback,
-    int64_t timeout_ms) {
+    RegionDataLoaderImpl::RegionDataLoaded callback) {
   callback_ = callback;
+  // This is the first and only time |LoadRules()| is called on the
+  // |region_data_supplier_|. This guarantees that the supplied callback
+  // |region_data_supplier_callback_| will be invoked resulting in the
+  // destruction of this instance.
+  // |LoadRules()| may use a network request that has an internal timeout of
+  // 5 seconds.
   region_data_supplier_.LoadRules(country_code,
                                   *region_data_supplier_callback_);
-
-  timer_.Start(FROM_HERE, base::TimeDelta::FromMilliseconds(timeout_ms),
-               base::BindOnce(&RegionDataLoaderImpl::OnRegionDataLoaded,
-                              base::Unretained(this), false, country_code, 0));
 }
 
 void RegionDataLoaderImpl::ClearCallback() {
@@ -46,7 +47,6 @@ void RegionDataLoaderImpl::ClearCallback() {
 void RegionDataLoaderImpl::OnRegionDataLoaded(bool success,
                                               const std::string& country_code,
                                               int unused_rule_count) {
-  timer_.Stop();
   if (!callback_.is_null()) {
     if (success) {
       std::string best_region_tree_language_tag;

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

@@ -39,8 +39,7 @@ class RegionDataLoaderImpl : public RegionDataLoader {
 
   // autofill::RegionDataLoader.
   void LoadRegionData(const std::string& country_code,
-                      RegionDataLoader::RegionDataLoaded callback,
-                      int64_t timeout_ms) override;
+                      RegionDataLoader::RegionDataLoaded callback) override;
   void ClearCallback() override;
 
  private:
@@ -58,7 +57,6 @@ class RegionDataLoaderImpl : public RegionDataLoader {
 
   std::string app_locale_;
   RegionDataLoader::RegionDataLoaded callback_;
-  base::OneShotTimer timer_;
 };
 
 }  // namespace autofill

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

@@ -8,14 +8,13 @@
 
 namespace autofill {
 
-TestRegionDataLoader::TestRegionDataLoader() {}
+TestRegionDataLoader::TestRegionDataLoader() = default;
 
-TestRegionDataLoader::~TestRegionDataLoader() {}
+TestRegionDataLoader::~TestRegionDataLoader() = default;
 
 void TestRegionDataLoader::LoadRegionData(
     const std::string& country_code,
-    autofill::RegionDataLoader::RegionDataLoaded callback,
-    int64_t unused_timeout_ms) {
+    autofill::RegionDataLoader::RegionDataLoaded callback) {
   if (synchronous_callback_) {
     SendRegionData(regions_, callback);
   } else {
@@ -45,9 +44,8 @@ void TestRegionDataLoader::SendRegionData(
     const std::vector<std::pair<std::string, std::string>>& regions,
     autofill::RegionDataLoader::RegionDataLoaded callback) {
   ::i18n::addressinput::RegionData root_region("");
-  for (const auto& region : regions) {
+  for (const auto& region : regions)
     root_region.AddSubRegion(region.first, region.second);
-  }
 
   callback.Run(root_region.sub_regions());
 }

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

@@ -20,9 +20,9 @@ class TestRegionDataLoader : public RegionDataLoader {
   ~TestRegionDataLoader() override;
 
   // RegionDataLoader.
-  void LoadRegionData(const std::string& country_code,
-                      autofill::RegionDataLoader::RegionDataLoaded callback,
-                      int64_t timeout_ms) override;
+  void LoadRegionData(
+      const std::string& country_code,
+      autofill::RegionDataLoader::RegionDataLoaded callback) override;
   void ClearCallback() override;
 
   std::string country_code() { return country_code_; }

components/autofill/core/browser/ui/region_combobox_model.cc

@@ -26,16 +26,14 @@ RegionComboboxModel::~RegionComboboxModel() {
 }
 
 void RegionComboboxModel::LoadRegionData(const std::string& country_code,
-                                         RegionDataLoader* region_data_loader,
-                                         int64_t timeout_ms) {
+                                         RegionDataLoader* region_data_loader) {
   DCHECK(region_data_loader);
   DCHECK(!region_data_loader_);
   region_data_loader_ = region_data_loader;
   region_data_loader_->LoadRegionData(
       country_code,
       base::BindRepeating(&RegionComboboxModel::OnRegionDataLoaded,
-                          weak_factory_.GetWeakPtr()),
-      timeout_ms);
+                          weak_factory_.GetWeakPtr()));
 }
 
 int RegionComboboxModel::GetItemCount() const {

components/autofill/core/browser/ui/region_combobox_model.h

@@ -34,8 +34,7 @@ class RegionComboboxModel : public ui::ComboboxModel {
   ~RegionComboboxModel() override;
 
   void LoadRegionData(const std::string& country_code,
-                      RegionDataLoader* region_data_loader,
-                      int64_t timeout_ms);
+                      RegionDataLoader* region_data_loader);
 
   bool IsPendingRegionDataLoad() const {
     return region_data_loader_ != nullptr;

components/autofill/core/browser/ui/region_combobox_model_unittest.cc

@@ -28,7 +28,7 @@ const char kOntarioName[] = "Ontario";
 TEST(RegionComboboxModelTest, QuebecOntarioRegions) {
   TestRegionDataLoader test_region_data_loader;
   RegionComboboxModel model;
-  model.LoadRegionData("", &test_region_data_loader, 0);
+  model.LoadRegionData("", &test_region_data_loader);
 
   std::vector<std::pair<std::string, std::string>> regions;
   regions.emplace_back(kQuebecCode, kQuebecName);
@@ -47,7 +47,7 @@ TEST(RegionComboboxModelTest, QuebecOntarioRegions) {
 TEST(RegionComboboxModelTest, FailingSource) {
   TestRegionDataLoader test_region_data_loader;
   RegionComboboxModel model;
-  model.LoadRegionData("", &test_region_data_loader, 0);
+  model.LoadRegionData("", &test_region_data_loader);
   test_region_data_loader.SendAsynchronousData(
       std::vector<std::pair<std::string, std::string>>());
 

third_party/libaddressinput/chromium/chrome_metadata_source.cc

@@ -94,6 +94,9 @@ void ChromeMetadataSource::Download(const std::string& key,
   std::unique_ptr<network::SimpleURLLoader> loader =
       network::SimpleURLLoader::Create(std::move(resource_request),
                                        traffic_annotation);
+  // Limit the request duration to 5 seconds.
+  loader->SetTimeoutDuration(base::TimeDelta::FromSeconds(5));
+
   auto it = requests_.insert(
       requests_.begin(),
       std::make_unique<Request>(key, std::move(loader), downloaded));