Commit ee4382ca authored by Maksim Moskvitin's avatar Maksim Moskvitin Committed by Commit Bot

[Sync:USS] Plumb sync controller for Nigori USS

If the experimental feature for Nigori USS is enabled, the new codepath
now uses a controller (ModelTypeController) to start and stop Nigori,
and to connect it to the ModelTypeWorker.

Because of lifetime issues with Nigori, and the special treatment for
the initial sync cycle during engine initialization (which predates the
instantiation of DataTypeManager), the interactions with the controller
are done manually, and on the sync thread. This is not ideal, but we
don't have better alternatives as of today, without intrusive changes
that would be barely feasible during the coexistence of both codepaths.

To avoid engine_impl's dependence on driver the registration of Nigori
datatype was moved back to SyncEngineBackend (partially revert
https://chromium-review.googlesource.com/c/chromium/src/+/1619813).

Bug: 922900
Change-Id: I22b20d3a0e2f396a0eddbef54cf2e429f02cc410
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1648178
Commit-Queue: Maksim Moskvitin <mmoskvitin@google.com>
Reviewed-by: default avatarMikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#667114}
parent d4346d78
...@@ -17,6 +17,8 @@ ...@@ -17,6 +17,8 @@
#include "components/invalidation/public/object_id_invalidation_map.h" #include "components/invalidation/public/object_id_invalidation_map.h"
#include "components/sync/base/invalidation_adapter.h" #include "components/sync/base/invalidation_adapter.h"
#include "components/sync/base/sync_base_switches.h" #include "components/sync/base/sync_base_switches.h"
#include "components/sync/driver/configure_context.h"
#include "components/sync/driver/model_type_controller.h"
#include "components/sync/driver/sync_driver_switches.h" #include "components/sync/driver/sync_driver_switches.h"
#include "components/sync/engine/cycle/commit_counters.h" #include "components/sync/engine/cycle/commit_counters.h"
#include "components/sync/engine/cycle/status_counters.h" #include "components/sync/engine/cycle/status_counters.h"
...@@ -29,6 +31,7 @@ ...@@ -29,6 +31,7 @@
#include "components/sync/engine/sync_manager.h" #include "components/sync/engine/sync_manager.h"
#include "components/sync/engine/sync_manager_factory.h" #include "components/sync/engine/sync_manager_factory.h"
#include "components/sync/engine_impl/sync_encryption_handler_impl.h" #include "components/sync/engine_impl/sync_encryption_handler_impl.h"
#include "components/sync/model_impl/forwarding_model_type_controller_delegate.h"
#include "components/sync/nigori/nigori_model_type_processor.h" #include "components/sync/nigori/nigori_model_type_processor.h"
#include "components/sync/nigori/nigori_sync_bridge_impl.h" #include "components/sync/nigori/nigori_sync_bridge_impl.h"
#include "components/sync/syncable/directory.h" #include "components/sync/syncable/directory.h"
...@@ -145,6 +148,34 @@ void SyncEngineBackend::OnInitializationComplete( ...@@ -145,6 +148,34 @@ void SyncEngineBackend::OnInitializationComplete(
ModelTypeSet new_control_types = ModelTypeSet new_control_types =
registrar_->ConfigureDataTypes(ControlTypes(), ModelTypeSet()); registrar_->ConfigureDataTypes(ControlTypes(), ModelTypeSet());
ModelTypeConnector* model_type_connector =
sync_manager_->GetModelTypeConnector();
if (nigori_controller_) {
// Having non-null |nigori_controller_| means that USS implementation of
// Nigori is enabled.
// The controller for Nigori is not exposed to the UI thread or the
// DataTypeManager, so we need to start it here manually.
ConfigureContext configure_context;
configure_context.authenticated_account_id = authenticated_account_id_;
configure_context.cache_guid = sync_manager_->cache_guid();
// TODO(crbug.com/922900): investigate whether we want to use
// STORAGE_IN_MEMORY in Butter mode.
configure_context.storage_option = STORAGE_ON_DISK;
configure_context.configuration_start_time = base::Time::Now();
nigori_controller_->LoadModels(configure_context, base::DoNothing());
DCHECK_EQ(nigori_controller_->state(), DataTypeController::MODEL_LOADED);
// TODO(crbug.com/922900): Do we need to call RegisterNonBlockingType() for
// Nigori?
model_type_connector->ConnectNonBlockingType(
NIGORI, nigori_controller_->ActivateManuallyForNigori());
} else {
// Control types don't have DataTypeControllers, but they need to have
// update handlers registered in ModelTypeRegistry.
for (ModelType control_type : ControlTypes()) {
model_type_connector->RegisterDirectoryType(control_type, GROUP_PASSIVE);
}
}
ModelSafeRoutingInfo routing_info; ModelSafeRoutingInfo routing_info;
registrar_->GetModelSafeRoutingInfo(&routing_info); registrar_->GetModelSafeRoutingInfo(&routing_info);
SDVLOG(1) << "Control Types " << ModelTypeSetToString(new_control_types) SDVLOG(1) << "Control Types " << ModelTypeSetToString(new_control_types)
...@@ -309,13 +340,19 @@ void SyncEngineBackend::DoInitialize(SyncEngine::InitParams params) { ...@@ -309,13 +340,19 @@ void SyncEngineBackend::DoInitialize(SyncEngine::InitParams params) {
// Load the previously persisted set of invalidation versions into memory. // Load the previously persisted set of invalidation versions into memory.
last_invalidation_versions_ = params.invalidation_versions; last_invalidation_versions_ = params.invalidation_versions;
authenticated_account_id_ = params.authenticated_account_id;
DCHECK(!registrar_); DCHECK(!registrar_);
DCHECK(params.registrar); DCHECK(params.registrar);
registrar_ = std::move(params.registrar); registrar_ = std::move(params.registrar);
if (base::FeatureList::IsEnabled(switches::kSyncUSSNigori)) { if (base::FeatureList::IsEnabled(switches::kSyncUSSNigori)) {
auto nigori_processor = std::make_unique<NigoriModelTypeProcessor>();
nigori_controller_ = std::make_unique<ModelTypeController>(
NIGORI, std::make_unique<ForwardingModelTypeControllerDelegate>(
nigori_processor->GetControllerDelegate().get()));
sync_encryption_handler_ = std::make_unique<NigoriSyncBridgeImpl>( sync_encryption_handler_ = std::make_unique<NigoriSyncBridgeImpl>(
std::make_unique<NigoriModelTypeProcessor>(), &encryptor_); std::move(nigori_processor), &encryptor_);
} else { } else {
sync_encryption_handler_ = std::make_unique<SyncEncryptionHandlerImpl>( sync_encryption_handler_ = std::make_unique<SyncEncryptionHandlerImpl>(
&user_share_, &encryptor_, params.restored_key_for_bootstrapping, &user_share_, &encryptor_, params.restored_key_for_bootstrapping,
...@@ -471,8 +508,11 @@ void SyncEngineBackend::DoShutdown(ShutdownReason reason) { ...@@ -471,8 +508,11 @@ void SyncEngineBackend::DoShutdown(ShutdownReason reason) {
registrar_ = nullptr; registrar_ = nullptr;
if (reason == DISABLE_SYNC) if (reason == DISABLE_SYNC) {
// TODO(crbug.com/922900): We may want to remove Nigori data from the
// storage if USS Nigori implementation is enabled.
syncable::Directory::DeleteDirectoryFiles(sync_data_folder_); syncable::Directory::DeleteDirectoryFiles(sync_data_folder_);
}
host_.Reset(); host_.Reset();
weak_ptr_factory_.InvalidateWeakPtrs(); weak_ptr_factory_.InvalidateWeakPtrs();
......
...@@ -32,6 +32,7 @@ ...@@ -32,6 +32,7 @@
namespace syncer { namespace syncer {
class ModelTypeController;
class SyncEngineImpl; class SyncEngineImpl;
class SyncEngineBackend : public base::RefCountedThreadSafe<SyncEngineBackend>, class SyncEngineBackend : public base::RefCountedThreadSafe<SyncEngineBackend>,
...@@ -221,6 +222,13 @@ class SyncEngineBackend : public base::RefCountedThreadSafe<SyncEngineBackend>, ...@@ -221,6 +222,13 @@ class SyncEngineBackend : public base::RefCountedThreadSafe<SyncEngineBackend>,
// The top-level syncapi entry point. Lives on the sync thread. // The top-level syncapi entry point. Lives on the sync thread.
std::unique_ptr<SyncManager> sync_manager_; std::unique_ptr<SyncManager> sync_manager_;
// Required for |nigori_controller_| LoadModels().
std::string authenticated_account_id_;
// Initialized in OnInitializationComplete() iff USS implementation of Nigori
// is enabled.
std::unique_ptr<ModelTypeController> nigori_controller_;
// Temporary holder of sync manager's initialization results. Set by // Temporary holder of sync manager's initialization results. Set by
// OnInitializeComplete, and consumed when we pass it via OnEngineInitialized // OnInitializeComplete, and consumed when we pass it via OnEngineInitialized
// in the final state of HandleInitializationSuccessOnFrontendLoop. // in the final state of HandleInitializationSuccessOnFrontendLoop.
......
...@@ -60,6 +60,17 @@ ModelTypeController::ModelTypeController( ...@@ -60,6 +60,17 @@ ModelTypeController::ModelTypeController(
ModelTypeController::~ModelTypeController() {} ModelTypeController::~ModelTypeController() {}
std::unique_ptr<DataTypeActivationResponse>
ModelTypeController::ActivateManuallyForNigori() {
// To avoid abuse of this temporary API, we restrict it to NIGORI.
DCHECK_EQ(NIGORI, type());
DCHECK_EQ(MODEL_LOADED, state_);
DCHECK(activation_response_);
state_ = RUNNING;
activated_ = true; // Not relevant, but for consistency.
return std::move(activation_response_);
}
bool ModelTypeController::ShouldLoadModelBeforeConfigure() const { bool ModelTypeController::ShouldLoadModelBeforeConfigure() const {
// USS datatypes require loading models because model controls storage where // USS datatypes require loading models because model controls storage where
// data type context and progress marker are persisted. // data type context and progress marker are persisted.
......
...@@ -38,6 +38,12 @@ class ModelTypeController : public DataTypeController { ...@@ -38,6 +38,12 @@ class ModelTypeController : public DataTypeController {
std::unique_ptr<ModelTypeControllerDelegate> delegate_in_memory); std::unique_ptr<ModelTypeControllerDelegate> delegate_in_memory);
~ModelTypeController() override; ~ModelTypeController() override;
// Steals the activation response, only used for Nigori.
// TODO(crbug.com/967677): Once all datatypes are in USS, we should redesign
// or remove RegisterWithBackend, and expose the activation response via
// LoadModels(), which is more natural in USS.
std::unique_ptr<DataTypeActivationResponse> ActivateManuallyForNigori();
// DataTypeController implementation. // DataTypeController implementation.
bool ShouldLoadModelBeforeConfigure() const override; bool ShouldLoadModelBeforeConfigure() const override;
void BeforeLoadModels(ModelTypeConfigurer* configurer) override; void BeforeLoadModels(ModelTypeConfigurer* configurer) override;
......
...@@ -198,6 +198,10 @@ UserShare* FakeSyncManager::GetUserShare() { ...@@ -198,6 +198,10 @@ UserShare* FakeSyncManager::GetUserShare() {
return test_user_share_.user_share(); return test_user_share_.user_share();
} }
ModelTypeConnector* FakeSyncManager::GetModelTypeConnector() {
return &fake_model_type_connector_;
}
std::unique_ptr<ModelTypeConnector> std::unique_ptr<ModelTypeConnector>
FakeSyncManager::GetModelTypeConnectorProxy() { FakeSyncManager::GetModelTypeConnectorProxy() {
return std::make_unique<FakeModelTypeConnector>(); return std::make_unique<FakeModelTypeConnector>();
......
...@@ -13,6 +13,7 @@ ...@@ -13,6 +13,7 @@
#include "base/macros.h" #include "base/macros.h"
#include "base/memory/ref_counted.h" #include "base/memory/ref_counted.h"
#include "base/observer_list.h" #include "base/observer_list.h"
#include "components/sync/engine/fake_model_type_connector.h"
#include "components/sync/engine/sync_manager.h" #include "components/sync/engine/sync_manager.h"
#include "components/sync/syncable/test_user_share.h" #include "components/sync/syncable/test_user_share.h"
#include "components/sync/test/fake_sync_encryption_handler.h" #include "components/sync/test/fake_sync_encryption_handler.h"
...@@ -99,6 +100,7 @@ class FakeSyncManager : public SyncManager { ...@@ -99,6 +100,7 @@ class FakeSyncManager : public SyncManager {
void SaveChanges() override; void SaveChanges() override;
void ShutdownOnSyncThread() override; void ShutdownOnSyncThread() override;
UserShare* GetUserShare() override; UserShare* GetUserShare() override;
ModelTypeConnector* GetModelTypeConnector() override;
std::unique_ptr<ModelTypeConnector> GetModelTypeConnectorProxy() override; std::unique_ptr<ModelTypeConnector> GetModelTypeConnectorProxy() override;
std::string cache_guid() override; std::string cache_guid() override;
std::string birthday() override; std::string birthday() override;
...@@ -147,6 +149,8 @@ class FakeSyncManager : public SyncManager { ...@@ -147,6 +149,8 @@ class FakeSyncManager : public SyncManager {
FakeSyncEncryptionHandler fake_encryption_handler_; FakeSyncEncryptionHandler fake_encryption_handler_;
FakeModelTypeConnector fake_model_type_connector_;
TestUserShare test_user_share_; TestUserShare test_user_share_;
// Number of invalidations received since startup. // Number of invalidations received since startup.
......
...@@ -339,6 +339,11 @@ class SyncManager { ...@@ -339,6 +339,11 @@ class SyncManager {
// May be called from any thread. // May be called from any thread.
virtual UserShare* GetUserShare() = 0; virtual UserShare* GetUserShare() = 0;
// Returns non-owning pointer to ModelTypeConnector. In contrast with
// ModelTypeConnectorProxy all calls are executed synchronously, thus the
// pointer should be used on sync thread.
virtual ModelTypeConnector* GetModelTypeConnector() = 0;
// Returns an instance of the main interface for registering sync types with // Returns an instance of the main interface for registering sync types with
// sync engine. // sync engine.
virtual std::unique_ptr<ModelTypeConnector> GetModelTypeConnectorProxy() = 0; virtual std::unique_ptr<ModelTypeConnector> GetModelTypeConnectorProxy() = 0;
......
...@@ -409,12 +409,6 @@ void SyncManagerImpl::Init(InitArgs* args) { ...@@ -409,12 +409,6 @@ void SyncManagerImpl::Init(InitArgs* args) {
scheduler_->OnCredentialsUpdated(); scheduler_->OnCredentialsUpdated();
} }
// Control types don't have DataTypeControllers, but they need to have
// update handlers registered in ModelTypeRegistry.
for (ModelType control_type : ControlTypes()) {
model_type_registry_->RegisterDirectoryType(control_type, GROUP_PASSIVE);
}
NotifyInitializationSuccess(); NotifyInitializationSuccess();
} }
...@@ -1004,6 +998,11 @@ UserShare* SyncManagerImpl::GetUserShare() { ...@@ -1004,6 +998,11 @@ UserShare* SyncManagerImpl::GetUserShare() {
return share_; return share_;
} }
ModelTypeConnector* SyncManagerImpl::GetModelTypeConnector() {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
return model_type_registry_.get();
}
std::unique_ptr<ModelTypeConnector> std::unique_ptr<ModelTypeConnector>
SyncManagerImpl::GetModelTypeConnectorProxy() { SyncManagerImpl::GetModelTypeConnectorProxy() {
DCHECK(initialized_); DCHECK(initialized_);
......
...@@ -91,6 +91,7 @@ class SyncManagerImpl ...@@ -91,6 +91,7 @@ class SyncManagerImpl
void SaveChanges() override; void SaveChanges() override;
void ShutdownOnSyncThread() override; void ShutdownOnSyncThread() override;
UserShare* GetUserShare() override; UserShare* GetUserShare() override;
ModelTypeConnector* GetModelTypeConnector() override;
std::unique_ptr<ModelTypeConnector> GetModelTypeConnectorProxy() override; std::unique_ptr<ModelTypeConnector> GetModelTypeConnectorProxy() override;
std::string cache_guid() override; std::string cache_guid() override;
std::string birthday() override; std::string birthday() override;
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment