Fix bookmark GUID not being updated upon incremental sync updates

The recent https://crrev.com/c/1981610 removed the logic to update
local bookmark GUIDs upon remote incremental sync updates, but
overlooked the issues in the browser-upgrade path: and old client would
indefinitely continue with local bookmark GUIDs that never got updated,
and yet during commits those arbitrary GUIDs could get uploaded because
PopulateFinalGuid() had been called, and in fact client tag hash does
represent the authoritative bookmark GUID.

The fix proposed here is to revert back to the logic prior to the
offending patch linked above, and in addition add extra precautions
when uploading changes, via the newly-introduced
SyncedBookmarkTracker::Entity::final_guid_matches().

Because it's hard to reason about the effect of bad data in the
affected version range, and since the protobuf field number was
recently introduced (with no branch-point in between),
BookmarkSpecifics is updated again to start using a clean proto field
number.

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

components/sync/protocol/bookmark_specifics.proto

@@ -33,6 +33,7 @@ message BookmarkSpecifics {
   optional string icon_url = 5;
   repeated MetaInfo meta_info = 6;
   reserved 7;
+  reserved 8;
   // Introduced in M81, it represents a globally unique and immutable ID.
   //
   // If present, it must be the same as originator_client_item_id, unless
@@ -43,5 +44,5 @@ message BookmarkSpecifics {
   //
   // If not present, the value can be safely inferred using the very same
   // methods listed above.
-  optional string guid = 8;
+  optional string guid = 9;
 }

components/sync_bookmarks/bookmark_local_changes_builder.cc

@@ -63,9 +63,12 @@ syncer::CommitRequestDataList BookmarkLocalChangesBuilder::BuildCommitRequests(
       data->unique_position = metadata->unique_position();
       // Assign specifics only for the non-deletion case. In case of deletion,
       // EntityData should contain empty specifics to indicate deletion.
+      // TODO(crbug.com/978430): has_final_guid() should be enough below
+      // assuming that all codepaths that populate the final GUID make sure the
+      // local model has the appropriate GUID too (and update if needed).
       data->specifics = CreateSpecificsFromBookmarkNode(
           node, bookmark_model_, /*force_favicon_load=*/true,
-          entity->has_final_guid());
+          entity->final_guid_matches(node->guid()));
       data->name = data->specifics.bookmark().title();
     }
     auto request = std::make_unique<syncer::CommitRequestData>();

components/sync_bookmarks/bookmark_remote_updates_handler.cc

@@ -131,14 +131,15 @@ void ApplyRemoteUpdate(
     return;
   }
 
-  // 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;
+  // 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);
   }
 
   UpdateBookmarkNodeFromSpecifics(update_entity.specifics.bookmark(), node,
@@ -227,16 +228,25 @@ void BookmarkRemoteUpdatesHandler::Process(
     // 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());
+    bool local_guid_needs_update = false;
+    const std::string& remote_guid = update_entity.specifics.bookmark().guid();
+    if (tracked_entity && !update_entity.is_deleted() &&
+        update_entity.server_defined_unique_tag.empty() &&
+        !tracked_entity->final_guid_matches(remote_guid)) {
+      DCHECK(base::IsValidGUID(remote_guid));
+      bookmark_tracker_->PopulateFinalGuid(update_entity.id, remote_guid);
+      // In many cases final_guid_matches() may return false because a final
+      // GUID is not known for sure, but actually it matches the local GUID.
+      if (tracked_entity->bookmark_node() &&
+          remote_guid != tracked_entity->bookmark_node()->guid()) {
+        local_guid_needs_update = true;
+      }
     }
 
-    if (tracked_entity && tracked_entity->metadata()->server_version() >=
-                              update->response_version) {
+    if (tracked_entity &&
+        tracked_entity->metadata()->server_version() >=
+            update->response_version &&
+        !local_guid_needs_update) {
       // Seen this update before; just ignore it.
       continue;
     }

components/sync_bookmarks/bookmark_remote_updates_handler_unittest.cc

@@ -436,7 +436,7 @@ TEST_F(BookmarkRemoteUpdatesHandlerWithInitialMergeTest,
 }
 
 TEST_F(BookmarkRemoteUpdatesHandlerWithInitialMergeTest,
-       ShouldLogGuidMismatchUponInvalidRemoteUpdate) {
+       ShouldUpdateGUIDValueUponRemoteUpdate) {
   const std::string kId = "id";
   const std::string kTitle = "title";
   const std::string kOldGuid = base::GenerateGUID();
@@ -478,16 +478,16 @@ TEST_F(BookmarkRemoteUpdatesHandlerWithInitialMergeTest,
   updates_handler()->Process(updates,
                              /*got_new_encryption_requirements=*/false);
 
-  // The GUID should not have been updated.
+  // The GUID should 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(kOldGuid));
+  EXPECT_THAT(bookmark_bar_node->children().front()->guid(), Eq(kNewGuid));
 
   histogram_tester.ExpectBucketCount(
       "Sync.ProblematicServerSideBookmarks",
       /*sample=*/ExpectedRemoteBookmarkUpdateError::kUnexpectedGuid,
-      /*count=*/1);
+      /*count=*/0);
 }
 
 TEST_F(BookmarkRemoteUpdatesHandlerWithInitialMergeTest,

components/sync_bookmarks/synced_bookmark_tracker.cc

@@ -95,8 +95,15 @@ bool SyncedBookmarkTracker::Entity::has_final_guid() const {
   return metadata_->has_client_tag_hash();
 }
 
+bool SyncedBookmarkTracker::Entity::final_guid_matches(
+    const std::string& guid) const {
+  return metadata_->has_client_tag_hash() &&
+         metadata_->client_tag_hash() ==
+             syncer::ClientTagHash::FromUnhashed(syncer::BOOKMARKS, guid)
+                 .value();
+}
+
 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());
 }
@@ -302,7 +309,6 @@ 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);
 }
 
@@ -539,6 +545,18 @@ 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

@@ -96,6 +96,9 @@ class SyncedBookmarkTracker {
     // otherwise).
     bool has_final_guid() const;
 
+    // Returns true if the final GUID is known and it matches |guid|.
+    bool final_guid_matches(const std::string& 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).
@@ -217,6 +220,11 @@ 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().