[Sync] Fix crash caused by duplicate bookmark entities

In case when there are several entities having the same GUID and they
have different types (one is a folder and another one is a URL), the
folder will be preferred because it may contain more bookmarks.

This patch fixes CHECK violation in SyncedBookmarkTracker::Add method
during the initial merge due to duplicate entities (with the same GUID).

Bug: 1081061
Change-Id: I45dd90d345c323a52b33d2fb5282a9b806a34f6a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2379728
Commit-Queue: Rushan Suleymanov <rushans@google.com>
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#803812}

components/sync_bookmarks/bookmark_model_merger.cc

@@ -208,6 +208,25 @@ void ReparentAllChildren(const std::string& from_parent_id,
   // No need to update iterators since splice doesn't invalidate them.
 }
 
+// Returns true the |next_update| is selected to keep and the |previous_update|
+// should be removed. False is returned otherwise. |next_update| and
+// |previous_update| must have the same GUID.
+bool CompareDuplicateUpdates(const UpdateResponseData& next_update,
+                             const UpdateResponseData& previous_update) {
+  DCHECK_EQ(next_update.entity.specifics.bookmark().guid(),
+            previous_update.entity.specifics.bookmark().guid());
+  DCHECK_NE(next_update.entity.id, previous_update.entity.id);
+
+  if (next_update.entity.is_folder != previous_update.entity.is_folder) {
+    // There are two entities, one of them is a folder and another one is a
+    // URL. Prefer to save the folder as it may contain many bookmarks.
+    return next_update.entity.is_folder;
+  }
+  // Choose the latest element to keep if both updates have the same type.
+  return next_update.entity.creation_time >
+         previous_update.entity.creation_time;
+}
+
 void DeduplicateValidUpdatesByGUID(UpdatesPerParentId* updates_per_parent_id) {
   DCHECK(updates_per_parent_id);
 
@@ -243,16 +262,13 @@ void DeduplicateValidUpdatesByGUID(UpdatesPerParentId* updates_per_parent_id) {
           MatchBookmarksGUIDDuplicates(update, duplicate_update);
       base::UmaHistogramEnumeration("Sync.BookmarksGUIDDuplicates",
                                     match_result);
-      if (match_result == BookmarksGUIDDuplicates::kDifferentTypes ||
-          !base::FeatureList::IsEnabled(
+      if (!base::FeatureList::IsEnabled(
               switches::kSyncDeduplicateAllBookmarksWithSameGUID)) {
-        // There shouldn't be cases with different types for duplicate
-        // entities.
         continue;
       }
 
-      // Choose the latest element to keep.
-      if (update.entity.creation_time > duplicate_update.entity.creation_time) {
+      if (CompareDuplicateUpdates(/*next_update=*/update,
+                                  /*previous_update=*/duplicate_update)) {
         updates_to_remove.push_back(it_and_success.first->second);
         // Update |guid_to_update| to find a duplicate folder and merge them.
         guid_to_update[guid_in_specifics] = updates_iter;
@@ -269,6 +285,10 @@ void DeduplicateValidUpdatesByGUID(UpdatesPerParentId* updates_per_parent_id) {
           updates_iter->entity.specifics.bookmark().guid();
       DCHECK_EQ(1U, guid_to_update.count(guid));
       DCHECK(guid_to_update[guid] != updates_iter);
+
+      // Never remove a folder if its duplicate is a URL.
+      DCHECK(guid_to_update[guid]->entity.is_folder);
+
       // Merge doesn't affect iterators.
       ReparentAllChildren(
           /*from_parent_id=*/updates_iter->entity.id,

components/sync_bookmarks/bookmark_model_merger_unittest.cc

@@ -2100,4 +2100,50 @@ TEST(BookmarkModelMergerTest, ShouldReuploadBookmarkOnEmptyGuid) {
   EXPECT_FALSE(tracker->GetEntityForSyncId(kFolder2Id)->IsUnsynced());
 }
 
+TEST(BookmarkModelMergerTest, ShouldRemoveDifferentTypeDuplicatesByGUID) {
+  const std::string kTitle = "Title";
+
+  const std::string kGUID = base::GenerateGUID();
+
+  // The remote model has 2 duplicates, a folder and a URL.
+  //
+  // -------- The remote model --------
+  // bookmark_bar
+  //  |- folder1(GUID)
+  //    |- folder11
+  //  |- URL1(GUID)
+  std::unique_ptr<bookmarks::BookmarkModel> bookmark_model =
+      bookmarks::TestBookmarkClient::CreateModel();
+
+  syncer::UpdateResponseDataList updates;
+  updates.push_back(CreateBookmarkBarNodeUpdateData());
+
+  updates.push_back(CreateUpdateResponseData(
+      /*server_id=*/"Id1", /*parent_id=*/kBookmarkBarId, kTitle,
+      /*url=*/"",
+      /*is_folder=*/true, MakeRandomPosition(), kGUID));
+  updates.push_back(CreateUpdateResponseData(
+      /*server_id=*/"Id11", /*parent_id=*/"Id1", "Some title",
+      /*url=*/"", /*is_folder=*/true, MakeRandomPosition()));
+
+  updates.push_back(CreateUpdateResponseData(
+      /*server_id=*/"Id2", /*parent_id=*/kBookmarkBarId, kTitle,
+      /*url=*/"http://url1.com", /*is_folder=*/false, MakeRandomPosition(),
+      kGUID));
+
+  base::HistogramTester histogram_tester;
+  std::unique_ptr<SyncedBookmarkTracker> tracker =
+      Merge(std::move(updates), bookmark_model.get());
+  const bookmarks::BookmarkNode* bookmark_bar_node =
+      bookmark_model->bookmark_bar_node();
+  ASSERT_THAT(bookmark_bar_node->children().size(), Eq(1u));
+  histogram_tester.ExpectUniqueSample(
+      "Sync.BookmarksGUIDDuplicates",
+      /*sample=*/ExpectedBookmarksGUIDDuplicates::kDifferentTypes,
+      /*count=*/1);
+  EXPECT_THAT(tracker->GetEntityForSyncId("Id1"), NotNull());
+  EXPECT_THAT(tracker->GetEntityForSyncId("Id2"), IsNull());
+  EXPECT_EQ(bookmark_bar_node->children().front()->children().size(), 1u);
+}
+
 }  // namespace sync_bookmarks