[AF] Keep profiles validity states updated.

Validate the profiles as soon as their validity states become out of
date and before updating the database. Also,
- Refactoring the personal data manager unittests.
- Validator should have a copy of the profile, as the pointer may
get out-dated.
- Handle delayed validations.
See go/af-pdm-scheduling for explaining the algorithm.
See go/autofill-use-validation.
Bug: 899254

Change-Id: If8a173302ec0766a4a549efa84faa4885d546b8c
Reviewed-on: https://chromium-review.googlesource.com/c/1395832
Commit-Queue: Parastoo Geranmayeh <parastoog@google.com>
Reviewed-by: Roger McFarlane <rogerm@chromium.org>
Reviewed-by: Sebastien Seguin-Gagnon <sebsg@chromium.org>
Reviewed-by: Jan Krcal <jkrcal@chromium.org>
Cr-Commit-Position: refs/heads/master@{#626250}

components/autofill/core/browser/BUILD.gn

@@ -368,6 +368,8 @@ jumbo_static_library("test_support") {
     "test_autofill_manager.h",
     "test_autofill_profile_validator.cc",
     "test_autofill_profile_validator.h",
+    "test_autofill_profile_validator_delayed.cc",
+    "test_autofill_profile_validator_delayed.h",
     "test_autofill_provider.cc",
     "test_autofill_provider.h",
     "test_credit_card_save_manager.cc",

components/autofill/core/browser/autofill_profile_validator.cc

@@ -14,7 +14,6 @@
 #include "base/strings/utf_string_conversions.h"
 #include "base/threading/sequenced_task_runner_handle.h"
 #include "base/time/time.h"
-#include "components/autofill/core/browser/autofill_profile.h"
 #include "components/autofill/core/browser/autofill_profile_validation_util.h"
 #include "third_party/libaddressinput/src/cpp/include/libaddressinput/address_data.h"
 #include "third_party/libaddressinput/src/cpp/include/libaddressinput/address_validator.h"
@@ -39,14 +38,13 @@ AutofillProfileValidator::ValidationRequest::ValidationRequest(
     base::WeakPtr<const AutofillProfile> profile,
     autofill::AddressValidator* validator,
     AutofillProfileValidatorCallback on_validated)
-    : profile_(profile),
+    : profile_(*profile),
       validator_(validator),
       on_validated_(std::move(on_validated)),
       has_responded_(false),
       weak_factory_(this) {
   on_timeout_.Reset(base::BindOnce(&ValidationRequest::OnRulesLoaded,
                              weak_factory_.GetWeakPtr()));
-  DCHECK(profile_);
   base::SequencedTaskRunnerHandle::Get()->PostDelayedTask(
       FROM_HERE, on_timeout_.callback(),
       base::TimeDelta::FromSeconds(kRulesLoadingTimeoutSeconds));
@@ -63,12 +61,8 @@ void AutofillProfileValidator::ValidationRequest::OnRulesLoaded() {
     return;
   has_responded_ = true;
 
-  if (!profile_)
-    return;
-
-  profile_validation_util::ValidateProfile(profile_.get(), validator_);
-
-  std::move(on_validated_).Run(profile_.get());
+  profile_validation_util::ValidateProfile(&profile_, validator_);
+  std::move(on_validated_).Run(&profile_);
 }
 
 AutofillProfileValidator::AutofillProfileValidator(
@@ -102,7 +96,7 @@ void AutofillProfileValidator::StartProfileValidation(
 
     // Start loading the rules for the region. If the rules were already in the
     // process of being loaded, this call will do nothing.
-    address_validator_.LoadRules(region_code);
+    LoadRulesForRegion(region_code);
   }
 }
 

components/autofill/core/browser/autofill_profile_validator.h

@@ -14,14 +14,13 @@
 
 #include "base/cancelable_callback.h"
 #include "base/macros.h"
+#include "components/autofill/core/browser/autofill_profile.h"
 #include "third_party/libaddressinput/chromium/chrome_address_validator.h"
 #include "third_party/libaddressinput/src/cpp/include/libaddressinput/source.h"
 #include "third_party/libaddressinput/src/cpp/include/libaddressinput/storage.h"
 
 namespace autofill {
 
-class AutofillProfile;
-
 using AutofillProfileValidatorCallback =
     base::OnceCallback<void(const AutofillProfile*)>;
 
@@ -45,6 +44,13 @@ class AutofillProfileValidator : public autofill::LoadRulesListener {
   void StartProfileValidation(const AutofillProfile* profile,
                               AutofillProfileValidatorCallback cb);
 
+ protected:
+  // Starts loading the rules for the specified |region_code|.
+  virtual void LoadRulesForRegion(const std::string& region_code);
+
+  // The address validator used to load rules.
+  AddressValidator address_validator_;
+
  private:
   // ValidationRequest loads Rules from the server and validates various fields
   // in an autofill profile.
@@ -60,7 +66,7 @@ class AutofillProfileValidator : public autofill::LoadRulesListener {
     void OnRulesLoaded();
 
    private:
-    base::WeakPtr<const AutofillProfile> profile_;
+    AutofillProfile profile_;
 
     // Not owned. Outlives this object.
     AddressValidator* validator_;
@@ -78,9 +84,6 @@ class AutofillProfileValidator : public autofill::LoadRulesListener {
   // Returns whether the rules for the specified |region_code| is loaded.
   bool AreRulesLoadedForRegion(const std::string& region_code);
 
-  // Starts loading the rules for the specified |region_code|.
-  void LoadRulesForRegion(const std::string& region_code);
-
   // Implementation of the LoadRulesListener interface. Called when the address
   // rules for the |region_code| have finished loading.
   void OnAddressValidationRulesLoaded(const std::string& region_code,
@@ -90,9 +93,6 @@ class AutofillProfileValidator : public autofill::LoadRulesListener {
   std::map<std::string, std::vector<std::unique_ptr<ValidationRequest>>>
       pending_requests_;
 
-  // The address validator used to load rules.
-  AddressValidator address_validator_;
-
   DISALLOW_COPY_AND_ASSIGN(AutofillProfileValidator);
 };
 

components/autofill/core/browser/personal_data_manager.h

@@ -5,6 +5,7 @@
 #ifndef COMPONENTS_AUTOFILL_CORE_BROWSER_PERSONAL_DATA_MANAGER_H_
 #define COMPONENTS_AUTOFILL_CORE_BROWSER_PERSONAL_DATA_MANAGER_H_
 
+#include <deque>
 #include <list>
 #include <memory>
 #include <set>
@@ -255,11 +256,16 @@ class PersonalDataManager : public KeyedService,
   void UpdateProfilesServerValidityMapsIfNeeded(
       const std::vector<AutofillProfile*>& profiles);
 
-  // Updates the validity states of |profiles| according to client side
-  // validation API: |client_profile_validator_|.
+  // Requests an update for the validity states of the |profiles| according to
+  // client side validation API: |client_profile_validator_|.
   void UpdateClientValidityStates(
       const std::vector<AutofillProfile*>& profiles);
 
+  // Requests an update for the validity states of the |profile| according to
+  // the client side validation API: |client_profile_validator_|. Returns true
+  // if the validation was requested.
+  bool UpdateClientValidityStates(const AutofillProfile& profile);
+
   // Returns the profiles to suggest to the user, ordered by frecency.
   std::vector<AutofillProfile*> GetProfilesToSuggest() const;
 
@@ -388,6 +394,11 @@ class PersonalDataManager : public KeyedService,
   // Triggered when a profile is added/updated/removed on db.
   void OnAutofillProfileChanged(const AutofillProfileDeepChange& change);
 
+  void set_client_profile_validator_for_test(
+      AutofillProfileValidator* validator) {
+    client_profile_validator_ = validator;
+  }
+
  protected:
   // Only PersonalDataManagerFactory and certain tests can create instances of
   // PersonalDataManager.
@@ -467,9 +478,16 @@ class PersonalDataManager : public KeyedService,
                            ClearCreditCardNonSettingsOrigins);
   FRIEND_TEST_ALL_PREFIXES(PersonalDataManagerTest,
                            MoveJapanCityToStreetAddress);
-  FRIEND_TEST_ALL_PREFIXES(PersonalDataManagerTest, RequestProfileValidity);
+  FRIEND_TEST_ALL_PREFIXES(PersonalDataManagerTest,
+                           RequestProfileServerValidity);
   FRIEND_TEST_ALL_PREFIXES(PersonalDataManagerTest,
                            GetProfileSuggestions_InvalidDataBasedOnServer);
+  FRIEND_TEST_ALL_PREFIXES(PersonalDataManagerMockTest,
+                           UpdateProfilesValidityStates_MoveToJapan);
+  FRIEND_TEST_ALL_PREFIXES(PersonalDataManagerMockTest,
+                           UpdateProfilesValidityStates_AddUpdateSet);
+  FRIEND_TEST_ALL_PREFIXES(PersonalDataManagerMockTest,
+                           UpdateProfilesValidityStates_Dedupe);
 
   friend class autofill::AutofillInteractiveTest;
   friend class autofill::PersonalDataManagerFactory;
@@ -477,6 +495,8 @@ class PersonalDataManager : public KeyedService,
   friend class FormDataImporterTest;
   friend class PersonalDataManagerTest;
   friend class PersonalDataManagerTestBase;
+  friend class PersonalDataManagerHelper;
+  friend class PersonalDataManagerMockTest;
   friend class SaveImportedProfileTest;
   friend class ProfileSyncServiceAutofillTest;
   friend class ::RemoveAutofillTester;
@@ -561,7 +581,8 @@ class PersonalDataManager : public KeyedService,
   // https://crbug.com/871301
   void MoveJapanCityToStreetAddress();
 
-  // Called when the |profile| is validated by the AutofillProfileValidator.
+  // Called when the |profile| is validated by the AutofillProfileValidator,
+  // updates the profiles on the |ongoing_profile_changes_| and the DB.
   virtual void OnValidated(const AutofillProfile* profile);
 
   // Get the profiles fields validity map by |guid|.
@@ -722,9 +743,11 @@ class PersonalDataManager : public KeyedService,
   // Resets |synced_profile_validity_|.
   void ResetProfileValidity();
 
-  // Add/Update/Remove profiles on DB.
+  // Add/Update/Remove |profile| on DB.
   void AddProfileToDB(const AutofillProfile& profile);
-  void UpdateProfileInDB(const AutofillProfile& profile);
+  // |enforced| is true when the update should happen regardless of an equal
+  // profile. (equal in the sense of AutofillProfile::EqualForUpdate)
+  void UpdateProfileInDB(const AutofillProfile& profile, bool enforced = false);
   void RemoveProfileFromDB(const std::string& guid);
 
   // Look at the next profile change for profile with guid = |guid|, and handle
@@ -764,11 +787,11 @@ class PersonalDataManager : public KeyedService,
 
   // Profiles validity read from the prefs. They are kept updated by
   // observing changes in pref_services. We need to set the
-  // |profile_validities_need_update| whenever this is changed.
+  // |profile_validities_need_update_| whenever this is changed.
   std::unique_ptr<UserProfileValidityMap> synced_profile_validity_;
 
   // A timely ordered list of on going changes for each profile.
-  std::unordered_map<std::string, std::queue<AutofillProfileDeepChange>>
+  std::unordered_map<std::string, std::deque<AutofillProfileDeepChange>>
       ongoing_profile_changes_;
 
   // The client side profile validator.

components/autofill/core/browser/test_autofill_profile_validator.cc

@@ -49,8 +49,18 @@ AutofillProfileValidator* TestAutofillProfileValidator::GetInstance() {
   return &(instance.Get().autofill_profile_validator_);
 }
 
+// static
+TestAutofillProfileValidatorDelayed*
+TestAutofillProfileValidator::GetDelayedInstance() {
+  static base::LazyInstance<TestAutofillProfileValidator>::DestructorAtExit
+      instance = LAZY_INSTANCE_INITIALIZER;
+  return &(instance.Get().autofill_profile_validator_delayed_);
+}
+
 TestAutofillProfileValidator::TestAutofillProfileValidator()
-    : autofill_profile_validator_(GetInputSource(), GetInputStorage()) {}
+    : autofill_profile_validator_(GetInputSource(), GetInputStorage()),
+      autofill_profile_validator_delayed_(GetInputSource(), GetInputStorage()) {
+}
 
 TestAutofillProfileValidator::~TestAutofillProfileValidator() {}
 

components/autofill/core/browser/test_autofill_profile_validator.h

@@ -8,6 +8,7 @@
 #include "base/lazy_instance.h"
 #include "base/macros.h"
 #include "components/autofill/core/browser/autofill_profile_validator.h"
+#include "components/autofill/core/browser/test_autofill_profile_validator_delayed.h"
 
 namespace autofill {
 
@@ -15,6 +16,7 @@ namespace autofill {
 class TestAutofillProfileValidator {
  public:
   static AutofillProfileValidator* GetInstance();
+  static TestAutofillProfileValidatorDelayed* GetDelayedInstance();
 
  private:
   friend struct base::LazyInstanceTraitsBase<TestAutofillProfileValidator>;
@@ -22,8 +24,9 @@ class TestAutofillProfileValidator {
   TestAutofillProfileValidator();
   ~TestAutofillProfileValidator();
 
-  // The only instance that exists.
+  // The only instance that exists of normal and delayed validators.
   AutofillProfileValidator autofill_profile_validator_;
+  TestAutofillProfileValidatorDelayed autofill_profile_validator_delayed_;
 
   DISALLOW_COPY_AND_ASSIGN(TestAutofillProfileValidator);
 };

components/autofill/core/browser/test_autofill_profile_validator_delayed.cc

+// Copyright 2018 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/autofill/core/browser/test_autofill_profile_validator_delayed.h"
+
+#include "base/bind.h"
+#include "base/bind_helpers.h"
+#include "base/cancelable_callback.h"
+#include "base/threading/sequenced_task_runner_handle.h"
+
+namespace autofill {
+namespace {
+
+const int kRulesDelayedLoadingTimeSeconds = 3;
+
+}  // namespace
+
+TestAutofillProfileValidatorDelayed::TestAutofillProfileValidatorDelayed(
+    std::unique_ptr<::i18n::addressinput::Source> source,
+    std::unique_ptr<::i18n::addressinput::Storage> storage)
+    : AutofillProfileValidator(std::move(source), std::move(storage)) {}
+
+TestAutofillProfileValidatorDelayed::~TestAutofillProfileValidatorDelayed() {}
+
+void TestAutofillProfileValidatorDelayed::LoadRulesInstantly(
+    const std::string& region_code) {
+  address_validator_.LoadRules(region_code);
+}
+
+void TestAutofillProfileValidatorDelayed::LoadRulesForRegion(
+    const std::string& region_code) {
+  base::SequencedTaskRunnerHandle::Get()->PostDelayedTask(
+      FROM_HERE,
+      base::BindOnce(&TestAutofillProfileValidatorDelayed::LoadRulesInstantly,
+                     base::Unretained(this), region_code),
+      base::TimeDelta::FromSeconds(kRulesDelayedLoadingTimeSeconds));
+}
+}  // namespace autofill

components/autofill/core/browser/test_autofill_profile_validator_delayed.h

+// Copyright 2018 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.
+
+#ifndef COMPONENTS_AUTOFILL_CORE_BROWSER_TEST_AUTOFILL_PROFILE_VALIDATOR_DELAYED_H_
+#define COMPONENTS_AUTOFILL_CORE_BROWSER_TEST_AUTOFILL_PROFILE_VALIDATOR_DELAYED_H_
+
+#include <memory>
+#include <string>
+
+#include "components/autofill/core/browser/autofill_profile_validator.h"
+#include "third_party/libaddressinput/chromium/chrome_address_validator.h"
+#include "third_party/libaddressinput/src/cpp/include/libaddressinput/source.h"
+#include "third_party/libaddressinput/src/cpp/include/libaddressinput/storage.h"
+
+namespace autofill {
+
+// Singleton that owns a single AutofillProfileValidator instance. It's a
+// delayed validator used in tests, to make sure that the system can handle
+// possible delays in the real world.
+class TestAutofillProfileValidatorDelayed : public AutofillProfileValidator {
+ public:
+  // Takes ownership of |source| and |storage|.
+  TestAutofillProfileValidatorDelayed(
+      std::unique_ptr<::i18n::addressinput::Source> source,
+      std::unique_ptr<::i18n::addressinput::Storage> storage);
+
+  ~TestAutofillProfileValidatorDelayed() override;
+
+  // Starts loading the rules for the specified |region_code|.
+  void LoadRulesForRegion(const std::string& region_code) override;
+
+ private:
+  void LoadRulesInstantly(const std::string& region_code);
+
+  DISALLOW_COPY_AND_ASSIGN(TestAutofillProfileValidatorDelayed);
+};
+
+}  // namespace autofill
+
+#endif  // COMPONENTS_AUTOFILL_CORE_BROWSER_TEST_AUTOFILL_PROFILE_VALIDATOR_DELAYED_H_

components/autofill/core/browser/webdata/autofill_change.h

@@ -97,10 +97,33 @@ class AutofillProfileDeepChange : public AutofillProfileChange {
 
   ~AutofillProfileDeepChange() override {}
 
-  AutofillProfile profile() const { return profile_; }
+  const AutofillProfile* profile() const { return &profile_; }
+  bool is_ongoing_on_background() const { return is_ongoing_on_background_; }
+  void set_is_ongoing_on_background() const {
+    is_ongoing_on_background_ = true;
+  }
+
+  void validation_effort_made() const { validation_effort_made_ = true; }
+  bool has_validation_effort_made() const { return validation_effort_made_; }
+
+  void set_enforce_update() { enforce_update_ = true; }
+  bool enforce_update() const { return enforce_update_; }
 
  private:
   AutofillProfile profile_;
+  // Is true when the change is taking place on the database side on the
+  // background.
+  mutable bool is_ongoing_on_background_ = false;
+  // Is true when the |profile_| has gone through the validation process.
+  // Note: This could be different from the
+  // profile_.is_client_validity_states_updated. |validation_effort_made_| shows
+  // that the effort has been made, but not necessarily successful, and profile
+  // validity may or may not be updated.
+  mutable bool validation_effort_made_ = false;
+
+  // Is true when the update should happen regardless of an equal profile.
+  // (equal in the sense of AutofillProfile::EqualForUpdate)
+  mutable bool enforce_update_ = false;
 };
 
 }  // namespace autofill