Avoid dependencies to legacy sync/syncable from sync/driver

There is no actual pre-USS datatype running and the code related to
syncable::Directory is dead. Some of these concepts had leaked into
higher layers in sync/driver, so this patch deletes them prior to
deleting the functionality within the sync engine.

In order to achieve this, SyncManagerImpl now takes ownership of
UserShare and NigoriHandler, but this should cause no behavioral
differences.

Change-Id: I56f7f325f87206b985c064bb1c5e97acb7b05980
Bug: 923287
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2264432
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Reviewed-by: Maksim Moskvitin <mmoskvitin@google.com>
Cr-Commit-Position: refs/heads/master@{#782861}

components/sync/base/BUILD.gn

@@ -29,6 +29,8 @@ jumbo_static_library("base") {
     "invalidation_helper.h",
     "invalidation_interface.cc",
     "invalidation_interface.h",
+    "legacy_directory_deletion.cc",
+    "legacy_directory_deletion.h",
     "logging.cc",
     "logging.h",
     "model_type.cc",

components/sync/base/legacy_directory_deletion.cc

+// Copyright 2020 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/base/legacy_directory_deletion.h"
+
+#include "base/files/file_enumerator.h"
+#include "base/files/file_util.h"
+#include "base/logging.h"
+
+namespace syncer {
+
+// Delete the directory database files from the sync data folder to cleanup
+// all files. The main purpose is to delete the legacy Directory files (sqlite)
+// but it also currently deletes the files corresponding to the modern
+// NigoriStorageImpl.
+void DeleteLegacyDirectoryFilesAndNigoriStorage(
+    const base::FilePath& directory_path) {
+  // We assume that the directory database files are all top level files, and
+  // use no folders. We also assume that there might be child folders under
+  // |directory_path| that are used for non-directory things, like storing
+  // ModelTypeStore/LevelDB data, and we expressly do not want to delete those.
+  if (!base::DirectoryExists(directory_path)) {
+    return;
+  }
+
+  base::FileEnumerator fe(directory_path, false, base::FileEnumerator::FILES);
+  for (base::FilePath current = fe.Next(); !current.empty();
+       current = fe.Next()) {
+    if (!base::DeleteFile(current, false)) {
+      DLOG(ERROR) << "Could not delete all sync directory files.";
+    }
+  }
+}
+
+}  // namespace syncer

components/sync/base/legacy_directory_deletion.h

+// Copyright 2020 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.
+
+#ifndef COMPONENTS_SYNC_BASE_LEGACY_DIRECTORY_DELETION_H_
+#define COMPONENTS_SYNC_BASE_LEGACY_DIRECTORY_DELETION_H_
+
+#include "base/files/file_path.h"
+
+namespace syncer {
+
+// Delete the directory database files from the sync data folder to cleanup
+// all files. The main purpose is to delete the legacy Directory files (sqlite)
+// but it also currently deletes the files corresponding to the modern
+// NigoriStorageImpl.
+void DeleteLegacyDirectoryFilesAndNigoriStorage(
+    const base::FilePath& directory_path);
+
+}  // namespace syncer
+
+#endif  // COMPONENTS_SYNC_BASE_LEGACY_DIRECTORY_DELETION_H_

components/sync/driver/DEPS

@@ -16,7 +16,6 @@ include_rules = [
   "+components/sync/model_impl",
   "+components/sync/nigori",
   "+components/sync/protocol",
-  "+components/sync/syncable",
   "+components/sync/test",
   "+components/sync_preferences",
   "+google/cacheinvalidation",

components/sync/driver/data_type_manager_impl.cc

@@ -459,10 +459,9 @@ void DataTypeManagerImpl::ProcessReconfigure() {
            << " busy.";
 
   // Note: ConfigureImpl is called directly, rather than posted, in order to
-  // ensure that any purging/unapplying/journaling happens while the set of
-  // failed types is still up to date. If stack unwinding were to be done
-  // via PostTask, the failed data types may be reset before the purging was
-  // performed.
+  // ensure that any purging happens while the set of failed types is still up
+  // to date. If stack unwinding were to be done via PostTask, the failed data
+  // types may be reset before the purging was performed.
   state_ = RETRYING;
   needs_reconfigure_ = false;
   ConfigureImpl(last_requested_types_, last_requested_context_);
@@ -558,8 +557,6 @@ ModelTypeSet DataTypeManagerImpl::PrepareConfigureParams(
     ModelTypeConfigurer::ConfigureParams* params) {
   // Divide up the types into their corresponding actions:
   // - Types which are newly enabled are downloaded.
-  // - Types which have encountered a fatal error (fatal_types) are deleted
-  //   from the directory and journaled in the delete journal.
   // - Types which have encountered a cryptographer error (crypto_types) are
   //   unapplied (local state is purged but sync state is not).
   // - All other types not in the routing info (types just disabled) are deleted
@@ -643,14 +640,6 @@ ModelTypeSet DataTypeManagerImpl::PrepareConfigureParams(
     types_to_purge.RemoveAll(unready_types);
   }
 
-  // If a type has already been disabled and unapplied or journaled, it will
-  // not be part of the |types_to_purge| set, and therefore does not need
-  // to be acted on again.
-  ModelTypeSet types_to_journal = Intersection(fatal_types, types_to_purge);
-  ModelTypeSet unapply_types = Union(crypto_types, clean_types);
-  unapply_types.RetainAll(types_to_purge);
-
-  DCHECK(Intersection(downloaded_types_, types_to_journal).Empty());
   DCHECK(Intersection(downloaded_types_, crypto_types).Empty());
   // |downloaded_types_| was already updated to include all enabled types.
   DCHECK(downloaded_types_.HasAll(types_to_download));
@@ -663,8 +652,6 @@ ModelTypeSet DataTypeManagerImpl::PrepareConfigureParams(
   params->disabled_types = disabled_types;
   params->to_download = types_to_download;
   params->to_purge = types_to_purge;
-  params->to_journal = types_to_journal;
-  params->to_unapply = unapply_types;
   params->ready_task = base::BindOnce(&DataTypeManagerImpl::DownloadReady,
                                       weak_ptr_factory_.GetWeakPtr(),
                                       download_types_queue_.front());
@@ -673,7 +660,6 @@ ModelTypeSet DataTypeManagerImpl::PrepareConfigureParams(
 
   DCHECK(Intersection(active_types, types_to_purge).Empty());
   DCHECK(Intersection(active_types, fatal_types).Empty());
-  DCHECK(Intersection(active_types, unapply_types).Empty());
   DCHECK(Intersection(active_types, inactive_types).Empty());
   return Difference(active_types, types_to_download);
 }

components/sync/driver/glue/sync_engine_backend.cc

@@ -17,6 +17,7 @@
 #include "components/invalidation/public/invalidation_util.h"
 #include "components/invalidation/public/topic_invalidation_map.h"
 #include "components/sync/base/invalidation_adapter.h"
+#include "components/sync/base/legacy_directory_deletion.h"
 #include "components/sync/base/sync_base_switches.h"
 #include "components/sync/driver/configure_context.h"
 #include "components/sync/driver/model_type_controller.h"
@@ -35,10 +36,6 @@
 #include "components/sync/nigori/nigori_model_type_processor.h"
 #include "components/sync/nigori/nigori_storage_impl.h"
 #include "components/sync/nigori/nigori_sync_bridge_impl.h"
-#include "components/sync/syncable/directory.h"
-#include "components/sync/syncable/nigori_handler_proxy.h"
-#include "components/sync/syncable/syncable_read_transaction.h"
-#include "components/sync/syncable/user_share.h"
 
 // Helper macros to log with the syncer thread name; useful when there
 // are multiple syncers involved.
@@ -149,8 +146,7 @@ void SyncEngineBackend::OnInitializationComplete(
   ModelTypeSet types_to_purge =
       Difference(ModelTypeSet::All(), GetRoutingInfoTypes(routing_info));
 
-  sync_manager_->PurgeDisabledTypes(types_to_purge, ModelTypeSet(),
-                                    ModelTypeSet());
+  sync_manager_->PurgeDisabledTypes(types_to_purge);
   sync_manager_->ConfigureSyncer(
       reason, new_control_types, SyncManager::SyncFeatureState::INITIALIZING,
       base::BindOnce(&SyncEngineBackend::DoInitialProcessControlTypes,
@@ -311,9 +307,6 @@ void SyncEngineBackend::DoInitialize(SyncEngine::InitParams params) {
       &encryptor_, base::BindRepeating(&Nigori::GenerateScryptSalt),
       params.restored_key_for_bootstrapping,
       params.restored_keystore_key_for_bootstrapping);
-  nigori_handler_proxy_ =
-      std::make_unique<syncable::NigoriHandlerProxy>(&user_share_);
-  sync_encryption_handler_->AddObserver(nigori_handler_proxy_.get());
 
   sync_manager_ = params.sync_manager_factory->CreateSyncManager(name_);
   sync_manager_->AddObserver(this);
@@ -332,9 +325,7 @@ void SyncEngineBackend::DoInitialize(SyncEngine::InitParams params) {
   args.authenticated_account_id = params.authenticated_account_id;
   args.invalidator_client_id = params.invalidator_client_id;
   args.engine_components_factory = std::move(params.engine_components_factory);
-  args.user_share = &user_share_;
   args.encryption_handler = sync_encryption_handler_.get();
-  args.nigori_handler = nigori_handler_proxy_.get();
   args.unrecoverable_error_handler = params.unrecoverable_error_handler;
   args.report_unrecoverable_error_function =
       params.report_unrecoverable_error_function;
@@ -446,9 +437,6 @@ void SyncEngineBackend::ShutdownOnUIThread() {
 void SyncEngineBackend::DoShutdown(ShutdownReason reason) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 
-  if (nigori_handler_proxy_) {
-    sync_encryption_handler_->RemoveObserver(nigori_handler_proxy_.get());
-  }
   // Having no |sync_manager_| means that initialization was failed and NIGORI
   // wasn't connected and started.
   // TODO(crbug.com/922900): this logic seems fragile, maybe initialization and
@@ -462,9 +450,7 @@ void SyncEngineBackend::DoShutdown(ShutdownReason reason) {
   registrar_ = nullptr;
 
   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_);
+    DeleteLegacyDirectoryFilesAndNigoriStorage(sync_data_folder_);
   }
 
   host_.Reset();
@@ -484,11 +470,9 @@ void SyncEngineBackend::DoDestroySyncManager() {
   }
 }
 
-void SyncEngineBackend::DoPurgeDisabledTypes(const ModelTypeSet& to_purge,
-                                             const ModelTypeSet& to_journal,
-                                             const ModelTypeSet& to_unapply) {
+void SyncEngineBackend::DoPurgeDisabledTypes(const ModelTypeSet& to_purge) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-  sync_manager_->PurgeDisabledTypes(to_purge, to_journal, to_unapply);
+  sync_manager_->PurgeDisabledTypes(to_purge);
   if (to_purge.Has(NIGORI)) {
     // We are using USS implementation of Nigori and someone asked us to purge
     // it's data. For regular datatypes it's controlled DataTypeManager, but

components/sync/driver/glue/sync_engine_backend.h

@@ -28,7 +28,6 @@
 #include "components/sync/engine/shutdown_reason.h"
 #include "components/sync/engine/sync_encryption_handler.h"
 #include "components/sync/engine/sync_status_observer.h"
-#include "components/sync/syncable/user_share.h"
 #include "url/gurl.h"
 
 namespace syncer {
@@ -36,12 +35,6 @@ namespace syncer {
 class ModelTypeController;
 class SyncEngineImpl;
 
-namespace syncable {
-
-class NigoriHandlerProxy;
-
-}  // namespace syncable
-
 class SyncEngineBackend : public base::RefCountedThreadSafe<SyncEngineBackend>,
                           public base::trace_event::MemoryDumpProvider,
                           public SyncManager::Observer,
@@ -141,16 +134,14 @@ class SyncEngineBackend : public base::RefCountedThreadSafe<SyncEngineBackend>,
   // The shutdown order is a bit complicated:
   // 1) Call ShutdownOnUIThread() from |frontend_loop_| to request sync manager
   //    to stop as soon as possible.
-  // 2) Post DoShutdown() to sync loop to clean up backend state, save
-  //    directory and destroy sync manager.
+  // 2) Post DoShutdown() to sync loop to clean up backend state and destroy
+  //    sync manager.
   void ShutdownOnUIThread();
   void DoShutdown(ShutdownReason reason);
   void DoDestroySyncManager();
 
   // Configuration methods that must execute on sync loop.
-  void DoPurgeDisabledTypes(const ModelTypeSet& to_purge,
-                            const ModelTypeSet& to_journal,
-                            const ModelTypeSet& to_unapply);
+  void DoPurgeDisabledTypes(const ModelTypeSet& to_purge);
   void DoConfigureSyncer(ModelTypeConfigurer::ConfigureParams params);
   void DoFinishConfigureDataTypes(
       ModelTypeSet types_to_config,
@@ -227,17 +218,9 @@ class SyncEngineBackend : public base::RefCountedThreadSafe<SyncEngineBackend>,
   // Our encryptor, which uses Chrome's encryption functions.
   SystemEncryptor encryptor_;
 
-  // We hold |user_share_| here as a dependency for |sync_encryption_handler_|.
-  // Should outlive |sync_encryption_handler_| and |sync_manager_|.
-  UserShare user_share_;
-
-  // Points to either SyncEncryptionHandlerImpl or NigoriSyncBridgeImpl
-  // depending on whether USS implementation of Nigori is enabled or not.
   // Should outlive |sync_manager_|.
   std::unique_ptr<SyncEncryptionHandler> sync_encryption_handler_;
 
-  std::unique_ptr<syncable::NigoriHandlerProxy> nigori_handler_proxy_;
-
   // The top-level syncapi entry point.  Lives on the sync thread.
   std::unique_ptr<SyncManager> sync_manager_;
 

components/sync/driver/glue/sync_engine_impl.cc

@@ -176,9 +176,8 @@ void SyncEngineImpl::Shutdown(ShutdownReason reason) {
 
 void SyncEngineImpl::ConfigureDataTypes(ConfigureParams params) {
   sync_task_runner_->PostTask(
-      FROM_HERE,
-      base::BindOnce(&SyncEngineBackend::DoPurgeDisabledTypes, backend_,
-                     params.to_purge, params.to_journal, params.to_unapply));
+      FROM_HERE, base::BindOnce(&SyncEngineBackend::DoPurgeDisabledTypes,
+                                backend_, params.to_purge));
   sync_task_runner_->PostTask(
       FROM_HERE, base::BindOnce(&SyncEngineBackend::DoConfigureSyncer, backend_,
                                 std::move(params)));

components/sync/driver/profile_sync_service.cc

@@ -30,6 +30,7 @@
 #include "components/signin/public/identity_manager/identity_manager.h"
 #include "components/signin/public/identity_manager/primary_account_mutator.h"
 #include "components/sync/base/bind_to_task_runner.h"
+#include "components/sync/base/legacy_directory_deletion.h"
 #include "components/sync/base/model_type.h"
 #include "components/sync/base/report_unrecoverable_error.h"
 #include "components/sync/base/stop_source.h"
@@ -48,7 +49,6 @@
 #include "components/sync/engine/polling_constants.h"
 #include "components/sync/engine/sync_encryption_handler.h"
 #include "components/sync/model/sync_error.h"
-#include "components/sync/syncable/directory.h"
 #include "components/version_info/version_info_values.h"
 #include "services/network/public/cpp/shared_url_loader_factory.h"
 
@@ -689,7 +689,7 @@ void ProfileSyncService::ShutdownImpl(ShutdownReason reason) {
       if (backend_task_runner_) {
         backend_task_runner_->PostTask(
             FROM_HERE,
-            base::BindOnce(&syncable::Directory::DeleteDirectoryFiles,
+            base::BindOnce(&DeleteLegacyDirectoryFilesAndNigoriStorage,
                            sync_client_->GetSyncDataPath()));
       }
       sync_prefs_.ClearLocalSyncTransportData();

components/sync/driver/profile_sync_service.h

@@ -257,8 +257,8 @@ class ProfileSyncService : public SyncService,
 
  private:
   // Passed as an argument to StopImpl to control whether or not the sync
-  // engine should clear its data directory when it shuts down. See StopImpl
-  // for more information.
+  // engine should clear its data when it shuts down. See StopImpl for more
+  // information.
   enum SyncStopDataFate {
     KEEP_DATA,
     CLEAR_DATA,

components/sync/engine/fake_sync_manager.cc

@@ -42,12 +42,6 @@ ModelTypeSet FakeSyncManager::GetAndResetPurgedTypes() {
   return purged_types;
 }
 
-ModelTypeSet FakeSyncManager::GetAndResetUnappliedTypes() {
-  ModelTypeSet unapplied_types = unapplied_types_;
-  unapplied_types_.Clear();
-  return unapplied_types;
-}
-
 ModelTypeSet FakeSyncManager::GetAndResetDownloadedTypes() {
   ModelTypeSet downloaded_types = downloaded_types_;
   downloaded_types_.Clear();
@@ -106,16 +100,11 @@ void FakeSyncManager::PurgePartiallySyncedTypes() {
   purged_types_.PutAll(partial_types);
 }
 
-void FakeSyncManager::PurgeDisabledTypes(ModelTypeSet to_purge,
-                                         ModelTypeSet to_journal,
-                                         ModelTypeSet to_unapply) {
+void FakeSyncManager::PurgeDisabledTypes(ModelTypeSet to_purge) {
   // Simulate cleaning up disabled types.
   purged_types_.PutAll(to_purge);
-  unapplied_types_.PutAll(to_unapply);
-  // Types from |to_unapply| should retain their server data and progress
-  // markers.
-  initial_sync_ended_types_.RemoveAll(Difference(to_purge, to_unapply));
-  progress_marker_types_.RemoveAll(Difference(to_purge, to_unapply));
+  initial_sync_ended_types_.RemoveAll(to_purge);
+  progress_marker_types_.RemoveAll(to_purge);
 }
 
 void FakeSyncManager::UpdateCredentials(const SyncCredentials& credentials) {

components/sync/engine/fake_sync_manager.h

@@ -49,11 +49,6 @@ class FakeSyncManager : public SyncManager {
   // call to GetAndResetPurgedTypes(), or since startup if never called.
   ModelTypeSet GetAndResetPurgedTypes();
 
-  // Returns those types that have been unapplied as part of purging disabled
-  // types since the last call to GetAndResetUnappliedTypes, or since startup if
-  // never called.
-  ModelTypeSet GetAndResetUnappliedTypes();
-
   // Returns those types that have been downloaded since the last call to
   // GetAndResetDownloadedTypes(), or since startup if never called.
   ModelTypeSet GetAndResetDownloadedTypes();
@@ -79,9 +74,7 @@ class FakeSyncManager : public SyncManager {
   ModelTypeSet GetTypesWithEmptyProgressMarkerToken(
       ModelTypeSet types) override;
   void PurgePartiallySyncedTypes() override;
-  void PurgeDisabledTypes(ModelTypeSet to_purge,
-                          ModelTypeSet to_journal,
-                          ModelTypeSet to_unapply) override;
+  void PurgeDisabledTypes(ModelTypeSet to_purge) override;
   void UpdateCredentials(const SyncCredentials& credentials) override;
   void InvalidateCredentials() override;
   void StartSyncingNormally(base::Time last_poll_time) override;
@@ -135,8 +128,6 @@ class FakeSyncManager : public SyncManager {
   ModelTypeSet configure_fail_types_;
   // The set of types that have been purged.
   ModelTypeSet purged_types_;
-  // Subset of |purged_types_| that were unapplied.
-  ModelTypeSet unapplied_types_;
   // The set of types that have been downloaded.
   ModelTypeSet downloaded_types_;
 

components/sync/engine/model_type_configurer.h

@@ -32,8 +32,6 @@ class ModelTypeConfigurer {
     ModelTypeSet disabled_types;
     ModelTypeSet to_download;
     ModelTypeSet to_purge;
-    ModelTypeSet to_journal;
-    ModelTypeSet to_unapply;
     // Run when configuration is done with the set of all types that failed
     // configuration (if its argument isn't empty, an error was encountered).
     // TODO(akalin): Use a Delegate class with OnConfigureSuccess,

components/sync/engine/sync_manager.cc

@@ -16,9 +16,7 @@ SyncManager::InitArgs::InitArgs()
     : enable_local_sync_backend(false),
       extensions_activity(nullptr),
       change_delegate(nullptr),
-      user_share(nullptr),
       encryption_handler(nullptr),
-      nigori_handler(nullptr),
       cancelation_signal(nullptr) {}
 
 SyncManager::InitArgs::~InitArgs() {}

components/sync/engine/sync_manager.h

@@ -54,11 +54,6 @@ class SyncCycleSnapshot;
 class SyncStatusObserver;
 class TypeDebugInfoObserver;
 class UnrecoverableErrorHandler;
-struct UserShare;
-
-namespace syncable {
-class NigoriHandler;
-}  // namespace syncable
 
 // SyncManager encapsulates syncable::Directory and serves as the parent of all
 // other objects in the sync API.  If multiple threads interact with the same
@@ -231,15 +226,9 @@ class SyncManager {
 
     std::unique_ptr<EngineComponentsFactory> engine_components_factory;
 
-    // Must outlive SyncManager.
-    UserShare* user_share;
-
     // Must outlive SyncManager.
     SyncEncryptionHandler* encryption_handler;
 
-    // Must outlive SyncManager.
-    syncable::NigoriHandler* nigori_handler;
-
     WeakHandle<UnrecoverableErrorHandler> unrecoverable_error_handler;
     base::RepeatingClosure report_unrecoverable_error_function;
 
@@ -287,13 +276,8 @@ class SyncManager {
   // Returns false if an error occurred, true otherwise.
   virtual void PurgePartiallySyncedTypes() = 0;
 
-  // Purge those disabled types as specified by |to_purge|. |to_journal| and
-  // |to_unapply| specify subsets that require special handling. |to_journal|
-  // types are saved into the delete journal, while |to_unapply| have only
-  // their local data deleted, while their server data is preserved.
-  virtual void PurgeDisabledTypes(ModelTypeSet to_purge,
-                                  ModelTypeSet to_journal,
-                                  ModelTypeSet to_unapply) = 0;
+  // Purge those disabled types as specified by |to_purge|.
+  virtual void PurgeDisabledTypes(ModelTypeSet to_purge) = 0;
 
   // Update tokens that we're using in Sync. Email must stay the same.
   virtual void UpdateCredentials(const SyncCredentials& credentials) = 0;

components/sync/engine_impl/sync_manager_impl.cc

@@ -134,8 +134,8 @@ SyncManagerImpl::SyncManagerImpl(
     const std::string& name,
     network::NetworkConnectionTracker* network_connection_tracker)
     : name_(name),
+      nigori_handler_proxy_(&user_share_),
       network_connection_tracker_(network_connection_tracker),
-      share_(nullptr),
       change_delegate_(nullptr),
       initialized_(false),
       observing_network_connectivity_changes_(false),
@@ -275,9 +275,6 @@ void SyncManagerImpl::Init(InitArgs* args) {
   report_unrecoverable_error_function_ =
       args->report_unrecoverable_error_function;
 
-  DCHECK(args->user_share);
-  share_ = args->user_share;
-
   DCHECK(args->encryption_handler);
   sync_encryption_handler_ = args->encryption_handler;
 
@@ -286,6 +283,7 @@ void SyncManagerImpl::Init(InitArgs* args) {
   // handler in order to receive notifications triggered during encryption
   // startup.
   sync_encryption_handler_->AddObserver(this);
+  sync_encryption_handler_->AddObserver(&nigori_handler_proxy_);
   sync_encryption_handler_->AddObserver(encryption_observer_proxy_.get());
   sync_encryption_handler_->AddObserver(&debug_info_event_listener_);
   sync_encryption_handler_->AddObserver(&js_sync_encryption_handler_observer_);
@@ -301,9 +299,9 @@ void SyncManagerImpl::Init(InitArgs* args) {
 
   DCHECK(backing_store);
 
-  share_->directory = std::make_unique<syncable::Directory>(
+  user_share_.directory = std::make_unique<syncable::Directory>(
       std::move(backing_store), args->unrecoverable_error_handler,
-      report_unrecoverable_error_function_, args->nigori_handler);
+      report_unrecoverable_error_function_, &nigori_handler_proxy_);
 
   DVLOG(1) << "AccountId: " << args->authenticated_account_id;
   if (!OpenDirectory(args)) {
@@ -345,7 +343,7 @@ void SyncManagerImpl::Init(InitArgs* args) {
   }
 
   model_type_registry_ = std::make_unique<ModelTypeRegistry>(
-      args->workers, share_, this, args->cancelation_signal,
+      args->workers, &user_share_, this, args->cancelation_signal,
       sync_encryption_handler_->GetKeystoreKeysHandler());
   sync_encryption_handler_->AddObserver(model_type_registry_.get());
 
@@ -456,8 +454,7 @@ void SyncManagerImpl::StartConfiguration() {
 }
 
 syncable::Directory* SyncManagerImpl::directory() {
-  DCHECK(share_);
-  return share_->directory.get();
+  return user_share_.directory.get();
 }
 
 bool SyncManagerImpl::OpenDirectory(const InitArgs* args) {
@@ -509,18 +506,13 @@ void SyncManagerImpl::PurgePartiallySyncedTypes() {
                                       ModelTypeSet());
 }
 
-void SyncManagerImpl::PurgeDisabledTypes(ModelTypeSet to_purge,
-                                         ModelTypeSet to_journal,
-                                         ModelTypeSet to_unapply) {
+void SyncManagerImpl::PurgeDisabledTypes(ModelTypeSet to_purge) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   DCHECK(initialized_);
   DVLOG(1) << "Purging disabled types:\n\t"
-           << "types to purge: " << ModelTypeSetToString(to_purge) << "\n\t"
-           << "types to journal: " << ModelTypeSetToString(to_journal) << "\n\t"
-           << "types to unapply: " << ModelTypeSetToString(to_unapply);
-  DCHECK(to_purge.HasAll(to_journal));
-  DCHECK(to_purge.HasAll(to_unapply));
-  directory()->PurgeEntriesWithTypeIn(to_purge, to_journal, to_unapply);
+           << "types to purge: " << ModelTypeSetToString(to_purge);
+  directory()->PurgeEntriesWithTypeIn(to_purge, /*to_journal=*/ModelTypeSet(),
+                                      /*to_unapply=*/ModelTypeSet());
 }
 
 void SyncManagerImpl::UpdateCredentials(const SyncCredentials& credentials) {
@@ -594,10 +586,7 @@ void SyncManagerImpl::ShutdownOnSyncThread() {
     directory()->SaveChanges();
   }
 
-  // TODO(crbug.com/922900): can this be replaced with DCHECK(share_)?
-  if (share_) {
-    share_->directory.reset();
-  }
+  user_share_.directory.reset();
 
   change_delegate_ = nullptr;
 
@@ -926,8 +915,7 @@ void SyncManagerImpl::SaveChanges() {
 
 UserShare* SyncManagerImpl::GetUserShare() {
   DCHECK(initialized_);
-  DCHECK(share_);
-  return share_;
+  return &user_share_;
 }
 
 ModelTypeConnector* SyncManagerImpl::GetModelTypeConnector() {

components/sync/engine_impl/sync_manager_impl.h

@@ -30,6 +30,8 @@
 #include "components/sync/js/js_backend.h"
 #include "components/sync/syncable/change_reorder_buffer.h"
 #include "components/sync/syncable/directory_change_delegate.h"
+#include "components/sync/syncable/nigori_handler_proxy.h"
+#include "components/sync/syncable/user_share.h"
 #include "services/network/public/cpp/network_connection_tracker.h"
 
 namespace syncer {
@@ -38,7 +40,6 @@ class Cryptographer;
 class ModelTypeRegistry;
 class SyncCycleContext;
 class TypeDebugInfoObserver;
-struct UserShare;
 
 // SyncManager encapsulates syncable::Directory and serves as the parent of all
 // other objects in the sync API.  If multiple threads interact with the same
@@ -71,9 +72,7 @@ class SyncManagerImpl
   ModelTypeSet GetTypesWithEmptyProgressMarkerToken(
       ModelTypeSet types) override;
   void PurgePartiallySyncedTypes() override;
-  void PurgeDisabledTypes(ModelTypeSet to_purge,
-                          ModelTypeSet to_journal,
-                          ModelTypeSet to_unapply) override;
+  void PurgeDisabledTypes(ModelTypeSet to_purge) override;
   void UpdateCredentials(const SyncCredentials& credentials) override;
   void InvalidateCredentials() override;
   void StartSyncingNormally(base::Time last_poll_time) override;
@@ -235,6 +234,10 @@ class SyncManagerImpl
 
   const std::string name_;
 
+  UserShare user_share_;
+
+  syncable::NigoriHandlerProxy nigori_handler_proxy_;
+
   network::NetworkConnectionTracker* network_connection_tracker_;
 
   SEQUENCE_CHECKER(sequence_checker_);
@@ -250,10 +253,6 @@ class SyncManagerImpl
   // WeakHandle when we construct it.
   WeakHandle<SyncManagerImpl> weak_handle_this_;
 
-  // We give a handle to share_ to clients of the API for use when constructing
-  // any transaction type.
-  UserShare* share_;
-
   // This can be called from any thread, but only between calls to
   // OpenDirectory() and ShutdownOnSyncThread().
   WeakHandle<SyncManager::ChangeObserver> change_observer_;

components/sync/engine_impl/sync_manager_impl_unittest.cc

@@ -791,9 +791,7 @@ class SyncManagerTest : public testing::Test,
     args.enable_local_sync_backend = enable_local_sync_backend;
     args.local_sync_backend_folder = temp_dir_.GetPath();
     args.engine_components_factory.reset(GetFactory());
-    args.user_share = &user_share_;
     args.encryption_handler = &encryption_handler_;
-    args.nigori_handler = &encryption_handler_;
     args.unrecoverable_error_handler =
         MakeWeakHandle(mock_unrecoverable_error_handler_.GetWeakPtr());
     args.cancelation_signal = &cancelation_signal_;
@@ -895,16 +893,6 @@ class SyncManagerTest : public testing::Test,
         trans->GetWrappedTrans());
   }
 
-  PassphraseType GetPassphraseType() {
-    ReadTransaction trans(FROM_HERE, sync_manager_.GetUserShare());
-    return GetPassphraseTypeWithTrans(&trans);
-  }
-
-  PassphraseType GetPassphraseTypeWithTrans(BaseTransaction* trans) {
-    return trans->GetDirectory()->GetNigoriHandler()->GetPassphraseType(
-        trans->GetWrappedTrans());
-  }
-
   void SimulateInvalidatorEnabledForTest(bool is_enabled) {
     DCHECK_CALLED_ON_VALID_SEQUENCE(sync_manager_.sequence_checker_);
     sync_manager_.SetInvalidatorEnabled(is_enabled);
@@ -929,13 +917,6 @@ class SyncManagerTest : public testing::Test,
                 OnCryptographerStateChanged(_, /*has_pending_keys=*/false));
   }
 
-  void SetCustomPassphraseAndCheck(const std::string& passphrase) {
-    EXPECT_CALL(*encryption_observer_,
-                OnPassphraseTypeChanged(PassphraseType::kCustomPassphrase, _));
-    sync_manager_.GetEncryptionHandler()->SetEncryptionPassphrase(passphrase);
-    EXPECT_EQ(PassphraseType::kCustomPassphrase, GetPassphraseType());
-  }
-
   bool HasUnrecoverableError() {
     return mock_unrecoverable_error_handler_.invocation_count() > 0;
   }
@@ -950,7 +931,6 @@ class SyncManagerTest : public testing::Test,
   scoped_refptr<ExtensionsActivity> extensions_activity_;
 
  protected:
-  UserShare user_share_;
   FakeSyncEncryptionHandler encryption_handler_;
   SyncManagerImpl sync_manager_;
   CancelationSignal cancelation_signal_;
@@ -1235,8 +1215,7 @@ TEST_F(SyncManagerTestWithMockScheduler, PurgeDisabledTypes) {
     SetProgressMarkerForType(type, true);
   }
 
-  sync_manager_.PurgeDisabledTypes(disabled_types, ModelTypeSet(),
-                                   ModelTypeSet());
+  sync_manager_.PurgeDisabledTypes(disabled_types);
   // Verify all the disabled types were purged.
   EXPECT_EQ(enabled_types,
             sync_manager_.GetUserShare()->directory->InitialSyncEndedTypes());
@@ -1337,8 +1316,7 @@ TEST_F(SyncManagerTest, PurgeDisabledTypes) {
 
   // Verify all the enabled types remain after cleanup, and all the disabled
   // types were purged.
-  sync_manager_.PurgeDisabledTypes(disabled_types, ModelTypeSet(),
-                                   ModelTypeSet());
+  sync_manager_.PurgeDisabledTypes(disabled_types);
   EXPECT_EQ(enabled_types,
             sync_manager_.GetUserShare()->directory->InitialSyncEndedTypes());
   EXPECT_EQ(disabled_types, sync_manager_.GetTypesWithEmptyProgressMarkerToken(
@@ -1351,155 +1329,13 @@ TEST_F(SyncManagerTest, PurgeDisabledTypes) {
       Difference(ModelTypeSet::All(), disabled_types);
 
   // Verify only the non-disabled types remain after cleanup.
-  sync_manager_.PurgeDisabledTypes(disabled_types, ModelTypeSet(),
-                                   ModelTypeSet());
+  sync_manager_.PurgeDisabledTypes(disabled_types);
   EXPECT_EQ(new_enabled_types,
             sync_manager_.GetUserShare()->directory->InitialSyncEndedTypes());
   EXPECT_EQ(disabled_types, sync_manager_.GetTypesWithEmptyProgressMarkerToken(
                                 ModelTypeSet::All()));
 }
 
-// Test PurgeDisabledTypes properly unapplies types by deleting their local data
-// and preserving their server data and progress marker.
-TEST_F(SyncManagerTest, PurgeUnappliedTypes) {
-  ModelTypeSet unapplied_types = ModelTypeSet(BOOKMARKS, PREFERENCES);
-  ModelTypeSet enabled_types = GetEnabledTypes();
-  ModelTypeSet disabled_types = Difference(ModelTypeSet::All(), enabled_types);
-
-  // The harness should have initialized the enabled_types for us.
-  EXPECT_EQ(enabled_types,
-            sync_manager_.GetUserShare()->directory->InitialSyncEndedTypes());
-
-  // Set progress markers for all types.
-  ModelTypeSet protocol_types = ProtocolTypes();
-  for (ModelType type : protocol_types) {
-    SetProgressMarkerForType(type, true);
-  }
-
-  // Add the following kinds of items:
-  // 1. Fully synced preference.
-  // 2. Locally created preference, server unknown, unsynced
-  // 3. Locally deleted preference, server known, unsynced
-  // 4. Server deleted preference, locally known.
-  // 5. Server created preference, locally unknown, unapplied.
-  // 6. A fully synced bookmark (no unique_client_tag).
-  UserShare* share = sync_manager_.GetUserShare();
-  sync_pb::EntitySpecifics pref_specifics;
-  AddDefaultFieldValue(PREFERENCES, &pref_specifics);
-  sync_pb::EntitySpecifics bm_specifics;
-  AddDefaultFieldValue(BOOKMARKS, &bm_specifics);
-  int pref1_meta =
-      MakeServerNode(share, PREFERENCES, "pref1", "hash1", pref_specifics);
-  int64_t pref2_meta = MakeNodeWithRoot(share, PREFERENCES, "pref2");
-  int pref3_meta =
-      MakeServerNode(share, PREFERENCES, "pref3", "hash3", pref_specifics);
-  int pref4_meta =
-      MakeServerNode(share, PREFERENCES, "pref4", "hash4", pref_specifics);
-  int pref5_meta =
-      MakeServerNode(share, PREFERENCES, "pref5", "hash5", pref_specifics);
-  int bookmark_meta =
-      MakeServerNode(share, BOOKMARKS, "bookmark", "", bm_specifics);
-
-  {
-    syncable::WriteTransaction trans(FROM_HERE, syncable::SYNCER,
-                                     share->directory.get());
-    // Pref's 1 and 2 are already set up properly.
-    // Locally delete pref 3.
-    syncable::MutableEntry pref3(&trans, GET_BY_HANDLE, pref3_meta);
-    pref3.PutIsDel(true);
-    pref3.PutIsUnsynced(true);
-    // Delete pref 4 at the server.
-    syncable::MutableEntry pref4(&trans, GET_BY_HANDLE, pref4_meta);
-    pref4.PutServerIsDel(true);
-    pref4.PutIsUnappliedUpdate(true);
-    pref4.PutServerVersion(2);
-    // Pref 5 is an new unapplied update.
-    syncable::MutableEntry pref5(&trans, GET_BY_HANDLE, pref5_meta);
-    pref5.PutIsUnappliedUpdate(true);
-    pref5.PutIsDel(true);
-    pref5.PutBaseVersion(-1);
-    // Bookmark is already set up properly
-  }
-
-  // Take a snapshot to clear all the dirty bits.
-  share->directory.get()->SaveChanges();
-
-  // Now request a purge for the unapplied types.
-  disabled_types.PutAll(unapplied_types);
-  sync_manager_.PurgeDisabledTypes(disabled_types, ModelTypeSet(),
-                                   unapplied_types);
-
-  // Verify the unapplied types still have progress markers and initial sync
-  // ended after cleanup.
-  EXPECT_TRUE(
-      sync_manager_.GetUserShare()->directory->InitialSyncEndedTypes().HasAll(
-          unapplied_types));
-  EXPECT_TRUE(
-      sync_manager_.GetTypesWithEmptyProgressMarkerToken(unapplied_types)
-          .Empty());
-
-  // Ensure the items were unapplied as necessary.
-  {
-    syncable::ReadTransaction trans(FROM_HERE, share->directory.get());
-    syncable::Entry pref_node(&trans, GET_BY_HANDLE, pref1_meta);
-    ASSERT_TRUE(pref_node.good());
-    EXPECT_TRUE(pref_node.GetKernelCopy().is_dirty());
-    EXPECT_FALSE(pref_node.GetIsUnsynced());
-    EXPECT_TRUE(pref_node.GetIsUnappliedUpdate());
-    EXPECT_TRUE(pref_node.GetIsDel());
-    EXPECT_GT(pref_node.GetServerVersion(), 0);
-    EXPECT_EQ(pref_node.GetBaseVersion(), -1);
-
-    // Pref 2 should just be locally deleted.
-    syncable::Entry pref2_node(&trans, GET_BY_HANDLE, pref2_meta);
-    ASSERT_TRUE(pref2_node.good());
-    EXPECT_TRUE(pref2_node.GetKernelCopy().is_dirty());
-    EXPECT_FALSE(pref2_node.GetIsUnsynced());
-    EXPECT_TRUE(pref2_node.GetIsDel());
-    EXPECT_FALSE(pref2_node.GetIsUnappliedUpdate());
-    EXPECT_TRUE(pref2_node.GetIsDel());
-    EXPECT_EQ(pref2_node.GetServerVersion(), 0);
-    EXPECT_EQ(pref2_node.GetBaseVersion(), -1);
-
-    syncable::Entry pref3_node(&trans, GET_BY_HANDLE, pref3_meta);
-    ASSERT_TRUE(pref3_node.good());
-    EXPECT_TRUE(pref3_node.GetKernelCopy().is_dirty());
-    EXPECT_FALSE(pref3_node.GetIsUnsynced());
-    EXPECT_TRUE(pref3_node.GetIsUnappliedUpdate());
-    EXPECT_TRUE(pref3_node.GetIsDel());
-    EXPECT_GT(pref3_node.GetServerVersion(), 0);
-    EXPECT_EQ(pref3_node.GetBaseVersion(), -1);
-
-    syncable::Entry pref4_node(&trans, GET_BY_HANDLE, pref4_meta);
-    ASSERT_TRUE(pref4_node.good());
-    EXPECT_TRUE(pref4_node.GetKernelCopy().is_dirty());
-    EXPECT_FALSE(pref4_node.GetIsUnsynced());
-    EXPECT_TRUE(pref4_node.GetIsUnappliedUpdate());
-    EXPECT_TRUE(pref4_node.GetIsDel());
-    EXPECT_GT(pref4_node.GetServerVersion(), 0);
-    EXPECT_EQ(pref4_node.GetBaseVersion(), -1);
-
-    // Pref 5 should remain untouched.
-    syncable::Entry pref5_node(&trans, GET_BY_HANDLE, pref5_meta);
-    ASSERT_TRUE(pref5_node.good());
-    EXPECT_FALSE(pref5_node.GetKernelCopy().is_dirty());
-    EXPECT_FALSE(pref5_node.GetIsUnsynced());
-    EXPECT_TRUE(pref5_node.GetIsUnappliedUpdate());
-    EXPECT_TRUE(pref5_node.GetIsDel());
-    EXPECT_GT(pref5_node.GetServerVersion(), 0);
-    EXPECT_EQ(pref5_node.GetBaseVersion(), -1);
-
-    syncable::Entry bookmark_node(&trans, GET_BY_HANDLE, bookmark_meta);
-    ASSERT_TRUE(bookmark_node.good());
-    EXPECT_TRUE(bookmark_node.GetKernelCopy().is_dirty());
-    EXPECT_FALSE(bookmark_node.GetIsUnsynced());
-    EXPECT_TRUE(bookmark_node.GetIsUnappliedUpdate());
-    EXPECT_TRUE(bookmark_node.GetIsDel());
-    EXPECT_GT(bookmark_node.GetServerVersion(), 0);
-    EXPECT_EQ(bookmark_node.GetBaseVersion(), -1);
-  }
-}
-
 // A test harness to exercise the code that processes and passes changes from
 // the "SYNCER"-WriteTransaction destructor, through the SyncManager, to the
 // ChangeProcessor.

components/sync/syncable/directory.cc

@@ -12,7 +12,6 @@
 
 #include "base/base64.h"
 #include "base/bind.h"
-#include "base/files/file_enumerator.h"
 #include "base/guid.h"
 #include "base/logging.h"
 #include "base/metrics/histogram_macros.h"
@@ -417,23 +416,6 @@ bool Directory::ReindexParentId(BaseWriteTransaction* trans,
   return true;
 }
 
-// static
-void Directory::DeleteDirectoryFiles(const base::FilePath& directory_path) {
-  // We assume that the directory database files are all top level files, and
-  // use no folders. We also assume that there might be child folders under
-  // |directory_path| that are used for non-directory things, like storing
-  // ModelTypeStore/LevelDB data, and we expressly do not want to delete those.
-  if (base::DirectoryExists(directory_path)) {
-    base::FileEnumerator fe(directory_path, false, base::FileEnumerator::FILES);
-    for (base::FilePath current = fe.Next(); !current.empty();
-         current = fe.Next()) {
-      if (!base::DeleteFile(current, false)) {
-        LOG(DFATAL) << "Could not delete all sync directory files.";
-      }
-    }
-  }
-}
-
 bool Directory::unrecoverable_error_set(const BaseTransaction* trans) const {
   DCHECK(trans != nullptr);
   return unrecoverable_error_set_;

components/sync/syncable/directory.h

@@ -490,12 +490,6 @@ class Directory {
   Kernel* kernel();
   const Kernel* kernel() const;
 
-  // Delete the directory database files from the sync data folder to cleanup
-  // backend data. This should happen the first time sync is enabled for a user,
-  // to prevent accidentally reusing old sync data, as well as shutdown when the
-  // user is no longer syncing.
-  static void DeleteDirectoryFiles(const base::FilePath& directory_path);
-
  private:
   friend class SyncableDirectoryTest;
   friend class syncer::TestUserShare;