Notify USS types about sync being stopped

We currently only stop sync (without disabling) in tests only, but we
expect this to be more common in the future (DICe, payments, Butter).
Instead of letting bridges do ugly workarounds, let propagate the
transition to the processor, and allow bridges to (if interested) do
something about it.

Bug: 681921
Change-Id: I9689346e71d7eeb0c7b13eb601d88722e51f5ae4
Reviewed-on: https://chromium-review.googlesource.com/1098915
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Reviewed-by: vitaliii <vitaliii@chromium.org>
Reviewed-by: Christian Dullweber <dullweber@chromium.org>
Reviewed-by: Jan Krcal <jkrcal@chromium.org>
Cr-Commit-Position: refs/heads/master@{#568375}

components/consent_auditor/consent_sync_bridge_impl.cc

@@ -135,35 +135,38 @@ void ConsentSyncBridgeImpl::OnSyncStarting() {
 #if !defined(OS_IOS)  // https://crbug.com/834042
   DCHECK(!authenticated_account_id_callback_.Run().empty());
 #endif  // !defined(OS_IOS)
-  bool was_sync_started = is_sync_starting_or_started_;
+  DCHECK(!is_sync_starting_or_started_);
+
   is_sync_starting_or_started_ = true;
-  if (store_ && change_processor()->IsTrackingMetadata() && !was_sync_started) {
+  if (store_ && change_processor()->IsTrackingMetadata()) {
     ReadAllDataAndResubmit();
   }
 }
 
-ModelTypeSyncBridge::DisableSyncResponse
-ConsentSyncBridgeImpl::ApplyDisableSyncChanges(
+ModelTypeSyncBridge::StopSyncResponse
+ConsentSyncBridgeImpl::ApplyStopSyncChanges(
     std::unique_ptr<MetadataChangeList> delete_metadata_change_list) {
-  // Sync can only be disabled after initialization.
+  // Sync can only be stopped after initialization.
   DCHECK(deferred_consents_while_initializing_.empty());
 
   is_sync_starting_or_started_ = false;
 
-  // Preserve all consents in the store, but delete their metadata, because it
-  // may become invalid when the sync is reenabled. It is important to report
-  // all user consents, thus, they are persisted for some time even after
-  // signout. We will try to resubmit these consents once the sync is enabled
-  // again. This may lead to same consent being submitted multiple times, but
-  // this is allowed.
-  std::unique_ptr<WriteBatch> batch = store_->CreateWriteBatch();
-  batch->TakeMetadataChangesFrom(std::move(delete_metadata_change_list));
-
-  store_->CommitWriteBatch(
-      std::move(batch),
-      base::BindOnce(&ConsentSyncBridgeImpl::OnCommit, base::AsWeakPtr(this)));
+  if (delete_metadata_change_list) {
+    // Preserve all consents in the store, but delete their metadata, because it
+    // may become invalid when the sync is reenabled. It is important to report
+    // all user consents, thus, they are persisted for some time even after
+    // signout. We will try to resubmit these consents once the sync is enabled
+    // again. This may lead to same consent being submitted multiple times, but
+    // this is allowed.
+    std::unique_ptr<WriteBatch> batch = store_->CreateWriteBatch();
+    batch->TakeMetadataChangesFrom(std::move(delete_metadata_change_list));
+
+    store_->CommitWriteBatch(std::move(batch),
+                             base::BindOnce(&ConsentSyncBridgeImpl::OnCommit,
+                                            base::AsWeakPtr(this)));
+  }
 
-  return DisableSyncResponse::kModelStillReadyToSync;
+  return StopSyncResponse::kModelStillReadyToSync;
 }
 
 void ConsentSyncBridgeImpl::ReadAllDataAndResubmit() {

components/consent_auditor/consent_sync_bridge_impl.h

@@ -43,7 +43,7 @@ class ConsentSyncBridgeImpl : public ConsentSyncBridge,
   void GetAllDataForDebugging(DataCallback callback) override;
   std::string GetClientTag(const EntityData& entity_data) override;
   std::string GetStorageKey(const EntityData& entity_data) override;
-  DisableSyncResponse ApplyDisableSyncChanges(
+  StopSyncResponse ApplyStopSyncChanges(
       std::unique_ptr<MetadataChangeList> delete_metadata_change_list) override;
 
   // ConsentSyncBridge implementation.

components/consent_auditor/consent_sync_bridge_impl_unittest.cc

@@ -185,8 +185,8 @@ TEST_F(ConsentSyncBridgeImplTest, ShouldNotDeleteConsentsWhenSyncIsDisabled) {
   ASSERT_THAT(GetAllData(), SizeIs(1));
 
   EXPECT_THAT(
-      bridge()->ApplyDisableSyncChanges(WriteBatch::CreateMetadataChangeList()),
-      Eq(ModelTypeSyncBridge::DisableSyncResponse::kModelStillReadyToSync));
+      bridge()->ApplyStopSyncChanges(WriteBatch::CreateMetadataChangeList()),
+      Eq(ModelTypeSyncBridge::StopSyncResponse::kModelStillReadyToSync));
   // The bridge may asynchronously query the store to choose what to delete.
   base::RunLoop().RunUntilIdle();
 
@@ -315,8 +315,8 @@ TEST_F(ConsentSyncBridgeImplTest,
   // User disables sync, hovewer, the consent hasn't been submitted yet. It is
   // preserved to be submitted when sync is re-enabled.
   EXPECT_THAT(
-      bridge()->ApplyDisableSyncChanges(WriteBatch::CreateMetadataChangeList()),
-      Eq(ModelTypeSyncBridge::DisableSyncResponse::kModelStillReadyToSync));
+      bridge()->ApplyStopSyncChanges(WriteBatch::CreateMetadataChangeList()),
+      Eq(ModelTypeSyncBridge::StopSyncResponse::kModelStillReadyToSync));
   // The bridge may asynchronously query the store to choose what to delete.
   base::RunLoop().RunUntilIdle();
 
@@ -465,8 +465,8 @@ TEST_F(ConsentSyncBridgeImplTest,
   ASSERT_THAT(GetAllData(), SizeIs(1));
 
   EXPECT_THAT(
-      bridge()->ApplyDisableSyncChanges(WriteBatch::CreateMetadataChangeList()),
-      Eq(ModelTypeSyncBridge::DisableSyncResponse::kModelStillReadyToSync));
+      bridge()->ApplyStopSyncChanges(WriteBatch::CreateMetadataChangeList()),
+      Eq(ModelTypeSyncBridge::StopSyncResponse::kModelStillReadyToSync));
   // The bridge may asynchronously query the store to choose what to delete.
   base::RunLoop().RunUntilIdle();
 
@@ -484,8 +484,8 @@ TEST_F(ConsentSyncBridgeImplTest,
   base::RunLoop().RunUntilIdle();
 
   EXPECT_THAT(
-      bridge()->ApplyDisableSyncChanges(WriteBatch::CreateMetadataChangeList()),
-      Eq(ModelTypeSyncBridge::DisableSyncResponse::kModelStillReadyToSync));
+      bridge()->ApplyStopSyncChanges(WriteBatch::CreateMetadataChangeList()),
+      Eq(ModelTypeSyncBridge::StopSyncResponse::kModelStillReadyToSync));
   base::RunLoop().RunUntilIdle();
 
   // The previous user signs in again and enables sync.

components/sync/BUILD.gn

@@ -63,6 +63,7 @@ jumbo_static_library("sync") {
     "base/stop_source.h",
     "base/sync_prefs.cc",
     "base/sync_prefs.h",
+    "base/sync_stop_metadata_fate.h",
     "base/syncer_error.cc",
     "base/syncer_error.h",
     "base/system_encryptor.cc",

components/sync/device_info/device_info_sync_bridge.cc

@@ -236,20 +236,22 @@ std::string DeviceInfoSyncBridge::GetStorageKey(const EntityData& entity_data) {
   return entity_data.specifics.device_info().cache_guid();
 }
 
-ModelTypeSyncBridge::DisableSyncResponse
-DeviceInfoSyncBridge::ApplyDisableSyncChanges(
+ModelTypeSyncBridge::StopSyncResponse
+DeviceInfoSyncBridge::ApplyStopSyncChanges(
     std::unique_ptr<MetadataChangeList> delete_metadata_change_list) {
   // TODO(skym, crbug.com/659263): Would it be reasonable to pulse_timer_.Stop()
   // or subscription_.reset() here?
 
   // Remove all local data, if sync is being disabled, the user has expressed
   // their desire to not have knowledge about other devices.
-  store_->DeleteAllDataAndMetadata(base::DoNothing());
-  if (!all_data_.empty()) {
-    all_data_.clear();
-    NotifyObservers();
+  if (delete_metadata_change_list) {
+    store_->DeleteAllDataAndMetadata(base::DoNothing());
+    if (!all_data_.empty()) {
+      all_data_.clear();
+      NotifyObservers();
+    }
   }
-  return DisableSyncResponse::kModelStillReadyToSync;
+  return StopSyncResponse::kModelStillReadyToSync;
 }
 
 bool DeviceInfoSyncBridge::IsSyncing() const {

components/sync/device_info/device_info_sync_bridge.h

@@ -52,7 +52,7 @@ class DeviceInfoSyncBridge : public ModelTypeSyncBridge,
   void GetAllDataForDebugging(DataCallback callback) override;
   std::string GetClientTag(const EntityData& entity_data) override;
   std::string GetStorageKey(const EntityData& entity_data) override;
-  DisableSyncResponse ApplyDisableSyncChanges(
+  StopSyncResponse ApplyStopSyncChanges(
       std::unique_ptr<MetadataChangeList> delete_metadata_change_list) override;
 
   // DeviceInfoTracker implementation.

components/sync/device_info/device_info_sync_bridge_unittest.cc

@@ -787,7 +787,7 @@ TEST_F(DeviceInfoSyncBridgeTest, SendLocalData) {
   EXPECT_EQ(2, change_count());
 }
 
-TEST_F(DeviceInfoSyncBridgeTest, ApplyDisableSyncChanges) {
+TEST_F(DeviceInfoSyncBridgeTest, ApplyStopSyncChanges) {
   InitializeAndPump();
   EXPECT_EQ(1u, bridge()->GetAllDeviceInfo().size());
   EXPECT_EQ(1, change_count());
@@ -801,7 +801,7 @@ TEST_F(DeviceInfoSyncBridgeTest, ApplyDisableSyncChanges) {
   EXPECT_EQ(2, change_count());
 
   // Should clear out all local data and notify observers.
-  bridge()->ApplyDisableSyncChanges({});
+  bridge()->ApplyStopSyncChanges(bridge()->CreateMetadataChangeList());
   EXPECT_EQ(0u, bridge()->GetAllDeviceInfo().size());
   EXPECT_EQ(3, change_count());
 

components/sync/driver/model_type_controller.cc

@@ -54,9 +54,10 @@ void RecordMemoryUsageHistogramHelperOnModelThread(
   delegate->RecordMemoryUsageHistogram();
 }
 
-void DisableSyncHelperOnModelThread(
+void StopSyncHelperOnModelThread(
+    SyncStopMetadataFate metadata_fate,
     base::WeakPtr<ModelTypeControllerDelegate> delegate) {
-  delegate->DisableSync();
+  delegate->OnSyncStopping(metadata_fate);
 }
 
 void ReportError(ModelType model_type,
@@ -238,13 +239,21 @@ void ModelTypeController::Stop(SyncStopMetadataFate metadata_fate) {
     return;
 
   // Check preferences if datatype is not in preferred datatypes. Only call
-  // DisableSync if the delegate is ready to handle it (controller is in loaded
+  // StopSync() if the delegate is ready to handle it (controller is in loaded
   // state).
   ModelTypeSet preferred_types =
       sync_prefs_.GetPreferredDataTypes(ModelTypeSet(type()));
-  if ((state() == MODEL_LOADED || state() == RUNNING) &&
-      (!sync_prefs_.IsFirstSetupComplete() || !preferred_types.Has(type()))) {
-    PostModelTask(FROM_HERE, base::BindOnce(&DisableSyncHelperOnModelThread));
+
+  if (state() == MODEL_LOADED || state() == RUNNING) {
+    // TODO(lixan@yandex-team.ru): Migrate away from preference-based inferring
+    // of |reason| and instead consume |metadata_fate|.
+    if (sync_prefs_.IsFirstSetupComplete() && preferred_types.Has(type())) {
+      PostModelTask(FROM_HERE, base::BindOnce(&StopSyncHelperOnModelThread,
+                                              KEEP_METADATA));
+    } else {
+      PostModelTask(FROM_HERE, base::BindOnce(&StopSyncHelperOnModelThread,
+                                              CLEAR_METADATA));
+    }
   }
 
   state_ = NOT_RUNNING;

components/sync/driver/model_type_controller_unittest.cc

@@ -53,10 +53,10 @@ class TestModelTypeProcessor : public FakeModelTypeControllerDelegate,
                                public FakeModelTypeChangeProcessor,
                                public FakeModelTypeProcessor {
  public:
-  explicit TestModelTypeProcessor(int* disable_sync_call_count)
+  explicit TestModelTypeProcessor(int* cleared_metadata_count)
       : FakeModelTypeControllerDelegate(kTestModelType),
         FakeModelTypeChangeProcessor(GetWeakPtr()),
-        disable_sync_call_count_(disable_sync_call_count),
+        cleared_metadata_count_(cleared_metadata_count),
         weak_factory_(this) {}
 
   // ModelTypeChangeProcessor implementation.
@@ -71,7 +71,11 @@ class TestModelTypeProcessor : public FakeModelTypeControllerDelegate,
             weak_factory_.GetWeakPtr(), base::ThreadTaskRunnerHandle::Get());
     std::move(callback).Run(std::move(activation_response));
   }
-  void DisableSync() override { (*disable_sync_call_count_)++; }
+  void OnSyncStopping(SyncStopMetadataFate metadata_fate) override {
+    if (metadata_fate == CLEAR_METADATA) {
+      (*cleared_metadata_count_)++;
+    }
+  }
 
   // ModelTypeProcessor implementation.
   void ConnectSync(std::unique_ptr<CommitQueue> commit_queue) override {
@@ -88,7 +92,7 @@ class TestModelTypeProcessor : public FakeModelTypeControllerDelegate,
  private:
   bool initial_sync_done_ = false;
   bool is_connected_ = false;
-  int* disable_sync_call_count_;
+  int* cleared_metadata_count_;
   base::WeakPtrFactory<TestModelTypeProcessor> weak_factory_;
   DISALLOW_COPY_AND_ASSIGN(TestModelTypeProcessor);
 };
@@ -254,7 +258,7 @@ class ModelTypeControllerTest : public testing::Test {
   SyncPrefs* sync_prefs() { return sync_prefs_.get(); }
   DataTypeController* controller() { return controller_.get(); }
   int load_models_done_count() { return load_models_done_count_; }
-  int disable_sync_call_count() { return disable_sync_call_count_; }
+  int cleared_metadata_count() { return cleared_metadata_count_; }
   SyncError load_models_last_error() { return load_models_last_error_; }
 
  private:
@@ -276,7 +280,7 @@ class ModelTypeControllerTest : public testing::Test {
 
   std::unique_ptr<ModelTypeChangeProcessor> CreateProcessor() {
     std::unique_ptr<TestModelTypeProcessor> processor =
-        std::make_unique<TestModelTypeProcessor>(&disable_sync_call_count_);
+        std::make_unique<TestModelTypeProcessor>(&cleared_metadata_count_);
     processor_ = processor.get();
     return std::move(processor);
   }
@@ -306,7 +310,7 @@ class ModelTypeControllerTest : public testing::Test {
   }
 
   int load_models_done_count_ = 0;
-  int disable_sync_call_count_ = 0;
+  int cleared_metadata_count_ = 0;
   bool association_callback_called_ = false;
   SyncError load_models_last_error_;
 
@@ -396,7 +400,7 @@ TEST_F(ModelTypeControllerTest, StopWhenDatatypeEnabled) {
   RunAllTasks();
   EXPECT_EQ(DataTypeController::NOT_RUNNING, controller()->state());
   // Ensure that DisableSync is not called.
-  EXPECT_EQ(0, disable_sync_call_count());
+  EXPECT_EQ(0, cleared_metadata_count());
   ExpectProcessorConnected(false);
 }
 
@@ -419,7 +423,7 @@ TEST_F(ModelTypeControllerTest, StopWhenDatatypeDisabled) {
   EXPECT_EQ(DataTypeController::NOT_RUNNING, controller()->state());
   // Ensure that DisableSync is called.
   PumpModelThread();
-  EXPECT_EQ(1, disable_sync_call_count());
+  EXPECT_EQ(1, cleared_metadata_count());
 }
 
 // Test emulates disabling sync by signing out. DisableSync should be called.
@@ -437,7 +441,7 @@ TEST_F(ModelTypeControllerTest, StopWithInitialSyncPrefs) {
   EXPECT_EQ(DataTypeController::NOT_RUNNING, controller()->state());
   // Ensure that DisableSync is called.
   PumpModelThread();
-  EXPECT_EQ(1, disable_sync_call_count());
+  EXPECT_EQ(1, cleared_metadata_count());
 }
 
 // Test emulates disabling sync when datatype is not loaded yet. DisableSync
@@ -453,7 +457,7 @@ TEST_F(ModelTypeControllerTest, StopBeforeLoadModels) {
   controller()->Stop(KEEP_METADATA);
   EXPECT_EQ(DataTypeController::NOT_RUNNING, controller()->state());
   // Ensure that DisableSync is not called.
-  EXPECT_EQ(0, disable_sync_call_count());
+  EXPECT_EQ(0, cleared_metadata_count());
 }
 
 }  // namespace syncer

components/sync/model/fake_model_type_controller_delegate.cc

@@ -24,7 +24,8 @@ void FakeModelTypeControllerDelegate::OnSyncStarting(
   }
 }
 
-void FakeModelTypeControllerDelegate::DisableSync() {}
+void FakeModelTypeControllerDelegate::OnSyncStopping(
+    SyncStopMetadataFate metadata_fate) {}
 
 void FakeModelTypeControllerDelegate::GetAllNodesForDebugging(
     ModelTypeControllerDelegate::AllNodesCallback callback) {

components/sync/model/fake_model_type_controller_delegate.h

@@ -24,7 +24,7 @@ class FakeModelTypeControllerDelegate : public ModelTypeControllerDelegate {
   // ModelTypeControllerDelegate overrides
   void OnSyncStarting(const DataTypeActivationRequest& request,
                       StartCallback callback) override;
-  void DisableSync() override;
+  void OnSyncStopping(SyncStopMetadataFate metadata_fate) override;
   void GetAllNodesForDebugging(AllNodesCallback callback) override;
   void GetStatusCountersForDebugging(StatusCountersCallback callback) override;
   void RecordMemoryUsageHistogram() override;

components/sync/model/model_type_controller_delegate.h

@@ -11,6 +11,7 @@
 #include "base/memory/weak_ptr.h"
 #include "base/values.h"
 #include "components/sync/base/model_type.h"
+#include "components/sync/base/sync_stop_metadata_fate.h"
 #include "components/sync/engine/cycle/status_counters.h"
 #include "components/sync/engine/data_type_activation_response.h"
 #include "components/sync/model/model_error.h"
@@ -40,9 +41,9 @@ class ModelTypeControllerDelegate {
                               StartCallback callback) = 0;
 
   // Indicates that we no longer want to do any sync-related things for this
-  // data type. Severs all ties to the sync thread, deletes all local sync
-  // metadata, and then destroys the change processor.
-  virtual void DisableSync() = 0;
+  // data type. Severs all ties to the sync thread, and depending on
+  // |metadata_fate|, might delete all local sync metadata.
+  virtual void OnSyncStopping(SyncStopMetadataFate metadata_fate) = 0;
 
   // Returns a ListValue representing all nodes for the type to |callback|.
   // Used for populating nodes in Sync Node Browser of chrome://sync-internals.

components/sync/model/model_type_sync_bridge.cc

@@ -37,12 +37,14 @@ ConflictResolution ModelTypeSyncBridge::ResolveConflict(
   return ConflictResolution::UseRemote();
 }
 
-ModelTypeSyncBridge::DisableSyncResponse
-ModelTypeSyncBridge::ApplyDisableSyncChanges(
+ModelTypeSyncBridge::StopSyncResponse ModelTypeSyncBridge::ApplyStopSyncChanges(
     std::unique_ptr<MetadataChangeList> delete_metadata_change_list) {
-  // Nothing to do if this fails, so just ignore the error it might return.
-  ApplySyncChanges(std::move(delete_metadata_change_list), EntityChangeList());
-  return DisableSyncResponse::kModelStillReadyToSync;
+  if (delete_metadata_change_list) {
+    // Nothing to do if this fails, so just ignore the error it might return.
+    ApplySyncChanges(std::move(delete_metadata_change_list),
+                     EntityChangeList());
+  }
+  return StopSyncResponse::kModelStillReadyToSync;
 }
 
 ModelTypeChangeProcessor* ModelTypeSyncBridge::change_processor() {

components/sync/model/model_type_sync_bridge.h

@@ -37,7 +37,7 @@ class ModelTypeSyncBridge : public base::SupportsWeakPtr<ModelTypeSyncBridge> {
   using DataCallback = base::OnceCallback<void(std::unique_ptr<DataBatch>)>;
   using StorageKeyList = std::vector<std::string>;
 
-  enum class DisableSyncResponse {
+  enum class StopSyncResponse {
     kModelStillReadyToSync,
     kModelNoLongerReadyToSync
   };
@@ -135,10 +135,12 @@ class ModelTypeSyncBridge : public base::SupportsWeakPtr<ModelTypeSyncBridge> {
       const EntityData& remote_data) const;
 
   // Similar to ApplySyncChanges() but called by the processor when sync
-  // is in the process of being disabled. |delete_metadata_change_list| contains
-  // a change list to remove all metadata that the processor knows about, but
-  // the bridge may decide to implement deletion by other means.
-  virtual DisableSyncResponse ApplyDisableSyncChanges(
+  // is in the process of being stopped. If |delete_metadata_change_list| is not
+  // null, it indicates that sync metadata must be deleted (i.e. the datatype
+  // was disabled), and |*delete_metadata_change_list| contains a change list to
+  // remove all metadata that the processor knows about (the bridge may decide
+  // to implement deletion by other means).
+  virtual StopSyncResponse ApplyStopSyncChanges(
       std::unique_ptr<MetadataChangeList> delete_metadata_change_list);
 
   // Needs to be informed about any model change occurring via Delete() and