Populate outgoing sync bookmark GUIDs

The field gets populated only if the committing client is sure about
the GUID being the authoritative one (consistent with the originator
client item ID).

Instead of introducing a new field for the originator item ID or a
boolean representing whether the GUID is indeed authoritative/final,
the existing field |client_tag_hash| is leveraged for this purpose,
which was unpopulated for bookmarks prior to this patch.

This is aligned with future plans to actually introduce a client tag
for bookmarks, just like for the rest of sync datatypes.

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

chrome/browser/sync/test/integration/bookmarks_helper.cc

@@ -373,6 +373,11 @@ bool NodesMatch(const BookmarkNode* node_a, const BookmarkNode* node_b) {
                << node_b->parent()->GetIndexOf(node_b);
     return false;
   }
+  if (node_a->guid() != node_b->guid()) {
+    LOG(ERROR) << "GUID mismatch: " << node_a->guid() << " vs. "
+               << node_b->guid();
+    return false;
+  }
   return true;
 }
 
@@ -530,7 +535,8 @@ const BookmarkNode* AddURL(int profile,
     const BookmarkNode* v_parent = nullptr;
     FindNodeInVerifier(model, parent, &v_parent);
     const BookmarkNode* v_node = GetVerifierBookmarkModel()->AddURL(
-        v_parent, index, base::UTF8ToUTF16(title), url);
+        v_parent, index, base::UTF8ToUTF16(title), url,
+        /*meta_info=*/nullptr, result->date_added(), result->guid());
     if (!v_node) {
       LOG(ERROR) << "Could not add bookmark " << title << " to the verifier";
       return nullptr;
@@ -573,7 +579,8 @@ const BookmarkNode* AddFolder(int profile,
     const BookmarkNode* v_parent = nullptr;
     FindNodeInVerifier(model, parent, &v_parent);
     const BookmarkNode* v_node = GetVerifierBookmarkModel()->AddFolder(
-        v_parent, index, base::UTF8ToUTF16(title));
+        v_parent, index, base::UTF8ToUTF16(title),
+        /*meta_info=*/nullptr, result->guid());
     if (!v_node) {
       LOG(ERROR) << "Could not add folder " << title << " to the verifier";
       return nullptr;

components/sync_bookmarks/bookmark_local_changes_builder.cc

@@ -64,7 +64,8 @@ syncer::CommitRequestDataList BookmarkLocalChangesBuilder::BuildCommitRequests(
       // Assign specifics only for the non-deletion case. In case of deletion,
       // EntityData should contain empty specifics to indicate deletion.
       data->specifics = CreateSpecificsFromBookmarkNode(
-          node, bookmark_model_, /*force_favicon_load=*/true);
+          node, bookmark_model_, /*force_favicon_load=*/true,
+          entity->has_final_guid());
       data->name = data->specifics.bookmark().title();
     }
     auto request = std::make_unique<syncer::CommitRequestData>();

components/sync_bookmarks/bookmark_model_merger.cc

@@ -614,8 +614,10 @@ void BookmarkModelMerger::ProcessLocalCreation(
             predecessor_entity->metadata()->unique_position()),
         suffix);
   }
+
   const sync_pb::EntitySpecifics specifics = CreateSpecificsFromBookmarkNode(
-      node, bookmark_model_, /*force_favicon_load=*/true);
+      node, bookmark_model_, /*force_favicon_load=*/true,
+      /*include_guid=*/true);
   bookmark_tracker_->Add(sync_id, node, server_version, creation_time,
                          pos.ToProto(), specifics);
   // Mark the entity that it needs to be committed.

components/sync_bookmarks/bookmark_model_observer_impl.cc

