diff --git a/components/consent_auditor/consent_sync_bridge_impl.cc b/components/consent_auditor/consent_sync_bridge_impl.cc index 00371b63096715..3738b53295f650 100644 --- a/components/consent_auditor/consent_sync_bridge_impl.cc +++ b/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 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 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 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() { diff --git a/components/consent_auditor/consent_sync_bridge_impl.h b/components/consent_auditor/consent_sync_bridge_impl.h index b7abb9cf98fafa..c77e9de94b985c 100644 --- a/components/consent_auditor/consent_sync_bridge_impl.h +++ b/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 delete_metadata_change_list) override; // ConsentSyncBridge implementation. diff --git a/components/consent_auditor/consent_sync_bridge_impl_unittest.cc b/components/consent_auditor/consent_sync_bridge_impl_unittest.cc index 0b96e3e1f4a42c..757b70625b2c4b 100644 --- a/components/consent_auditor/consent_sync_bridge_impl_unittest.cc +++ b/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. diff --git a/components/sync/BUILD.gn b/components/sync/BUILD.gn index e63df9996975b1..632ad1ce0f43dc 100644 --- a/components/sync/BUILD.gn +++ b/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", diff --git a/components/sync/device_info/device_info_sync_bridge.cc b/components/sync/device_info/device_info_sync_bridge.cc index 6a6579a1119d08..93958cf031a17f 100644 --- a/components/sync/device_info/device_info_sync_bridge.cc +++ b/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 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 { diff --git a/components/sync/device_info/device_info_sync_bridge.h b/components/sync/device_info/device_info_sync_bridge.h index 6d64d830840d80..4ac6cd46c11de9 100644 --- a/components/sync/device_info/device_info_sync_bridge.h +++ b/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 delete_metadata_change_list) override; // DeviceInfoTracker implementation. diff --git a/components/sync/device_info/device_info_sync_bridge_unittest.cc b/components/sync/device_info/device_info_sync_bridge_unittest.cc index 776642cb27f79b..0d3c20fb6643cb 100644 --- a/components/sync/device_info/device_info_sync_bridge_unittest.cc +++ b/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()); diff --git a/components/sync/driver/model_type_controller.cc b/components/sync/driver/model_type_controller.cc index 67c1f056af8e2d..b7a0325d077baf 100644 --- a/components/sync/driver/model_type_controller.cc +++ b/components/sync/driver/model_type_controller.cc @@ -54,9 +54,10 @@ void RecordMemoryUsageHistogramHelperOnModelThread( delegate->RecordMemoryUsageHistogram(); } -void DisableSyncHelperOnModelThread( +void StopSyncHelperOnModelThread( + SyncStopMetadataFate metadata_fate, base::WeakPtr 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; diff --git a/components/sync/driver/model_type_controller_unittest.cc b/components/sync/driver/model_type_controller_unittest.cc index 71b68ff96c7f62..236fd6b783b807 100644 --- a/components/sync/driver/model_type_controller_unittest.cc +++ b/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 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 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 CreateProcessor() { std::unique_ptr processor = - std::make_unique(&disable_sync_call_count_); + std::make_unique(&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 diff --git a/components/sync/model/fake_model_type_controller_delegate.cc b/components/sync/model/fake_model_type_controller_delegate.cc index 6102d528b6061f..57eeb29d34e054 100644 --- a/components/sync/model/fake_model_type_controller_delegate.cc +++ b/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) { diff --git a/components/sync/model/fake_model_type_controller_delegate.h b/components/sync/model/fake_model_type_controller_delegate.h index 8a83db6a232149..c4589cf8b83153 100644 --- a/components/sync/model/fake_model_type_controller_delegate.h +++ b/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; diff --git a/components/sync/model/model_type_controller_delegate.h b/components/sync/model/model_type_controller_delegate.h index 08f1d60d00d4b6..bd4799dc53b0e1 100644 --- a/components/sync/model/model_type_controller_delegate.h +++ b/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. diff --git a/components/sync/model/model_type_sync_bridge.cc b/components/sync/model/model_type_sync_bridge.cc index ea6bf689819c05..8e3b4251341add 100644 --- a/components/sync/model/model_type_sync_bridge.cc +++ b/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 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() { diff --git a/components/sync/model/model_type_sync_bridge.h b/components/sync/model/model_type_sync_bridge.h index 3576f742a54444..a3b87c7f62ce77 100644 --- a/components/sync/model/model_type_sync_bridge.h +++ b/components/sync/model/model_type_sync_bridge.h @@ -37,7 +37,7 @@ class ModelTypeSyncBridge : public base::SupportsWeakPtr { using DataCallback = base::OnceCallback)>; using StorageKeyList = std::vector; - enum class DisableSyncResponse { + enum class StopSyncResponse { kModelStillReadyToSync, kModelNoLongerReadyToSync }; @@ -135,10 +135,12 @@ class ModelTypeSyncBridge : public base::SupportsWeakPtr { 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 delete_metadata_change_list); // Needs to be informed about any model change occurring via Delete() and diff --git a/components/sync/model_impl/client_tag_based_model_type_processor.cc b/components/sync/model_impl/client_tag_based_model_type_processor.cc index 9acdd3569e8203..766b85da474905 100644 --- a/components/sync/model_impl/client_tag_based_model_type_processor.cc +++ b/components/sync/model_impl/client_tag_based_model_type_processor.cc @@ -62,8 +62,9 @@ ClientTagBasedModelTypeProcessor::ClientTagBasedModelTypeProcessor( bridge_(nullptr), dump_stack_(dump_stack), commit_only_(commit_only), - weak_ptr_factory_(this) { - ResetState(); + weak_ptr_factory_for_controller_(this), + weak_ptr_factory_for_worker_(this) { + ResetState(CLEAR_METADATA); } ClientTagBasedModelTypeProcessor::~ClientTagBasedModelTypeProcessor() { @@ -151,7 +152,7 @@ void ClientTagBasedModelTypeProcessor::ConnectIfReady() { activation_response->model_type_state = model_type_state_; activation_response->type_processor = std::make_unique( - weak_ptr_factory_.GetWeakPtr(), + weak_ptr_factory_for_worker_.GetWeakPtr(), base::ThreadTaskRunnerHandle::Get()); std::move(start_callback_).Run(std::move(activation_response)); } @@ -164,35 +165,58 @@ bool ClientTagBasedModelTypeProcessor::IsConnected() const { return !!worker_; } -void ClientTagBasedModelTypeProcessor::DisableSync() { +void ClientTagBasedModelTypeProcessor::OnSyncStopping( + SyncStopMetadataFate metadata_fate) { DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_); // Disabling sync for a type never happens before the model is ready to sync. DCHECK(model_ready_to_sync_); - std::unique_ptr change_list = - bridge_->CreateMetadataChangeList(); - for (const auto& kv : entities_) { - change_list->ClearMetadata(kv.second->storage_key()); - } - change_list->ClearModelTypeState(); - - const ModelTypeSyncBridge::DisableSyncResponse response = - bridge_->ApplyDisableSyncChanges(std::move(change_list)); - - // Reset all the internal state of the processor. - ResetState(); - - switch (response) { - case ModelTypeSyncBridge::DisableSyncResponse::kModelStillReadyToSync: - // The model is still ready to sync (with the same |bridge_|) - replay the - // initialization. - ModelReadyToSync(std::make_unique()); + switch (metadata_fate) { + case KEEP_METADATA: { + switch (bridge_->ApplyStopSyncChanges( + /*delete_metadata_change_list=*/nullptr)) { + case ModelTypeSyncBridge::StopSyncResponse::kModelStillReadyToSync: + // The model is still ready to sync (with the same |bridge_|) and same + // sync metadata. + ResetState(KEEP_METADATA); + break; + case ModelTypeSyncBridge::StopSyncResponse::kModelNoLongerReadyToSync: + // Model not ready to sync, so wait until the bridge calls + // ModelReadyToSync(), and meanwhile throw away all metadata. + ResetState(CLEAR_METADATA); + break; + } break; - case ModelTypeSyncBridge::DisableSyncResponse::kModelNoLongerReadyToSync: - // Model not ready to sync, so wait until the bridge calls - // ModelReadyToSync(). - model_ready_to_sync_ = false; + } + + case CLEAR_METADATA: { + // Clear persisted metadata. + std::unique_ptr change_list = + bridge_->CreateMetadataChangeList(); + for (const auto& kv : entities_) { + change_list->ClearMetadata(kv.second->storage_key()); + } + change_list->ClearModelTypeState(); + + const ModelTypeSyncBridge::StopSyncResponse response = + bridge_->ApplyStopSyncChanges(std::move(change_list)); + + // Reset all the internal state of the processor. + ResetState(CLEAR_METADATA); + + switch (response) { + case ModelTypeSyncBridge::StopSyncResponse::kModelStillReadyToSync: + // The model is still ready to sync (with the same |bridge_|) - replay + // the initialization. + ModelReadyToSync(std::make_unique()); + break; + case ModelTypeSyncBridge::StopSyncResponse::kModelNoLongerReadyToSync: + // Model not ready to sync, so wait until the bridge calls + // ModelReadyToSync(). + break; + } break; + } } } @@ -226,7 +250,7 @@ void ClientTagBasedModelTypeProcessor::ReportError(const ModelError& error) { base::WeakPtr ClientTagBasedModelTypeProcessor::GetControllerDelegateOnUIThread() { - return weak_ptr_factory_.GetWeakPtr(); + return weak_ptr_factory_for_controller_.GetWeakPtr(); } void ClientTagBasedModelTypeProcessor::ConnectSync( @@ -244,7 +268,7 @@ void ClientTagBasedModelTypeProcessor::DisconnectSync() { DCHECK(IsConnected()); DVLOG(1) << "Disconnecting sync for " << ModelTypeToString(type_); - weak_ptr_factory_.InvalidateWeakPtrs(); + weak_ptr_factory_for_worker_.InvalidateWeakPtrs(); worker_.reset(); for (const auto& kv : entities_) { @@ -402,7 +426,7 @@ void ClientTagBasedModelTypeProcessor::GetLocalChanges( std::move(entities_requiring_data), base::BindRepeating( &ClientTagBasedModelTypeProcessor::OnPendingDataLoaded, - weak_ptr_factory_.GetWeakPtr(), max_entries, callback)); + weak_ptr_factory_for_worker_.GetWeakPtr(), max_entries, callback)); } else { // All commit data can be availbale in memory for those entries passed in // the .put() method. @@ -1024,21 +1048,29 @@ void ClientTagBasedModelTypeProcessor::RemoveEntity( entities_.erase(entity->metadata().client_tag_hash()); } -void ClientTagBasedModelTypeProcessor::ResetState() { +void ClientTagBasedModelTypeProcessor::ResetState( + SyncStopMetadataFate metadata_fate) { // This should reset all mutable fields (except for |bridge_|). worker_.reset(); model_error_.reset(); - model_ready_to_sync_ = false; - entities_.clear(); - storage_key_to_tag_hash_.clear(); - model_type_state_ = sync_pb::ModelTypeState(); start_callback_ = StartCallback(); error_handler_ = ModelErrorHandler(); cached_gc_directive_version_ = 0; cached_gc_directive_aged_out_day_ = base::Time::FromDoubleT(0); + switch (metadata_fate) { + case KEEP_METADATA: + break; + case CLEAR_METADATA: + model_ready_to_sync_ = false; + entities_.clear(); + storage_key_to_tag_hash_.clear(); + model_type_state_ = sync_pb::ModelTypeState(); + break; + } + // Do not let any delayed callbacks to be called. - weak_ptr_factory_.InvalidateWeakPtrs(); + weak_ptr_factory_for_worker_.InvalidateWeakPtrs(); } void ClientTagBasedModelTypeProcessor::GetAllNodesForDebugging( @@ -1047,7 +1079,7 @@ void ClientTagBasedModelTypeProcessor::GetAllNodesForDebugging( return; bridge_->GetAllDataForDebugging(base::BindOnce( &ClientTagBasedModelTypeProcessor::MergeDataWithMetadataForDebugging, - weak_ptr_factory_.GetWeakPtr(), std::move(callback))); + weak_ptr_factory_for_worker_.GetWeakPtr(), std::move(callback))); } void ClientTagBasedModelTypeProcessor::MergeDataWithMetadataForDebugging( diff --git a/components/sync/model_impl/client_tag_based_model_type_processor.h b/components/sync/model_impl/client_tag_based_model_type_processor.h index b57d570987180a..68c18b489e562e 100644 --- a/components/sync/model_impl/client_tag_based_model_type_processor.h +++ b/components/sync/model_impl/client_tag_based_model_type_processor.h @@ -15,6 +15,7 @@ #include "base/optional.h" #include "base/sequence_checker.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/model_type_processor.h" #include "components/sync/engine/non_blocking_sync_common.h" @@ -84,7 +85,7 @@ class ClientTagBasedModelTypeProcessor : public ModelTypeProcessor, // ModelTypeControllerDelegate implementation. 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; @@ -208,7 +209,7 @@ class ClientTagBasedModelTypeProcessor : public ModelTypeProcessor, // TODO(jkrcal): Replace the helper function by grouping the state naturally // into a few structs / nested classes so that the state can be reset by // resetting these structs. - void ResetState(); + void ResetState(SyncStopMetadataFate metadata_fate); // Adds metadata to all data returned by the bridge. // TODO(jkrcal): Mark as const (together with functions it depends on such as @@ -302,8 +303,14 @@ class ClientTagBasedModelTypeProcessor : public ModelTypeProcessor, SEQUENCE_CHECKER(sequence_checker_); + // WeakPtrFactory for this processor for ModelTypeController (only gets + // invalidated during destruction). + base::WeakPtrFactory + weak_ptr_factory_for_controller_; + // WeakPtrFactory for this processor which will be sent to sync thread. - base::WeakPtrFactory weak_ptr_factory_; + base::WeakPtrFactory + weak_ptr_factory_for_worker_; DISALLOW_COPY_AND_ASSIGN(ClientTagBasedModelTypeProcessor); }; diff --git a/components/sync/model_impl/client_tag_based_model_type_processor_unittest.cc b/components/sync/model_impl/client_tag_based_model_type_processor_unittest.cc index 489bddc055bee5..c4ed03c514fd82 100644 --- a/components/sync/model_impl/client_tag_based_model_type_processor_unittest.cc +++ b/components/sync/model_impl/client_tag_based_model_type_processor_unittest.cc @@ -1208,11 +1208,39 @@ TEST_F(ClientTagBasedModelTypeProcessorTest, Disconnect) { EXPECT_TRUE(worker()->HasPendingCommitForHash(kHash3)); } +// Test proper handling of stop (without disabling sync) and re-enable. +// +// Creates items in various states of commit and verifies they do NOT attempt to +// commit on re-enable. +TEST_F(ClientTagBasedModelTypeProcessorTest, Stop) { + InitializeToReadyState(); + + // The first item is fully committed. + WriteItemAndAck(kKey1, kValue1); + + // The second item has a commit request in progress. + bridge()->WriteItem(kKey2, kValue2); + EXPECT_TRUE(worker()->HasPendingCommitForHash(kHash2)); + + type_processor()->OnSyncStopping(KEEP_METADATA); + EXPECT_TRUE(type_processor()->IsTrackingMetadata()); + + // The third item is added after disable. + bridge()->WriteItem(kKey3, kValue3); + + // Now we re-enable. + OnSyncStarting(); + worker()->UpdateFromServer(); + + // Once we're ready to commit, only the newest items should be committed. + worker()->VerifyPendingCommits({{kHash3}}); +} + // Test proper handling of disable and re-enable. // // Creates items in various states of commit and verifies they re-attempt to // commit on re-enable. -TEST_F(ClientTagBasedModelTypeProcessorTest, Disable) { +TEST_F(ClientTagBasedModelTypeProcessorTest, StopAndClearMetadata) { InitializeToReadyState(); // The first item is fully committed. @@ -1222,7 +1250,7 @@ TEST_F(ClientTagBasedModelTypeProcessorTest, Disable) { bridge()->WriteItem(kKey2, kValue2); EXPECT_TRUE(worker()->HasPendingCommitForHash(kHash2)); - type_processor()->DisableSync(); + type_processor()->OnSyncStopping(CLEAR_METADATA); EXPECT_FALSE(type_processor()->IsTrackingMetadata()); // The third item is added after disable. diff --git a/components/sync/user_events/user_event_sync_bridge.cc b/components/sync/user_events/user_event_sync_bridge.cc index 564f807f281737..398a1a62bd175f 100644 --- a/components/sync/user_events/user_event_sync_bridge.cc +++ b/components/sync/user_events/user_event_sync_bridge.cc @@ -158,28 +158,31 @@ void UserEventSyncBridge::OnSyncStarting() { #if !defined(OS_IOS) // https://crbug.com/834042 DCHECK(!GetAuthenticatedAccountId().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 -UserEventSyncBridge::ApplyDisableSyncChanges( +ModelTypeSyncBridge::StopSyncResponse UserEventSyncBridge::ApplyStopSyncChanges( std::unique_ptr delete_metadata_change_list) { - // Sync can only be disabled after initialization. + // Sync can only be stopped after initialization. DCHECK(deferred_user_events_while_initializing_.empty()); is_sync_starting_or_started_ = false; - // Delete everything except user consents. With DICE the signout may happen - // frequently. It is important to report all user consents, thus, they are - // persisted for some time even after signout. - store_->ReadAllData(base::BindOnce( - &UserEventSyncBridge::OnReadAllDataToDelete, base::AsWeakPtr(this), - std::move(delete_metadata_change_list))); - return DisableSyncResponse::kModelStillReadyToSync; + if (delete_metadata_change_list) { + // Delete everything except user consents. With DICE the signout may happen + // frequently. It is important to report all user consents, thus, they are + // persisted for some time even after signout. + store_->ReadAllData(base::BindOnce( + &UserEventSyncBridge::OnReadAllDataToDelete, base::AsWeakPtr(this), + std::move(delete_metadata_change_list))); + } + + return StopSyncResponse::kModelStillReadyToSync; } void UserEventSyncBridge::OnReadAllDataToDelete( diff --git a/components/sync/user_events/user_event_sync_bridge.h b/components/sync/user_events/user_event_sync_bridge.h index c70490c149eac6..8d0d2046e982e8 100644 --- a/components/sync/user_events/user_event_sync_bridge.h +++ b/components/sync/user_events/user_event_sync_bridge.h @@ -44,7 +44,7 @@ class UserEventSyncBridge : 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 delete_metadata_change_list) override; void RecordUserEvent(std::unique_ptr specifics); diff --git a/components/sync/user_events/user_event_sync_bridge_unittest.cc b/components/sync/user_events/user_event_sync_bridge_unittest.cc index 49a0588a9e36ce..ecd2d5e2113eac 100644 --- a/components/sync/user_events/user_event_sync_bridge_unittest.cc +++ b/components/sync/user_events/user_event_sync_bridge_unittest.cc @@ -193,21 +193,21 @@ TEST_F(UserEventSyncBridgeTest, SingleRecord) { ElementsAre(Pair(storage_key, MatchesUserEvent(specifics)))); } -TEST_F(UserEventSyncBridgeTest, ApplyDisableSyncChanges) { +TEST_F(UserEventSyncBridgeTest, ApplyStopSyncChanges) { const UserEventSpecifics specifics(CreateSpecifics(1u, 2u, 3u)); bridge()->RecordUserEvent(std::make_unique(specifics)); 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(); EXPECT_THAT(GetAllData(), IsEmpty()); } -TEST_F(UserEventSyncBridgeTest, ApplyDisableSyncChangesShouldKeepConsents) { +TEST_F(UserEventSyncBridgeTest, ApplyStopSyncChangesShouldKeepConsents) { UserEventSpecifics user_event_specifics(CreateSpecifics(2u, 2u, 2u)); auto* consent = user_event_specifics.mutable_user_consent(); consent->set_feature(UserEventSpecifics::UserConsent::CHROME_SYNC); @@ -217,8 +217,8 @@ TEST_F(UserEventSyncBridgeTest, ApplyDisableSyncChangesShouldKeepConsents) { 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(); @@ -418,7 +418,7 @@ TEST_F(UserEventSyncBridgeTest, bridge()->RecordUserEvent(std::make_unique(consent)); - bridge()->ApplyDisableSyncChanges(WriteBatch::CreateMetadataChangeList()); + bridge()->ApplyStopSyncChanges(WriteBatch::CreateMetadataChangeList()); // The bridge may asynchronously query the store to choose what to delete. base::RunLoop().RunUntilIdle(); @@ -563,7 +563,7 @@ TEST_F(UserEventSyncBridgeTest, ShouldSubmitPersistedConsentOnlyIfSameAccount) { std::make_unique(user_event_specifics)); ASSERT_THAT(GetAllData(), SizeIs(1)); - bridge()->ApplyDisableSyncChanges(WriteBatch::CreateMetadataChangeList()); + bridge()->ApplyStopSyncChanges(WriteBatch::CreateMetadataChangeList()); // The bridge may asynchronously query the store to choose what to delete. base::RunLoop().RunUntilIdle(); @@ -581,7 +581,7 @@ TEST_F(UserEventSyncBridgeTest, ShouldSubmitPersistedConsentOnlyIfSameAccount) { bridge()->OnSyncStarting(); base::RunLoop().RunUntilIdle(); - bridge()->ApplyDisableSyncChanges(WriteBatch::CreateMetadataChangeList()); + bridge()->ApplyStopSyncChanges(WriteBatch::CreateMetadataChangeList()); base::RunLoop().RunUntilIdle(); // The previous user signs in again and enables sync. diff --git a/components/sync_bookmarks/bookmark_model_type_processor.cc b/components/sync_bookmarks/bookmark_model_type_processor.cc index b07032c873dbbd..2162e5f9b34f4e 100644 --- a/components/sync_bookmarks/bookmark_model_type_processor.cc +++ b/components/sync_bookmarks/bookmark_model_type_processor.cc @@ -541,7 +541,8 @@ void BookmarkModelTypeProcessor::ConnectIfReady() { std::move(start_callback_).Run(std::move(activation_context)); } -void BookmarkModelTypeProcessor::DisableSync() { +void BookmarkModelTypeProcessor::OnSyncStopping( + syncer::SyncStopMetadataFate metadata_fate) { DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_); NOTIMPLEMENTED(); } diff --git a/components/sync_bookmarks/bookmark_model_type_processor.h b/components/sync_bookmarks/bookmark_model_type_processor.h index 7ba225a331655a..81039d779a5355 100644 --- a/components/sync_bookmarks/bookmark_model_type_processor.h +++ b/components/sync_bookmarks/bookmark_model_type_processor.h @@ -48,7 +48,7 @@ class BookmarkModelTypeProcessor : public syncer::ModelTypeProcessor, // ModelTypeControllerDelegate implementation. void OnSyncStarting(const syncer::DataTypeActivationRequest& request, StartCallback start_callback) override; - void DisableSync() override; + void OnSyncStopping(syncer::SyncStopMetadataFate metadata_fate) override; void GetAllNodesForDebugging(AllNodesCallback callback) override; void GetStatusCountersForDebugging(StatusCountersCallback callback) override; void RecordMemoryUsageHistogram() override; diff --git a/components/sync_sessions/session_sync_bridge.cc b/components/sync_sessions/session_sync_bridge.cc index 243f1fa8a72e4c..88a4787e0a40c5 100644 --- a/components/sync_sessions/session_sync_bridge.cc +++ b/components/sync_sessions/session_sync_bridge.cc @@ -291,15 +291,14 @@ std::string SessionSyncBridge::GetStorageKey( return SessionStore::GetStorageKey(entity_data.specifics.session()); } -ModelTypeSyncBridge::DisableSyncResponse -SessionSyncBridge::ApplyDisableSyncChanges( +ModelTypeSyncBridge::StopSyncResponse SessionSyncBridge::ApplyStopSyncChanges( std::unique_ptr delete_metadata_change_list) { local_session_event_router_->Stop(); - if (syncing_) { + if (syncing_ && delete_metadata_change_list) { syncing_->store->DeleteAllDataAndMetadata(); } syncing_.reset(); - return DisableSyncResponse::kModelNoLongerReadyToSync; + return StopSyncResponse::kModelNoLongerReadyToSync; } std::unique_ptr @@ -338,17 +337,6 @@ void SessionSyncBridge::OnFaviconVisited(const GURL& page_url, } void SessionSyncBridge::OnSyncStarting() { - // |syncing_| can be non-null during testing, if ProfileSyncService is - // restarted without having disabled sync, because USS bridges currently do - // not receive a notification when sync is stopped (without actually - // disabling sync, e.g. browser shutdown). - // TODO(mastiz): Remove the workaround below once a proper signal is plumbed. - if (syncing_) { - local_session_event_router_->Stop(); - syncing_.reset(); // Prevents metadata from being deleted. - change_processor()->GetControllerDelegateOnUIThread()->DisableSync(); - } - DCHECK(!syncing_); session_store_factory_.Run(base::BindOnce( diff --git a/components/sync_sessions/session_sync_bridge.h b/components/sync_sessions/session_sync_bridge.h index 1a9be717b9222e..4dce7ddf76a9c9 100644 --- a/components/sync_sessions/session_sync_bridge.h +++ b/components/sync_sessions/session_sync_bridge.h @@ -73,7 +73,7 @@ class SessionSyncBridge : public AbstractSessionsSyncManager, void GetAllDataForDebugging(DataCallback callback) override; std::string GetClientTag(const syncer::EntityData& entity_data) override; std::string GetStorageKey(const syncer::EntityData& entity_data) override; - DisableSyncResponse ApplyDisableSyncChanges( + StopSyncResponse ApplyStopSyncChanges( std::unique_ptr delete_metadata_change_list) override; diff --git a/components/sync_sessions/session_sync_bridge_unittest.cc b/components/sync_sessions/session_sync_bridge_unittest.cc index e9b4b87c8016c4..337a08999452b4 100644 --- a/components/sync_sessions/session_sync_bridge_unittest.cc +++ b/components/sync_sessions/session_sync_bridge_unittest.cc @@ -1002,7 +1002,7 @@ TEST_F(SessionSyncBridgeTest, ShouldDisableSyncAndReenable) { ASSERT_THAT(GetAllData(), Not(IsEmpty())); EXPECT_CALL(mock_processor(), ModelReadyToSync(_)).Times(0); - real_processor()->DisableSync(); + real_processor()->OnSyncStopping(syncer::CLEAR_METADATA); EXPECT_CALL(mock_processor(), ModelReadyToSync(IsEmptyMetadataBatch())); StartSyncing();