Revert "Consistently clear server credit cards with USS"

This reverts commit 3efff7c9.

Reason for revert:

Findit (https://goo.gl/kROfz5) identified CL at revision 595787 as the
culprit for flakes in the build cycles as shown on:
https://findit-for-me.appspot.com/waterfall/flake/flake-culprit?key=ag9zfmZpbmRpdC1mb3ItbWVyQwsSDEZsYWtlQ3VscHJpdCIxY2hyb21pdW0vM2VmZmY3YzlkOGMxZTUzYTg5NGE5YmY1MTY2MTVhNWM3ZjliMjU2Ygw

Sample Failed Build: https://ci.chromium.org/buildbot/tryserver.chromium.win/win10_chromium_x64_rel_ng/106570

Sample Failed Step: sync_integration_tests (with patch) on Windows-10-15063

Sample Flaky Test: USS/SingleClientWalletSyncTest.ClearOnStopSync/1

Original change's description:
> Consistently clear server credit cards with USS
> 
> AutofillWalletDataTypeController is used (pre-USS) for both AUTOFILL_WALLET_DATA
> and AUTOFILL_WALLET_METADATA. When Sync is stopped, it calls
> PersonalDataManager::ClearAllServerData. Since there are two instances of the
> controller, that happens twice (which is unnecessary but doesn't hurt). However,
> when AUTOFILL_WALLET_DATA is on USS, then the remaining ClearAllServerData call
> (for the metadata controller) still happens, which brings Sync into a bad state
> since the data is now gone, but the progress marker is still there (see bug 885211).
> 
> This CL changes AutofillWalletDataTypeController to only call ClearAllServerData
> for AUTOFILL_WALLET_DATA (i.e. it won't be called anymore if Wallet is on USS).
> Instead, AUTOFILL_WALLET_DATA gets special-cased in ModelAssociationManager to
> clear even in the STOP_SYNC case. This achieves the same "clear data on stopping
> sync, but *not* on browser shutdown", but without requiring a special side channel
> to the SyncService.
> 
> One consequence is that with AUTOFILL_WALLET_DATA on USS, the clearing now happens
> asynchronously (because it goes through the DB), which requires some test changes.
> 
> Bug: 889941
> Change-Id: I54c36050c81fa862acfc0052f355ff1645a8b292
> Reviewed-on: https://chromium-review.googlesource.com/1251041
> Commit-Queue: Marc Treib <treib@chromium.org>
> Reviewed-by: Sebastien Seguin-Gagnon <sebsg@chromium.org>
> Reviewed-by: Jan Krcal <jkrcal@chromium.org>
> Reviewed-by: Florian Uunk <feuunk@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#595787}

Change-Id: I429e5ef10c8d5b858f36ce69cc2b41b30e318fd4
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 889941, 891254
Reviewed-on: https://chromium-review.googlesource.com/1255573
Cr-Commit-Position: refs/heads/master@{#595831}

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

@@ -493,36 +493,6 @@ IN_PROC_BROWSER_TEST_P(SingleClientWalletSyncTest, ClearOnDisableSync) {
       {CreateDefaultSyncWalletAddress(), CreateDefaultSyncWalletCard()});
   ASSERT_TRUE(SetupSync());
 
-  // Make sure the card is in the DB.
-  autofill::PersonalDataManager* pdm = GetPersonalDataManager(0);
-  ASSERT_NE(nullptr, pdm);
-  ASSERT_EQ(1uL, pdm->GetCreditCards().size());
-
-  // Turn off sync, the card should be gone.
-  GetSyncService(0)->RequestStop(syncer::SyncService::CLEAR_DATA);
-  WaitForOnPersonalDataChanged(/*should_trigger_refresh=*/false, pdm);
-
-  ASSERT_EQ(0uL, pdm->GetCreditCards().size());
-
-  // Turn sync on again, the card should come back.
-  GetSyncService(0)->RequestStart();
-  // RequestStop(CLEAR_DATA) also clears the "first setup complete" flag, so
-  // set it again.
-  GetSyncService(0)->SetFirstSetupComplete();
-  // Wait until Sync restores the card and it arrives at PDM.
-  WaitForOnPersonalDataChanged(/*should_trigger_refresh=*/false, pdm);
-
-  ASSERT_EQ(1uL, pdm->GetCreditCards().size());
-}
-
-// Wallet data should get cleared from the database when sync is (temporarily)
-// stopped, e.g. due to the Sync feature toggle in Android settings.
-IN_PROC_BROWSER_TEST_P(SingleClientWalletSyncTest, ClearOnStopSync) {
-  InitWithDefaultFeatures();
-  GetFakeServer()->SetWalletData(
-      {CreateDefaultSyncWalletAddress(), CreateDefaultSyncWalletCard()});
-  ASSERT_TRUE(SetupSync());
-
   // Make sure the card is in the DB.
   autofill::PersonalDataManager* pdm = GetPersonalDataManager(0);
   ASSERT_NE(nullptr, pdm);
@@ -530,17 +500,9 @@ IN_PROC_BROWSER_TEST_P(SingleClientWalletSyncTest, ClearOnStopSync) {
   ASSERT_EQ(1uL, cards.size());
 
   // Turn off sync, the card should be gone.
-  GetSyncService(0)->RequestStop(syncer::SyncService::KEEP_DATA);
-  WaitForOnPersonalDataChanged(/*should_trigger_refresh=*/false, pdm);
-
-  ASSERT_EQ(0uL, pdm->GetCreditCards().size());
-
-  // Turn sync on again, the card should come back.
-  GetSyncService(0)->RequestStart();
-  // Wait until Sync restores the card and it arrives at PDM.
-  WaitForOnPersonalDataChanged(/*should_trigger_refresh=*/false, pdm);
-
-  ASSERT_EQ(1uL, pdm->GetCreditCards().size());
+  ASSERT_TRUE(GetClient(0)->DisableSyncForAllDatatypes());
+  cards = pdm->GetCreditCards();
+  ASSERT_EQ(0uL, cards.size());
 }
 
 // ChromeOS does not sign out, so the test below does not apply.
@@ -559,7 +521,6 @@ IN_PROC_BROWSER_TEST_P(SingleClientWalletSyncTest, ClearOnSignOut) {
 
   // Turn off sync, the card should be gone.
   GetClient(0)->SignOutPrimaryAccount();
-  WaitForOnPersonalDataChanged(/*should_trigger_refresh=*/false, pdm);
 
   ASSERT_EQ(0uL, pdm->GetCreditCards().size());
 }
@@ -675,8 +636,6 @@ IN_PROC_BROWSER_TEST_P(SingleClientWalletSyncTest, ClearOnDisableWalletSync) {
 
   // Turn off autofill sync, the card should be gone.
   ASSERT_TRUE(GetClient(0)->DisableSyncForDatatype(syncer::AUTOFILL));
-  WaitForOnPersonalDataChanged(/*should_trigger_refresh=*/false, pdm);
-
   cards = pdm->GetCreditCards();
   ASSERT_EQ(0uL, cards.size());
 }

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

@@ -10,7 +10,6 @@
 #include "chrome/browser/sync/test/integration/sessions_helper.h"
 #include "chrome/browser/sync/test/integration/sync_test.h"
 #include "chrome/common/webui_url_constants.h"
-#include "components/autofill/core/common/autofill_prefs.h"
 #include "components/browser_sync/profile_sync_service.h"
 #include "components/sync/base/sync_prefs.h"
 #include "components/sync/engine/polling_constants.h"
@@ -78,39 +77,18 @@ IN_PROC_BROWSER_TEST_F(TwoClientPollingSyncTest, ShouldPollOnStartup) {
 
   ASSERT_TRUE(SetupSync()) << "SetupSync() failed.";
 
-  // Disable syncing of AUTOFILL_WALLET_DATA: That type is special-cased to
-  // clear its data even with KEEP_DATA, which means we'd always send a regular
-  // GetUpdates request on starting Sync again, and so we'd have no need for a
-  // poll.
-  GetClient(0)->DisableSyncForDatatype(syncer::AUTOFILL);
-  GetClient(1)->DisableSyncForDatatype(syncer::AUTOFILL);
-  // TODO(crbug.com/890737): Once AUTOFILL_WALLET_DATA gets properly disabled
-  // based on the pref, we can just disable that instead of all of AUTOFILL:
-  // autofill::prefs::SetPaymentsIntegrationEnabled(GetProfile(0)->GetPrefs(),
-  //                                                false);
-  // autofill::prefs::SetPaymentsIntegrationEnabled(GetProfile(1)->GetPrefs(),
-  //                                                false);
-
   // Phase 1.
   ASSERT_TRUE(CheckInitialState(0));
   ASSERT_TRUE(CheckInitialState(1));
   ASSERT_TRUE(OpenTab(0, GURL(chrome::kChromeUIHistoryURL)));
   GetClient(0)->AwaitMutualSyncCycleCompletion(GetClient(1));
 
-  ASSERT_FALSE(GetSyncService(0)->GetActiveDataTypes().Has(
-      syncer::AUTOFILL_WALLET_DATA));
-  ASSERT_FALSE(GetSyncService(1)->GetActiveDataTypes().Has(
-      syncer::AUTOFILL_WALLET_DATA));
-
   // Phase 2.
   // Disconnect client 1 from sync and write another change from client 0.
   // Disconnnect the remote client from the invalidation service.
   DisableNotificationsForClient(1);
   // Make sure no extra sync cycles get triggered by test infrastructure.
   StopConfigurationRefresher();
-  // Note: It's important to specify KEEP_DATA here - if we CLEAR_DATA, then
-  // we'll do a regular GetUpdates at the next startup, so there'd be no need
-  // for a poll.
   GetClient(1)->StopSyncService(syncer::SyncService::KEEP_DATA);
 
   ASSERT_TRUE(OpenTab(0, GURL(kURL1)));

components/autofill/core/browser/autofill_wallet_data_type_controller.cc

@@ -71,29 +71,23 @@ bool AutofillWalletDataTypeController::StartModels() {
 void AutofillWalletDataTypeController::StopModels() {
   DCHECK(CalledOnValidThread());
 
-  // This controller is used by two data types, we need to clear the data only
-  // once. (In particular, if AUTOFILL_WALLET_DATA is on USS (and thus doesn't
-  // use this controller), we *don't* want any ClearAllServerData call).
-  if (type() == syncer::AUTOFILL_WALLET_DATA) {
-    // This function is called when shutting down (nothing is changing), when
-    // sync is disabled completely, or when wallet sync is disabled. In the
-    // cases where wallet sync or sync in general is disabled, clear wallet
-    // cards and addresses copied from the server. This is different than other
-    // sync cases since this type of data reflects what's on the server rather
-    // than syncing local data between clients, so this extra step is required.
-    syncer::SyncService* service = sync_client_->GetSyncService();
-
-    // CanSyncFeatureStart indicates if sync is currently enabled at all. The
-    // preferred data type indicates if wallet sync data is enabled, and
-    // currently_enabled_ indicates if the other prefs are enabled. All of these
-    // have to be enabled to sync wallet data.
-    if (!service->CanSyncFeatureStart() ||
-        !service->GetPreferredDataTypes().Has(type()) || !currently_enabled_) {
-      autofill::PersonalDataManager* pdm =
-          sync_client_->GetPersonalDataManager();
-      if (pdm)
-        pdm->ClearAllServerData();
-    }
+  // This function is called when shutting down (nothing is changing), when
+  // sync is disabled completely, or when wallet sync is disabled. In the
+  // cases where wallet sync or sync in general is disabled, clear wallet cards
+  // and addresses copied from the server. This is different than other sync
+  // cases since this type of data reflects what's on the server rather than
+  // syncing local data between clients, so this extra step is required.
+  syncer::SyncService* service = sync_client_->GetSyncService();
+
+  // CanSyncFeatureStart indicates if sync is currently enabled at all. The
+  // preferred data type indicates if wallet sync data/metadata is enabled, and
+  // currently_enabled_ indicates if the other prefs are enabled. All of these
+  // have to be enabled to sync wallet data/metadata.
+  if (!service->CanSyncFeatureStart() ||
+      !service->GetPreferredDataTypes().Has(type()) || !currently_enabled_) {
+    autofill::PersonalDataManager* pdm = sync_client_->GetPersonalDataManager();
+    if (pdm)
+      pdm->ClearAllServerData();
   }
 }
 

components/sync/BUILD.gn

@@ -215,7 +215,6 @@ jumbo_static_library("sync") {
     "engine/polling_constants.h",
     "engine/sequenced_model_worker.cc",
     "engine/sequenced_model_worker.h",
-    "engine/shutdown_reason.cc",
     "engine/shutdown_reason.h",
     "engine/sync_auth_provider.h",
     "engine/sync_backend_registrar.cc",

components/sync/driver/data_type_manager_impl.cc

@@ -303,7 +303,7 @@ void DataTypeManagerImpl::Restart() {
   // If we're performing a "catch up", first stop the model types to ensure the
   // call to Initialize triggers model association.
   if (catch_up_in_progress_)
-    model_association_manager_.Stop(STOP_SYNC);
+    model_association_manager_.Stop(KEEP_METADATA);
   download_started_ = false;
   model_association_manager_.Initialize(
       /*desired_types=*/last_enabled_types_,
@@ -794,9 +794,21 @@ void DataTypeManagerImpl::StopImpl(ShutdownReason reason) {
   // Invalidate weak pointer to drop download callbacks.
   weak_ptr_factory_.InvalidateWeakPtrs();
 
+  // Leave metadata If we do not disable sync completely.
+  SyncStopMetadataFate metadata_fate = KEEP_METADATA;
+  switch (reason) {
+    case STOP_SYNC:
+      break;
+    case DISABLE_SYNC:
+      metadata_fate = CLEAR_METADATA;
+      break;
+    case BROWSER_SHUTDOWN:
+      break;
+  }
+
   // Stop all data types. This may trigger association callback but the
   // callback will do nothing because state is set to STOPPING above.
-  model_association_manager_.Stop(reason);
+  model_association_manager_.Stop(metadata_fate);
 
   // Individual data type controllers might still be STOPPING, but we don't
   // reflect that in |state_| because, for all practical matters, the manager is

components/sync/driver/model_association_manager.cc

@@ -16,7 +16,6 @@
 #include "base/metrics/histogram_macros.h"
 #include "base/trace_event/trace_event.h"
 #include "components/sync/base/model_type.h"
-#include "components/sync/base/sync_stop_metadata_fate.h"
 #include "components/sync/model/sync_merge_result.h"
 
 namespace syncer {
@@ -127,7 +126,7 @@ void ModelAssociationManager::Initialize(ModelTypeSet desired_types,
   notified_about_ready_for_configure_ = false;
 
   DVLOG(1) << "ModelAssociationManager: Stopping disabled types.";
-  std::map<DataTypeController*, ShutdownReason> types_to_stop;
+  std::map<DataTypeController*, SyncStopMetadataFate> types_to_stop;
   for (const auto& type_and_dtc : *controllers_) {
     DataTypeController* dtc = type_and_dtc.second.get();
     // We stop a datatype if it's not desired. Independently of being desired,
@@ -136,9 +135,9 @@ void ModelAssociationManager::Initialize(ModelTypeSet desired_types,
     if ((dtc->state() != DataTypeController::NOT_RUNNING &&
          !desired_types_.Has(dtc->type())) ||
         dtc->state() == DataTypeController::STOPPING) {
-      const ShutdownReason reason =
-          preferred_types.Has(dtc->type()) ? STOP_SYNC : DISABLE_SYNC;
-      types_to_stop[dtc] = reason;
+      const SyncStopMetadataFate metadata_fate =
+          preferred_types.Has(dtc->type()) ? KEEP_METADATA : CLEAR_METADATA;
+      types_to_stop[dtc] = metadata_fate;
     }
   }
 
@@ -150,18 +149,18 @@ void ModelAssociationManager::Initialize(ModelTypeSet desired_types,
       base::BindOnce(&ModelAssociationManager::LoadEnabledTypes,
                      weak_ptr_factory_.GetWeakPtr()));
 
-  for (const auto& dtc_and_reason : types_to_stop) {
-    DataTypeController* dtc = dtc_and_reason.first;
-    const ShutdownReason reason = dtc_and_reason.second;
-    DVLOG(1) << "ModelAssociationManager: stop " << dtc->name() << " due to "
-             << ShutdownReasonToString(reason);
-    StopDatatype(SyncError(), reason, dtc, barrier_closure);
+  for (const auto& dtc_and_metadata_fate : types_to_stop) {
+    DataTypeController* dtc = dtc_and_metadata_fate.first;
+    const SyncStopMetadataFate metadata_fate = dtc_and_metadata_fate.second;
+    DVLOG(1) << "ModelAssociationManager: stop " << dtc->name() << " with "
+             << SyncStopMetadataFateToString(metadata_fate);
+    StopDatatype(SyncError(), metadata_fate, dtc, barrier_closure);
   }
 }
 
 void ModelAssociationManager::StopDatatype(
     const SyncError& error,
-    ShutdownReason shutdown_reason,
+    SyncStopMetadataFate metadata_fate,
     DataTypeController* dtc,
     DataTypeController::StopCallback callback) {
   loaded_types_.Remove(dtc->type());
@@ -171,25 +170,6 @@ void ModelAssociationManager::StopDatatype(
   DCHECK(error.IsSet() || (dtc->state() != DataTypeController::NOT_RUNNING));
 
   delegate_->OnSingleDataTypeWillStop(dtc->type(), error);
-
-  // Leave metadata if we do not disable sync completely.
-  SyncStopMetadataFate metadata_fate = KEEP_METADATA;
-  switch (shutdown_reason) {
-    case STOP_SYNC:
-      // Special case: For AUTOFILL_WALLET_DATA, we want to clear all data even
-      // when Sync is stopped temporarily.
-      // TODO(crbug.com/890361): Consider moving this decision into the
-      // individual controller
-      if (dtc->type() == AUTOFILL_WALLET_DATA) {
-        metadata_fate = CLEAR_METADATA;
-      }
-      break;
-    case DISABLE_SYNC:
-      metadata_fate = CLEAR_METADATA;
-      break;
-    case BROWSER_SHUTDOWN:
-      break;
-  }
   dtc->Stop(metadata_fate, std::move(callback));
 }
 
@@ -274,7 +254,7 @@ void ModelAssociationManager::StartAssociationAsync(
   }
 }
 
-void ModelAssociationManager::Stop(ShutdownReason shutdown_reason) {
+void ModelAssociationManager::Stop(SyncStopMetadataFate metadata_fate) {
   // Ignore callbacks from controllers.
   weak_ptr_factory_.InvalidateWeakPtrs();
 
@@ -285,7 +265,7 @@ void ModelAssociationManager::Stop(ShutdownReason shutdown_reason) {
         dtc->state() != DataTypeController::STOPPING) {
       // We don't really wait until all datatypes have been fully stopped, which
       // is only required (and in fact waited for) when Initialize() is called.
-      StopDatatype(SyncError(), shutdown_reason, dtc, base::DoNothing());
+      StopDatatype(SyncError(), metadata_fate, dtc, base::DoNothing());
       DVLOG(1) << "ModelAssociationManager: Stopped " << dtc->name();
     }
   }
@@ -355,7 +335,8 @@ void ModelAssociationManager::TypeStartCallback(
     DVLOG(1) << "ModelAssociationManager: Type encountered an error.";
     desired_types_.Remove(type);
     DataTypeController* dtc = controllers_->find(type)->second.get();
-    StopDatatype(local_merge_result.error(), STOP_SYNC, dtc, base::DoNothing());
+    StopDatatype(local_merge_result.error(), KEEP_METADATA, dtc,
+                 base::DoNothing());
     NotifyDelegateIfReadyForConfigure();
 
     // Update configuration result.
@@ -429,7 +410,7 @@ void ModelAssociationManager::ModelAssociationDone(State new_state) {
                                 static_cast<int>(MODEL_TYPE_COUNT));
       StopDatatype(SyncError(FROM_HERE, SyncError::DATATYPE_ERROR,
                              "Association timed out.", dtc->type()),
-                   STOP_SYNC, dtc, base::DoNothing());
+                   KEEP_METADATA, dtc, base::DoNothing());
     }
   }
 

components/sync/driver/model_association_manager.h

@@ -11,12 +11,12 @@
 #include "base/memory/weak_ptr.h"
 #include "base/timer/timer.h"
 
+#include "components/sync/base/sync_stop_metadata_fate.h"
 #include "components/sync/base/weak_handle.h"
 #include "components/sync/driver/configure_context.h"
 #include "components/sync/driver/data_type_controller.h"
 #include "components/sync/driver/data_type_manager.h"
 #include "components/sync/engine/data_type_association_stats.h"
-#include "components/sync/engine/shutdown_reason.h"
 
 namespace syncer {
 
@@ -94,7 +94,9 @@ class ModelAssociationManager {
                   const ConfigureContext& context);
 
   // Can be called at any time. Synchronously stops all datatypes.
-  void Stop(ShutdownReason shutdown_reason);
+  // If |metadata_fate| equals  CLEAR_METADATA controllers should clear sync
+  // metadata.
+  void Stop(SyncStopMetadataFate metadata_fate);
 
   // Should only be called after Initialize to start the actual association.
   // |types_to_associate| should be subset of |desired_types| in Initialize().
@@ -136,7 +138,7 @@ class ModelAssociationManager {
 
   // A helper to stop an individual datatype.
   void StopDatatype(const SyncError& error,
-                    ShutdownReason shutdown_reason,
+                    SyncStopMetadataFate metadata_fate,
                     DataTypeController* dtc,
                     DataTypeController::StopCallback callback);
 

components/sync/driver/model_association_manager_unittest.cc

@@ -120,7 +120,7 @@ TEST_F(SyncModelAssociationManagerTest, StopModelBeforeFinish) {
 
   EXPECT_EQ(GetController(controllers_, BOOKMARKS)->state(),
             DataTypeController::ASSOCIATING);
-  model_association_manager.Stop(STOP_SYNC);
+  model_association_manager.Stop(KEEP_METADATA);
   EXPECT_EQ(GetController(controllers_, BOOKMARKS)->state(),
             DataTypeController::NOT_RUNNING);
   EXPECT_EQ(
@@ -148,7 +148,7 @@ TEST_F(SyncModelAssociationManagerTest, StopAfterFinish) {
             DataTypeController::ASSOCIATING);
   GetController(controllers_, BOOKMARKS)->FinishStart(DataTypeController::OK);
 
-  model_association_manager.Stop(STOP_SYNC);
+  model_association_manager.Stop(KEEP_METADATA);
   EXPECT_EQ(GetController(controllers_, BOOKMARKS)->state(),
             DataTypeController::NOT_RUNNING);
   EXPECT_EQ(
@@ -572,7 +572,7 @@ TEST_F(SyncModelAssociationManagerTest, StopClearMetadata) {
   ASSERT_EQ(GetController(controllers_, BOOKMARKS)->state(),
             DataTypeController::MODEL_LOADED);
 
-  model_association_manager.Stop(DISABLE_SYNC);
+  model_association_manager.Stop(CLEAR_METADATA);
 
   EXPECT_EQ(GetController(controllers_, BOOKMARKS)->state(),
             DataTypeController::NOT_RUNNING);

components/sync/engine/shutdown_reason.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/sync/engine/shutdown_reason.h"
-
-#include "base/logging.h"
-
-namespace syncer {
-
-const char* ShutdownReasonToString(ShutdownReason reason) {
-  switch (reason) {
-    case STOP_SYNC:
-      return "STOP_SYNC";
-    case DISABLE_SYNC:
-      return "DISABLE_SYNC";
-    case BROWSER_SHUTDOWN:
-      return "BROWSER_SHUTDOWN";
-  }
-
-  NOTREACHED();
-  return "";
-}
-
-}  // namespace syncer

components/sync/engine/shutdown_reason.h

@@ -14,8 +14,6 @@ enum ShutdownReason {
   BROWSER_SHUTDOWN,  // Browser is closed.
 };
 
-const char* ShutdownReasonToString(ShutdownReason reason);
-
 }  // namespace syncer
 
 #endif  // COMPONENTS_SYNC_ENGINE_SHUTDOWN_REASON_H_