Reland "Reland "Fix flaky test ServerAddressConvertsToSameLocalAddress""

This is a reland of ba52e783

The test flakiness introduced by the previous attempt has been fixed by
another CL [1] that removed unneccessary expectations). Thus, this CL
should be safe to land again.

[1] https://chromium-review.googlesource.com/c/chromium/src/+/2022651

Original change's description:
> 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}

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

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

@@ -188,9 +188,8 @@ IN_PROC_BROWSER_TEST_F(TwoClientAutofillProfileSyncTest,
   EXPECT_EQ(verified_origin, GetAllAutoFillProfiles(1)[0]->origin());
 }
 
-IN_PROC_BROWSER_TEST_F(
-    TwoClientAutofillProfileSyncTest,
-    AddDuplicateProfiles_OneIsVerified_NonverifiedComesLater) {
+IN_PROC_BROWSER_TEST_F(TwoClientAutofillProfileSyncTest,
+                       AddDuplicateProfiles_OneAtStart_OtherComesLater) {
   ASSERT_TRUE(SetupClients());
 
   AutofillProfile profile0 = autofill::test::GetFullProfile();
@@ -198,25 +197,21 @@ 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, /*expected_count=*/1U).Wait());
 
-  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, /*expected_count=*/1U).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());
 }
 
 // Tests that a null profile does not get synced across clients.

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