Reland "Fix flaky test ServerAddressConvertsToSameLocalAddress"

This is a reland of commit b3873b4d.

The CL fixes a race condition in production code of autofill profile
sync bridge by modifying merge logic of the datatype. Previously, the
logic for dealing with duplicate entities was to always prefer the
remote one and silently delete the local one. This could end up with
two clients having a different entry each and the sync server having
both duplicate entries. The new behavior, introduced in this CL is to
keep the profile with greater GUID, lexicographically.

This change should also remove flakiness of one integration test (that
uncovered the actual issue), so the CL reenables the test.

The original CL caused flakiness in another integration test since GUIDs
in that test are randomly attributed. In this reland, we fix this by
making the expectation on the number of sync deletions depend on which
one of the test GUIDs is bigger.

Bug: 917498
Change-Id: I13994ac5bef8fcae41d37350e08ee80ab90f0916
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2002571
Auto-Submit: Jan Krcal <jkrcal@chromium.org>
Commit-Queue: Jan Krcal <jkrcal@chromium.org>
Commit-Queue: Maxim Kolosovskiy <kolos@chromium.org>
Reviewed-by: Maxim Kolosovskiy <kolos@chromium.org>
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#733636}

chrome/browser/sync/test/integration/two_client_autofill_sync_test.cc

@@ -188,7 +188,6 @@ IN_PROC_BROWSER_TEST_F(TwoClientAutofillProfileSyncTest,
                                LOCAL_DELETION, 0);
 }
 
