diff --git a/components/brave_sync/brave_sync_prefs.cc b/components/brave_sync/brave_sync_prefs.cc index 589a103a68b0..113ce85324ff 100644 --- a/components/brave_sync/brave_sync_prefs.cc +++ b/components/brave_sync/brave_sync_prefs.cc @@ -14,6 +14,7 @@ namespace prefs { const char kSyncDeviceId[] = "brave_sync.device_id"; const char kSyncSeed[] = "brave_sync.seed"; +const char kSyncPrevSeed[] = "brave_sync.previous_seed"; const char kSyncDeviceName[] = "brave_sync.device_name"; const char kSyncBookmarksBaseOrder[] = "brave_sync.bookmarks_base_order"; const char kSyncEnabled[] = "brave_sync.enabled"; @@ -36,6 +37,14 @@ void Prefs::SetSeed(const std::string& seed) { pref_service_->SetString(kSyncSeed, seed); } +std::string Prefs::GetPrevSeed() const { + return pref_service_->GetString(kSyncPrevSeed); +} + +void Prefs::SetPrevSeed(const std::string& seed) { + pref_service_->SetString(kSyncPrevSeed, seed); +} + std::string Prefs::GetThisDeviceId() const { return pref_service_->GetString(kSyncDeviceId); } diff --git a/components/brave_sync/brave_sync_prefs.h b/components/brave_sync/brave_sync_prefs.h index 0d4b9e5cb8b7..68dc5a2d9217 100644 --- a/components/brave_sync/brave_sync_prefs.h +++ b/components/brave_sync/brave_sync_prefs.h @@ -28,6 +28,8 @@ extern const char kSyncDeviceId[]; // String of 32 comma separated bytes // like "145,58,125,111,85,164,236,38,204,67,40,31,182,114,14,152,242,..." extern const char kSyncSeed[]; +// For storing previous seed after reset. It won't be cleared by Clear() +extern const char kSyncPrevSeed[]; // String of current device namefor sync extern const char kSyncDeviceName[]; // The initial bookmarks order, in a format of @@ -57,6 +59,8 @@ class Prefs { std::string GetSeed() const; void SetSeed(const std::string& seed); + std::string GetPrevSeed() const; + void SetPrevSeed(const std::string& seed); std::string GetThisDeviceId() const; void SetThisDeviceId(const std::string& device_id); std::string GetThisDeviceName() const; diff --git a/components/brave_sync/brave_sync_service_factory.cc b/components/brave_sync/brave_sync_service_factory.cc index ec65e83f9158..088e0349da28 100644 --- a/components/brave_sync/brave_sync_service_factory.cc +++ b/components/brave_sync/brave_sync_service_factory.cc @@ -51,6 +51,7 @@ void BraveSyncServiceFactory::RegisterProfilePrefs( user_prefs::PrefRegistrySyncable* registry) { registry->RegisterStringPref(prefs::kSyncDeviceId, std::string()); registry->RegisterStringPref(prefs::kSyncSeed, std::string()); + registry->RegisterStringPref(prefs::kSyncPrevSeed, std::string()); registry->RegisterStringPref(prefs::kSyncDeviceName, std::string()); registry->RegisterStringPref(prefs::kSyncBookmarksBaseOrder, std::string()); diff --git a/components/brave_sync/brave_sync_service_impl.cc b/components/brave_sync/brave_sync_service_impl.cc index b125155f533f..52be3bb857ef 100644 --- a/components/brave_sync/brave_sync_service_impl.cc +++ b/components/brave_sync/brave_sync_service_impl.cc @@ -239,16 +239,9 @@ void BraveSyncServiceImpl::OnResetSync() { const std::string device_id = sync_prefs_->GetThisDeviceId(); - bookmark_change_processor_->Reset(); - + // We have to send delete record and wait for library deleted response then we + // can reset it by ResetInternal() OnDeleteDevice(device_id); - - sync_prefs_->Clear(); - - sync_configured_ = false; - sync_initialized_ = false; - - sync_prefs_->SetSyncEnabled(false); } void BraveSyncServiceImpl::GetSettingsAndDevices( @@ -372,8 +365,16 @@ void BraveSyncServiceImpl::OnSaveInitData(const Uint8Array& seed, std::string seed_str = StrFromUint8Array(seed); std::string device_id_str = StrFromUint8Array(device_id); + std::string prev_seed_str = sync_prefs_->GetPrevSeed(); + sync_words_.clear(); DCHECK(!seed_str.empty()); + if (prev_seed_str == seed_str) { // reconnecitng to previous sync chain + sync_prefs_->SetPrevSeed(std::string()); + } else if (!prev_seed_str.empty()) { // connect/create to new sync chain + bookmark_change_processor_->Reset(true); + sync_prefs_->SetPrevSeed(std::string()); + } // else {} no previous sync chain sync_prefs_->SetSeed(seed_str); sync_prefs_->SetThisDeviceId(device_id_str); @@ -494,7 +495,9 @@ void BraveSyncServiceImpl::OnResolvedPreferences(const RecordsList& records) { sync_prefs_->SetSyncDevices(*sync_devices); - if (this_device_deleted || contains_only_one_device) + if (this_device_deleted) + ResetSyncInternal(); + if (contains_only_one_device) OnResetSync(); } @@ -663,4 +666,17 @@ void BraveSyncServiceImpl::NotifyHaveSyncWords( observer.OnHaveSyncWords(this, sync_words); } +void BraveSyncServiceImpl::ResetSyncInternal() { + bookmark_change_processor_->Reset(false); + + sync_prefs_->SetPrevSeed(sync_prefs_->GetSeed()); + + sync_prefs_->Clear(); + + sync_configured_ = false; + sync_initialized_ = false; + + sync_prefs_->SetSyncEnabled(false); +} + } // namespace brave_sync diff --git a/components/brave_sync/brave_sync_service_impl.h b/components/brave_sync/brave_sync_service_impl.h index 06c1bad4b38c..bbe416dcb678 100644 --- a/components/brave_sync/brave_sync_service_impl.h +++ b/components/brave_sync/brave_sync_service_impl.h @@ -172,6 +172,8 @@ class BraveSyncServiceImpl void NotifySyncStateChanged(); void NotifyHaveSyncWords(const std::string& sync_words); + void ResetSyncInternal(); + std::unique_ptr sync_client_; // True when is in active sync chain @@ -183,6 +185,8 @@ class BraveSyncServiceImpl // while being initializing bool initializing_ = false; + bool reseting_ = false; + std::string sync_words_; std::unique_ptr settings_; diff --git a/components/brave_sync/brave_sync_service_unittest.cc b/components/brave_sync/brave_sync_service_unittest.cc index 5cc2e70a56da..4d4164b81083 100644 --- a/components/brave_sync/brave_sync_service_unittest.cc +++ b/components/brave_sync/brave_sync_service_unittest.cc @@ -371,9 +371,11 @@ TEST_F(BraveSyncServiceTest, GetSeed) { // Service gets seed from client via BraveSyncServiceImpl::OnSaveInitData const auto binary_seed = Uint8Array(16, 77); + EXPECT_TRUE(sync_service()->sync_prefs_->GetPrevSeed().empty()); sync_service()->OnSaveInitData(binary_seed, {0}); std::string expected_seed = StrFromUint8Array(binary_seed); EXPECT_EQ(sync_service()->GetSeed(), expected_seed); + EXPECT_TRUE(sync_service()->sync_prefs_->GetPrevSeed().empty()); } bool DevicesContains(SyncDevices* devices, const std::string& id, @@ -415,6 +417,7 @@ TEST_F(BraveSyncServiceTest, OnDeleteDevice) { EXPECT_CALL(*observer(), OnSyncStateChanged(sync_service())).Times(1); sync_service()->OnResolvedPreferences(records); + sync_service()->sync_prefs_->SetThisDeviceId("1"); auto devices = sync_service()->sync_prefs_->GetSyncDevices(); EXPECT_TRUE(DevicesContains(devices.get(), "1", "device1")); @@ -440,6 +443,7 @@ TEST_F(BraveSyncServiceTest, OnDeleteDevice) { } TEST_F(BraveSyncServiceTest, OnDeleteDeviceWhenOneDevice) { + sync_service()->sync_prefs_->SetThisDeviceId("1"); RecordsList records; records.push_back(SimpleDeviceRecord( jslib::SyncRecord::Action::A_CREATE, @@ -456,14 +460,21 @@ TEST_F(BraveSyncServiceTest, OnDeleteDeviceWhenOneDevice) { EXPECT_TRUE(DevicesContains(devices.get(), "2", "device2")); using brave_sync::jslib::SyncRecord; - EXPECT_CALL(*sync_client(), SendSyncRecords("PREFERENCES", - ContainsDeviceRecord(SyncRecord::Action::A_DELETE, "device2"))).Times(1); + // device 2 => device 1 + EXPECT_CALL(*sync_client(), SendSyncRecords).Times(2); sync_service()->OnDeleteDevice("2"); RecordsList resolved_records; auto resolved_record = SyncRecord::Clone(*records.at(1)); resolved_record->action = jslib::SyncRecord::Action::A_DELETE; resolved_records.push_back(std::move(resolved_record)); + EXPECT_CALL(*observer(), OnSyncStateChanged(sync_service())).Times(1); + sync_service()->OnResolvedPreferences(resolved_records); + + resolved_records.clear(); + resolved_record = SyncRecord::Clone(*records.at(0)); + resolved_record->action = jslib::SyncRecord::Action::A_DELETE; + resolved_records.push_back(std::move(resolved_record)); EXPECT_CALL(*observer(), OnSyncStateChanged(sync_service())).Times(3); sync_service()->OnResolvedPreferences(resolved_records); @@ -475,6 +486,7 @@ TEST_F(BraveSyncServiceTest, OnDeleteDeviceWhenOneDevice) { } TEST_F(BraveSyncServiceTest, OnDeleteDeviceWhenSelfDeleted) { + sync_service()->sync_prefs_->SetThisDeviceId("1"); RecordsList records; records.push_back(SimpleDeviceRecord( jslib::SyncRecord::Action::A_CREATE, @@ -510,11 +522,12 @@ TEST_F(BraveSyncServiceTest, OnDeleteDeviceWhenSelfDeleted) { } TEST_F(BraveSyncServiceTest, OnResetSync) { - EXPECT_CALL(*sync_client(), OnSyncEnabledChanged).Times(AtLeast(2)); + EXPECT_CALL(*sync_client(), OnSyncEnabledChanged).Times(AtLeast(1)); EXPECT_CALL(*observer(), OnSyncStateChanged(sync_service())).Times(AtLeast(3)); sync_service()->OnSetupSyncNewToSync("this_device"); EXPECT_TRUE(profile()->GetPrefs()->GetBoolean( brave_sync::prefs::kSyncEnabled)); + sync_service()->sync_prefs_->SetThisDeviceId("0"); RecordsList records; records.push_back(SimpleDeviceRecord( @@ -532,6 +545,11 @@ TEST_F(BraveSyncServiceTest, OnResetSync) { EXPECT_TRUE(DevicesContains(devices.get(), "1", "device1")); sync_service()->OnResetSync(); + RecordsList resolved_records; + auto resolved_record = jslib::SyncRecord::Clone(*records.at(0)); + resolved_record->action = jslib::SyncRecord::Action::A_DELETE; + resolved_records.push_back(std::move(resolved_record)); + sync_service()->OnResolvedPreferences(resolved_records); auto devices_final = sync_service()->sync_prefs_->GetSyncDevices(); EXPECT_FALSE(DevicesContains(devices_final.get(), "0", "this_device")); diff --git a/components/brave_sync/client/bookmark_change_processor.cc b/components/brave_sync/client/bookmark_change_processor.cc index 0004e2f30f62..8fb1bd63e225 100644 --- a/components/brave_sync/client/bookmark_change_processor.cc +++ b/components/brave_sync/client/bookmark_change_processor.cc @@ -405,12 +405,12 @@ void BookmarkChangeProcessor::BookmarkNodeChildrenReordered( // this should be safe to ignore as it's only called for managed bookmarks } -void BookmarkChangeProcessor::Reset() { +void BookmarkChangeProcessor::Reset(bool clear_meta_info) { ui::TreeNodeIterator iterator(bookmark_model_->root_node()); bookmark_model_->BeginExtensiveChanges(); - { + if (clear_meta_info) { ScopedPauseObserver pause(this); while (iterator.has_next()) { const bookmarks::BookmarkNode* node = iterator.Next(); @@ -425,6 +425,9 @@ void BookmarkChangeProcessor::Reset() { auto* deleted_node = GetDeletedNodeRoot(); CHECK(deleted_node); deleted_node->DeleteAll(); + auto* pending_node = GetPendingNodeRoot(); + CHECK(pending_node); + pending_node->DeleteAll(); bookmark_model_->EndExtensiveChanges(); } diff --git a/components/brave_sync/client/bookmark_change_processor.h b/components/brave_sync/client/bookmark_change_processor.h index 1188b3838b8f..f45a5a769737 100644 --- a/components/brave_sync/client/bookmark_change_processor.h +++ b/components/brave_sync/client/bookmark_change_processor.h @@ -35,7 +35,7 @@ class BookmarkChangeProcessor : public ChangeProcessor, // ChangeProcessor implementation void Start() override; void Stop() override; - void Reset() override; + void Reset(bool clear_meta_info) override; void ApplyChangesFromSyncModel(const RecordsList &records) override; void GetAllSyncData( const std::vector>& records, diff --git a/components/brave_sync/client/bookmark_change_processor_unittest.cc b/components/brave_sync/client/bookmark_change_processor_unittest.cc index bbde700e54c1..95cb1048e5a3 100644 --- a/components/brave_sync/client/bookmark_change_processor_unittest.cc +++ b/components/brave_sync/client/bookmark_change_processor_unittest.cc @@ -241,7 +241,7 @@ void BraveBookmarkChangeProcessorTest::AddSimpleHierarchy( GURL("https://c.com/")); } -TEST_F(BraveBookmarkChangeProcessorTest, Reset) { +TEST_F(BraveBookmarkChangeProcessorTest, ResetClearMeta) { // Reset does clear of the metainfo, but // to fillup the metainfo now need to send it to sync change_processor()->Start(); @@ -260,14 +260,64 @@ TEST_F(BraveBookmarkChangeProcessorTest, Reset) { EXPECT_TRUE(HasAnySyncMetaInfo(node_b)); EXPECT_TRUE(HasAnySyncMetaInfo(node_c)); - change_processor()->Reset(); + model()->AddURL(GetDeletedNodeRoot(), 0, + base::ASCIIToUTF16("A.com - title"), + GURL("https://a.com/")); + model()->AddURL(GetPendingNodeRoot(), 0, + base::ASCIIToUTF16("A.com - title"), + GURL("https://a.com/")); + EXPECT_FALSE(GetDeletedNodeRoot()->empty()); + EXPECT_FALSE(GetPendingNodeRoot()->empty()); + + change_processor()->Reset(true); EXPECT_FALSE(HasAnySyncMetaInfo(folder1)); EXPECT_FALSE(HasAnySyncMetaInfo(node_a)); EXPECT_FALSE(HasAnySyncMetaInfo(node_b)); EXPECT_FALSE(HasAnySyncMetaInfo(node_c)); + EXPECT_TRUE(GetDeletedNodeRoot()->empty()); + EXPECT_TRUE(GetPendingNodeRoot()->empty()); +} + +TEST_F(BraveBookmarkChangeProcessorTest, ResetPreserveMeta) { + // Reset does clear of the metainfo, but + // to fillup the metainfo now need to send it to sync + change_processor()->Start(); + + const BookmarkNode* folder1; + const BookmarkNode* node_a; + const BookmarkNode* node_b; + const BookmarkNode* node_c; + AddSimpleHierarchy(&folder1, &node_a, &node_b, &node_c); + + EXPECT_CALL(*sync_client(), SendSyncRecords("BOOKMARKS", _)).Times(1); + change_processor()->SendUnsynced(base::TimeDelta::FromMinutes(10)); + + EXPECT_TRUE(HasAnySyncMetaInfo(folder1)); + EXPECT_TRUE(HasAnySyncMetaInfo(node_a)); + EXPECT_TRUE(HasAnySyncMetaInfo(node_b)); + EXPECT_TRUE(HasAnySyncMetaInfo(node_c)); + + model()->AddURL(GetDeletedNodeRoot(), 0, + base::ASCIIToUTF16("A.com - title"), + GURL("https://a.com/")); + model()->AddURL(GetPendingNodeRoot(), 0, + base::ASCIIToUTF16("A.com - title"), + GURL("https://a.com/")); + EXPECT_FALSE(GetDeletedNodeRoot()->empty()); + EXPECT_FALSE(GetPendingNodeRoot()->empty()); + + change_processor()->Reset(false); + + EXPECT_TRUE(HasAnySyncMetaInfo(folder1)); + EXPECT_TRUE(HasAnySyncMetaInfo(node_a)); + EXPECT_TRUE(HasAnySyncMetaInfo(node_b)); + EXPECT_TRUE(HasAnySyncMetaInfo(node_c)); + EXPECT_TRUE(GetDeletedNodeRoot()->empty()); + EXPECT_TRUE(GetPendingNodeRoot()->empty()); } + TEST_F(BraveBookmarkChangeProcessorTest, DISABLED_InitialSync) { // BookmarkChangeProcessor::InitialSync does not do anything now // All work for obtaining order is done in background.js diff --git a/components/brave_sync/model/change_processor.h b/components/brave_sync/model/change_processor.h index 225ab585a3df..3a74711a0bf9 100644 --- a/components/brave_sync/model/change_processor.h +++ b/components/brave_sync/model/change_processor.h @@ -21,7 +21,10 @@ class ChangeProcessor { virtual void Stop() = 0; // reset all sync data, but do not delete local records - virtual void Reset() = 0; + // with clear_meta_info == false, we perserve meta info for reconnecting to + // previous sync chain and only clear permanent nodes managed by sync. + // If we want to connect/create to new sync chain, we should clear meta info. + virtual void Reset(bool clear_meta_info) = 0; // setup permanent nodes virtual void InitialSync() = 0;