Merge pull request #4816 from brave/sync_recover_after_dup_id

Recreating bookmarks with duplicated sync object id

Author: AlexeyBarabash

browser/profiles/brave_bookmark_model_loaded_observer.cc

@@ -44,6 +44,8 @@ void BraveBookmarkModelLoadedObserver::BookmarkModelLoaded(
 
 #if BUILDFLAG(ENABLE_BRAVE_SYNC)
   BraveProfileSyncServiceImpl::AddNonClonedBookmarkKeys(model);
+  BraveProfileSyncServiceImpl::MigrateDuplicatedBookmarksObjectIds(profile_,
+                                                                   model);
 #endif
 
   BookmarkModelLoadedObserver::BookmarkModelLoaded(model, ids_reassigned);

chromium_src/components/sync/engine_impl/commit.cc

@@ -92,6 +92,7 @@ brave_sync::RecordsListPtr ConvertCommitsToBraveRecords(
     if (entity.specifics().has_bookmark()) {
       const sync_pb::BookmarkSpecifics& bm_specifics =
           entity.specifics().bookmark();
+
       auto record = std::make_unique<SyncRecord>();
       record->objectData = brave_sync::jslib_const::SyncObjectData_BOOKMARK;
 
@@ -138,6 +139,17 @@ brave_sync::RecordsListPtr ConvertCommitsToBraveRecords(
           record->action = brave_sync::jslib::SyncRecord::Action::A_UPDATE;
       }
 
+      if (entity.deleted() && entity.version() == 0 &&
+        record->objectId.empty()) {
+        // This is for recover profile after crash with duplicated object ids
+        // When doing delete of duplicated bookmark, pretend it has a new
+        // object id to got through nudge/pull cycle
+        // We are ok, if other devices would get this record, because they have
+        // nothing to delete in fact
+        record->objectId = brave_sync::tools::GenerateObjectId();
+        record->action = brave_sync::jslib::SyncRecord::Action::A_DELETE;
+      }
+
       DCHECK(!record->objectId.empty());
 
       MetaInfo metaInfo;

components/brave_sync/brave_profile_sync_service_impl.cc

@@ -6,6 +6,7 @@
 #include "brave/components/brave_sync/brave_profile_sync_service_impl.h"
 
 #include <algorithm>
+#include <map>
 #include <utility>
 #include <vector>
 
@@ -165,6 +166,57 @@ SyncRecordPtr PrepareResolvedDevice(SyncDevice* device,
   record->SetDevice(std::move(device_record));
   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>;
+
+void FillObjectsMap(const bookmarks::BookmarkNode* parent,
+                    ObjectIdToNodes* object_id_nodes) {
+  for (size_t i = 0; i < parent->children().size(); ++i) {
+    const bookmarks::BookmarkNode* current_child = parent->children()[i].get();
+    std::string object_id;
+    if (current_child->GetMetaInfo("object_id", &object_id) &&
+        !object_id.empty()) {
+      (*object_id_nodes)[object_id].insert(current_child);
+    }
+    if (current_child->is_folder()) {
+      FillObjectsMap(current_child, object_id_nodes);
+    }
+  }
+}
+
+void ClearDuplicatedNodes(ObjectIdToNodes* object_id_nodes,
+                          bookmarks::BookmarkModel* model) {
+  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;
+    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());
+      }
+    }
+  }
+}
 }  // namespace
 
 BraveProfileSyncServiceImpl::BraveProfileSyncServiceImpl(Profile* profile,
@@ -755,6 +807,30 @@ void BraveProfileSyncServiceImpl::SetPermanentNodesOrder(
   brave_sync_prefs_->SetMigratedBookmarksVersion(2);
 }
 
+// static
+void BraveProfileSyncServiceImpl::MigrateDuplicatedBookmarksObjectIds(
+    Profile* profile,
+    BookmarkModel* model) {
+  DCHECK(model);
+  DCHECK(model->loaded());
+
+  bool duplicated_bookmarks_recovered =
+      profile->GetPrefs()->GetBoolean(prefs::kDuplicatedBookmarksRecovered);
+  if (duplicated_bookmarks_recovered) {
+    return;
+  }
+
+  // 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
+  ObjectIdToNodes object_id_nodes;
+  FillObjectsMap(model->root_node(), &object_id_nodes);
+  ClearDuplicatedNodes(&object_id_nodes, model);
+
+  profile->GetPrefs()->SetBoolean(prefs::kDuplicatedBookmarksRecovered, true);
+}
+
 std::unique_ptr<SyncRecord>
 BraveProfileSyncServiceImpl::BookmarkNodeToSyncBookmark(
     const bookmarks::BookmarkNode* node) {

components/brave_sync/brave_profile_sync_service_impl.h

@@ -187,6 +187,9 @@ class BraveProfileSyncServiceImpl
 
   BraveSyncService* GetSyncService() const override;
 
+  static void MigrateDuplicatedBookmarksObjectIds(Profile* profile,
+                                                  BookmarkModel* model);
+
  private:
   FRIEND_TEST_ALL_PREFIXES(::BraveSyncServiceTest, BookmarkAdded);
   FRIEND_TEST_ALL_PREFIXES(::BraveSyncServiceTest, BookmarkDeleted);

components/brave_sync/brave_sync_prefs.cc

@@ -43,6 +43,8 @@ const char kSyncMigrateBookmarksVersion[]
                                        = "brave_sync.migrate_bookmarks_version";
 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";
 
 Prefs::Prefs(PrefService* pref_service) : pref_service_(pref_service) {}
 
@@ -72,6 +74,7 @@ void Prefs::RegisterProfilePrefs(user_prefs::PrefRegistrySyncable* registry) {
 
   registry->RegisterListPref(prefs::kSyncRecordsToResend);
   registry->RegisterDictionaryPref(prefs::kSyncRecordsToResendMeta);
+  registry->RegisterBooleanPref(kDuplicatedBookmarksRecovered, false);
 }
 
 std::string Prefs::GetSeed() const {

components/brave_sync/brave_sync_prefs.h

@@ -66,12 +66,14 @@ extern const char kSyncDeviceList[];
 // the sync api version from the server
 extern const char kSyncApiVersion[];
 // The version of bookmarks state: 0,1,... .
-// Current to migrate to is 1.
+// Current to migrate to is 2.
 extern const char kSyncMigrateBookmarksVersion[];
 // Cached object_id list for unconfirmed records
 extern const char kSyncRecordsToResend[];
 // Meta info of kSyncRecordsToResend
 extern const char kSyncRecordsToResendMeta[];
+// Flag indicates we had recovered duplicated bookmarks object ids
+extern const char kDuplicatedBookmarksRecovered[];
 
 class Prefs {
  public:

components/brave_sync/brave_sync_service_unittest.cc

@@ -1631,3 +1631,51 @@ TEST_F(BraveSyncServiceTest, AddNonClonedBookmarkKeys) {
   EXPECT_FALSE(bookmark_copy->GetMetaInfo("version", &meta_version));
   EXPECT_TRUE(meta_version.empty());
 }
+
+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");
+
+  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");
+
+  sync_service()->AddNonClonedBookmarkKeys(model());
+
+  // Do the migration
+  BraveProfileSyncServiceImpl::MigrateDuplicatedBookmarksObjectIds(profile(),
+                                                                   model());
+  bookmark_copy = model()->other_node()->children().at(1).get();
+
+  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");
+
+  std::string meta_migrated_order;
+  EXPECT_TRUE(bookmark_copy->GetMetaInfo("order", &meta_migrated_order));
+  EXPECT_NE(meta_migrated_order, "255.255.255.3");
+}