Commit 482d7a70 authored by Mikel Astiz's avatar Mikel Astiz Committed by Commit Bot

Add CHECK to detect local bookmark GUID duplicates

BookmarkModel is expected to have a unique GUID per bookmark.
BookmarkModel does not currently have [D]CHECKs to enforce this, but
it does get verified upon loading of bookmarks from storage.

Upon sync's initial merge, the presence of duplicate GUIDs locally is
specially problematic, in particular if GUID-based merging logic
is enabled (currently disabled by default). This is because local nodes
may end up untracked, which violates the invariant for bookmark sync
to work (and may later run into [D]CHECKs).

Change-Id: I76f6139ae341fe1ed4e1f37cc37d96704be0910d
Bug: 978430
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2030764
Auto-Submit: Mikel Astiz <mastiz@chromium.org>
Reviewed-by: default avatarMohamed Amir Yosef <mamir@chromium.org>
Commit-Queue: Mohamed Amir Yosef <mamir@chromium.org>
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#736866}
parent b5bc449e
...@@ -394,7 +394,16 @@ BookmarkModelMerger::FindGuidMatchesOrReassignLocal( ...@@ -394,7 +394,16 @@ BookmarkModelMerger::FindGuidMatchesOrReassignLocal(
continue; continue;
} }
guid_to_match_map.emplace(node->guid(), GuidMatch{node, remote_node}); bool success =
guid_to_match_map.emplace(node->guid(), GuidMatch{node, remote_node})
.second;
// Insertion must have succeeded unless there were duplicate GUIDs in the
// local BookmarkModel (invariant violation that gets resolved upon
// restart).
// TODO(crbug.com/516866): The below CHECK is added to debug some crashes.
// Should be converted to a DCHECK after the root cause if found.
CHECK(success);
} }
for (const bookmarks::BookmarkNode* node : nodes_to_replace_guid) { for (const bookmarks::BookmarkNode* node : nodes_to_replace_guid) {
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment