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

This reverts commit d35a5b72.

Reason for revert: suspect of causing crashes.

Bug: 1040498

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}

TBR=mastiz@chromium.org,mamir@chromium.org

Change-Id: Ia25a087dacc36e8ca6ad351a4664e9478b1ba23a
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 978430, 1004205
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1991628Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#729720}

components/sync_bookmarks/bookmark_model_merger.cc

@@ -347,6 +347,10 @@ 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;
@@ -384,14 +388,9 @@ BookmarkModelMerger::FindGuidMatchesOrReassignLocal(
 
     if (node->is_folder() != remote_entity.is_folder ||
         (node->is_url() &&
-         node->url() != remote_entity.specifics.bookmark().url()) ||
-        !base::FeatureList::IsEnabled(switches::kMergeBookmarksUsingGUIDs)) {
+         node->url() != remote_entity.specifics.bookmark().url())) {
       // If local node and its remote node match are conflicting in node type or
-      // 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).
+      // URL, replace local GUID with a random GUID.
       // 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);
@@ -594,12 +593,10 @@ void BookmarkModelMerger::ProcessLocalCreation(
   const bookmarks::BookmarkNode* node = parent->children()[index].get();
   DCHECK(!FindMatchingRemoteNodeByGUID(node));
   DCHECK(base::IsValidGUID(node->guid()));
-
-  // 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 std::string sync_id =
+      base::FeatureList::IsEnabled(switches::kMergeBookmarksUsingGUIDs)
+          ? node->guid()
+          : base::GenerateGUID();
   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_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()));
-
-  // 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 std::string sync_id =
+      base::FeatureList::IsEnabled(switches::kMergeBookmarksUsingGUIDs)
+          ? node->guid()
+          : base::GenerateGUID();
   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,8 +134,10 @@ TEST_F(BookmarkModelObserverImplTest,
       bookmark_tracker()->GetEntitiesWithLocalChanges(kMaxEntries);
   ASSERT_THAT(local_changes.size(), 1U);
   EXPECT_THAT(local_changes[0]->bookmark_node(), Eq(bookmark_node));
-  EXPECT_THAT(local_changes[0]->metadata()->server_id(),
-              Eq(bookmark_node->guid()));
+  if (base::FeatureList::IsEnabled(switches::kMergeBookmarksUsingGUIDs)) {
+    EXPECT_THAT(local_changes[0]->metadata()->server_id(),
+                Eq(bookmark_node->guid()));
+  }
 }
 
 TEST_F(BookmarkModelObserverImplTest,