Revert "Merge pull request #4816 from brave/sync_recover_after_dup_id"

This reverts commit 7bff18b, reversing
changes made to 64062cc.

Author: AlexeyBarabash

browser/profiles/brave_bookmark_model_loaded_observer.cc

@@ -44,8 +44,6 @@ 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,7 +92,6 @@ 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;
 
@@ -139,17 +138,6 @@ 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,7 +6,6 @@
 #include "brave/components/brave_sync/brave_profile_sync_service_impl.h"
 
 #include <algorithm>
-#include <map>
 #include <utility>
 #include <vector>
 
@@ -166,57 +165,6 @@ 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,
@@ -807,30 +755,6 @@ 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,9 +187,6 @@ 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,8 +43,6 @@ 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) {}
 
@@ -74,7 +72,6 @@ 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,14 +66,12 @@ 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 2.
+// Current to migrate to is 1.
 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,51 +1631,3 @@ 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");
-}