Commit 51be6f11 authored by Marc Treib's avatar Marc Treib Committed by Commit Bot

Bookmark sync: Use random GUIDs as temporary server IDs

...instead of the actual GUID of the bookmark node.
This fixes a crash, see linked bug.
The problem is that right now, before the GUID-aware merge algorithm is
rolled out, bookmark GUIDs are not actually guaranteed to be unique.
This can result in two separate bookmark nodes getting assigned the
same server ID, resulting in a CHECK() crash.

This is an immediate fix for the crash; longer-term we need to make
sure that bookmark node GUIDs are actually guaranteed to be unique.

Bug: 1004205
Change-Id: Ic2e5e17b025b65e0739e6cb82a58053ff11209a9
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1864800
Commit-Queue: Marc Treib <treib@chromium.org>
Commit-Queue: Mohamed Amir Yosef <mamir@chromium.org>
Auto-Submit: Marc Treib <treib@chromium.org>
Reviewed-by: default avatarMohamed Amir Yosef <mamir@chromium.org>
Cr-Commit-Position: refs/heads/master@{#706999}
parent 2fbd78eb
...@@ -470,7 +470,10 @@ void BookmarkModelMerger::ProcessLocalCreation( ...@@ -470,7 +470,10 @@ void BookmarkModelMerger::ProcessLocalCreation(
const bookmarks::BookmarkNode* node = parent->children()[index].get(); const bookmarks::BookmarkNode* node = parent->children()[index].get();
DCHECK(!FindMatchingRemoteUpdateByGUID(node)); DCHECK(!FindMatchingRemoteUpdateByGUID(node));
DCHECK(base::IsValidGUID(node->guid())); DCHECK(base::IsValidGUID(node->guid()));
const std::string sync_id = node->guid(); // TODO(crbug.com/978430): Consider using |node->guid()| instead of generating
// a new random GUID. However, currently that can lead to crashes due to
// duplicate server IDs, see crbug.com/1004205.
const std::string sync_id = base::GenerateGUID();
const int64_t server_version = syncer::kUncommittedVersion; const int64_t server_version = syncer::kUncommittedVersion;
const base::Time creation_time = base::Time::Now(); const base::Time creation_time = base::Time::Now();
const std::string& suffix = syncer::GenerateSyncableBookmarkHash( const std::string& suffix = syncer::GenerateSyncableBookmarkHash(
......
...@@ -93,11 +93,12 @@ void BookmarkModelObserverImpl::BookmarkNodeAdded( ...@@ -93,11 +93,12 @@ void BookmarkModelObserverImpl::BookmarkNodeAdded(
// Similar to the directory implementation here: // Similar to the directory implementation here:
// https://cs.chromium.org/chromium/src/components/sync/syncable/mutable_entry.cc?l=237&gsn=CreateEntryKernel // https://cs.chromium.org/chromium/src/components/sync/syncable/mutable_entry.cc?l=237&gsn=CreateEntryKernel
// Assign a temp server id for the entity. Will be overriden by the actual // Assign a temp server id for the entity. Will be overriden by the actual
// server id upon receiving commit response. As this value later populates the // server id upon receiving commit response.
// originator_client_item_id field in EntityData, which replaces the GUID in
// legacy clients, it is set to the node's actual GUID.
DCHECK(base::IsValidGUID(node->guid())); DCHECK(base::IsValidGUID(node->guid()));
const std::string sync_id = node->guid(); // TODO(crbug.com/978430): Consider using |node->guid()| instead of generating
// a new random GUID. However, currently that can lead to crashes due to
// duplicate server IDs, see crbug.com/1004205.
const std::string sync_id = base::GenerateGUID();
const int64_t server_version = syncer::kUncommittedVersion; const int64_t server_version = syncer::kUncommittedVersion;
const base::Time creation_time = base::Time::Now(); const base::Time creation_time = base::Time::Now();
const sync_pb::UniquePosition unique_position = const sync_pb::UniquePosition unique_position =
......
...@@ -133,8 +133,12 @@ TEST_F(BookmarkModelObserverImplTest, ...@@ -133,8 +133,12 @@ TEST_F(BookmarkModelObserverImplTest,
bookmark_tracker()->GetEntitiesWithLocalChanges(kMaxEntries); bookmark_tracker()->GetEntitiesWithLocalChanges(kMaxEntries);
ASSERT_THAT(local_changes.size(), 1U); ASSERT_THAT(local_changes.size(), 1U);
EXPECT_THAT(local_changes[0]->bookmark_node(), Eq(bookmark_node)); EXPECT_THAT(local_changes[0]->bookmark_node(), Eq(bookmark_node));
EXPECT_THAT(local_changes[0]->metadata()->server_id(), // TODO(crbug.com/978430): Consider using |node->guid()| instead of generating
Eq(bookmark_node->guid())); // new random GUIDs as the temporary server ID, and then reinstate the
// following expectation. However, currently that can lead to crashes due to
// duplicate server IDs, see crbug.com/1004205.
// EXPECT_THAT(local_changes[0]->metadata()->server_id(),
// Eq(bookmark_node->guid()));
} }
TEST_F(BookmarkModelObserverImplTest, TEST_F(BookmarkModelObserverImplTest,
......
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