Sync duplicated object id migration fixes (uplift to 1.7.x)

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

@@ -7,6 +7,7 @@
 
 #include <algorithm>
 #include <map>
+#include <set>
 #include <utility>
 #include <vector>
 
@@ -167,19 +168,8 @@ SyncRecordPtr PrepareResolvedDevice(SyncDevice* device,
   return record;
 }
 
-struct BookmarkByDateAddedComparator {
-  bool operator()(const bookmarks::BookmarkNode* lhs,
-                  const bookmarks::BookmarkNode* rhs) const {
-    DCHECK(lhs);
-    DCHECK(rhs);
-    DCHECK(!tools::IsTimeEmpty(lhs->date_added()));
-    DCHECK(!tools::IsTimeEmpty(rhs->date_added()));
-    return lhs->date_added() < rhs->date_added();
-  }
-};
-using SortedNodes =
-    std::set<const bookmarks::BookmarkNode*, BookmarkByDateAddedComparator>;
-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) {
@@ -196,27 +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) {
-      // Nodes are sorted from oldest to newest, go to the second by age
-      SortedNodes::iterator it_nodes = nodes.begin();
-      ++it_nodes;
-      for (; 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);
-        brave_sync::AddBraveMetaInfo(parent->children()[original_index].get());
-      }
+      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,
@@ -808,27 +824,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());
 
-  bool duplicated_bookmarks_recovered =
-      profile->GetPrefs()->GetBoolean(prefs::kDuplicatedBookmarksRecovered);
-  if (duplicated_bookmarks_recovered) {
-    return;
+  int migrated_version = profile->GetPrefs()->GetInteger(
+      prefs::kDuplicatedBookmarksMigrateVersion);
+
+  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()->SetBoolean(prefs::kDuplicatedBookmarksRecovered, true);
+  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.cc

@@ -15,6 +15,7 @@
 
 void MigrateBraveSyncPrefs(PrefService* prefs) {
   prefs->ClearPref(brave_sync::prefs::kSyncPrevSeed);
+  prefs->ClearPref(brave_sync::prefs::kDuplicatedBookmarksRecovered);
 }
 
 namespace brave_sync {
@@ -45,6 +46,8 @@ const char kSyncRecordsToResend[] = "brave_sync_records_to_resend";
 const char kSyncRecordsToResendMeta[] = "brave_sync_records_to_resend_meta";
 const char kDuplicatedBookmarksRecovered[] =
     "brave_sync_duplicated_bookmarks_recovered";
+const char kDuplicatedBookmarksMigrateVersion[] =
+    "brave_sync_duplicated_bookmarks_migrate_version";
 
 Prefs::Prefs(PrefService* pref_service) : pref_service_(pref_service) {}
 
@@ -75,6 +78,7 @@ void Prefs::RegisterProfilePrefs(user_prefs::PrefRegistrySyncable* registry) {
   registry->RegisterListPref(prefs::kSyncRecordsToResend);
   registry->RegisterDictionaryPref(prefs::kSyncRecordsToResendMeta);
   registry->RegisterBooleanPref(kDuplicatedBookmarksRecovered, false);
+  registry->RegisterIntegerPref(prefs::kDuplicatedBookmarksMigrateVersion, 0);
 }
 
 std::string Prefs::GetSeed() const {

components/brave_sync/brave_sync_prefs.h

@@ -72,9 +72,13 @@ extern const char kSyncMigrateBookmarksVersion[];
 extern const char kSyncRecordsToResend[];
 // Meta info of kSyncRecordsToResend
 extern const char kSyncRecordsToResendMeta[];
-// Flag indicates we had recovered duplicated bookmarks object ids
+// Flag indicates we had recovered duplicated bookmarks object ids (deprecated)
 extern const char kDuplicatedBookmarksRecovered[];
 
+// Version indicates had recovered duplicated bookmarks object ids:
+// 2 - we had migrated object ids
+extern const char kDuplicatedBookmarksMigrateVersion[];
+
 class Prefs {
  public:
   explicit Prefs(PrefService* pref_service);

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"
@@ -1632,50 +1633,128 @@ 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);
 
-  const bookmarks::BookmarkNode* bookmark_copy =
-      model()->other_node()->children().at(1).get();
-
-  std::string meta_object_id;
-  EXPECT_TRUE(bookmark_copy->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_EQ(meta_sync_timestamp, "sync_timestamp_value");
-  std::string meta_version;
-  EXPECT_TRUE(bookmark_copy->GetMetaInfo("version", &meta_version));
-  EXPECT_EQ(meta_version, "version_value");
+  model()->Copy(bookmark_a1, model()->other_node(), 2);
+
+  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(node->GetMetaInfo("object_id", &meta_object_id));
+    EXPECT_EQ(meta_object_id, "object_id_value");
+    std::string 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(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(node)->set_date_added(base::Time());
+  }
 
   sync_service()->AddNonClonedBookmarkKeys(model());
 
   // Do the migration
-  BraveProfileSyncServiceImpl::MigrateDuplicatedBookmarksObjectIds(profile(),
-                                                                   model());
-  bookmark_copy = model()->other_node()->children().at(1).get();
+  bool result =
+      BraveProfileSyncServiceImpl::MigrateDuplicatedBookmarksObjectIds(
+          true,
+          profile(),
+          model());
+  EXPECT_TRUE(result);
+
+  // All the bookmarks after migration must not have sync meta info
+  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;
+    std::string migrated_version;
+    EXPECT_FALSE(bookmark->GetMetaInfo("object_id", &migrated_object_id));
+    EXPECT_FALSE(bookmark->GetMetaInfo("order", &migrated_order));
+    EXPECT_FALSE(
+        bookmark->GetMetaInfo("sync_timestamp", &migrated_sync_timestamp));
+    EXPECT_FALSE(bookmark->GetMetaInfo("version", &migrated_version));
+  }
+}
 
-  std::string meta_migrated_object_id;
-  EXPECT_TRUE(
-      bookmark_copy->GetMetaInfo("object_id", &meta_migrated_object_id));
-  EXPECT_NE(meta_migrated_object_id, "object_id_value");
+TEST_F(BraveSyncServiceTest, SyncDisabledMigrateDuplicatedBookmarksObjectIds) {
+  AsMutable(model()->other_node())->SetMetaInfo("order", kOtherNodeOrder);
 
-  std::string meta_migrated_order;
-  EXPECT_TRUE(bookmark_copy->GetMetaInfo("order", &meta_migrated_order));
-  EXPECT_NE(meta_migrated_order, "255.255.255.3");
+  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);
 }

components/brave_sync/syncer_helper.cc

@@ -103,8 +103,8 @@ void AddBraveMetaInfo(const bookmarks::BookmarkNode* node) {
   DCHECK(!sync_timestamp.empty());
   // Set other_node to have same sync_timestamp as least added child
   if (node->parent()->type() == bookmarks::BookmarkNode::OTHER_NODE) {
-    tools::AsMutable(node->parent())->SetMetaInfo("sync_timestamp",
-                                                  sync_timestamp);
+    tools::AsMutable(node->parent())
+        ->SetMetaInfo("sync_timestamp", sync_timestamp);
   }
 }