@@ -66,8 +66,8 @@ void BookmarkModelObserverImpl::BookmarkNodeMoved(
   const sync_pb::UniquePosition unique_position =
       ComputePosition(*new_parent, new_index, sync_id).ToProto();
 
-  sync_pb::EntitySpecifics specifics =
-      CreateSpecificsFromBookmarkNode(node, model, /*force_favicon_load=*/true);
+  sync_pb::EntitySpecifics specifics = CreateSpecificsFromBookmarkNode(
+      node, model, /*force_favicon_load=*/true, entity->has_final_guid());
 
   bookmark_tracker_->Update(sync_id, entity->metadata()->server_version(),
                             modification_time, unique_position, specifics);
@@ -106,8 +106,8 @@ void BookmarkModelObserverImpl::BookmarkNodeAdded(
       ComputePosition(*parent, index, sync_id).ToProto();
 
   sync_pb::EntitySpecifics specifics =
-      CreateSpecificsFromBookmarkNode(node, model, /*force_favicon_load=*/true);
-
+      CreateSpecificsFromBookmarkNode(node, model, /*force_favicon_load=*/true,
+                                      /*include_guid=*/true);
   bookmark_tracker_->Add(sync_id, node, server_version, creation_time,
                          unique_position, specifics);
   // Mark the entity that it needs to be committed.
@@ -184,9 +184,11 @@ void BookmarkModelObserverImpl::BookmarkNodeChanged(
     //    start tracking the node.
     return;
   }
+
   const base::Time modification_time = base::Time::Now();
-  sync_pb::EntitySpecifics specifics =
-      CreateSpecificsFromBookmarkNode(node, model, /*force_favicon_load=*/true);
+  sync_pb::EntitySpecifics specifics = CreateSpecificsFromBookmarkNode(
+      node, model, /*force_favicon_load=*/true, entity->has_final_guid());
+
   // TODO(crbug.com/516866): The below CHECKs are added to debug some crashes.
   // Should be removed after figuring out the reason for the crash.
   CHECK_EQ(entity, bookmark_tracker_->GetEntityForBookmarkNode(node));
@@ -266,7 +268,8 @@ void BookmarkModelObserverImpl::BookmarkNodeChildrenReordered(
                    : syncer::UniquePosition::After(position, suffix);
 
     const sync_pb::EntitySpecifics specifics = CreateSpecificsFromBookmarkNode(
-        child.get(), model, /*force_favicon_load=*/true);
+        child.get(), model, /*force_favicon_load=*/true,
+        entity->has_final_guid());
 
     bookmark_tracker_->Update(sync_id, entity->metadata()->server_version(),
                               modification_time, position.ToProto(), specifics);

components/sync_bookmarks/bookmark_model_type_processor.cc

@@ -17,6 +17,7 @@
 #include "components/bookmarks/browser/bookmark_model.h"
 #include "components/bookmarks/browser/bookmark_node.h"
 #include "components/bookmarks/browser/bookmark_utils.h"
+#include "components/sync/base/client_tag_hash.h"
 #include "components/sync/base/data_type_histogram.h"
 #include "components/sync/base/model_type.h"
 #include "components/sync/base/time.h"
@@ -593,8 +594,10 @@ void BookmarkModelTypeProcessor::AppendNodeAndChildrenForDebugging(
   data.name = base::UTF16ToUTF8(node->GetTitle());
   data.is_folder = node->is_folder();
   data.unique_position = metadata->unique_position();
-  data.specifics = CreateSpecificsFromBookmarkNode(
-      node, bookmark_model_, /*force_favicon_load=*/false);
+  data.specifics = CreateSpecificsFromBookmarkNode(node, bookmark_model_,
+                                                   /*force_favicon_load=*/false,
+                                                   entity->has_final_guid());
+
   if (node->is_permanent_node()) {
     data.server_defined_unique_tag =
         ComputeServerDefinedUniqueTagForDebugging(node, bookmark_model_);

components/sync_bookmarks/bookmark_remote_updates_handler.cc

@@ -131,16 +131,16 @@ void ApplyRemoteUpdate(
     return;
   }
 
-  // If there is a different GUID in the specifics and it is valid, we must
-  // replace the entire node in order to use it, as GUIDs are immutable. Further
-  // updates are then applied to the new node instead.
-  if (update_entity.specifics.bookmark().guid() != node->guid() &&
-      base::IsValidGUID(update_entity.specifics.bookmark().guid())) {
-    const bookmarks::BookmarkNode* old_node = node;
-    node = ReplaceBookmarkNodeGUID(
-        node, update_entity.specifics.bookmark().guid(), model);
-    tracker->UpdateBookmarkNodePointer(old_node, node);
+  // The GUID is immutable and cannot change. This check is somewhat redundant
+  // here because there is a similar one also accounted to kUnexpectedGuid, but
+  // in theory a misbehaving server could trigger this codepath.
+  // TODO(crbug.com/1032052): If the tracker becomes client-tag-centric, this
+  // shouldn't be necessary.
+  if (update_entity.specifics.bookmark().guid() != node->guid()) {
+    LogProblematicBookmark(RemoteBookmarkUpdateError::kUnexpectedGuid);
+    return;
   }
+
   UpdateBookmarkNodeFromSpecifics(update_entity.specifics.bookmark(), node,
                                   model, favicon_service);
   // Compute index information before updating the |tracker|.
@@ -222,6 +222,19 @@ void BookmarkRemoteUpdatesHandler::Process(
 
     const SyncedBookmarkTracker::Entity* tracked_entity =
         bookmark_tracker_->GetEntityForSyncId(update_entity.id);
+
+    // This may be a good chance to populate the client tag for the first time.
+    // TODO(crbug.com/1032052): Remove this code once all local sync metadata
+    // is required to populate the client tag (and be considered invalid
+    // otherwise).
+    if (tracked_entity && !tracked_entity->has_final_guid() &&
+        !update_entity.is_deleted() &&
+        update_entity.server_defined_unique_tag.empty()) {
+      DCHECK(base::IsValidGUID(update_entity.specifics.bookmark().guid()));
+      bookmark_tracker_->PopulateFinalGuid(
+          update_entity.id, update_entity.specifics.bookmark().guid());
+    }
+
     if (tracked_entity && tracked_entity->metadata()->server_version() >=
                               update->response_version) {
       // Seen this update before; just ignore it.
@@ -237,9 +250,7 @@ void BookmarkRemoteUpdatesHandler::Process(
     // |original_client_item_id|. If we have a entry by that description, we
     // should update the |sync_id| in |bookmark_tracker_|. The rest of code will
     // handle this a conflict and adjust the model if needed.
-    if (update_entity.originator_cache_guid ==
-            bookmark_tracker_->model_type_state().cache_guid() &&
-        bookmark_tracker_->GetEntityForSyncId(
+    if (bookmark_tracker_->GetEntityForSyncId(
             update_entity.originator_client_item_id) != nullptr) {
       if (tracked_entity) {
         // We generally shouldn't have an entry for both the old ID and the new

components/sync_bookmarks/bookmark_remote_updates_handler_unittest.cc

@@ -436,7 +436,7 @@ TEST_F(BookmarkRemoteUpdatesHandlerWithInitialMergeTest,
 }
 
 TEST_F(BookmarkRemoteUpdatesHandlerWithInitialMergeTest,
-       ShouldUpdateGUIDValueUponRemoteUpdate) {
+       ShouldLogGuidMismatchUponInvalidRemoteUpdate) {
   const std::string kId = "id";
   const std::string kTitle = "title";
   const std::string kOldGuid = base::GenerateGUID();
@@ -474,14 +474,20 @@ TEST_F(BookmarkRemoteUpdatesHandlerWithInitialMergeTest,
       syncer::UniquePosition::InitialPosition(
           syncer::UniquePosition::RandomSuffix())));
 
+  base::HistogramTester histogram_tester;
   updates_handler()->Process(updates,
                              /*got_new_encryption_requirements=*/false);
 
-  // The GUID should have been updated.
+  // The GUID should not have been updated.
   const bookmarks::BookmarkNode* bookmark_bar_node =
       bookmark_model()->bookmark_bar_node();
   ASSERT_THAT(bookmark_bar_node->children().size(), Eq(1u));
-  EXPECT_THAT(bookmark_bar_node->children().front()->guid(), Eq(kNewGuid));
+  EXPECT_THAT(bookmark_bar_node->children().front()->guid(), Eq(kOldGuid));
+
+  histogram_tester.ExpectBucketCount(
+      "Sync.ProblematicServerSideBookmarks",
+      /*sample=*/ExpectedRemoteBookmarkUpdateError::kUnexpectedGuid,
+      /*count=*/1);
 }
 
 TEST_F(BookmarkRemoteUpdatesHandlerWithInitialMergeTest,
@@ -592,6 +598,7 @@ TEST_F(BookmarkRemoteUpdatesHandlerWithInitialMergeTest,
   //  |- node4
 
   std::vector<std::string> ids;
+  std::vector<std::string> guids;
   std::vector<syncer::UniquePosition> positions;
 
   syncer::UniquePosition position = syncer::UniquePosition::InitialPosition(
@@ -599,11 +606,13 @@ TEST_F(BookmarkRemoteUpdatesHandlerWithInitialMergeTest,
   syncer::UpdateResponseDataList updates;
   for (int i = 0; i < 5; i++) {
     ids.push_back("node" + base::NumberToString(i));
+    guids.push_back(base::GenerateGUID());
     position = syncer::UniquePosition::After(
         position, syncer::UniquePosition::RandomSuffix());
     positions.push_back(position);
     updates.push_back(CreateUpdateResponseData(
-        /*server_id=*/ids[i], /*parent_id=*/kBookmarkBarId, /*title=*/ids[i],
+        /*server_id=*/ids[i], /*parent_id=*/kBookmarkBarId, /*guid=*/guids[i],
+        /*title=*/ids[i],
         /*is_deletion=*/false, /*version=*/0,
         /*unique_position=*/positions[i]));
   }
@@ -626,6 +635,7 @@ TEST_F(BookmarkRemoteUpdatesHandlerWithInitialMergeTest,
   updates.push_back(CreateUpdateResponseData(
       /*server_id=*/ids[3],
       /*parent_id=*/kBookmarkBarId,
+      /*guid=*/guids[3],
       /*title=*/ids[3],
       /*is_deletion=*/false,
       /*version=*/1,
@@ -652,6 +662,7 @@ TEST_F(BookmarkRemoteUpdatesHandlerWithInitialMergeTest,
   //  |- node4
 
   std::vector<std::string> ids;
+  std::vector<std::string> guids;
   std::vector<syncer::UniquePosition> positions;
 
   syncer::UniquePosition position = syncer::UniquePosition::InitialPosition(
@@ -659,11 +670,13 @@ TEST_F(BookmarkRemoteUpdatesHandlerWithInitialMergeTest,
   syncer::UpdateResponseDataList updates;
   for (int i = 0; i < 5; i++) {
     ids.push_back("node" + base::NumberToString(i));
+    guids.push_back(base::GenerateGUID());
     position = syncer::UniquePosition::After(
         position, syncer::UniquePosition::RandomSuffix());
     positions.push_back(position);
     updates.push_back(CreateUpdateResponseData(
-        /*server_id=*/ids[i], /*parent_id=*/kBookmarkBarId, /*title=*/ids[i],
+        /*server_id=*/ids[i], /*parent_id=*/kBookmarkBarId, /*guid=*/guids[i],
+        /*title=*/ids[i],
         /*is_deletion=*/false, /*version=*/0,
         /*unique_position=*/positions[i]));
   }
@@ -686,6 +699,7 @@ TEST_F(BookmarkRemoteUpdatesHandlerWithInitialMergeTest,
   updates.push_back(CreateUpdateResponseData(
       /*server_id=*/ids[1],
       /*parent_id=*/kBookmarkBarId,
+      /*guid=*/guids[1],
       /*title=*/ids[1],
       /*is_deletion=*/false,
       /*version=*/1,
@@ -712,6 +726,7 @@ TEST_F(BookmarkRemoteUpdatesHandlerWithInitialMergeTest,
   //  |- node4
 
   std::vector<std::string> ids;
+  std::vector<std::string> guids;
   std::vector<syncer::UniquePosition> positions;
 
   syncer::UniquePosition position = syncer::UniquePosition::InitialPosition(
@@ -719,11 +734,13 @@ TEST_F(BookmarkRemoteUpdatesHandlerWithInitialMergeTest,
   syncer::UpdateResponseDataList updates;
   for (int i = 0; i < 5; i++) {
     ids.push_back("node" + base::NumberToString(i));
+    guids.push_back(base::GenerateGUID());
     position = syncer::UniquePosition::After(
         position, syncer::UniquePosition::RandomSuffix());
     positions.push_back(position);
     updates.push_back(CreateUpdateResponseData(
-        /*server_id=*/ids[i], /*parent_id=*/kBookmarkBarId, /*title=*/ids[i],
+        /*server_id=*/ids[i], /*parent_id=*/kBookmarkBarId, /*guid=*/guids[i],
+        /*title=*/ids[i],
         /*is_deletion=*/false, /*version=*/0,
         /*unique_position=*/positions[i]));
   }
@@ -746,6 +763,7 @@ TEST_F(BookmarkRemoteUpdatesHandlerWithInitialMergeTest,
   updates.push_back(CreateUpdateResponseData(
       /*server_id=*/ids[4],
       /*parent_id=*/ids[1],
+      /*guid=*/guids[4],
       /*title=*/ids[4],
       /*is_deletion=*/false,
       /*version=*/1,
@@ -1300,6 +1318,7 @@ TEST_F(BookmarkRemoteUpdatesHandlerWithInitialMergeTest,
 TEST_F(BookmarkRemoteUpdatesHandlerWithInitialMergeTest,
        ShouldResolveConflictBetweenLocalAndRemoteUpdatesWithRemote) {
   const std::string kId = "id";
+  const std::string kGuid = base::GenerateGUID();
   const std::string kTitle = "title";
   const std::string kNewRemoteTitle = "remote title";
 
@@ -1308,6 +1327,7 @@ TEST_F(BookmarkRemoteUpdatesHandlerWithInitialMergeTest,
   updates.push_back(CreateUpdateResponseData(
       /*server_id=*/kId,
       /*parent_id=*/kBookmarkBarId,
+      /*guid=*/kGuid,
       /*title=*/kTitle,
       /*is_deletion=*/false,
       /*version=*/0,
@@ -1330,6 +1350,7 @@ TEST_F(BookmarkRemoteUpdatesHandlerWithInitialMergeTest,
   updates.push_back(CreateUpdateResponseData(
       /*server_id=*/kId,
       /*parent_id=*/kBookmarkBarId,
+      /*guid=*/kGuid,
       /*title=*/kNewRemoteTitle,
       /*is_deletion=*/false,
       /*version=*/1,

components/sync_bookmarks/bookmark_specifics_conversions.cc

@@ -184,7 +184,8 @@ std::string InferGuidForLegacyBookmark(
 sync_pb::EntitySpecifics CreateSpecificsFromBookmarkNode(
     const bookmarks::BookmarkNode* node,
     bookmarks::BookmarkModel* model,
-    bool force_favicon_load) {
+    bool force_favicon_load,
+    bool include_guid) {
   sync_pb::EntitySpecifics specifics;
   sync_pb::BookmarkSpecifics* bm_specifics = specifics.mutable_bookmark();
   if (!node->is_folder()) {
@@ -194,8 +195,9 @@ sync_pb::EntitySpecifics CreateSpecificsFromBookmarkNode(
   DCHECK(!node->guid().empty());
   DCHECK(base::IsValidGUID(node->guid())) << "Actual: " << node->guid();
 
-  // TODO(crbug.com/978430): populate the GUID, as long as it's known to be
-  // consistent with the originator client item ID.
+  if (include_guid) {
+    bm_specifics->set_guid(node->guid());
+  }
 
   bm_specifics->set_title(SpecificsTitleFromNodeTitle(node->GetTitle()));
   bm_specifics->set_creation_time_us(

components/sync_bookmarks/bookmark_specifics_conversions.h

@@ -24,10 +24,14 @@ class FaviconService;
 
 namespace sync_bookmarks {
 
+// TODO(crbug.com/978430): Remove argument |include_guid| once the client tag
+// hash is required to be populated during sync metadata validation upon
+// startup in SyncedBookmarkTracker::BookmarkModelMatchesMetadata().
 sync_pb::EntitySpecifics CreateSpecificsFromBookmarkNode(
     const bookmarks::BookmarkNode* node,
     bookmarks::BookmarkModel* model,
-    bool force_favicon_load);
+    bool force_favicon_load,
+    bool include_guid);
 
 // Creates a bookmark node under the given parent node from the given specifics.
 // Returns the newly created node. Callers must verify that

components/sync_bookmarks/bookmark_specifics_conversions_unittest.cc

@@ -76,10 +76,9 @@ TEST(BookmarkSpecificsConversionsTest, ShouldCreateSpecificsFromBookmarkNode) {
   model->SetNodeMetaInfo(node, kKey2, kValue2);
 
   sync_pb::EntitySpecifics specifics = CreateSpecificsFromBookmarkNode(
-      node, model.get(), /*force_favicon_load=*/false);
+      node, model.get(), /*force_favicon_load=*/false, /*include_guid=*/true);
   const sync_pb::BookmarkSpecifics& bm_specifics = specifics.bookmark();
-  // TODO(crbug.com/978430): Update expectation once the GUID is populated.
-  EXPECT_FALSE(bm_specifics.has_guid());
+  EXPECT_THAT(bm_specifics.guid(), Eq(node->guid()));
   EXPECT_THAT(bm_specifics.title(), Eq(kTitle));
   EXPECT_THAT(GURL(bm_specifics.url()), Eq(kUrl));
   EXPECT_THAT(
@@ -93,6 +92,28 @@ TEST(BookmarkSpecificsConversionsTest, ShouldCreateSpecificsFromBookmarkNode) {
   }
 }
 
+TEST(BookmarkSpecificsConversionsTest,
+     ShouldCreateSpecificsFromBookmarkNodeWithoutGuid) {
+  const GURL kUrl("http://www.url.com");
+  const std::string kTitle = "Title";
+
+  std::unique_ptr<bookmarks::BookmarkModel> model =
+      bookmarks::TestBookmarkClient::CreateModel();
+
+  const bookmarks::BookmarkNode* bookmark_bar_node = model->bookmark_bar_node();
+  const bookmarks::BookmarkNode* node = model->AddURL(
+      /*parent=*/bookmark_bar_node, /*index=*/0, base::UTF8ToUTF16(kTitle),
+      kUrl);
+  ASSERT_THAT(node, NotNull());
+
+  sync_pb::EntitySpecifics specifics = CreateSpecificsFromBookmarkNode(
+      node, model.get(), /*force_favicon_load=*/false, /*include_guid=*/false);
+  const sync_pb::BookmarkSpecifics& bm_specifics = specifics.bookmark();
+  ASSERT_THAT(bm_specifics.title(), Eq(kTitle));
+  ASSERT_THAT(GURL(bm_specifics.url()), Eq(kUrl));
+  EXPECT_FALSE(bm_specifics.has_guid());
+}
+
 TEST(BookmarkSpecificsConversionsTest,
      ShouldCreateSpecificsFromBookmarkNodeWithIllegalTitle) {
   std::unique_ptr<bookmarks::BookmarkModel> model =
@@ -107,7 +128,7 @@ TEST(BookmarkSpecificsConversionsTest,
         GURL("http://www.url.com"));
     ASSERT_THAT(node, NotNull());
     sync_pb::EntitySpecifics specifics = CreateSpecificsFromBookmarkNode(
-        node, model.get(), /*force_favicon_load=*/false);
+        node, model.get(), /*force_favicon_load=*/false, /*include_guid=*/true);
     // Legacy clients append a space to illegal titles.
     EXPECT_THAT(specifics.bookmark().title(), Eq(illegal_title + " "));
   }
@@ -123,7 +144,7 @@ TEST(BookmarkSpecificsConversionsTest,
   ASSERT_THAT(node, NotNull());
 
   sync_pb::EntitySpecifics specifics = CreateSpecificsFromBookmarkNode(
-      node, model.get(), /*force_favicon_load=*/false);
+      node, model.get(), /*force_favicon_load=*/false, /*include_guid=*/true);
   const sync_pb::BookmarkSpecifics& bm_specifics = specifics.bookmark();
   EXPECT_FALSE(bm_specifics.has_url());
 }
@@ -145,7 +166,7 @@ TEST(BookmarkSpecificsConversionsTest,
   ASSERT_FALSE(node->is_favicon_loaded());
   ASSERT_THAT(client_ptr->GetLoadFaviconRequestsForTest(), Eq(0));
   sync_pb::EntitySpecifics specifics = CreateSpecificsFromBookmarkNode(
-      node, model.get(), /*force_favicon_load=*/true);
+      node, model.get(), /*force_favicon_load=*/true, /*include_guid=*/true);
   EXPECT_THAT(client_ptr->GetLoadFaviconRequestsForTest(), Eq(1));
 }
 
@@ -166,7 +187,7 @@ TEST(BookmarkSpecificsConversionsTest,
   ASSERT_FALSE(node->is_favicon_loaded());
   ASSERT_THAT(client_ptr->GetLoadFaviconRequestsForTest(), Eq(0));
   sync_pb::EntitySpecifics specifics = CreateSpecificsFromBookmarkNode(
-      node, model.get(), /*force_favicon_load=*/false);
+      node, model.get(), /*force_favicon_load=*/false, /*include_guid=*/true);
   EXPECT_THAT(client_ptr->GetLoadFaviconRequestsForTest(), Eq(0));
 }
 

components/sync_bookmarks/synced_bookmark_tracker.cc

@@ -15,6 +15,7 @@
 #include "base/trace_event/memory_usage_estimator.h"
 #include "components/bookmarks/browser/bookmark_model.h"
 #include "components/bookmarks/browser/bookmark_node.h"
+#include "components/sync/base/client_tag_hash.h"
 #include "components/sync/base/time.h"
 #include "components/sync/base/unique_position.h"
 #include "components/sync/model/entity_data.h"
@@ -43,11 +44,7 @@ enum class CorruptionReason {
 void HashSpecifics(const sync_pb::EntitySpecifics& specifics,
                    std::string* hash) {
   DCHECK_GT(specifics.ByteSize(), 0);
-  // TODO(crbug.com/978430): Include GUID in hash when committing is supported.
-  sync_pb::EntitySpecifics specifics_without_guid = specifics;
-  specifics_without_guid.mutable_bookmark()->clear_guid();
-  base::Base64Encode(
-      base::SHA1HashString(specifics_without_guid.SerializeAsString()), hash);
+  base::Base64Encode(base::SHA1HashString(specifics.SerializeAsString()), hash);
 }
 
 }  // namespace
@@ -94,6 +91,16 @@ bool SyncedBookmarkTracker::Entity::MatchesSpecificsHash(
   return hash == metadata_->specifics_hash();
 }
 
+bool SyncedBookmarkTracker::Entity::has_final_guid() const {
+  return metadata_->has_client_tag_hash();
+}
+
+void SyncedBookmarkTracker::Entity::set_final_guid(const std::string& guid) {
+  DCHECK(!has_final_guid());
+  metadata_->set_client_tag_hash(
+      syncer::ClientTagHash::FromUnhashed(syncer::BOOKMARKS, guid).value());
+}
+
 size_t SyncedBookmarkTracker::Entity::EstimateMemoryUsage() const {
   using base::trace_event::EstimateMemoryUsage;
   size_t memory_usage = 0;
@@ -169,6 +176,14 @@ bool SyncedBookmarkTracker::BookmarkModelMatchesMetadata(
     // The entry is valid. If it's not a tombstone, collect its node id to
     // compare it later with the ids in the bookmark model.
     if (!bookmark_metadata.metadata().is_deleted()) {
+      // TODO(crbug.com/1032052): If the client-tag is known, verify that it
+      // matches the bookmark's GUID.
+
+      // TODO(crbug.com/1032052): Eventually empty client-tag-hashes should be
+      // disallowed and treated as invalid sync metadata. This requires a grace
+      // period for most clients to receive at least one update per bookmark.
+      // When that's achieved, has_final_guid() and set_final_guid() could be
+      // removed.
       metadata_node_ids.push_back(bookmark_metadata.id());
     }
   }
@@ -242,6 +257,11 @@ void SyncedBookmarkTracker::Add(const std::string& sync_id,
   metadata->set_sequence_number(0);
   metadata->set_acked_sequence_number(0);
   metadata->mutable_unique_position()->CopyFrom(unique_position);
+  // For any newly added bookmark, be it a local creation or a remote one, the
+  // authoritative final GUID is known from start.
+  metadata->set_client_tag_hash(syncer::ClientTagHash::FromUnhashed(
+                                    syncer::BOOKMARKS, bookmark_node->guid())
+                                    .value());
   HashSpecifics(specifics, metadata->mutable_specifics_hash());
   auto entity = std::make_unique<Entity>(bookmark_node, std::move(metadata));
   bookmark_node_to_entities_map_[bookmark_node] = entity.get();
@@ -261,6 +281,7 @@ void SyncedBookmarkTracker::Update(
   Entity* entity = GetMutableEntityForSyncId(sync_id);
   DCHECK(entity);
   DCHECK_EQ(entity->metadata()->server_id(), sync_id);
+
   entity->metadata()->set_server_version(server_version);
   entity->metadata()->set_modification_time(
       syncer::TimeToProtoTime(modification_time));
@@ -277,6 +298,14 @@ void SyncedBookmarkTracker::UpdateServerVersion(const std::string& sync_id,
   entity->metadata()->set_server_version(server_version);
 }
 
+void SyncedBookmarkTracker::PopulateFinalGuid(const std::string& sync_id,
+                                              const std::string& guid) {
+  Entity* entity = GetMutableEntityForSyncId(sync_id);
+  DCHECK(entity);
+  DCHECK(!entity->has_final_guid());
+  entity->set_final_guid(guid);
+}
+
 void SyncedBookmarkTracker::MarkCommitMayHaveStarted(
     const std::string& sync_id) {
   Entity* entity = GetMutableEntityForSyncId(sync_id);
@@ -510,18 +539,6 @@ void SyncedBookmarkTracker::UpdateSyncForLocalCreationIfNeeded(
   sync_id_to_entities_map_.erase(old_id);
 }
 
-void SyncedBookmarkTracker::UpdateBookmarkNodePointer(
-    const bookmarks::BookmarkNode* old_node,
-    const bookmarks::BookmarkNode* new_node) {
-  if (old_node == new_node) {
-    return;
-  }
-  bookmark_node_to_entities_map_[new_node] =
-      bookmark_node_to_entities_map_[old_node];
-  bookmark_node_to_entities_map_[new_node]->set_bookmark_node(new_node);
-  bookmark_node_to_entities_map_.erase(old_node);
-}
-
 void SyncedBookmarkTracker::AckSequenceNumber(const std::string& sync_id) {
   Entity* entity = GetMutableEntityForSyncId(sync_id);
   DCHECK(entity);

components/sync_bookmarks/synced_bookmark_tracker.h

@@ -87,6 +87,20 @@ class SyncedBookmarkTracker {
       commit_may_have_started_ = value;
     }
 
+    // Returns whether the bookmark's GUID is known to match the server-side
+    // originator client item ID (or for pre-2015 bookmarks, the equivalent
+    // inferred GUID). This function may return false negatives since the
+    // required local metadata got populated with M81.
+    // TODO(crbug.com/1032052): Remove this code once all local sync metadata
+    // is required to populate the client tag (and be considered invalid
+    // otherwise).
+    bool has_final_guid() const;
+
+    // TODO(crbug.com/1032052): Remove this code once all local sync metadata
+    // is required to populate the client tag (and be considered invalid
+    // otherwise).
+    void set_final_guid(const std::string& guid);
+
     // Returns the estimate of dynamically allocated memory in bytes.
     size_t EstimateMemoryUsage() const;
 
@@ -149,6 +163,9 @@ class SyncedBookmarkTracker {
   // Updates the server version of an existing entry for the |sync_id|.
   void UpdateServerVersion(const std::string& sync_id, int64_t server_version);
 
+  // Populates a bookmark's final GUID.
+  void PopulateFinalGuid(const std::string& sync_id, const std::string& guid);
+
   // Marks an existing entry for |sync_id| that a commit request might have been
   // sent to the server.
   void MarkCommitMayHaveStarted(const std::string& sync_id);
@@ -200,11 +217,6 @@ class SyncedBookmarkTracker {
   void UpdateSyncForLocalCreationIfNeeded(const std::string& old_id,
                                           const std::string& new_id);
 
-  // Informs the tracker that a BookmarkNode has been replaced. It updates
-  // the internal state of the tracker accordingly.
-  void UpdateBookmarkNodePointer(const bookmarks::BookmarkNode* old_node,
-                                 const bookmarks::BookmarkNode* new_node);
-
   // Set the value of |EntityMetadata.acked_sequence_number| in the entity with
   // |sync_id| to be equal to |EntityMetadata.sequence_number| such that it is
   // not returned in GetEntitiesWithLocalChanges().