[Payments] Avoid a crash caused by AddressValidatorTest.

third_party/libaddressinput/chromium/chrome_address_validator.cc

 // Copyright 2014 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 "third_party/libaddressinput/chromium/chrome_address_validator.h"
 
 #include <cmath>
 
 #include "base/bind.h"
 #include "base/location.h"
@@ skipping 145 matching lines @@
 void AddressValidator::RulesLoaded(bool success,
                                    const std::string& region_code,
                                    int) {
   if (load_rules_listener_)
     load_rules_listener_->OnAddressValidationRulesLoaded(region_code, success);
 
   // Count the first failed attempt to load rules as well.
   if (success || attempts_number_[region_code] + 1 >= kMaxAttemptsNumber)
     return;
 
-  base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
-      FROM_HERE, base::Bind(&AddressValidator::RetryLoadRules,
-                            weak_factory_.GetWeakPtr(), region_code),
-      GetBaseRetryPeriod() * pow(2, attempts_number_[region_code]++));
+  // No need to retry if it's synchronous (which means that it's a test.)
+  if (base::ThreadTaskRunnerHandle::IsSet()) {

[sebsg, 2017/06/23 17:26:04]

I don't think we should modify the production code for the sake of the test. I
think you fixed the crash by loading "OZ", and it make much more sense for that
test.

If you do this the test will still fail though, because in
OnAddressValidationRulesLoaded of the test class, it expects the rules to be
loaded successfully. You'll need to change that assumption to depend on a
variable, since in the case of this test is should be RULES_UNAVAILABLE.  I'd be
much more comfortable with that change since it only affects the test class.

Thanks!

[Hirondelle, 2017/06/23 20:30:11]

Anyway, if it's synchronous, the next line would crash, and if it's async, this
line would not make a difference. But at the end, if you insist that it's not a
good design, we can remove the test.
No. The crash was fixed by adding this line. That change was introduced later
(when I was taking a look at countryinfo.txt out of curiosity, and noticed
inconsistent data on "ZZ", and changed this to avoid confusion. Also, because OZ
reminds me of the "Wizard of Oz" ;) )
No. The OnAddressValidationRulesLoaded checks for 
ASSERT_EQ(success, status == AddressValidator::SUCCESS) and NOT for
ASSERT_EQ(success, AddressValidator::SUCCESS). 

which is mostly for the sake of testing Validate(). This class doesn't require
success == 1.
I was confused for a good while by this line, the tests passing, and success =
0! 

You'll need to change that assumption to depend on a

[sebsg, 2017/06/23 21:59:32]

What I suggest is changing the expected status for the test. Right now it's
hardcoded (line 82) but you can change it. You can make it be RulesUnavailable
for the SubKeysNotExist test. It seems like it works on my machine.

[Parastoo, 2017/06/27 18:14:31]

Done.

+    base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
+        FROM_HERE,
+        base::Bind(&AddressValidator::RetryLoadRules,
+                   weak_factory_.GetWeakPtr(), region_code),
+        GetBaseRetryPeriod() * pow(2, attempts_number_[region_code]++));
+  }
 }
 
 void AddressValidator::RetryLoadRules(const std::string& region_code) {
   // Do not reset retry count.
   supplier_->LoadRules(region_code, *rules_loaded_);
 }
 
 }  // namespace autofill