Reland "Bookmark sync: use GUID as temporary server ID during commit"

This is a reland of d35a5b72, which
includes a fix to prevent the issue of the BookmarkModel being modified
while iterated, which can invalidate iterators due to how GUID
reassignment is implemented.

Original change's description:
> Bookmark sync: use GUID as temporary server ID during commit
>
> This patch effectively relands the logic reverted in
> https://chromium-review.googlesource.com/c/chromium/src/+/1864800 which
> caused crashes in the past (CHECK failures).
>
> In this new form, the underlying issue is addressed: for users that
> don't use GUID-based merging of bookmarks, local GUIDs are reassigned
> randomly to avoid GUID collisions in the local bookmark model.
>
> Bug: 978430,1004205
> Change-Id: I812d1cfe1c12b766799e6c5ec238f95fd16139ad
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1983231
> Commit-Queue: Mikel Astiz <mastiz@chromium.org>
> Reviewed-by: Mohamed Amir Yosef <mamir@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#729405}

Bug: 978430, 1004205
Change-Id: Ie86f42a1d8985e325fca5741d43aefc6ef0955c5
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1993295
Auto-Submit: Mikel Astiz <mastiz@chromium.org>
Reviewed-by: Mohamed Amir Yosef <mamir@chromium.org>
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#730100}

components/sync_bookmarks/bookmark_model_merger.cc

