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

@@ -397,12 +397,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();
@@ -417,6 +417,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

@@ -33,7 +33,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;