[Payments] Add timeout to the address_normalizer.

components/payments/address_normalizer.cc

Index: components/payments/address_normalizer.cc
diff --git a/components/payments/address_normalizer.cc b/components/payments/address_normalizer.cc
index f8dd35a3082d7eafb1967ad3b7669c237b87b057..aa5accf6dd0e6fb0aabdbd766a5eb65e1430ca1f 100644
--- a/components/payments/address_normalizer.cc
+++ b/components/payments/address_normalizer.cc
@@ -7,7 +7,9 @@
 #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"
@@ -28,21 +30,38 @@ class AddressNormalizationRequest : public AddressNormalizer::Request {
   // 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.
@@ -70,6 +89,9 @@ class AddressNormalizationRequest : public AddressNormalizer::Request {
   AddressNormalizer::Delegate* delegate_;
   autofill::AddressValidator* address_validator_;
 
+  bool has_responded_;
+  base::CancelableCallback<void()> on_timeout_;
+
   DISALLOW_COPY_AND_ASSIGN(AddressNormalizationRequest);
 };
 
@@ -93,10 +115,13 @@ bool AddressNormalizer::AreRulesLoadedForRegion(
 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)) {
@@ -114,6 +139,10 @@ void AddressNormalizer::StartAddressNormalization(
     }
 
     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);
   }
 }