@@ -347,10 +347,6 @@ BookmarkModelMerger::FindGuidMatchesOrReassignLocal(
     bookmarks::BookmarkModel* bookmark_model) {
   DCHECK(bookmark_model);
 
-  if (!base::FeatureList::IsEnabled(switches::kMergeBookmarksUsingGUIDs)) {
-    return {};
-  }
-
   // Build a temporary lookup table for remote GUIDs.
   std::unordered_map<std::string, const RemoteTreeNode*>
       guid_to_remote_node_map;
@@ -362,6 +358,9 @@ BookmarkModelMerger::FindGuidMatchesOrReassignLocal(
   // Iterate through all local bookmarks to find matches by GUID.
   std::unordered_map<std::string, BookmarkModelMerger::GuidMatch>
       guid_to_match_map;
+  // Because ReplaceBookmarkNodeGUID() cannot be used while iterating the local
+  // bookmark model, a temporary list is constructed first to reassign later.
+  std::vector<const bookmarks::BookmarkNode*> nodes_to_replace_guid;
   ui::TreeNodeIterator<const bookmarks::BookmarkNode> iterator(
       bookmark_model->root_node());
   while (iterator.has_next()) {
@@ -388,18 +387,27 @@ BookmarkModelMerger::FindGuidMatchesOrReassignLocal(
 
     if (node->is_folder() != remote_entity.is_folder ||
         (node->is_url() &&
-         node->url() != remote_entity.specifics.bookmark().url())) {
+         node->url() != remote_entity.specifics.bookmark().url()) ||
+        !base::FeatureList::IsEnabled(switches::kMergeBookmarksUsingGUIDs)) {
       // If local node and its remote node match are conflicting in node type or
-      // URL, replace local GUID with a random GUID.
+      // URL, replace local GUID with a random GUID. The logic is applied
+      // unconditionally if kMergeBookmarksUsingGUIDs is disabled, since no
+      // GUID-based matches take place and GUIDs need to be reassigned to avoid
+      // collisions (they will be reassigned once again if there is a semantic
+      // match).
       // TODO(crbug.com/978430): Local GUIDs should also be reassigned if they
       // match a remote originator_client_item_id.
-      ReplaceBookmarkNodeGUID(node, base::GenerateGUID(), bookmark_model);
+      nodes_to_replace_guid.push_back(node);
       continue;
     }
 
     guid_to_match_map.emplace(node->guid(), GuidMatch{node, remote_node});
   }
 
+  for (const bookmarks::BookmarkNode* node : nodes_to_replace_guid) {
+    ReplaceBookmarkNodeGUID(node, base::GenerateGUID(), bookmark_model);
+  }
+
   return guid_to_match_map;
 }
 
@@ -593,10 +601,12 @@ void BookmarkModelMerger::ProcessLocalCreation(
   const bookmarks::BookmarkNode* node = parent->children()[index].get();
   DCHECK(!FindMatchingRemoteNodeByGUID(node));
   DCHECK(base::IsValidGUID(node->guid()));
-  const std::string sync_id =
-      base::FeatureList::IsEnabled(switches::kMergeBookmarksUsingGUIDs)
-          ? node->guid()
-          : base::GenerateGUID();
+
+  // The node's GUID cannot run into collisions because
+  // FindGuidMatchesOrReassignLocal() takes care of reassigning local GUIDs if
+  // they won't actually be merged with the remote bookmark with the same GUID
+  // (e.g. incompatible types).
+  const std::string sync_id = node->guid();
   const int64_t server_version = syncer::kUncommittedVersion;
   const base::Time creation_time = base::Time::Now();
   const std::string& suffix = syncer::GenerateSyncableBookmarkHash(

components/sync_bookmarks/bookmark_model_merger_unittest.cc

@@ -1146,6 +1146,69 @@ TEST(BookmarkModelMergerTest, ShouldReplaceBookmarkGUIDWithConflictingTypes) {
   EXPECT_FALSE(bookmark_bar_node->children()[1]->is_folder());
 }
 
+TEST(BookmarkModelMergerTest,
+     ShouldReplaceBookmarkGUIDWithConflictingTypesAndLocalChildren) {
+  base::test::ScopedFeatureList override_features;
+  override_features.InitAndEnableFeature(switches::kMergeBookmarksUsingGUIDs);
+
+  const std::string kGuid = base::GenerateGUID();
+  const std::string kGuid2 = base::GenerateGUID();
+
+  std::unique_ptr<bookmarks::BookmarkModel> bookmark_model =
+      bookmarks::TestBookmarkClient::CreateModel();
+
+  // -------- The local model --------
+  // bookmark_bar
+  //  | - folder (kGuid)
+  //    | - bookmark (kGuid2)
+
+  const bookmarks::BookmarkNode* bookmark_bar_node =
+      bookmark_model->bookmark_bar_node();
+  const bookmarks::BookmarkNode* folder = bookmark_model->AddFolder(
+      /*parent=*/bookmark_bar_node, /*index=*/0,
+      base::UTF8ToUTF16("Folder Title"), nullptr, kGuid);
+  const bookmarks::BookmarkNode* bookmark = bookmark_model->AddURL(
+      /*parent=*/folder, /*index=*/0, base::UTF8ToUTF16("Foo's title"),
+      GURL("http://foo.com"), nullptr, base::Time::Now(), kGuid2);
+  ASSERT_TRUE(folder);
+  ASSERT_TRUE(bookmark);
+  ASSERT_THAT(bookmark_bar_node->children(), ElementRawPointersAre(folder));
+  ASSERT_THAT(folder->children(), ElementRawPointersAre(bookmark));
+
+  // -------- The remote model --------
+  // bookmark_bar
+  //  | - bookmark (kGuid)
+
+  syncer::UpdateResponseDataList updates;
+  updates.push_back(CreateBookmarkBarNodeUpdateData());
+
+  updates.push_back(CreateUpdateResponseData(
+      /*server_id=*/"Id", /*parent_id=*/kBookmarkBarId, "Bar's title",
+      "http://bar.com/",
+      /*is_folder=*/false,
+      /*unique_position=*/MakeRandomPosition(),
+      /*guid=*/kGuid));
+
+  Merge(std::move(updates), bookmark_model.get());
+
+  // -------- The merged model --------
+  // bookmark_bar
+  //  | - bookmark (kGuid)
+  //  | - folder ([new GUID])
+  //    | - bookmark (kGuid2)
+
+  // Conflicting node GUID should have been replaced.
+  ASSERT_EQ(bookmark_bar_node->children().size(), 2u);
+  EXPECT_EQ(bookmark_bar_node->children()[0]->guid(), kGuid);
+  EXPECT_NE(bookmark_bar_node->children()[1]->guid(), kGuid);
+  EXPECT_NE(bookmark_bar_node->children()[1]->guid(), kGuid2);
+  EXPECT_FALSE(bookmark_bar_node->children()[0]->is_folder());
+  EXPECT_TRUE(bookmark_bar_node->children()[1]->is_folder());
+  EXPECT_EQ(bookmark_bar_node->children()[1]->children().size(), 1u);
+  EXPECT_FALSE(bookmark_bar_node->children()[1]->children()[0]->is_folder());
+  EXPECT_EQ(bookmark_bar_node->children()[1]->children()[0]->guid(), kGuid2);
+}
+
 // Tests that the GUID-based matching algorithm handles well the case where a
 // local bookmark matches a remote bookmark that is orphan. In this case the
 // remote node should be ignored and the local bookmark included in the merged

components/sync_bookmarks/bookmark_model_observer_impl.cc

@@ -96,10 +96,10 @@ void BookmarkModelObserverImpl::BookmarkNodeAdded(
   // Assign a temp server id for the entity. Will be overriden by the actual
   // server id upon receiving commit response.
   DCHECK(base::IsValidGUID(node->guid()));
-  const std::string sync_id =
-      base::FeatureList::IsEnabled(switches::kMergeBookmarksUsingGUIDs)
-          ? node->guid()
-          : base::GenerateGUID();
+
+  // Local bookmark creations should have used a random GUID so it's safe to
+  // use it as originator client item ID, without the risk for collision.
+  const std::string sync_id = node->guid();
   const int64_t server_version = syncer::kUncommittedVersion;
   const base::Time creation_time = base::Time::Now();
   const sync_pb::UniquePosition unique_position =

components/sync_bookmarks/bookmark_model_observer_impl_unittest.cc

@@ -134,10 +134,8 @@ TEST_F(BookmarkModelObserverImplTest,
       bookmark_tracker()->GetEntitiesWithLocalChanges(kMaxEntries);
   ASSERT_THAT(local_changes.size(), 1U);
   EXPECT_THAT(local_changes[0]->bookmark_node(), Eq(bookmark_node));
-  if (base::FeatureList::IsEnabled(switches::kMergeBookmarksUsingGUIDs)) {
-    EXPECT_THAT(local_changes[0]->metadata()->server_id(),
-                Eq(bookmark_node->guid()));
-  }
+  EXPECT_THAT(local_changes[0]->metadata()->server_id(),
+              Eq(bookmark_node->guid()));
 }
 
 TEST_F(BookmarkModelObserverImplTest,