Reland "[AF Wallet] Ignore remote metadata deletions in USS sync"

This reverts commit a75e19c4.

Reason for reland: fixed the test flakiness.

Original change's description:
> Revert "[AF Wallet] Ignore remote metadata deletions in USS sync"
>
> This reverts commit 948191bb.
>
> Reason for revert: introduces flakiness in integration tests
>
> Original change's description:
> > [AF Wallet] Ignore remote metadata deletions in USS sync
> >
> > This CL changes behavior of the wallet_metadata sync bridge to ignore
> > remote deletions. This is safe as every deletion should be eventually
> > processed locally and for corner cases there is anyway already a GC
> > routine.
> >
> > This is motivated by observed ping-pongs of deletions-creations between
> > clients running Directory code and clients running USS code, leading
> > to considerably increased number of address conversions on USS. This
> > CL breaks these ping-pongs and should get the conversion counts on
> > comparable levels.
> >
> > Bug: 953693
> > Change-Id: I1a30f51d4727a08f84ef33091089d2a7a366b4df
> > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1583896
> > Commit-Queue: Jan Krcal <jkrcal@chromium.org>
> > Reviewed-by: Mikel Astiz <mastiz@chromium.org>
> > Cr-Commit-Position: refs/heads/master@{#654413}
>
> TBR=jkrcal@chromium.org,mastiz@chromium.org
>
> Change-Id: Ia8afa93aa834101d154e1a1a3db3996f686d1786
> No-Presubmit: true
> No-Tree-Checks: true
> No-Try: true
> Bug: 953693
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1583711
> Reviewed-by: Jan Krcal <jkrcal@chromium.org>
> Commit-Queue: Jan Krcal <jkrcal@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#654471}

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

Change-Id: I737e61569db88219f10db68817b03b5467389df4
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 953693
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1584310Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Reviewed-by: Mohamed Amir Yosef <mamir@chromium.org>
Reviewed-by: Jan Krcal <jkrcal@chromium.org>
Commit-Queue: Jan Krcal <jkrcal@chromium.org>
Cr-Commit-Position: refs/heads/master@{#654484}

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

@@ -60,7 +60,8 @@ class TwoClientWalletSyncTest : public UssWalletSwitchToggler, public SyncTest {
   }
   ~TwoClientWalletSyncTest() override {}
 
-  bool TestUsesSelfNotifications() override { return false; }
+  // Needed for AwaitQuiescence().
+  bool TestUsesSelfNotifications() override { return true; }
 
   bool SetupSync() override {
     test_clock_.SetNow(kArbitraryDefaultTime);
@@ -505,6 +506,9 @@ IN_PROC_BROWSER_TEST_P(TwoClientWalletSyncTest,
        CreateSyncWalletAddress(/*name=*/"address-1", /*company=*/"Company-1"),
        CreateDefaultSyncPaymentsCustomerData()});
   ASSERT_TRUE(SetupSync());
+  // Wait until sync settles (for the wallet metadata) before we change the
+  // data again.
+  ASSERT_TRUE(AwaitQuiescence());
 
   // Grab the current address on the first client.
   std::vector<AutofillProfile*> server_addresses = GetServerProfiles(0);
@@ -553,6 +557,9 @@ IN_PROC_BROWSER_TEST_P(TwoClientWalletSyncTest,
        CreateSyncWalletAddress(/*name=*/"address-1", /*company=*/"Company-1"),
        CreateDefaultSyncPaymentsCustomerData()});
   ASSERT_TRUE(SetupSync());
+  // Wait until sync settles (for the wallet metadata) before we change the
+  // data again.
+  ASSERT_TRUE(AwaitQuiescence());
 
   // Grab the current card on the first client.
   std::vector<CreditCard*> credit_cards = GetServerCreditCards(0);

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

@@ -660,9 +660,15 @@ AutofillWalletMetadataSyncBridge::MergeRemoteChanges(
         break;
       }
       case EntityChange::ACTION_DELETE: {
-        cache_.erase(change->storage_key());
-        is_any_local_modified |= RemoveServerMetadata(
-            table, parsed_storage_key.type, parsed_storage_key.metadata_id);
+        // We intentionally ignore remote deletions in order to avoid
+        // delete-create ping pongs (if we delete metadata for address data
+        // entity that still locally exists, PDM will think the server address
+        // has not been converted to a local address yet and will trigger
+        // conversion that in turn triggers creating and committing the metadata
+        // entity again).
+        // This is safe because this client will delete the wallet_metadata
+        // entity locally as soon as the wallet_data entity gets deleted.
+        // Corner cases are handled by DeleteOldOrphanMetadata().
         break;
       }
     }

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

@@ -1034,9 +1034,9 @@ TEST_F(AutofillWalletMetadataSyncBridgeTest,
                                    EqualsSpecifics(remote_card)));
 }
 
