[Payments] Add timeout to the address_normalizer.

components/payments/address_normalizer.cc

 // Copyright 2017 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "components/payments/address_normalizer.h"
 
 #include <memory>
 #include <utility>
 
+#include "base/cancelable_callback.h"
 #include "base/strings/utf_string_conversions.h"
+#include "base/threading/sequenced_task_runner_handle.h"
 #include "components/autofill/core/browser/address_i18n.h"
 #include "third_party/libaddressinput/chromium/chrome_address_validator.h"
 #include "third_party/libaddressinput/src/cpp/include/libaddressinput/address_data.h"
 #include "third_party/libaddressinput/src/cpp/include/libaddressinput/source.h"
 #include "third_party/libaddressinput/src/cpp/include/libaddressinput/storage.h"
 
 namespace {
 using ::i18n::addressinput::Source;
 using ::i18n::addressinput::Storage;
 }  // namespace
 
 namespace payments {
 
 namespace {
 
 class AddressNormalizationRequest : public AddressNormalizer::Request {
  public:
   // The |delegate| and |address_validator| need to outlive this Request.
   AddressNormalizationRequest(const AutofillProfile& profile,
                               const std::string& region_code,
+                              int timeout_seconds,
                               AddressNormalizer::Delegate* delegate,
                               autofill::AddressValidator* address_validator)
       : profile_(profile),
         region_code_(region_code),
         delegate_(delegate),
-        address_validator_(address_validator) {}
+        address_validator_(address_validator),
+        has_responded_(false),
+        on_timeout_(
+            base::Bind(&::payments::AddressNormalizationRequest::OnRulesLoaded,
+                       base::Unretained(this),
+                       false)) {
+    base::SequencedTaskRunnerHandle::Get()->PostDelayedTask(
+        FROM_HERE, on_timeout_.callback(),
+        base::TimeDelta::FromSeconds(timeout_seconds));
+  }
 
   ~AddressNormalizationRequest() override {}
 
   void OnRulesLoaded(bool success) override {
+    on_timeout_.Cancel();
+
+    if (has_responded_)
+      return;
+    has_responded_ = true;

[please use gerrit instead, 2017/02/21 17:15:12]

This pattern is used to guard against OnRulesLoaded() being called multiple
times. There's no valid use case for that happening. Therefore, it's a bug.
Let's not add bug-handling logic to the code. Instead, let's fix the bug and
remove |has_responded_|.

[sebsg, 2017/02/21 18:30:59]

With the current code, if the timeout happens first, when the rules are finally
loaded or aborted this will be called a second time. This is why I added this so
that the delegate only gets called once.

Example, the server is not available and it takes 3 seconds for the rule loader
to give up. Someone asks to normalize with a timeout of 2 seconds. In that case,
the timeout will trigger then the rules loaded will return with success=false.

[please use gerrit instead, 2017/02/21 19:56:16]

Is it possible for rules to never load? If the rules never load, this object
will continue occupying memory in pending_normalization_ map. I wonder if it's
possible to delete this object upon timeout.

[sebsg, 2017/02/21 22:17:07]

As discussed offline, The rule loader will always return, but in the case where
it returns after a timeout has occured, we don't want to call the delegate
twice. Added comment. Thanks!

+
     if (!success) {
       delegate_->OnCouldNotNormalize(profile_);
       return;
     }
 
+    // The rules should be loaded.
     DCHECK(address_validator_->AreRulesLoadedForRegion(region_code_));
 
     // Create the AddressData from the profile.
     ::i18n::addressinput::AddressData address_data =
         *autofill::i18n::CreateAddressDataFromAutofillProfile(profile_,
                                                               region_code_);
 
     // Normalize the address.
     if (address_validator_ &&
         address_validator_->NormalizeAddress(&address_data)) {
       profile_.SetRawInfo(autofill::ADDRESS_HOME_STATE,
                           base::UTF8ToUTF16(address_data.administrative_area));
       profile_.SetRawInfo(autofill::ADDRESS_HOME_CITY,
                           base::UTF8ToUTF16(address_data.locality));
       profile_.SetRawInfo(autofill::ADDRESS_HOME_DEPENDENT_LOCALITY,
                           base::UTF8ToUTF16(address_data.dependent_locality));
     }
 
     delegate_->OnAddressNormalized(profile_);
   }
 
  private:
   AutofillProfile profile_;
   std::string region_code_;
   AddressNormalizer::Delegate* delegate_;
   autofill::AddressValidator* address_validator_;
 
+  bool has_responded_;
+  base::CancelableCallback<void()> on_timeout_;
+
   DISALLOW_COPY_AND_ASSIGN(AddressNormalizationRequest);
 };
 
 }  // namespace
 
 AddressNormalizer::AddressNormalizer(std::unique_ptr<Source> source,
                                      std::unique_ptr<Storage> storage)
     : address_validator_(std::move(source), std::move(storage), this) {}
 
 AddressNormalizer::~AddressNormalizer() {}
 
 void AddressNormalizer::LoadRulesForRegion(const std::string& region_code) {
   address_validator_.LoadRules(region_code);
 }
 
 bool AddressNormalizer::AreRulesLoadedForRegion(
     const std::string& region_code) {
   return address_validator_.AreRulesLoadedForRegion(region_code);
 }
 
 void AddressNormalizer::StartAddressNormalization(
     const AutofillProfile& profile,
     const std::string& region_code,
+    int timeout_seconds,
     AddressNormalizer::Delegate* requester) {
+  DCHECK(timeout_seconds >= 0);
+
   std::unique_ptr<AddressNormalizationRequest> request(
-      new AddressNormalizationRequest(profile, region_code, requester,
-                                      &address_validator_));
+      new AddressNormalizationRequest(profile, region_code, timeout_seconds,

[Mathieu, 2017/02/21 17:40:43]

let's use base::MakeUnique instead of new

[sebsg, 2017/02/21 18:30:59]

Done.

+                                      requester, &address_validator_));
 
   // Check if the rules are already loaded.
   if (AreRulesLoadedForRegion(region_code)) {
     request->OnRulesLoaded(true);
   } else {
     // Setup the variables so the profile gets normalized when the rules have
     // finished loading.
     auto it = pending_normalization_.find(region_code);
     if (it == pending_normalization_.end()) {
       // If no entry exists yet, create the entry and assign it to |it|.
       it = pending_normalization_
                .insert(std::make_pair(region_code,
                                       std::vector<std::unique_ptr<Request>>()))
                .first;
     }
 
     it->second.push_back(std::move(request));
+
+    // Start loading the rules for that region. If the rule were already in the

[please use gerrit instead, 2017/02/21 17:15:12]

s/ rule / rules /

[sebsg, 2017/02/21 18:30:59]

Done.

+    // process of being loaded, this call will do nothing.
+    LoadRulesForRegion(region_code);
   }
 }
 
 void AddressNormalizer::OnAddressValidationRulesLoaded(
     const std::string& region_code,
     bool success) {
   // Check if an address normalization is pending.
   auto it = pending_normalization_.find(region_code);
   if (it != pending_normalization_.end()) {
     for (size_t i = 0; i < it->second.size(); ++i) {
       it->second[i]->OnRulesLoaded(success);
     }
     pending_normalization_.erase(it);
   }
 }
 
 }  // namespace payments