Refactor bookmark GUID-matching logic

Prior to this patch, the two existing functions to build GUID-keyed maps
were heavily coupled and there were fragile underlying assumptions that
made it work (e.g. there were potentially entries in
|guid_to_remote_node_map_| that didn't actually match any local GUID).

Instead, let's centralize the matching logic into one single function,
and adopt a unified map to represent the matches. This simplifies the
code and enforces more strict invariants without the need for DCHECKs.

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

components/sync_bookmarks/bookmark_model_merger.cc

@@ -237,9 +237,8 @@ BookmarkModelMerger::BookmarkModelMerger(
       favicon_service_(favicon_service),
       bookmark_tracker_(bookmark_tracker),
       remote_forest_(BuildRemoteForest(std::move(updates))),
-      guid_to_remote_node_map_(BuildGUIDToRemoteNodeMap(remote_forest_)),
-      guid_to_local_node_map_(
-          BuildGUIDToLocalNodeMap(bookmark_model_, guid_to_remote_node_map_)) {
+      guid_to_match_map_(
+          FindGuidMatchesOrReassignLocal(remote_forest_, bookmark_model_)) {
   DCHECK(bookmark_tracker_->IsEmpty());
   DCHECK(favicon_service);
 }
@@ -307,63 +306,62 @@ BookmarkModelMerger::RemoteForest BookmarkModelMerger::BuildRemoteForest(
 }
 
 // static
-std::unordered_map<std::string, const BookmarkModelMerger::RemoteTreeNode*>
-BookmarkModelMerger::BuildGUIDToRemoteNodeMap(
-    const RemoteForest& remote_forest) {
+std::unordered_map<std::string, BookmarkModelMerger::GuidMatch>
+BookmarkModelMerger::FindGuidMatchesOrReassignLocal(
+    const RemoteForest& remote_forest,
+    bookmarks::BookmarkModel* bookmark_model) {
+  DCHECK(bookmark_model);
+
   // TODO(crbug.com/978430): Handle potential duplicate GUIDs within remote
   // updates.
-  std::unordered_map<std::string, const RemoteTreeNode*>
-      guid_to_remote_node_map;
+
   if (!base::FeatureList::IsEnabled(switches::kMergeBookmarksUsingGUIDs)) {
-    return guid_to_remote_node_map;
+    return {};
   }
 
+  // Build a temporary lookup table for remote GUIDs.
+  std::unordered_map<std::string, const RemoteTreeNode*>
+      guid_to_remote_node_map;
   for (const auto& tree_tag_and_root : remote_forest) {
     tree_tag_and_root.second.EmplaceSelfAndDescendantsByGUID(
         &guid_to_remote_node_map);
   }
 
-  return guid_to_remote_node_map;
-}
-
-// static
-std::unordered_map<std::string, const bookmarks::BookmarkNode*>
-BookmarkModelMerger::BuildGUIDToLocalNodeMap(
-    bookmarks::BookmarkModel* bookmark_model,
-    const std::unordered_map<std::string, const RemoteTreeNode*>&
-        guid_to_remote_node_map) {
-  DCHECK(bookmark_model);
-
-  std::unordered_map<std::string, const bookmarks::BookmarkNode*>
-      guid_to_local_node_map;
-  if (!base::FeatureList::IsEnabled(switches::kMergeBookmarksUsingGUIDs)) {
-    return guid_to_local_node_map;
-  }
-
+  // Iterate through all local bookmarks to find matches by GUID.
+  std::unordered_map<std::string, BookmarkModelMerger::GuidMatch>
+      guid_to_match_map;
   ui::TreeNodeIterator<const bookmarks::BookmarkNode> iterator(
       bookmark_model->root_node());
   while (iterator.has_next()) {
-    const bookmarks::BookmarkNode* node = iterator.Next();
+    const bookmarks::BookmarkNode* const node = iterator.Next();
     DCHECK(base::IsValidGUID(node->guid()));
+
     if (node->is_permanent_node()) {
       continue;
     }
+
     const auto remote_it = guid_to_remote_node_map.find(node->guid());
     if (remote_it == guid_to_remote_node_map.end()) {
       continue;
     }
-    // If local node and its remote node match are conflicting in node type or
-    // URL, replace local GUID with a random GUID.
-    const syncer::EntityData& remote_entity = remote_it->second->entity();
+
+    const RemoteTreeNode* const remote_node = remote_it->second;
+    const syncer::EntityData& remote_entity = remote_node->entity();
     if (node->is_folder() != remote_entity.is_folder ||
         (node->is_url() &&
          node->url() != remote_entity.specifics.bookmark().url())) {
-      node =
-          ReplaceBookmarkNodeGUID(node, base::GenerateGUID(), bookmark_model);
+      // If local node and its remote node match are conflicting in node type or
+      // URL, replace local GUID with a random GUID.
+      // TODO(crbug.com/978430): Local GUIDs should also be reassigned if they
+      // match a remote originator_item_id.
+      ReplaceBookmarkNodeGUID(node, base::GenerateGUID(), bookmark_model);
+      continue;
     }
-    guid_to_local_node_map.emplace(node->guid(), node);
+
+    guid_to_match_map.emplace(node->guid(), GuidMatch{node, remote_node});
   }
-  return guid_to_local_node_map;
+
+  return guid_to_match_map;
 }
 
 void BookmarkModelMerger::MergeSubtree(
@@ -422,25 +420,27 @@ const bookmarks::BookmarkNode* BookmarkModelMerger::FindMatchingLocalNode(
     const bookmarks::BookmarkNode* local_parent,
     size_t local_child_start_index) const {
   // Try to match child by GUID. If we can't, try to match child by semantics.
-  const bookmarks::BookmarkNode* matching_local_node =
+  const bookmarks::BookmarkNode* matching_local_node_by_guid =
       FindMatchingLocalNodeByGUID(remote_child);
-  if (!matching_local_node) {
-    // All local nodes up to |remote_index-1| have processed already. Look for a
-    // matching local node starting with the local node at position
-    // |local_child_start_index|. FindMatchingChildBySemanticsStartingAt()
-    // returns kInvalidIndex in the case where no semantics match was found or
-    // the semantics match found is GUID-matchable to a different node.
-    const size_t local_index = FindMatchingChildBySemanticsStartingAt(
-        /*remote_node=*/remote_child,
-        /*local_parent=*/local_parent,
-        /*starting_child_index=*/local_child_start_index);
-    if (local_index == kInvalidIndex) {
-      // If no match found, return.
-      return nullptr;
-    }
-    matching_local_node = local_parent->children()[local_index].get();
+  if (matching_local_node_by_guid) {
+    return matching_local_node_by_guid;
+  }
+
+  // All local nodes up to |remote_index-1| have processed already. Look for a
+  // matching local node starting with the local node at position
+  // |local_child_start_index|. FindMatchingChildBySemanticsStartingAt()
+  // returns kInvalidIndex in the case where no semantics match was found or
+  // the semantics match found is GUID-matchable to a different node.
+  const size_t local_index = FindMatchingChildBySemanticsStartingAt(
+      /*remote_node=*/remote_child,
+      /*local_parent=*/local_parent,
+      /*starting_child_index=*/local_child_start_index);
+  if (local_index == kInvalidIndex) {
+    // If no match found, return.
+    return nullptr;
   }
-  return matching_local_node;
+
+  return local_parent->children()[local_index].get();
 }
 
 const bookmarks::BookmarkNode*
@@ -471,8 +471,6 @@ BookmarkModelMerger::UpdateBookmarkNodeFromSpecificsIncludingGUID(
     return local_node;
   }
   DCHECK(base::IsValidGUID(specifics.guid()));
-  // We do not update the GUID maps upon node replacement as per the comment
-  // in bookmark_model_merger.h.
   return ReplaceBookmarkNodeGUID(local_node, specifics.guid(), bookmark_model_);
 }
 
@@ -574,7 +572,7 @@ void BookmarkModelMerger::ProcessLocalCreation(
     if (FindMatchingRemoteNodeByGUID(node->children()[i].get())) {
       continue;
     }
-    ProcessLocalCreation(node, i);
+    ProcessLocalCreation(/*parent=*/node, i);
   }
 }
 
@@ -612,36 +610,25 @@ const BookmarkModelMerger::RemoteTreeNode*
 BookmarkModelMerger::FindMatchingRemoteNodeByGUID(
     const bookmarks::BookmarkNode* local_node) const {
   DCHECK(local_node);
-  // Ensure matching nodes are of the same type and have the same URL,
-  // guaranteed by BuildGUIDToLocalNodeMap().
-  const auto it = guid_to_remote_node_map_.find(local_node->guid());
-  if (it == guid_to_remote_node_map_.end()) {
+
+  const auto it = guid_to_match_map_.find(local_node->guid());
+  if (it == guid_to_match_map_.end()) {
     return nullptr;
   }
 
-  const RemoteTreeNode* const remote_node = it->second;
-  DCHECK(remote_node);
-  DCHECK_EQ(local_node->is_folder(), remote_node->entity().is_folder);
-  DCHECK_EQ(local_node->url(),
-            remote_node->entity().specifics.bookmark().url());
-  return remote_node;
+  return it->second.remote_node;
 }
 
 const bookmarks::BookmarkNode* BookmarkModelMerger::FindMatchingLocalNodeByGUID(
     const RemoteTreeNode& remote_node) const {
   const syncer::EntityData& remote_entity = remote_node.entity();
   const auto it =
-      guid_to_local_node_map_.find(remote_entity.specifics.bookmark().guid());
-  if (it == guid_to_local_node_map_.end()) {
+      guid_to_match_map_.find(remote_entity.specifics.bookmark().guid());
+  if (it == guid_to_match_map_.end()) {
     return nullptr;
   }
-  DCHECK(!remote_entity.specifics.bookmark().guid().empty());
-  const bookmarks::BookmarkNode* local_node = it->second;
-  // Ensure matching nodes are of the same type and have the same URL,
-  // guaranteed by BuildGUIDToLocalNodeMap().
-  DCHECK_EQ(local_node->is_folder(), remote_entity.is_folder);
-  DCHECK_EQ(local_node->url(), remote_entity.specifics.bookmark().url());
-  return local_node;
+
+  return it->second.local_node;
 }
 
 }  // namespace sync_bookmarks

components/sync_bookmarks/bookmark_model_merger.h

@@ -56,6 +56,14 @@ class BookmarkModelMerger {
   // a permanent node, keyed by server-defined unique tag of the root.
   using RemoteForest = std::unordered_map<std::string, RemoteTreeNode>;
 
+  // Represents a pair of bookmarks, one local and one remote, that have been
+  // matched by GUID. They are guaranteed to have the same type and URL (if
+  // applicable).
+  struct GuidMatch {
+    const bookmarks::BookmarkNode* local_node;
+    const RemoteTreeNode* remote_node;
+  };
+
   // Constructs the remote bookmark tree to be merged. Each entry in the
   // returned map is a permanent node, identified (keyed) by the server-defined
   // tag. All invalid updates are filtered out, including invalid bookmark
@@ -63,22 +71,12 @@ class BookmarkModelMerger {
   // sends tombstones as part of the initial download.
   static RemoteForest BuildRemoteForest(syncer::UpdateResponseDataList updates);
 
-  // Constructs a map from GUID to corresponding remote nodes, used in the merge
-  // process to determine GUID-based node matches.
-  static std::unordered_map<std::string, const RemoteTreeNode*>
-  BuildGUIDToRemoteNodeMap(const RemoteForest& remote_forest);
-
-  // Constructs a map from GUID to corresponding local node, used in the merge
-  // process to determine GUID-based node matches. |bookmark_model| must not be
-  // null. |guid_to_remote_node_map| is used to detect conflicting GUID matches
-  // between local and remote bookmarks for the cases where they cannot be
-  // merged (e.g. folder vs non-folder) and in such cases regenerate a random
-  // GUID for the local bookmark.
-  static std::unordered_map<std::string, const bookmarks::BookmarkNode*>
-  BuildGUIDToLocalNodeMap(
-      bookmarks::BookmarkModel* bookmark_model,
-      const std::unordered_map<std::string, const RemoteTreeNode*>&
-          guid_to_remote_node_map);
+  // Computes bookmark pairs that should be matched by GUID. Local bookmark
+  // GUIDs may be regenerated for the case where they collide with a remote GUID
+  // that is not compatible (e.g. folder vs non-folder).
+  static std::unordered_map<std::string, GuidMatch>
+  FindGuidMatchesOrReassignLocal(const RemoteForest& remote_forest,
+                                 bookmarks::BookmarkModel* bookmark_model);
 
   // Merges a local and a remote subtrees. The input nodes are two equivalent
   // local and remote nodes. This method tries to recursively match their
@@ -151,15 +149,7 @@ class BookmarkModelMerger {
   // Preprocessed remote nodes in the form a forest where each tree's root is a
   // permanent node. Computed upon construction via BuildRemoteForest().
   const RemoteForest remote_forest_;
-  // Maps GUIDs to their respective remote nodes. Used for GUID-based node
-  // matching and is populated at the beginning of the merge process.
-  const std::unordered_map<std::string, const RemoteTreeNode*>
-      guid_to_remote_node_map_;
-  // Maps GUIDs to their respective local nodes. Used for GUID-based
-  // node matching and is populated at the beginning of the merge process.
-  // Is not updated upon potential changes to the model during merge.
-  const std::unordered_map<std::string, const bookmarks::BookmarkNode*>
-      guid_to_local_node_map_;
+  std::unordered_map<std::string, GuidMatch> guid_to_match_map_;
 
   DISALLOW_COPY_AND_ASSIGN(BookmarkModelMerger);
 };

components/sync_bookmarks/bookmark_specifics_conversions.cc

@@ -255,6 +255,12 @@ const bookmarks::BookmarkNode* ReplaceBookmarkNodeGUID(
   }
   const bookmarks::BookmarkNode* new_node;
   DCHECK(base::IsValidGUID(guid));
+
+  if (node->guid() == guid) {
+    // Nothing to do.
+    return node;
+  }
+
   if (node->is_folder()) {
     new_node =
         model->AddFolder(node->parent(), node->parent()->GetIndexOf(node),