Merge pull request #1135 from brave/sync-delete-reset

Prevent duplicate nodes from reconnect and refine reset to make sure deleted record is sent

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);
 }

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;

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());
 

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

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<BraveSyncClient> 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<brave_sync::Settings> settings_;

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"));

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<const bookmarks::BookmarkNode>
  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();
 }
 

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<std::unique_ptr<jslib::SyncRecord>>& records,

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

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;