-// Test that remote deletions are properly propagated into the local database.
+// Test that remote deletions are ignored.
 TEST_F(AutofillWalletMetadataSyncBridgeTest,
-       RemoteDeletion_ShouldDeleteExistingLocalData) {
+       RemoteDeletion_ShouldNotDeleteExistingLocalData) {
   // Perform initial sync to create sync data & metadata.
   ResetBridge(/*initial_sync_done=*/false);
   WalletMetadataSpecifics profile =
@@ -1053,87 +1053,19 @@ TEST_F(AutofillWalletMetadataSyncBridgeTest,
   ASSERT_THAT(GetLocalSyncMetadataStorageKeys(),
               UnorderedElementsAre(kAddr1StorageKey, kCard1StorageKey));
 
-  // Now delete the profile. Changes should happen in the local database.
+  // Now delete the profile.
+  // We still need to commit the updated progress marker and sync metadata.
   EXPECT_CALL(*backend(), CommitChanges());
-  EXPECT_CALL(*backend(), NotifyOfMultipleAutofillChanges());
-  ReceiveTombstones({profile, card});
-
-  // Verify that both data and sync metadata is gone.
-  EXPECT_FALSE(real_processor()->IsTrackingEntityForTest(kAddr1StorageKey));
-  EXPECT_FALSE(real_processor()->IsTrackingEntityForTest(kCard1StorageKey));
-  EXPECT_THAT(GetLocalSyncMetadataStorageKeys(), IsEmpty());
-  EXPECT_THAT(GetAllLocalDataInclRestart(), IsEmpty());
-}
-
-// Test that remote deletions are properly handled even when the local data is
-// already deleted. We should still delete sync metadata both from the DB and
-// from the processor internal memory.
-TEST_F(AutofillWalletMetadataSyncBridgeTest,
-       RemoteDeletion_ShouldNotNotifyChangesWhenLocalDataIsGone) {
-  // Perform initial sync to create sync data & metadata.
-  ResetBridge(/*initial_sync_done=*/false);
-  WalletMetadataSpecifics profile =
-      CreateWalletMetadataSpecificsForAddressWithDetails(
-          kAddr1SpecificsId, /*use_count=*/10, /*use_date=*/20);
-  WalletMetadataSpecifics card =
-      CreateWalletMetadataSpecificsForCardWithDetails(
-          kCard1SpecificsId, /*use_count=*/30, /*use_date=*/40);
-  StartSyncing({profile, card});
-
-  // Clear the data from the local DB, sync metadata stays untouched both in the
-  // processor and in the local DB.
-  table()->SetServerProfiles({});
-  table()->SetServerCreditCards({});
-  ASSERT_TRUE(real_processor()->IsTrackingEntityForTest(kAddr1StorageKey));
-  ASSERT_TRUE(real_processor()->IsTrackingEntityForTest(kCard1StorageKey));
-  ASSERT_THAT(GetLocalSyncMetadataStorageKeys(),
-              UnorderedElementsAre(kAddr1StorageKey, kCard1StorageKey));
-
-  // Send deletions from the server. We should commit to write the new progress
-  // marker down.
-  EXPECT_CALL(*backend(), CommitChanges());
-  // Since the data is already deleted, it should not notify about changes. The
-  // entities should however get deleted from the processor.
+  // Changes should _not_ happen in the local autofill database.
   EXPECT_CALL(*backend(), NotifyOfMultipleAutofillChanges()).Times(0);
   ReceiveTombstones({profile, card});
 
-  // Verify that sync metadata is gone from both the processor and local DB.
+  // Verify that even though the processor does not track these entities any
+  // more and the sync metadata is gone, the actual data entities still exist in
+  // the local DB.
   EXPECT_FALSE(real_processor()->IsTrackingEntityForTest(kAddr1StorageKey));
   EXPECT_FALSE(real_processor()->IsTrackingEntityForTest(kCard1StorageKey));
   EXPECT_THAT(GetLocalSyncMetadataStorageKeys(), IsEmpty());
-  EXPECT_THAT(GetAllLocalDataInclRestart(), IsEmpty());
-}
-
-// Test that remote deletions are ignored if there is no sync metadata for the
-// entity.
-TEST_F(AutofillWalletMetadataSyncBridgeTest,
-       RemoteDeletion_ShouldNotDeleteLocalDataWithoutSyncMetadata) {
-  WalletMetadataSpecifics profile =
-      CreateWalletMetadataSpecificsForAddressWithDetails(
-          kAddr1SpecificsId, /*use_count=*/10, /*use_date=*/20);
-  WalletMetadataSpecifics card =
-      CreateWalletMetadataSpecificsForCardWithDetails(
-          kCard1SpecificsId, /*use_count=*/30, /*use_date=*/40);
-
-  table()->SetServerProfiles({CreateServerProfileFromSpecifics(profile)});
-  table()->SetServerCreditCards({CreateServerCreditCardFromSpecifics(card)});
-
-  // Do not perform initial sync. This way, no sync metadata gets created, the
-  // processor does not know about the entities and thus, it should not delete
-  // them.
-  ResetBridge(/*initial_sync_done=*/true);
-  StartSyncing({});
-  ASSERT_FALSE(real_processor()->IsTrackingEntityForTest(kAddr1StorageKey));
-  ASSERT_FALSE(real_processor()->IsTrackingEntityForTest(kCard1StorageKey));
-  ASSERT_THAT(GetLocalSyncMetadataStorageKeys(), IsEmpty());
-
-  // Send deletions from the server. We should commit to write the new progress
-  // marker down.
-  EXPECT_CALL(*backend(), CommitChanges());
-  // The actual deletion should get ignored.
-  EXPECT_CALL(*backend(), NotifyOfMultipleAutofillChanges()).Times(0);
-  ReceiveTombstones({profile, card});
-
   EXPECT_THAT(
       GetAllLocalDataInclRestart(),
       UnorderedElementsAre(EqualsSpecifics(profile), EqualsSpecifics(card)));