From e444efc7a1f47e72c654f632ed9d6ea8e8daf695 Mon Sep 17 00:00:00 2001 From: "haitaol@chromium.org" Date: Fri, 15 Aug 2014 14:24:27 +0000 Subject: Let SyncBackupManager keep backup data in memory until shutdown. Only persist backup data to disk if shutdown is due to switching to sync. BUG=362679 Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=289709 Review URL: https://codereview.chromium.org/455023003 Cr-Commit-Position: refs/heads/master@{#289840} git-svn-id: svn://svn.chromium.org/chrome/trunk/src@289840 0039d316-1c4b-4281-b951-d872f2087c98 --- chrome/browser/sync/test_profile_sync_service.cc | 5 +- sync/BUILD.gn | 3 + .../internal_components_factory_impl.cc | 17 ++- .../public/internal_components_factory.h | 16 +++ .../public/internal_components_factory_impl.h | 6 +- .../public/test/test_internal_components_factory.h | 17 +-- sync/internal_api/sync_backup_manager.cc | 14 ++- sync/internal_api/sync_backup_manager.h | 1 + sync/internal_api/sync_backup_manager_unittest.cc | 41 ++++++-- sync/internal_api/sync_manager_impl.cc | 4 +- sync/internal_api/sync_manager_impl_unittest.cc | 20 +++- sync/internal_api/sync_rollback_manager.cc | 1 + sync/internal_api/sync_rollback_manager_base.cc | 10 +- sync/internal_api/sync_rollback_manager_base.h | 8 +- .../sync_rollback_manager_base_unittest.cc | 7 +- .../internal_api/sync_rollback_manager_unittest.cc | 22 ++-- .../test/test_internal_components_factory.cc | 19 +++- sync/sync.gyp | 2 + sync/sync_tests.gypi | 1 + .../deferred_on_disk_directory_backing_store.cc | 73 +++++++++++++ .../deferred_on_disk_directory_backing_store.h | 45 ++++++++ ...red_on_disk_directory_backing_store_unittest.cc | 114 +++++++++++++++++++++ 22 files changed, 386 insertions(+), 60 deletions(-) create mode 100644 sync/syncable/deferred_on_disk_directory_backing_store.cc create mode 100644 sync/syncable/deferred_on_disk_directory_backing_store.h create mode 100644 sync/syncable/deferred_on_disk_directory_backing_store_unittest.cc diff --git a/chrome/browser/sync/test_profile_sync_service.cc b/chrome/browser/sync/test_profile_sync_service.cc index 6242dd9..1d46b26 100644 --- a/chrome/browser/sync/test_profile_sync_service.cc +++ b/chrome/browser/sync/test_profile_sync_service.cc @@ -61,8 +61,9 @@ void SyncBackendHostForProfileSyncTest::InitCore( InternalComponentsFactory::Switches factory_switches = options->internal_components_factory->GetSwitches(); options->internal_components_factory.reset( - new TestInternalComponentsFactory(factory_switches, - syncer::STORAGE_IN_MEMORY)); + new TestInternalComponentsFactory( + factory_switches, InternalComponentsFactory::STORAGE_IN_MEMORY, + NULL)); SyncBackendHostImpl::InitCore(options.Pass()); } diff --git a/sync/BUILD.gn b/sync/BUILD.gn index 02d38e1..daa4d5d 100644 --- a/sync/BUILD.gn +++ b/sync/BUILD.gn @@ -311,6 +311,8 @@ source_set("sync_core") { "sessions/sync_session_context.h", "sessions/sync_session.h", "syncable/blob.h", + "syncable/deferred_on_disk_directory_backing_store.cc", + "syncable/deferred_on_disk_directory_backing_store.h", "syncable/directory_backing_store.cc", "syncable/directory_backing_store.h", "syncable/directory.cc", @@ -615,6 +617,7 @@ test("sync_unit_tests") { "sessions/model_type_registry_unittest.cc", "sessions/nudge_tracker_unittest.cc", "sessions/status_controller_unittest.cc", + "syncable/deferred_on_disk_directory_backing_store_unittest.cc", "syncable/directory_backing_store_unittest.cc", "syncable/directory_unittest.cc", "syncable/directory_unittest.h", diff --git a/sync/internal_api/internal_components_factory_impl.cc b/sync/internal_api/internal_components_factory_impl.cc index e764a16..dbc4243 100644 --- a/sync/internal_api/internal_components_factory_impl.cc +++ b/sync/internal_api/internal_components_factory_impl.cc @@ -8,6 +8,7 @@ #include "sync/engine/syncer.h" #include "sync/engine/sync_scheduler_impl.h" #include "sync/sessions/sync_session_context.h" +#include "sync/syncable/deferred_on_disk_directory_backing_store.h" #include "sync/syncable/on_disk_directory_backing_store.h" using base::TimeDelta; @@ -59,9 +60,19 @@ InternalComponentsFactoryImpl::BuildContext( scoped_ptr InternalComponentsFactoryImpl::BuildDirectoryBackingStore( - const std::string& dir_name, const base::FilePath& backing_filepath) { - return scoped_ptr( - new syncable::OnDiskDirectoryBackingStore(dir_name, backing_filepath)); + StorageOption storage, const std::string& dir_name, + const base::FilePath& backing_filepath) { + if (storage == STORAGE_ON_DISK) { + return scoped_ptr( + new syncable::OnDiskDirectoryBackingStore(dir_name, backing_filepath)); + } else if (storage == STORAGE_ON_DISK_DEFERRED) { + return scoped_ptr( + new syncable::DeferredOnDiskDirectoryBackingStore(dir_name, + backing_filepath)); + } else { + NOTREACHED(); + return scoped_ptr(); + } } InternalComponentsFactory::Switches diff --git a/sync/internal_api/public/internal_components_factory.h b/sync/internal_api/public/internal_components_factory.h index f43804a..f077af0 100644 --- a/sync/internal_api/public/internal_components_factory.h +++ b/sync/internal_api/public/internal_components_factory.h @@ -72,6 +72,21 @@ class SYNC_EXPORT InternalComponentsFactory { PreCommitUpdatesPolicy pre_commit_updates_policy; }; + // For selecting the types of storage to use to persist sync data when + // BuildDirectoryBackingStore() is called. + enum StorageOption { + // BuildDirectoryBackingStore should not use persistent on-disk storage. + STORAGE_IN_MEMORY, + // Use this if you want BuildDirectoryBackingStore to create/use a real + // on disk store. + STORAGE_ON_DISK, + // Use this to defer creating on-disk database until + // DirectoryBackingStore::SaveChanges() is called. + STORAGE_ON_DISK_DEFERRED, + // Use this to test the case where a directory fails to load. + STORAGE_INVALID + }; + virtual ~InternalComponentsFactory() {} virtual scoped_ptr BuildScheduler( @@ -90,6 +105,7 @@ class SYNC_EXPORT InternalComponentsFactory { virtual scoped_ptr BuildDirectoryBackingStore( + StorageOption storage, const std::string& dir_name, const base::FilePath& backing_filepath) = 0; diff --git a/sync/internal_api/public/internal_components_factory_impl.h b/sync/internal_api/public/internal_components_factory_impl.h index 7c364dc..c727509 100644 --- a/sync/internal_api/public/internal_components_factory_impl.h +++ b/sync/internal_api/public/internal_components_factory_impl.h @@ -34,9 +34,9 @@ class SYNC_EXPORT InternalComponentsFactoryImpl const std::string& invalidator_client_id) OVERRIDE; virtual scoped_ptr - BuildDirectoryBackingStore( - const std::string& dir_name, - const base::FilePath& backing_filepath) OVERRIDE; + BuildDirectoryBackingStore(StorageOption storage, + const std::string& dir_name, + const base::FilePath& backing_filepath) OVERRIDE; virtual Switches GetSwitches() const OVERRIDE; diff --git a/sync/internal_api/public/test/test_internal_components_factory.h b/sync/internal_api/public/test/test_internal_components_factory.h index 3b7d3c5..0c6b909 100644 --- a/sync/internal_api/public/test/test_internal_components_factory.h +++ b/sync/internal_api/public/test/test_internal_components_factory.h @@ -9,20 +9,11 @@ namespace syncer { -enum StorageOption { - // BuildDirectoryBackingStore should not use persistent on-disk storage. - STORAGE_IN_MEMORY, - // Use this if you want BuildDirectoryBackingStore to create a real - // on disk store. - STORAGE_ON_DISK, - // Use this to test the case where a directory fails to load. - STORAGE_INVALID -}; - class TestInternalComponentsFactory : public InternalComponentsFactory { public: explicit TestInternalComponentsFactory(const Switches& switches, - StorageOption option); + StorageOption option, + StorageOption* storage_used); virtual ~TestInternalComponentsFactory(); virtual scoped_ptr BuildScheduler( @@ -41,6 +32,7 @@ class TestInternalComponentsFactory : public InternalComponentsFactory { virtual scoped_ptr BuildDirectoryBackingStore( + StorageOption storage, const std::string& dir_name, const base::FilePath& backing_filepath) OVERRIDE; @@ -48,7 +40,8 @@ class TestInternalComponentsFactory : public InternalComponentsFactory { private: const Switches switches_; - const StorageOption storage_option_; + const StorageOption storage_override_; + StorageOption* storage_used_; DISALLOW_COPY_AND_ASSIGN(TestInternalComponentsFactory); }; diff --git a/sync/internal_api/sync_backup_manager.cc b/sync/internal_api/sync_backup_manager.cc index be8b872..d626229 100644 --- a/sync/internal_api/sync_backup_manager.cc +++ b/sync/internal_api/sync_backup_manager.cc @@ -23,6 +23,7 @@ void SyncBackupManager::Init(InitArgs* args) { if (SyncRollbackManagerBase::InitInternal( args->database_location, args->internal_components_factory.get(), + InternalComponentsFactory::STORAGE_ON_DISK_DEFERRED, args->unrecoverable_error_handler.Pass(), args->report_unrecoverable_error_function)) { GetUserShare()->directory->CollectMetaHandleCounts( @@ -34,10 +35,8 @@ void SyncBackupManager::Init(InitArgs* args) { } void SyncBackupManager::SaveChanges() { - if (initialized()) { + if (initialized()) NormalizeEntries(); - GetUserShare()->directory->SaveChanges(); - } } SyncStatus SyncBackupManager::GetDetailedStatus() const { @@ -122,6 +121,15 @@ void SyncBackupManager::HideSyncPreference(ModelType type) { } } +void SyncBackupManager::ShutdownOnSyncThread(ShutdownReason reason) { + if (reason == SWITCH_MODE_SYNC) { + NormalizeEntries(); + GetUserShare()->directory->SaveChanges(); + } + + SyncRollbackManagerBase::ShutdownOnSyncThread(reason); +} + void SyncBackupManager::RegisterDirectoryTypeDebugInfoObserver( syncer::TypeDebugInfoObserver* observer) {} diff --git a/sync/internal_api/sync_backup_manager.h b/sync/internal_api/sync_backup_manager.h index f8fa1aa..9a9fdbc 100644 --- a/sync/internal_api/sync_backup_manager.h +++ b/sync/internal_api/sync_backup_manager.h @@ -24,6 +24,7 @@ class SYNC_EXPORT_PRIVATE SyncBackupManager : public SyncRollbackManagerBase { virtual void Init(InitArgs* args) OVERRIDE; virtual void SaveChanges() OVERRIDE; virtual SyncStatus GetDetailedStatus() const OVERRIDE; + virtual void ShutdownOnSyncThread(ShutdownReason reason) OVERRIDE; // DirectoryChangeDelegate implementation. virtual ModelTypeSet HandleTransactionEndingChangeEvent( diff --git a/sync/internal_api/sync_backup_manager_unittest.cc b/sync/internal_api/sync_backup_manager_unittest.cc index e771ea3..7c6c568 100644 --- a/sync/internal_api/sync_backup_manager_unittest.cc +++ b/sync/internal_api/sync_backup_manager_unittest.cc @@ -49,7 +49,8 @@ class SyncBackupManagerTest : public syncer::SyncManager::Observer, CHECK(temp_dir_.CreateUniqueTempDir()); } - void InitManager(SyncManager* manager, StorageOption storage_option) { + void InitManager(SyncManager* manager, + InternalComponentsFactory::StorageOption storage_option) { manager_ = manager; EXPECT_CALL(*this, OnInitializationComplete(_, _, _, _)) .WillOnce(WithArgs<2>(Invoke(this, @@ -65,8 +66,11 @@ class SyncBackupManagerTest : public syncer::SyncManager::Observer, args.service_url = GURL("https://example.com/"); args.post_factory = scoped_ptr().Pass(); args.internal_components_factory.reset(new TestInternalComponentsFactory( - InternalComponentsFactory::Switches(), storage_option)); + InternalComponentsFactory::Switches(), storage_option, + &storage_used_)); manager->Init(&args); + EXPECT_EQ(InternalComponentsFactory::STORAGE_ON_DISK_DEFERRED, + storage_used_); loop_.PostTask(FROM_HERE, run_loop.QuitClosure()); run_loop.Run(); } @@ -82,7 +86,6 @@ class SyncBackupManagerTest : public syncer::SyncManager::Observer, node.InitUniqueByCreation(type, type_root, client_tag)); } - private: void ConfigureSyncer() { manager_->ConfigureSyncer(CONFIGURE_REASON_NEW_CLIENT, ModelTypeSet(SEARCH_ENGINES), @@ -105,11 +108,12 @@ class SyncBackupManagerTest : public syncer::SyncManager::Observer, base::ScopedTempDir temp_dir_; base::MessageLoop loop_; // Needed for WeakHandle SyncManager* manager_; + InternalComponentsFactory::StorageOption storage_used_; }; -TEST_F(SyncBackupManagerTest, NormalizeAndPersist) { +TEST_F(SyncBackupManagerTest, NormalizeEntry) { scoped_ptr manager(new SyncBackupManager); - InitManager(manager.get(), STORAGE_ON_DISK); + InitManager(manager.get(), InternalComponentsFactory::STORAGE_IN_MEMORY); CreateEntry(manager->GetUserShare(), SEARCH_ENGINES, "test"); @@ -134,11 +138,20 @@ TEST_F(SyncBackupManagerTest, NormalizeAndPersist) { EXPECT_TRUE(pref.GetEntry()->GetId().ServerKnows()); EXPECT_FALSE(pref.GetEntry()->GetIsUnsynced()); } - manager->ShutdownOnSyncThread(STOP_SYNC); +} + +TEST_F(SyncBackupManagerTest, PersistWithSwitchToSyncShutdown) { + scoped_ptr manager(new SyncBackupManager); + InitManager(manager.get(), + InternalComponentsFactory::STORAGE_ON_DISK_DEFERRED); + + CreateEntry(manager->GetUserShare(), SEARCH_ENGINES, "test"); + manager->SaveChanges(); + manager->ShutdownOnSyncThread(SWITCH_MODE_SYNC); // Reopen db to verify entry is persisted. manager.reset(new SyncBackupManager); - InitManager(manager.get(), STORAGE_ON_DISK); + InitManager(manager.get(), InternalComponentsFactory::STORAGE_ON_DISK); { ReadTransaction trans(FROM_HERE, manager->GetUserShare()); ReadNode pref(&trans); @@ -149,10 +162,22 @@ TEST_F(SyncBackupManagerTest, NormalizeAndPersist) { } } +TEST_F(SyncBackupManagerTest, DontPersistWithOtherShutdown) { + scoped_ptr manager(new SyncBackupManager); + InitManager(manager.get(), + InternalComponentsFactory::STORAGE_ON_DISK_DEFERRED); + + CreateEntry(manager->GetUserShare(), SEARCH_ENGINES, "test"); + manager->SaveChanges(); + manager->ShutdownOnSyncThread(STOP_SYNC); + EXPECT_FALSE(base::PathExists( + temp_dir_.path().Append(syncable::Directory::kSyncDatabaseFilename))); +} + TEST_F(SyncBackupManagerTest, FailToInitialize) { // Test graceful shutdown on initialization failure. scoped_ptr manager(new SyncBackupManager); - InitManager(manager.get(), STORAGE_INVALID); + InitManager(manager.get(), InternalComponentsFactory::STORAGE_INVALID); } } // anonymous namespace diff --git a/sync/internal_api/sync_manager_impl.cc b/sync/internal_api/sync_manager_impl.cc index 13b6517..e12ec8f 100644 --- a/sync/internal_api/sync_manager_impl.cc +++ b/sync/internal_api/sync_manager_impl.cc @@ -345,8 +345,8 @@ void SyncManagerImpl::Init(InitArgs* args) { scoped_ptr backing_store = args->internal_components_factory->BuildDirectoryBackingStore( - args->credentials.email, - absolute_db_path).Pass(); + InternalComponentsFactory::STORAGE_ON_DISK, + args->credentials.email, absolute_db_path).Pass(); DCHECK(backing_store.get()); share_.directory.reset( diff --git a/sync/internal_api/sync_manager_impl_unittest.cc b/sync/internal_api/sync_manager_impl_unittest.cc index 5518d4f..4571d08 100644 --- a/sync/internal_api/sync_manager_impl_unittest.cc +++ b/sync/internal_api/sync_manager_impl_unittest.cc @@ -834,6 +834,8 @@ class SyncManagerTest : public testing::Test, sync_manager_.GetEncryptionHandler()->AddObserver(&encryption_observer_); EXPECT_TRUE(js_backend_.IsInitialized()); + EXPECT_EQ(InternalComponentsFactory::STORAGE_ON_DISK, + storage_used_); if (initialization_succeeded_) { for (ModelSafeRoutingInfo::iterator i = routing_info.begin(); @@ -951,7 +953,9 @@ class SyncManagerTest : public testing::Test, } virtual InternalComponentsFactory* GetFactory() { - return new TestInternalComponentsFactory(GetSwitches(), STORAGE_IN_MEMORY); + return new TestInternalComponentsFactory( + GetSwitches(), InternalComponentsFactory::STORAGE_IN_MEMORY, + &storage_used_); } // Returns true if we are currently encrypting all sync data. May @@ -1011,6 +1015,7 @@ class SyncManagerTest : public testing::Test, StrictMock manager_observer_; StrictMock encryption_observer_; InternalComponentsFactory::Switches switches_; + InternalComponentsFactory::StorageOption storage_used_; }; TEST_F(SyncManagerTest, GetAllNodesForTypeTest) { @@ -2409,8 +2414,10 @@ class ComponentsFactory : public TestInternalComponentsFactory { public: ComponentsFactory(const Switches& switches, SyncScheduler* scheduler_to_use, - sessions::SyncSessionContext** session_context) - : TestInternalComponentsFactory(switches, syncer::STORAGE_IN_MEMORY), + sessions::SyncSessionContext** session_context, + InternalComponentsFactory::StorageOption* storage_used) + : TestInternalComponentsFactory( + switches, InternalComponentsFactory::STORAGE_IN_MEMORY, storage_used), scheduler_to_use_(scheduler_to_use), session_context_(session_context) {} virtual ~ComponentsFactory() {} @@ -2433,7 +2440,8 @@ class SyncManagerTestWithMockScheduler : public SyncManagerTest { SyncManagerTestWithMockScheduler() : scheduler_(NULL) {} virtual InternalComponentsFactory* GetFactory() OVERRIDE { scheduler_ = new MockSyncScheduler(); - return new ComponentsFactory(GetSwitches(), scheduler_, &session_context_); + return new ComponentsFactory(GetSwitches(), scheduler_, &session_context_, + &storage_used_); } MockSyncScheduler* scheduler() { return scheduler_; } @@ -3177,7 +3185,9 @@ class SyncManagerInitInvalidStorageTest : public SyncManagerTest { } virtual InternalComponentsFactory* GetFactory() OVERRIDE { - return new TestInternalComponentsFactory(GetSwitches(), STORAGE_INVALID); + return new TestInternalComponentsFactory( + GetSwitches(), InternalComponentsFactory::STORAGE_INVALID, + &storage_used_); } }; diff --git a/sync/internal_api/sync_rollback_manager.cc b/sync/internal_api/sync_rollback_manager.cc index b4100c7..e7b8ea6 100644 --- a/sync/internal_api/sync_rollback_manager.cc +++ b/sync/internal_api/sync_rollback_manager.cc @@ -26,6 +26,7 @@ void SyncRollbackManager::Init(InitArgs* args) { if (SyncRollbackManagerBase::InitInternal( args->database_location, args->internal_components_factory.get(), + InternalComponentsFactory::STORAGE_ON_DISK, args->unrecoverable_error_handler.Pass(), args->report_unrecoverable_error_function)) { change_delegate_ = args->change_delegate; diff --git a/sync/internal_api/sync_rollback_manager_base.cc b/sync/internal_api/sync_rollback_manager_base.cc index 67e152b..a2f14e4 100644 --- a/sync/internal_api/sync_rollback_manager_base.cc +++ b/sync/internal_api/sync_rollback_manager_base.cc @@ -5,7 +5,6 @@ #include "sync/internal_api/sync_rollback_manager_base.h" #include "sync/internal_api/public/base/model_type.h" -#include "sync/internal_api/public/internal_components_factory.h" #include "sync/internal_api/public/read_node.h" #include "sync/internal_api/public/read_transaction.h" #include "sync/internal_api/public/write_transaction.h" @@ -53,12 +52,13 @@ SyncRollbackManagerBase::~SyncRollbackManagerBase() { bool SyncRollbackManagerBase::InitInternal( const base::FilePath& database_location, InternalComponentsFactory* internal_components_factory, + InternalComponentsFactory::StorageOption storage, scoped_ptr unrecoverable_error_handler, ReportUnrecoverableErrorFunction report_unrecoverable_error_function) { unrecoverable_error_handler_ = unrecoverable_error_handler.Pass(); report_unrecoverable_error_function_ = report_unrecoverable_error_function; - if (!InitBackupDB(database_location, internal_components_factory)) { + if (!InitBackupDB(database_location, internal_components_factory, storage)) { NotifyInitializationFailure(); return false; } @@ -140,7 +140,6 @@ void SyncRollbackManagerBase::SaveChanges() { void SyncRollbackManagerBase::ShutdownOnSyncThread(ShutdownReason reason) { if (initialized_) { - SaveChanges(); share_.directory->Close(); share_.directory.reset(); initialized_ = false; @@ -239,12 +238,13 @@ scoped_ptr SyncRollbackManagerBase::GetAllNodesForType( bool SyncRollbackManagerBase::InitBackupDB( const base::FilePath& sync_folder, - InternalComponentsFactory* internal_components_factory) { + InternalComponentsFactory* internal_components_factory, + InternalComponentsFactory::StorageOption storage) { base::FilePath backup_db_path = sync_folder.Append( syncable::Directory::kSyncDatabaseFilename); scoped_ptr backing_store = internal_components_factory->BuildDirectoryBackingStore( - "backup", backup_db_path).Pass(); + storage, "backup", backup_db_path).Pass(); DCHECK(backing_store.get()); share_.directory.reset( diff --git a/sync/internal_api/sync_rollback_manager_base.h b/sync/internal_api/sync_rollback_manager_base.h index b6ffcd7..b92cafb 100644 --- a/sync/internal_api/sync_rollback_manager_base.h +++ b/sync/internal_api/sync_rollback_manager_base.h @@ -10,6 +10,7 @@ #include "sync/base/sync_export.h" #include "sync/internal_api/public/http_post_provider_factory.h" +#include "sync/internal_api/public/internal_components_factory.h" #include "sync/internal_api/public/sync_manager.h" #include "sync/internal_api/public/user_share.h" #include "sync/syncable/directory_change_delegate.h" @@ -99,6 +100,7 @@ class SYNC_EXPORT_PRIVATE SyncRollbackManagerBase : bool InitInternal( const base::FilePath& database_location, InternalComponentsFactory* internal_components_factory, + InternalComponentsFactory::StorageOption storage, scoped_ptr unrecoverable_error_handler, ReportUnrecoverableErrorFunction report_unrecoverable_error_function); @@ -118,9 +120,9 @@ class SYNC_EXPORT_PRIVATE SyncRollbackManagerBase : void NotifyInitializationSuccess(); void NotifyInitializationFailure(); - bool InitBackupDB( - const base::FilePath& sync_folder, - InternalComponentsFactory* internal_components_factory); + bool InitBackupDB(const base::FilePath& sync_folder, + InternalComponentsFactory* internal_components_factory, + InternalComponentsFactory::StorageOption storage); bool InitTypeRootNode(ModelType type); void InitBookmarkFolder(const std::string& folder); diff --git a/sync/internal_api/sync_rollback_manager_base_unittest.cc b/sync/internal_api/sync_rollback_manager_base_unittest.cc index a0866ea..95662f6 100644 --- a/sync/internal_api/sync_rollback_manager_base_unittest.cc +++ b/sync/internal_api/sync_rollback_manager_base_unittest.cc @@ -25,6 +25,7 @@ class SyncTestRollbackManager : public SyncRollbackManagerBase { SyncRollbackManagerBase::InitInternal( args->database_location, args->internal_components_factory.get(), + InternalComponentsFactory::STORAGE_IN_MEMORY, args->unrecoverable_error_handler.Pass(), args->report_unrecoverable_error_function); } @@ -37,12 +38,16 @@ class SyncRollbackManagerBaseTest : public testing::Test { args.database_location = base::FilePath(base::FilePath::kCurrentDirectory); args.service_url = GURL("https://example.com/"); args.internal_components_factory.reset(new TestInternalComponentsFactory( - InternalComponentsFactory::Switches(), STORAGE_IN_MEMORY)); + InternalComponentsFactory::Switches(), + InternalComponentsFactory::STORAGE_IN_MEMORY, + &storage_used_)); manager_.Init(&args); + EXPECT_EQ(InternalComponentsFactory::STORAGE_IN_MEMORY, storage_used_); } SyncTestRollbackManager manager_; base::MessageLoop loop_; // Needed for WeakHandle + InternalComponentsFactory::StorageOption storage_used_; }; TEST_F(SyncRollbackManagerBaseTest, InitTypeOnConfiguration) { diff --git a/sync/internal_api/sync_rollback_manager_unittest.cc b/sync/internal_api/sync_rollback_manager_unittest.cc index b243552..ebf9fed 100644 --- a/sync/internal_api/sync_rollback_manager_unittest.cc +++ b/sync/internal_api/sync_rollback_manager_unittest.cc @@ -102,7 +102,8 @@ class SyncRollbackManagerTest : public testing::Test, } void InitManager(SyncManager* manager, ModelTypeSet types, - TestChangeDelegate* delegate, StorageOption storage_option) { + TestChangeDelegate* delegate, + InternalComponentsFactory::StorageOption storage_option) { manager_ = manager; types_ = types; @@ -118,9 +119,12 @@ class SyncRollbackManagerTest : public testing::Test, args.service_url = GURL("https://example.com/"); args.workers.push_back(worker_); args.change_delegate = delegate; + + InternalComponentsFactory::StorageOption storage_used; args.internal_components_factory.reset(new TestInternalComponentsFactory( - InternalComponentsFactory::Switches(), storage_option)); + InternalComponentsFactory::Switches(), storage_option, &storage_used)); manager->Init(&args); + EXPECT_EQ(storage_option, storage_used); loop_.PostTask(FROM_HERE, run_loop.QuitClosure()); run_loop.Run(); } @@ -130,9 +134,9 @@ class SyncRollbackManagerTest : public testing::Test, SyncBackupManager backup_manager; TestChangeDelegate delegate; InitManager(&backup_manager, ModelTypeSet(type), &delegate, - STORAGE_ON_DISK); + InternalComponentsFactory::STORAGE_ON_DISK_DEFERRED); CreateEntry(backup_manager.GetUserShare(), type, client_tag); - backup_manager.ShutdownOnSyncThread(STOP_SYNC); + backup_manager.ShutdownOnSyncThread(SWITCH_MODE_SYNC); } // Verify entry with |client_tag| exists in sync directory. @@ -183,7 +187,7 @@ TEST_F(SyncRollbackManagerTest, RollbackBasic) { TestChangeDelegate delegate; SyncRollbackManager rollback_manager; InitManager(&rollback_manager, ModelTypeSet(PREFERENCES), &delegate, - STORAGE_ON_DISK); + InternalComponentsFactory::STORAGE_ON_DISK); // Simulate a new entry added during type initialization. int64 new_pref_id = @@ -207,7 +211,7 @@ TEST_F(SyncRollbackManagerTest, NoRollbackOfTypesNotBackedUp) { TestChangeDelegate delegate; SyncRollbackManager rollback_manager; InitManager(&rollback_manager, ModelTypeSet(PREFERENCES, APPS), &delegate, - STORAGE_ON_DISK); + InternalComponentsFactory::STORAGE_ON_DISK); // Simulate new entry added during type initialization. int64 new_pref_id = @@ -235,7 +239,7 @@ TEST_F(SyncRollbackManagerTest, BackupDbNotChangedOnAbort) { scoped_ptr rollback_manager( new SyncRollbackManager); InitManager(rollback_manager.get(), ModelTypeSet(PREFERENCES), &delegate, - STORAGE_ON_DISK); + InternalComponentsFactory::STORAGE_ON_DISK); // Simulate a new entry added during type initialization. CreateEntry(rollback_manager->GetUserShare(), PREFERENCES, "pref2"); @@ -246,7 +250,7 @@ TEST_F(SyncRollbackManagerTest, BackupDbNotChangedOnAbort) { // Verify new entry was not persisted. rollback_manager.reset(new SyncRollbackManager); InitManager(rollback_manager.get(), ModelTypeSet(PREFERENCES), &delegate, - STORAGE_ON_DISK); + InternalComponentsFactory::STORAGE_ON_DISK); EXPECT_FALSE(VerifyEntry(rollback_manager->GetUserShare(), PREFERENCES, "pref2")); } @@ -256,7 +260,7 @@ TEST_F(SyncRollbackManagerTest, OnInitializationFailure) { scoped_ptr rollback_manager( new SyncRollbackManager); InitManager(rollback_manager.get(), ModelTypeSet(PREFERENCES), NULL, - STORAGE_ON_DISK); + InternalComponentsFactory::STORAGE_ON_DISK); } } // anonymous namespace diff --git a/sync/internal_api/test/test_internal_components_factory.cc b/sync/internal_api/test/test_internal_components_factory.cc index c7d94e9..34c678b 100644 --- a/sync/internal_api/test/test_internal_components_factory.cc +++ b/sync/internal_api/test/test_internal_components_factory.cc @@ -5,6 +5,7 @@ #include "sync/internal_api/public/test/test_internal_components_factory.h" #include "sync/sessions/sync_session_context.h" +#include "sync/syncable/deferred_on_disk_directory_backing_store.h" #include "sync/syncable/in_memory_directory_backing_store.h" #include "sync/syncable/on_disk_directory_backing_store.h" #include "sync/syncable/invalid_directory_backing_store.h" @@ -14,9 +15,11 @@ namespace syncer { TestInternalComponentsFactory::TestInternalComponentsFactory( const Switches& switches, - StorageOption option) + StorageOption option, + StorageOption* storage_used) : switches_(switches), - storage_option_(option) { + storage_override_(option), + storage_used_(storage_used) { } TestInternalComponentsFactory::~TestInternalComponentsFactory() { } @@ -54,8 +57,12 @@ TestInternalComponentsFactory::BuildContext( scoped_ptr TestInternalComponentsFactory::BuildDirectoryBackingStore( - const std::string& dir_name, const base::FilePath& backing_filepath) { - switch (storage_option_) { + StorageOption storage, const std::string& dir_name, + const base::FilePath& backing_filepath) { + if (storage_used_) + *storage_used_ = storage; + + switch (storage_override_) { case STORAGE_IN_MEMORY: return scoped_ptr( new syncable::InMemoryDirectoryBackingStore(dir_name)); @@ -63,6 +70,10 @@ TestInternalComponentsFactory::BuildDirectoryBackingStore( return scoped_ptr( new syncable::OnDiskDirectoryBackingStore(dir_name, backing_filepath)); + case STORAGE_ON_DISK_DEFERRED: + return scoped_ptr( + new syncable::DeferredOnDiskDirectoryBackingStore(dir_name, + backing_filepath)); case STORAGE_INVALID: return scoped_ptr( new syncable::InvalidDirectoryBackingStore()); diff --git a/sync/sync.gyp b/sync/sync.gyp index ad8fd34..9ced955 100644 --- a/sync/sync.gyp +++ b/sync/sync.gyp @@ -353,6 +353,8 @@ 'sessions/sync_session_context.cc', 'sessions/sync_session_context.h', 'syncable/blob.h', + 'syncable/deferred_on_disk_directory_backing_store.cc', + 'syncable/deferred_on_disk_directory_backing_store.h', 'syncable/dir_open_result.h', 'syncable/directory.cc', 'syncable/directory.h', diff --git a/sync/sync_tests.gypi b/sync/sync_tests.gypi index b7b68c3..ddcdf25 100644 --- a/sync/sync_tests.gypi +++ b/sync/sync_tests.gypi @@ -317,6 +317,7 @@ 'sessions/model_type_registry_unittest.cc', 'sessions/nudge_tracker_unittest.cc', 'sessions/status_controller_unittest.cc', + 'syncable/deferred_on_disk_directory_backing_store_unittest.cc', 'syncable/directory_backing_store_unittest.cc', 'syncable/directory_unittest.cc', 'syncable/directory_unittest.h', diff --git a/sync/syncable/deferred_on_disk_directory_backing_store.cc b/sync/syncable/deferred_on_disk_directory_backing_store.cc new file mode 100644 index 0000000..0367e5b --- /dev/null +++ b/sync/syncable/deferred_on_disk_directory_backing_store.cc @@ -0,0 +1,73 @@ +// Copyright 2014 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 "sync/syncable/deferred_on_disk_directory_backing_store.h" + +#include "base/logging.h" +#include "base/metrics/histogram.h" +#include "base/stl_util.h" +#include "sync/syncable/syncable-inl.h" + +namespace syncer { +namespace syncable { + +DeferredOnDiskDirectoryBackingStore::DeferredOnDiskDirectoryBackingStore( + const std::string& dir_name, const base::FilePath& backing_filepath) + : DirectoryBackingStore(dir_name), + backing_filepath_(backing_filepath), + db_is_on_disk_(false) { +} + +DeferredOnDiskDirectoryBackingStore::~DeferredOnDiskDirectoryBackingStore() {} + +bool DeferredOnDiskDirectoryBackingStore::SaveChanges( + const Directory::SaveChangesSnapshot& snapshot) { + DCHECK(CalledOnValidThread()); + + // Back out early if there is nothing to save. + if (snapshot.dirty_metas.empty() && snapshot.metahandles_to_purge.empty() && + snapshot.delete_journals.empty() && + snapshot.delete_journals_to_purge.empty()) { + return true; + } + + if (!db_is_on_disk_) { + if (!base::DeleteFile(backing_filepath_, false)) + return false; + + // Reopen DB on disk. + db_.reset(new sql::Connection); + db_->set_exclusive_locking(); + db_->set_page_size(4096); + if (!db_->Open(backing_filepath_) || !InitializeTables()) + return false; + + db_is_on_disk_ = true; + } + + return DirectoryBackingStore::SaveChanges(snapshot); +} + +DirOpenResult DeferredOnDiskDirectoryBackingStore::Load( + Directory::MetahandlesMap* handles_map, + JournalIndex* delete_journals, + Directory::KernelLoadInfo* kernel_load_info) { + // Open an in-memory database at first to create initial sync data needed by + // Directory. + CHECK(!db_->is_open()); + if (!db_->OpenInMemory()) + return FAILED_OPEN_DATABASE; + + if (!InitializeTables()) + return FAILED_OPEN_DATABASE; + if (!LoadEntries(handles_map)) + return FAILED_DATABASE_CORRUPT; + if (!LoadInfo(kernel_load_info)) + return FAILED_DATABASE_CORRUPT; + + return OPENED; +} + +} // namespace syncable +} // namespace syncer diff --git a/sync/syncable/deferred_on_disk_directory_backing_store.h b/sync/syncable/deferred_on_disk_directory_backing_store.h new file mode 100644 index 0000000..9e6429a --- /dev/null +++ b/sync/syncable/deferred_on_disk_directory_backing_store.h @@ -0,0 +1,45 @@ +// Copyright 2014 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 SYNC_SYNCABLE_DEFERRED_ON_DISK_DIRECTORY_BACKING_STORE_H_ +#define SYNC_SYNCABLE_DEFERRED_ON_DISK_DIRECTORY_BACKING_STORE_H_ + +#include "base/files/file_path.h" +#include "sync/base/sync_export.h" +#include "sync/syncable/directory_backing_store.h" + +namespace syncer { +namespace syncable { + +// Store used for backing up user's data before first sync. It creates an +// in-memory store first and switch to on-disk store if SaveChanges() is +// called, which only happens when SyncBackupManager is shut down and a +// syncing backend is to be created. Thus we guarantee that user data is not +// persisted until user is actually going to sync. +class SYNC_EXPORT_PRIVATE DeferredOnDiskDirectoryBackingStore + : public DirectoryBackingStore { + public: + DeferredOnDiskDirectoryBackingStore(const std::string& dir_name, + const base::FilePath& backing_filepath); + virtual ~DeferredOnDiskDirectoryBackingStore(); + virtual DirOpenResult Load( + Directory::MetahandlesMap* handles_map, + JournalIndex* delete_journals, + Directory::KernelLoadInfo* kernel_load_info) OVERRIDE; + virtual bool SaveChanges(const Directory::SaveChangesSnapshot& snapshot) + OVERRIDE; + + private: + base::FilePath backing_filepath_; + + // Whether current DB is on disk. + bool db_is_on_disk_; + + DISALLOW_COPY_AND_ASSIGN(DeferredOnDiskDirectoryBackingStore); +}; + +} // namespace syncable +} // namespace syncer + +#endif // SYNC_SYNCABLE_DEFERRED_ON_DISK_DIRECTORY_BACKING_STORE_H_ diff --git a/sync/syncable/deferred_on_disk_directory_backing_store_unittest.cc b/sync/syncable/deferred_on_disk_directory_backing_store_unittest.cc new file mode 100644 index 0000000..b1053b8 --- /dev/null +++ b/sync/syncable/deferred_on_disk_directory_backing_store_unittest.cc @@ -0,0 +1,114 @@ +// Copyright 2014 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 "base/files/scoped_temp_dir.h" +#include "base/stl_util.h" +#include "sync/syncable/deferred_on_disk_directory_backing_store.h" +#include "sync/syncable/directory.h" +#include "sync/syncable/entry_kernel.h" +#include "sync/syncable/on_disk_directory_backing_store.h" +#include "sync/syncable/syncable_delete_journal.h" +#include "testing/gtest/include/gtest/gtest.h" + +namespace syncer { +namespace syncable { +namespace { + +static const base::FilePath::CharType kSyncDataFolderName[] = + FILE_PATH_LITERAL("Sync Data"); + +class DeferredOnDiskDirectoryBackingStoreTest : public testing::Test { + protected: + virtual void SetUp() OVERRIDE { + CHECK(temp_dir_.CreateUniqueTempDir()); + db_path_ = temp_dir_.path().Append(kSyncDataFolderName); + } + + virtual void TearDown() OVERRIDE { + STLDeleteValues(&handles_map_); + } + + base::ScopedTempDir temp_dir_; + base::FilePath db_path_; + Directory::MetahandlesMap handles_map_; + JournalIndex delete_journals_; + Directory::KernelLoadInfo kernel_load_info_; +}; + +// Test initialization of root entry when calling Load(). +TEST_F(DeferredOnDiskDirectoryBackingStoreTest, Load) { + DeferredOnDiskDirectoryBackingStore store("test", db_path_); + EXPECT_EQ(OPENED, store.Load(&handles_map_, &delete_journals_, + &kernel_load_info_)); + EXPECT_TRUE(delete_journals_.empty()); + ASSERT_EQ(1u, handles_map_.size()); // root node + ASSERT_TRUE(handles_map_.count(1)); + EntryKernel* root = handles_map_[1]; + EXPECT_TRUE(root->ref(ID).IsRoot()); + EXPECT_EQ(1, root->ref(META_HANDLE)); + EXPECT_TRUE(root->ref(IS_DIR)); +} + +// Test on-disk DB is not created if SaveChanges() is not called. +TEST_F(DeferredOnDiskDirectoryBackingStoreTest, + DontPersistIfSavingChangesNotCalled) { + { + // Open and close. + DeferredOnDiskDirectoryBackingStore store("test", db_path_); + EXPECT_EQ(OPENED, store.Load(&handles_map_, &delete_journals_, + &kernel_load_info_)); + } + + EXPECT_FALSE(base::PathExists(db_path_)); +} + +// Test on-disk DB is not created when there are no changes. +TEST_F(DeferredOnDiskDirectoryBackingStoreTest, + DontPersistWhenSavingNoChanges) { + { + // Open and close. + DeferredOnDiskDirectoryBackingStore store("test", db_path_); + EXPECT_EQ(OPENED, store.Load(&handles_map_, &delete_journals_, + &kernel_load_info_)); + + Directory::SaveChangesSnapshot snapshot; + store.SaveChanges(snapshot); + } + + EXPECT_FALSE(base::PathExists(db_path_)); +} + +// Test changes are persisted to disk when SaveChanges() is called. +TEST_F(DeferredOnDiskDirectoryBackingStoreTest, PersistWhenSavingValidChanges) { + { + // Open and close. + DeferredOnDiskDirectoryBackingStore store("test", db_path_); + EXPECT_EQ(OPENED, store.Load(&handles_map_, &delete_journals_, + &kernel_load_info_)); + + Directory::SaveChangesSnapshot snapshot; + EntryKernel* entry = new EntryKernel(); + entry->put(ID, Id::CreateFromClientString("test_entry")); + entry->put(META_HANDLE, 2); + entry->mark_dirty(NULL); + snapshot.dirty_metas.insert(entry); + store.SaveChanges(snapshot); + } + + STLDeleteValues(&handles_map_); + + ASSERT_TRUE(base::PathExists(db_path_)); + OnDiskDirectoryBackingStore read_store("test", db_path_); + EXPECT_EQ(OPENED, read_store.Load(&handles_map_, &delete_journals_, + &kernel_load_info_)); + ASSERT_EQ(2u, handles_map_.size()); + ASSERT_TRUE(handles_map_.count(1)); // root node + ASSERT_TRUE(handles_map_.count(2)); + EntryKernel* entry = handles_map_[2]; + EXPECT_EQ(Id::CreateFromClientString("test_entry"), entry->ref(ID)); +} + +} // namespace +} // namespace syncable +} // namespace syncer -- cgit v1.1