Revert "Reland "Fix flaky test ServerAddressConvertsToSameLocalAddress""

This reverts commit ba52e783.

Reason for revert: This caused flakiness in the TwoClientAutofillProfileSyncTest.ClientsAddSameProfile test.

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}

TBR=kolos@chromium.org,jkrcal@chromium.org,mastiz@chromium.org

Change-Id: Ifad46c8c079b24a07c044b9d6a5ffdc77a439617
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 917498
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2013281Reviewed-by: Jan Krcal <jkrcal@chromium.org>
Commit-Queue: Jan Krcal <jkrcal@chromium.org>
Cr-Commit-Position: refs/heads/master@{#733910}

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

@@ -188,6 +188,7 @@ 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());
@@ -209,20 +210,13 @@ IN_PROC_BROWSER_TEST_F(TwoClientAutofillProfileSyncTest,
   EXPECT_EQ(verified_origin, GetAllAutoFillProfiles(0)[0]->origin());
   EXPECT_EQ(verified_origin, GetAllAutoFillProfiles(1)[0]->origin());
 
-  // 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);
-  }
+  histograms.ExpectBucketCount("Sync.ModelTypeEntityChange3.AUTOFILL_PROFILE",
+                               LOCAL_DELETION, 0);
 }
 
-IN_PROC_BROWSER_TEST_F(TwoClientAutofillProfileSyncTest,
-                       AddDuplicateProfiles_OneAtStart_OtherComesLater) {
+IN_PROC_BROWSER_TEST_F(
+    TwoClientAutofillProfileSyncTest,
+    AddDuplicateProfiles_OneIsVerified_NonverifiedComesLater) {
   ASSERT_TRUE(SetupClients());
   base::HistogramTester histograms;
 
@@ -231,23 +225,26 @@ IN_PROC_BROWSER_TEST_F(TwoClientAutofillProfileSyncTest,
       autofill::test::GetVerifiedProfile();  // I.e. Full + Verified.
   std::string verified_origin = profile1.origin();
 
-  AddProfile(0, profile0);
+  // We start by having the verified profile.
+  AddProfile(1, profile1);
   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 verified) profile on the other client, afterwards.
-  AddProfile(1, profile1);
+  // Add the same (but non-verified) profile on the other client, afterwards.
+  AddProfile(0, profile0);
   EXPECT_TRUE(AutofillProfileChecker(0, 1).Wait());
 
-  // The latter addition is caught in deduplication logic in PDM to sync. As a
-  // result, both clients end up with the non-verified profile.
+  // The profiles should de-duplicate via sync on both other client, the
+  // verified one should win.
   EXPECT_EQ(1U, GetAllAutoFillProfiles(0).size());
   EXPECT_EQ(1U, GetAllAutoFillProfiles(0).size());
 
-  EXPECT_NE(verified_origin, GetAllAutoFillProfiles(0)[0]->origin());
-  EXPECT_NE(verified_origin, GetAllAutoFillProfiles(1)[0]->origin());
+  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);

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,
-                       ServerAddressConvertsToSameLocalAddress) {
+                       DISABLED_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 autofill_profile_sync_difference_tracker*=jkrcal@chromium.org
-per-file autofill_profile_sync_difference_tracker*=file://components/sync/OWNERS
+per-file *syncable_service*=jkrcal@chromium.org
+per-file *syncable_service*=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,18 +251,13 @@ base::Optional<syncer::ModelError> AutofillProfileSyncBridge::FlushSyncTracker(
                      base::Unretained(web_data_backend_))));
 
   std::vector<std::unique_ptr<AutofillProfile>> 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));
+  RETURN_IF_ERROR(tracker->FlushToSync(&profiles_to_upload_to_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,11 +64,6 @@ 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;
@@ -76,76 +71,30 @@ AutofillProfileSyncDifferenceTracker::IncorporateRemoteProfile(
     // Look for exact duplicates, compare only profile contents (and
     // ignore origin and language code in comparison).
     if (local.Compare(*remote) == 0) {
-      // A duplicate found: keep the version with the bigger storage key.
+      // We found a duplicate, we keep the new (remote) one and delete the
+      // local one.
       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 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);
+          << " 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));
       }
-      return base::nullopt;
+      // Delete the local profile that gets replaced by |remote|.
+      DeleteFromLocal(local_storage_key);
+      break;
     }
   }
 
-  // If no duplicate was found, just add the remote profile.
   add_to_local_.push_back(std::move(remote));
   return base::nullopt;
 }
@@ -183,14 +132,10 @@ Optional<ModelError> AutofillProfileSyncDifferenceTracker::FlushToLocal(
 }
 
 Optional<ModelError> AutofillProfileSyncDifferenceTracker::FlushToSync(
-    std::vector<std::unique_ptr<AutofillProfile>>* profiles_to_upload_to_sync,
-    std::vector<std::string>* profiles_to_delete_from_sync) {
+    std::vector<std::unique_ptr<AutofillProfile>>* profiles_to_upload_to_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;
 }
 
@@ -255,11 +200,9 @@ AutofillProfileInitialSyncDifferenceTracker::IncorporateRemoteDelete(
 }
 
 Optional<ModelError> AutofillProfileInitialSyncDifferenceTracker::FlushToSync(
-    std::vector<std::unique_ptr<AutofillProfile>>* profiles_to_upload_to_sync,
-    std::vector<std::string>* profiles_to_delete_from_sync) {
+    std::vector<std::unique_ptr<AutofillProfile>>* profiles_to_upload_to_sync) {
   // First, flush standard updates to sync.
-  AutofillProfileSyncDifferenceTracker::FlushToSync(
-      profiles_to_upload_to_sync, profiles_to_delete_from_sync);
+  AutofillProfileSyncDifferenceTracker::FlushToSync(profiles_to_upload_to_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,12 +50,11 @@ class AutofillProfileSyncDifferenceTracker {
       base::OnceClosure autofill_changes_callback);
 
   // Writes into |profiles_to_upload_to_sync| all autofill profiles to be sent
-  // 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.
+  // to the sync server. After flushing, not 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::string>* profiles_to_delete_from_sync);
+      std::vector<std::unique_ptr<AutofillProfile>>*
+          profiles_to_upload_to_sync);
 
  protected:
   // If the entry is found, |entry| will be return, otherwise base::nullopt is
@@ -90,7 +89,7 @@ class AutofillProfileSyncDifferenceTracker {
 
   // We use unique_ptrs for storing AutofillProfile to avoid unnecessary copies.
 
-  // Local data, mapped by storage key. Use GetLocalOnlyEntries() to access it.
+  // Local data, mapped by storage key. Use unique_to_local() 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
@@ -99,13 +98,9 @@ class AutofillProfileSyncDifferenceTracker {
   std::vector<std::unique_ptr<AutofillProfile>> add_to_local_;
   std::vector<std::unique_ptr<AutofillProfile>> update_to_local_;
 
-  // Contains data for entries that existed on both sync and local sides
+  // Contains merged 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);
@@ -121,8 +116,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,
-      std::vector<std::string>* profiles_to_delete_from_sync) override;
+      std::vector<std::unique_ptr<AutofillProfile>>* profiles_to_upload_to_sync)
+      override;
 
   // Performs an additional pass through remote entries incorporated from sync
   // to find any similarities with local entries. Should be run after all