Sync duplicated object id migration (3)

browser/profiles/brave_bookmark_model_loaded_observer.cc

@@ -6,6 +6,7 @@
 #include "brave/browser/profiles/brave_bookmark_model_loaded_observer.h"
 
 #include "brave/common/pref_names.h"
+#include "brave/components/brave_sync/features.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/sync/profile_sync_service_factory.h"
 #include "components/bookmarks/browser/bookmark_model.h"
@@ -44,8 +45,10 @@ void BraveBookmarkModelLoadedObserver::BookmarkModelLoaded(
 
 #if BUILDFLAG(ENABLE_BRAVE_SYNC)
   BraveProfileSyncServiceImpl::AddNonClonedBookmarkKeys(model);
-  BraveProfileSyncServiceImpl::MigrateDuplicatedBookmarksObjectIds(profile_,
-                                                                   model);
+  BraveProfileSyncServiceImpl::MigrateDuplicatedBookmarksObjectIds(
+      base::FeatureList::IsEnabled(brave_sync::features::kBraveSync),
+      profile_,
+      model);
 #endif
 
   BookmarkModelLoadedObserver::BookmarkModelLoaded(model, ids_reassigned);

components/brave_sync/brave_profile_sync_service_impl.cc

@@ -168,20 +168,8 @@ SyncRecordPtr PrepareResolvedDevice(SyncDevice* device,
   return record;
 }
 
-struct IndexInParentComparator {
-  bool operator()(const bookmarks::BookmarkNode* lhs,
-                  const bookmarks::BookmarkNode* rhs) const {
-    DCHECK(lhs);
-    DCHECK(rhs);
-    // Nodes equal by object_id we want to sort by index in parent
-    // We need that to make easier the assigning of orders in the case
-    // when orders would be re-calculated on migration of object_id
-    return lhs->parent()->GetIndexOf(lhs) < rhs->parent()->GetIndexOf(rhs);
-  }
-};
-using SortedNodes = std::multiset<const bookmarks::BookmarkNode*,
-                                  IndexInParentComparator>;
-using ObjectIdToNodes = std::map<std::string, SortedNodes>;
+using NodesSet = std::set<const bookmarks::BookmarkNode*>;
+using ObjectIdToNodes = std::map<std::string, NodesSet>;
 
 void FillObjectsMap(const bookmarks::BookmarkNode* parent,
                     ObjectIdToNodes* object_id_nodes) {
@@ -198,25 +186,53 @@ void FillObjectsMap(const bookmarks::BookmarkNode* parent,
   }
 }
 
+void AddDeletedChildren(const BookmarkNode* node, NodesSet* deleted_nodes) {
+  for (const auto& child : node->children()) {
+    deleted_nodes->insert(child.get());
+    if (node->is_folder()) {
+      AddDeletedChildren(child.get(), deleted_nodes);
+    }
+  }
+}
+
 void ClearDuplicatedNodes(ObjectIdToNodes* object_id_nodes,
                           bookmarks::BookmarkModel* model) {
+  size_t nodes_recreated = 0;
+  NodesSet nodes_with_duplicates;
   for (ObjectIdToNodes::iterator it_object_id = object_id_nodes->begin();
        it_object_id != object_id_nodes->end(); ++it_object_id) {
-    const SortedNodes& nodes = it_object_id->second;
+    const NodesSet& nodes = it_object_id->second;
     if (nodes.size() > 1) {
-      // Re-create all group of nodes which have the same object_id
-      for (SortedNodes::iterator it_nodes = nodes.begin();
-           it_nodes != nodes.end(); ++it_nodes) {
-        const bookmarks::BookmarkNode* node = *it_nodes;
-        // Copy and delete node
-        const auto* parent = node->parent();
-        size_t original_index = parent->GetIndexOf(node);
-        model->Copy(node, parent, original_index);
-        model->Remove(node);
-      }
+      nodes_with_duplicates.insert(nodes.begin(), nodes.end());
     }
   }
+
+  NodesSet deleted_nodes;
+  for (const bookmarks::BookmarkNode* node : nodes_with_duplicates) {
+    if (deleted_nodes.find(node) != deleted_nodes.end()) {
+      // Node already has been deleted
+      continue;
+    }
+
+    deleted_nodes.insert(node);
+    if (node->is_folder()) {
+      AddDeletedChildren(node, &deleted_nodes);
+    }
+
+    const auto* parent = node->parent();
+    size_t original_index = parent->GetIndexOf(node);
+    VLOG(1) << "[BraveSync] " << __func__
+            << " Copying node into index=" << original_index;
+    model->Copy(node, parent, original_index);
+    VLOG(1) << "[BraveSync] " << __func__ << " Removing original node";
+    model->Remove(node);
+    nodes_recreated++;
+  }
+
+  VLOG(1) << "[BraveSync] " << __func__
+          << " done nodes_recreated=" << nodes_recreated;
 }
+
 }  // namespace
 
 BraveProfileSyncServiceImpl::BraveProfileSyncServiceImpl(Profile* profile,
@@ -809,28 +825,33 @@ void BraveProfileSyncServiceImpl::SetPermanentNodesOrder(
 }
 
 // static
-void BraveProfileSyncServiceImpl::MigrateDuplicatedBookmarksObjectIds(
+bool BraveProfileSyncServiceImpl::MigrateDuplicatedBookmarksObjectIds(
+    bool sync_enabled,
     Profile* profile,
     BookmarkModel* model) {
+  if (!sync_enabled) {
+    return false;
+  }
+
   DCHECK(model);
   DCHECK(model->loaded());
 
   int migrated_version = profile->GetPrefs()->GetInteger(
       prefs::kDuplicatedBookmarksMigrateVersion);
 
-  if (migrated_version >= 1) {
-    return;
+  if (migrated_version >= 2) {
+    return true;
   }
 
   // Copying bookmarks through brave://bookmarks page could duplicate brave sync
   // metadata, which caused crash during chromium sync run
-  // Go through nodes and re-create the oldest ones who have duplicated
-  // object_id
+  // Go through nodes and re-create those ones who have duplicated object_id
   ObjectIdToNodes object_id_nodes;
   FillObjectsMap(model->root_node(), &object_id_nodes);
   ClearDuplicatedNodes(&object_id_nodes, model);
 
-  profile->GetPrefs()->SetInteger(prefs::kDuplicatedBookmarksMigrateVersion, 1);
+  profile->GetPrefs()->SetInteger(prefs::kDuplicatedBookmarksMigrateVersion, 2);
+  return true;
 }
 
 std::unique_ptr<SyncRecord>

components/brave_sync/brave_profile_sync_service_impl.h

@@ -187,7 +187,8 @@ class BraveProfileSyncServiceImpl
 
   BraveSyncService* GetSyncService() const override;
 
-  static void MigrateDuplicatedBookmarksObjectIds(Profile* profile,
+  static bool MigrateDuplicatedBookmarksObjectIds(bool sync_enabled,
+                                                  Profile* profile,
                                                   BookmarkModel* model);
 
  private:

components/brave_sync/brave_sync_prefs.h

@@ -76,8 +76,7 @@ extern const char kSyncRecordsToResendMeta[];
 extern const char kDuplicatedBookmarksRecovered[];
 
 // Version indicates had recovered duplicated bookmarks object ids:
-// 1 - we had migrated object ids
-// 2 - we have migrated broken bookmarks orders
+// 2 - we had migrated object ids
 extern const char kDuplicatedBookmarksMigrateVersion[];
 
 class Prefs {

components/brave_sync/brave_sync_service_unittest.cc

@@ -15,6 +15,7 @@
 #include "brave/components/brave_sync/brave_sync_service_observer.h"
 #include "brave/components/brave_sync/client/brave_sync_client_impl.h"
 #include "brave/components/brave_sync/client/client_ext_impl_data.h"
+#include "brave/components/brave_sync/features.h"
 #include "brave/components/brave_sync/jslib_const.h"
 #include "brave/components/brave_sync/jslib_messages.h"
 #include "brave/components/brave_sync/settings.h"
@@ -1633,56 +1634,94 @@ TEST_F(BraveSyncServiceTest, AddNonClonedBookmarkKeys) {
   EXPECT_TRUE(meta_version.empty());
 }
 
+namespace {
+
+void SetBraveMeta(const bookmarks::BookmarkNode* node,
+                  const std::string& object_id,
+                  const std::string& order,
+                  const std::string& sync_timestamp,
+                  const std::string& version) {
+  bookmarks::BookmarkNode* mutable_node = AsMutable(node);
+  mutable_node->SetMetaInfo("object_id", object_id);
+  mutable_node->SetMetaInfo("order", order);
+  mutable_node->SetMetaInfo("sync_timestamp", sync_timestamp);
+  mutable_node->SetMetaInfo("version", version);
+}
+
+void GetAllNodes(const bookmarks::BookmarkNode* parent,
+                 std::set<const bookmarks::BookmarkNode*>* all_nodes) {
+  for (size_t i = 0; i < parent->children().size(); ++i) {
+    const bookmarks::BookmarkNode* current_child = parent->children()[i].get();
+    all_nodes->insert(current_child);
+    if (current_child->is_folder()) {
+      GetAllNodes(current_child, all_nodes);
+    }
+  }
+}
+
+}  //  namespace
+
 TEST_F(BraveSyncServiceTest, MigrateDuplicatedBookmarksObjectIds) {
   AsMutable(model()->other_node())->SetMetaInfo("order", kOtherNodeOrder);
 
   const bookmarks::BookmarkNode* bookmark_a1 =
       model()->AddURL(model()->other_node(), 0, base::ASCIIToUTF16("A1"),
                       GURL("https://a1.com"));
 
-  AsMutable(bookmark_a1)->SetMetaInfo("object_id", "object_id_value");
-  AsMutable(bookmark_a1)->SetMetaInfo("order", "255.255.255.3");
-  AsMutable(bookmark_a1)->SetMetaInfo("sync_timestamp", "sync_timestamp_value");
-  AsMutable(bookmark_a1)->SetMetaInfo("version", "version_value");
+  SetBraveMeta(bookmark_a1, "object_id_value", "255.255.255.3",
+               "sync_timestamp_value", "version_value");
 
   model()->Copy(bookmark_a1, model()->other_node(), 1);
 
   model()->Copy(bookmark_a1, model()->other_node(), 2);
 
-  for (size_t i = 1; i <= 2; ++i) {
-    const bookmarks::BookmarkNode* bookmark_copy =
-        model()->other_node()->children().at(i).get();
+  const bookmarks::BookmarkNode* folder_f1 =
+      model()->AddFolder(model()->other_node(), 3, base::ASCIIToUTF16("F1"));
+  SetBraveMeta(folder_f1, "object_id_value", "255.255.255.5",
+               "sync_timestamp_value", "version_value");
+
+  const bookmarks::BookmarkNode* bookmark_b1 = model()->AddURL(
+      folder_f1, 0, base::ASCIIToUTF16("B1"), GURL("https://b1.com"));
+  SetBraveMeta(bookmark_b1, "object_id_value", "255.255.255.5.1",
+               "sync_timestamp_value", "version_value");
+
+  model()->Copy(folder_f1, model()->other_node(), 4);
+  model()->Copy(folder_f1, model()->other_node(), 5);
+  model()->Move(model()->other_node()->children()[5].get(), folder_f1, 0);
 
+  std::set<const bookmarks::BookmarkNode*> all_nodes;
+  GetAllNodes(model()->other_node(), &all_nodes);
+  for (const bookmarks::BookmarkNode* node : all_nodes) {
     // Verify fields after copying
     std::string meta_object_id;
-    EXPECT_TRUE(bookmark_copy->GetMetaInfo("object_id", &meta_object_id));
+    EXPECT_TRUE(node->GetMetaInfo("object_id", &meta_object_id));
     EXPECT_EQ(meta_object_id, "object_id_value");
-    std::string meta_order;
-    EXPECT_TRUE(bookmark_copy->GetMetaInfo("order", &meta_order));
-    EXPECT_EQ(meta_order, "255.255.255.3");
     std::string meta_sync_timestamp;
-    EXPECT_TRUE(
-        bookmark_copy->GetMetaInfo("sync_timestamp", &meta_sync_timestamp));
+    EXPECT_TRUE(node->GetMetaInfo("sync_timestamp", &meta_sync_timestamp));
     EXPECT_EQ(meta_sync_timestamp, "sync_timestamp_value");
     std::string meta_version;
-    EXPECT_TRUE(bookmark_copy->GetMetaInfo("version", &meta_version));
+    EXPECT_TRUE(node->GetMetaInfo("version", &meta_version));
     EXPECT_EQ(meta_version, "version_value");
 
     // Simulate all bookmarks don`t have added time, as a worse case,
     // but happened on live profile
-    AsMutable(bookmark_copy)->set_date_added(base::Time());
+    AsMutable(node)->set_date_added(base::Time());
   }
 
   sync_service()->AddNonClonedBookmarkKeys(model());
 
   // Do the migration
-  BraveProfileSyncServiceImpl::MigrateDuplicatedBookmarksObjectIds(profile(),
-                                                                   model());
+  bool result =
+      BraveProfileSyncServiceImpl::MigrateDuplicatedBookmarksObjectIds(
+          true,
+          profile(),
+          model());
+  EXPECT_TRUE(result);
 
   // All the bookmarks after migration must not have sync meta info
-  for (size_t i = 0; i <= 2; ++i) {
-    const bookmarks::BookmarkNode* bookmark =
-        model()->other_node()->children().at(i).get();
+  all_nodes.clear();
+  GetAllNodes(model()->other_node(), &all_nodes);
+  for (const bookmarks::BookmarkNode* bookmark : all_nodes) {
     std::string migrated_object_id;
     std::string migrated_order;
     std::string migrated_sync_timestamp;
@@ -1694,3 +1733,29 @@ TEST_F(BraveSyncServiceTest, MigrateDuplicatedBookmarksObjectIds) {
     EXPECT_FALSE(bookmark->GetMetaInfo("version", &migrated_version));
   }
 }
+
+TEST_F(BraveSyncServiceTest, SyncDisabledMigrateDuplicatedBookmarksObjectIds) {
+  AsMutable(model()->other_node())->SetMetaInfo("order", kOtherNodeOrder);
+
+  const bookmarks::BookmarkNode* bookmark_a1 =
+      model()->AddURL(model()->other_node(), 0, base::ASCIIToUTF16("A1"),
+                      GURL("https://a1.com"));
+
+  SetBraveMeta(bookmark_a1, "object_id_value", "255.255.255.3",
+               "sync_timestamp_value", "version_value");
+
+  model()->Copy(bookmark_a1, model()->other_node(), 1);
+
+  model()->Copy(bookmark_a1, model()->other_node(), 2);
+
+  // Do the migration
+  bool result =
+      BraveProfileSyncServiceImpl::MigrateDuplicatedBookmarksObjectIds(
+          false,
+          profile(),
+          model());
+
+  // Should return false, because sync is disable
+  // Migration will be a no-op
+  EXPECT_FALSE(result);
+}