-// Flaky on all platform. See crbug.com/971666
 IN_PROC_BROWSER_TEST_F(TwoClientAutofillProfileSyncTest,
                        AddDuplicateProfiles_OneIsVerified) {
   ASSERT_TRUE(SetupClients());
@@ -210,13 +209,20 @@ IN_PROC_BROWSER_TEST_F(TwoClientAutofillProfileSyncTest,
   EXPECT_EQ(verified_origin, GetAllAutoFillProfiles(0)[0]->origin());
   EXPECT_EQ(verified_origin, GetAllAutoFillProfiles(1)[0]->origin());
 
-  histograms.ExpectBucketCount("Sync.ModelTypeEntityChange3.AUTOFILL_PROFILE",
-                               LOCAL_DELETION, 0);
+  // Among duplicate profiles, sync prefers the one with largest GUID. If
+  // |profile0| (committed first) has a smaller GUID, client 1 should upload its
+  // deletion to the server. Otherwise, no deletion should occur.
+  if (profile1.guid() > profile0.guid()) {
+    histograms.ExpectBucketCount("Sync.ModelTypeEntityChange3.AUTOFILL_PROFILE",
+                                 LOCAL_DELETION, 1);
+  } else {
+    histograms.ExpectBucketCount("Sync.ModelTypeEntityChange3.AUTOFILL_PROFILE",
+                                 LOCAL_DELETION, 0);
+  }
 }
 
-IN_PROC_BROWSER_TEST_F(
-    TwoClientAutofillProfileSyncTest,
-    AddDuplicateProfiles_OneIsVerified_NonverifiedComesLater) {
+IN_PROC_BROWSER_TEST_F(TwoClientAutofillProfileSyncTest,
+                       AddDuplicateProfiles_OneAtStart_OtherComesLater) {
   ASSERT_TRUE(SetupClients());
   base::HistogramTester histograms;
 
@@ -225,26 +231,23 @@ IN_PROC_BROWSER_TEST_F(
       autofill::test::GetVerifiedProfile();  // I.e. Full + Verified.
   std::string verified_origin = profile1.origin();
 
-  // We start by having the verified profile.
-  AddProfile(1, profile1);
+  AddProfile(0, profile0);
   ASSERT_TRUE(SetupSync());
   EXPECT_TRUE(AutofillProfileChecker(0, 1).Wait());
 
   EXPECT_EQ(1U, GetAllAutoFillProfiles(0).size());
-  EXPECT_EQ(verified_origin, GetAllAutoFillProfiles(0)[0]->origin());
-  EXPECT_EQ(verified_origin, GetAllAutoFillProfiles(1)[0]->origin());
 
-  // Add the same (but non-verified) profile on the other client, afterwards.
-  AddProfile(0, profile0);
+  // Add the same (but verified) profile on the other client, afterwards.
+  AddProfile(1, profile1);
   EXPECT_TRUE(AutofillProfileChecker(0, 1).Wait());
 
-  // The profiles should de-duplicate via sync on both other client, the
-  // verified one should win.
+  // The latter addition is caught in deduplication logic in PDM to sync. As a
+  // result, both clients end up with the non-verified profile.
   EXPECT_EQ(1U, GetAllAutoFillProfiles(0).size());
   EXPECT_EQ(1U, GetAllAutoFillProfiles(0).size());
 
-  EXPECT_EQ(verified_origin, GetAllAutoFillProfiles(0)[0]->origin());
-  EXPECT_EQ(verified_origin, GetAllAutoFillProfiles(1)[0]->origin());
+  EXPECT_NE(verified_origin, GetAllAutoFillProfiles(0)[0]->origin());
+  EXPECT_NE(verified_origin, GetAllAutoFillProfiles(1)[0]->origin());
 
   histograms.ExpectBucketCount("Sync.ModelTypeEntityChange3.AUTOFILL_PROFILE",
                                LOCAL_DELETION, 0);

chrome/browser/sync/test/integration/two_client_wallet_sync_test.cc

@@ -465,7 +465,7 @@ IN_PROC_BROWSER_TEST_F(
 
 // Flaky. http://crbug.com/917498
 IN_PROC_BROWSER_TEST_F(TwoClientWalletSyncTest,
-                       DISABLED_ServerAddressConvertsToSameLocalAddress) {
+                       ServerAddressConvertsToSameLocalAddress) {
   GetFakeServer()->SetWalletData(
       {CreateSyncWalletAddress(/*name=*/"address-1", /*company=*/"Company-1"),
        CreateDefaultSyncPaymentsCustomerData()});

components/autofill/core/browser/webdata/OWNERS

 per-file *sync_bridge*=jkrcal@chromium.org
 per-file *sync_bridge*=file://components/sync/OWNERS
 
-per-file *syncable_service*=jkrcal@chromium.org
-per-file *syncable_service*=file://components/sync/OWNERS
+per-file autofill_profile_sync_difference_tracker*=jkrcal@chromium.org
+per-file autofill_profile_sync_difference_tracker*=file://components/sync/OWNERS
 
 per-file *type_controller*=jkrcal@chromium.org
 per-file *type_controller*=file://components/sync/OWNERS

components/autofill/core/browser/webdata/autofill_profile_sync_bridge.cc

@@ -251,13 +251,18 @@ base::Optional<syncer::ModelError> AutofillProfileSyncBridge::FlushSyncTracker(
                      base::Unretained(web_data_backend_))));
 
   std::vector<std::unique_ptr<AutofillProfile>> profiles_to_upload_to_sync;
-  RETURN_IF_ERROR(tracker->FlushToSync(&profiles_to_upload_to_sync));
+  std::vector<std::string> profiles_to_delete_from_sync;
+  RETURN_IF_ERROR(tracker->FlushToSync(&profiles_to_upload_to_sync,
+                                       &profiles_to_delete_from_sync));
   for (const std::unique_ptr<AutofillProfile>& entry :
        profiles_to_upload_to_sync) {
     change_processor()->Put(GetStorageKeyFromAutofillProfile(*entry),
                             CreateEntityDataFromAutofillProfile(*entry),
                             metadata_change_list.get());
   }
+  for (const std::string& storage_key : profiles_to_delete_from_sync) {
+    change_processor()->Delete(storage_key, metadata_change_list.get());
+  }
 
   return static_cast<syncer::SyncMetadataStoreChangeList*>(
              metadata_change_list.get())

components/autofill/core/browser/webdata/autofill_profile_sync_difference_tracker.cc

@@ -64,6 +64,11 @@ AutofillProfileSyncDifferenceTracker::IncorporateRemoteProfile(
   }
 
   // Check if profile appears under a different storage key to be de-duplicated.
+  // TODO(crbug.com/1043683): Deal with rare cases when an remote update
+  // contains several exact duplicates (with different guids). We should not
+  // only search in local only entries but also in |update_to_local_| and
+  // |add_to_local_|. Likely needs a bit of refactoring to make the resulting
+  // code easy to understand.
   for (const auto& pair : *GetLocalOnlyEntries()) {
     const std::string& local_storage_key = pair.first;
     const AutofillProfile& local = *pair.second;
@@ -71,30 +76,76 @@ AutofillProfileSyncDifferenceTracker::IncorporateRemoteProfile(
     // Look for exact duplicates, compare only profile contents (and
     // ignore origin and language code in comparison).
     if (local.Compare(*remote) == 0) {
-      // We found a duplicate, we keep the new (remote) one and delete the
-      // local one.
+      // A duplicate found: keep the version with the bigger storage key.
       DVLOG(2)
           << "[AUTOFILL SYNC] The profile "
           << base::UTF16ToUTF8(local.GetRawInfo(NAME_FIRST))
           << base::UTF16ToUTF8(local.GetRawInfo(NAME_LAST))
-          << " already exists with a different storage key; keep the remote key"
-          << remote_storage_key << " and delete the local key "
-          << local_storage_key;
-
-      // Ensure that a verified profile can never revert back to an unverified
-      // one. In such a case, take over the local origin for the new (remote)
-      // entry.
-      if (local.IsVerified() && !remote->IsVerified()) {
-        remote->set_origin(local.origin());
-        // Save a copy of the remote profile also to sync.
-        save_to_sync_.push_back(std::make_unique<AutofillProfile>(*remote));
+          << " already exists with a different storage key; keep the bigger "
+          << (remote_storage_key > local_storage_key ? "remote" : "local")
+          << " key " << std::max(remote_storage_key, local_storage_key)
+          << " and delete the smaller key "
+          << std::min(remote_storage_key, local_storage_key);
+      if (remote_storage_key > local_storage_key) {
+        // We keep the remote entity and delete the local one.
+        // Ensure that a verified profile can never revert back to an unverified
+        // one. In such a case, take over the old origin for the new entry.
+        if (local.IsVerified() && !remote->IsVerified()) {
+          remote->set_origin(local.origin());
+          // Save a copy of the remote profile also to sync.
+          save_to_sync_.push_back(std::make_unique<AutofillProfile>(*remote));
+        }
+        add_to_local_.push_back(std::move(remote));
+        // Deleting from sync is a no-op if it is local-only so far.
+        // There are a few ways how a synced local entry A could theoretically
+        // receive a remote duplicate B with a larger GUID:
+        //  1) Remote entity B got uploaded by another client through initial
+        //     sync. That client thus also knew about A and issued a deletion of
+        //     A at the same time. This client (if receiving creation of B
+        //     first) resolves the conflict in the same way and re-issues the
+        //     deletion of A. In most cases the redundant deletion does not even
+        //     get sent as the processor already knows A got deleted remotely.
+        //  2) Remote entity B got uploaded by another client through race
+        //     condition (i.e. not knowing about A, yet). In practice, this only
+        //     happens when two clients simultaneously convert a server profile
+        //     into local profiles. If the other client goes offline before
+        //     receiving A, this client is responsible for deleting A from the
+        //     server and thus must issue a deletion. (In most cases, the other
+        //     client does not go offline and thus both clients issue a deletion
+        //     of A independently).
+        //  3) (a paranoid case) Remote entity B got uploaded by another client
+        //     by an error, i.e. already as a duplicate given their local state.
+        //     Through standard flows, it should be impossible (duplicates are
+        //     cought early in PDM code so such a change attempt does not even
+        //     propagate to the sync bridge). Still, it's good to treat this
+        //     case here for robustness.
+        delete_from_sync_.insert(local_storage_key);
+        DeleteFromLocal(local_storage_key);
+      } else {
+        // We keep the local entity and delete the remote one.
+        // Ensure that a verified profile can never revert back to an unverified
+        // one. In such a case, modify the origin and re-upload. Otherwise,
+        // there's no need to upload it: either is was already uploaded before
+        // (if this is incremental sync) or we'll upload it with all the
+        // remaining data in GetLocalOnlyEntries (if this is an initial sync).
+        if (remote->IsVerified() && !local.IsVerified()) {
+          auto modified_local = std::make_unique<AutofillProfile>(local);
+          modified_local->set_origin(remote->origin());
+          update_to_local_.push_back(
+              std::make_unique<AutofillProfile>(*modified_local));
+          save_to_sync_.push_back(std::move(modified_local));
+          // The local entity is already marked for upload so it is not local
+          // only anymore (we do not want to upload it once again while flushing
+          // if this is initial sync).
+          GetLocalOnlyEntries()->erase(local_storage_key);
+        }
+        delete_from_sync_.insert(remote_storage_key);
       }
-      // Delete the local profile that gets replaced by |remote|.
-      DeleteFromLocal(local_storage_key);
-      break;
+      return base::nullopt;
     }
   }
 
+  // If no duplicate was found, just add the remote profile.
   add_to_local_.push_back(std::move(remote));
   return base::nullopt;
 }
@@ -132,10 +183,14 @@ Optional<ModelError> AutofillProfileSyncDifferenceTracker::FlushToLocal(
 }
 
 Optional<ModelError> AutofillProfileSyncDifferenceTracker::FlushToSync(
-    std::vector<std::unique_ptr<AutofillProfile>>* profiles_to_upload_to_sync) {
+    std::vector<std::unique_ptr<AutofillProfile>>* profiles_to_upload_to_sync,
+    std::vector<std::string>* profiles_to_delete_from_sync) {
   for (std::unique_ptr<AutofillProfile>& entry : save_to_sync_) {
     profiles_to_upload_to_sync->push_back(std::move(entry));
   }
+  for (const std::string& entry : delete_from_sync_) {
+    profiles_to_delete_from_sync->push_back(std::move(entry));
+  }
   return base::nullopt;
 }
 
@@ -200,9 +255,11 @@ AutofillProfileInitialSyncDifferenceTracker::IncorporateRemoteDelete(
 }
 
 Optional<ModelError> AutofillProfileInitialSyncDifferenceTracker::FlushToSync(
-    std::vector<std::unique_ptr<AutofillProfile>>* profiles_to_upload_to_sync) {
+    std::vector<std::unique_ptr<AutofillProfile>>* profiles_to_upload_to_sync,
+    std::vector<std::string>* profiles_to_delete_from_sync) {
   // First, flush standard updates to sync.
-  AutofillProfileSyncDifferenceTracker::FlushToSync(profiles_to_upload_to_sync);
+  AutofillProfileSyncDifferenceTracker::FlushToSync(
+      profiles_to_upload_to_sync, profiles_to_delete_from_sync);
 
   // For initial sync, we additionally need to upload all local only entries.
   if (!GetLocalOnlyEntries()) {

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

@@ -50,11 +50,12 @@ class AutofillProfileSyncDifferenceTracker {
       base::OnceClosure autofill_changes_callback);
 
   // Writes into |profiles_to_upload_to_sync| all autofill profiles to be sent
-  // to the sync server. After flushing, not further remote changes should get
-  // incorporated.
+  // to the sync server, and into |profiles_to_delete_from_sync| the storage
+  // keys of all profiles to be deleted from the server. After flushing, no
+  // further remote changes should get incorporated.
   virtual base::Optional<syncer::ModelError> FlushToSync(
-      std::vector<std::unique_ptr<AutofillProfile>>*
-          profiles_to_upload_to_sync);
+      std::vector<std::unique_ptr<AutofillProfile>>* profiles_to_upload_to_sync,
+      std::vector<std::string>* profiles_to_delete_from_sync);
 
  protected:
   // If the entry is found, |entry| will be return, otherwise base::nullopt is
@@ -89,7 +90,7 @@ class AutofillProfileSyncDifferenceTracker {
 
   // We use unique_ptrs for storing AutofillProfile to avoid unnecessary copies.
 
-  // Local data, mapped by storage key. Use unique_to_local() to access it.
+  // Local data, mapped by storage key. Use GetLocalOnlyEntries() to access it.
   std::map<std::string, std::unique_ptr<AutofillProfile>> local_only_entries_;
 
   // Contain changes (originating from sync) that need to be saved to the local
@@ -98,9 +99,13 @@ class AutofillProfileSyncDifferenceTracker {
   std::vector<std::unique_ptr<AutofillProfile>> add_to_local_;
   std::vector<std::unique_ptr<AutofillProfile>> update_to_local_;
 
-  // Contains merged data for entries that existed on both sync and local sides
+  // Contains data for entries that existed on both sync and local sides
   // and need to be saved back to sync.
   std::vector<std::unique_ptr<AutofillProfile>> save_to_sync_;
+  // Contains storage keys for entries that existed on both sync and local
+  // sides and need to be deleted from sync (because the conflict resolution
+  // preferred the local copies).
+  std::set<std::string> delete_from_sync_;
 
  private:
   DISALLOW_COPY_AND_ASSIGN(AutofillProfileSyncDifferenceTracker);
@@ -116,8 +121,8 @@ class AutofillProfileInitialSyncDifferenceTracker
       const std::string& storage_key) override;
 
   base::Optional<syncer::ModelError> FlushToSync(
-      std::vector<std::unique_ptr<AutofillProfile>>* profiles_to_upload_to_sync)
-      override;
+      std::vector<std::unique_ptr<AutofillProfile>>* profiles_to_upload_to_sync,
+      std::vector<std::string>* profiles_to_delete_from_sync) override;
 
   // Performs an additional pass through remote entries incorporated from sync
   // to find any similarities with local entries. Should be run after all