[sync] Explose bookmark client tags in sync protocol to server

The logic is behind a newly-introduced feature toggle: the idea is that
bookmark commits to the server now contain a client tag hash, which is
the hash of the bookmark's GUID.

Populating this field is a simple change in BookmarkLocalChangesBuilder
but additional sophistication is introduced to guard against in-flight
commits and the time a user transitions from the feature being disabled
to being enabled (which can theoretically run into duplicates).

Note that the validation of the client tag hash for incoming updates
is *NOT* behind a feature toggle, but that is believed to be
non-controversial because there aren't many other options for defining
the client tag for bookmarks.

Bug: 1032052
Change-Id: I07448ad405849b29295c3090d3708d3a562e32e7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2623934Reviewed-by: Rushan Suleymanov <rushans@google.com>
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#844035}

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

@@ -18,6 +18,7 @@
 #include "chrome/browser/sync/test/integration/updated_progress_marker_checker.h"
 #include "components/bookmarks/browser/bookmark_model.h"
 #include "components/bookmarks/browser/url_and_title.h"
+#include "components/sync/base/client_tag_hash.h"
 #include "components/sync/driver/profile_sync_service.h"
 #include "components/sync/engine_impl/bookmark_update_preprocessing.h"
 #include "components/sync/engine_impl/loopback_server/loopback_server_entity.h"
@@ -157,6 +158,15 @@ class SingleClientBookmarksSyncTestWithEnabledReuploadPreexistingBookmarks
   }
 };
 
+class SingleClientBookmarksSyncTestWithEnabledClientTags : public SyncTest {
+ public:
+  SingleClientBookmarksSyncTestWithEnabledClientTags()
+      : SyncTest(SINGLE_CLIENT) {
+    feature_list_.InitAndEnableFeature(
+        switches::kSyncUseClientTagForBookmarkCommits);
+  }
+};
+
 IN_PROC_BROWSER_TEST_F(SingleClientBookmarksSyncTestWithVerifier, Sanity) {
   ASSERT_TRUE(SetupClients()) << "SetupClients() failed.";
 
@@ -1499,4 +1509,29 @@ IN_PROC_BROWSER_TEST_F(
                   .Wait());
 }
 
+IN_PROC_BROWSER_TEST_F(SingleClientBookmarksSyncTestWithEnabledClientTags,
+                       CommitLocalCreationWithClientTag) {
+  ASSERT_TRUE(SetupSync());
+
+  const std::string kTitle = "Title";
+  const BookmarkNode* folder = AddFolder(
+      kSingleProfileIndex, GetOtherNode(kSingleProfileIndex), 0, kTitle);
+
+  // Wait until the local bookmark gets committed.
+  ASSERT_TRUE(bookmarks_helper::ServerBookmarksEqualityChecker(
+                  GetSyncService(kSingleProfileIndex), GetFakeServer(),
+                  {{kTitle, /*url=*/GURL()}},
+                  /*cryptographer=*/nullptr)
+                  .Wait());
+
+  // Verify the client tag hash was committed to the server.
+  std::vector<sync_pb::SyncEntity> server_bookmarks =
+      GetFakeServer()->GetSyncEntitiesByModelType(syncer::BOOKMARKS);
+  ASSERT_EQ(1u, server_bookmarks.size());
+  EXPECT_EQ(server_bookmarks[0].client_defined_unique_tag(),
+            syncer::ClientTagHash::FromUnhashed(
+                syncer::BOOKMARKS, folder->guid().AsLowercaseString())
+                .value());
+}
+
 }  // namespace

components/sync/engine_impl/commit_contribution_impl.cc

@@ -197,7 +197,12 @@ void CommitContributionImpl::PopulateCommitProto(
   commit_proto->set_id_string(entity_data.id);
   // Populate client_defined_unique_tag only for non-bookmark and non-Nigori
   // data types.
-  if (type != BOOKMARKS && type != NIGORI) {
+  if (type == NIGORI) {
+    // Client tags are irrelevant for NIGORI (it uses the root node).
+  } else if (type != BOOKMARKS ||
+             !entity_data.client_tag_hash.value().empty()) {
+    // The client tag is mandatory for all datatypes except bookmarks, and
+    // experimental for bookmarks (behind feature toggle).
     commit_proto->set_client_defined_unique_tag(
         entity_data.client_tag_hash.value());
   }

components/sync/protocol/bookmark_model_metadata.proto

@@ -46,4 +46,12 @@ message BookmarkModelMetadata {
   // is required to populate the client tag (and be considered invalid
   // otherwise).
   optional int64 last_sync_time = 4;
+
+  // Represents whether bookmark commits sent to the server (most importantly
+  // creations) populate client tags. This is a layer on top of the usual
+  // FeatureList to avoid risky transitions during startup, to guard against
+  // in-flight commits.
+  // TODO(crbug.com/1032052): remove this code when the logic is enabled by
+  // default and enforced to true upon startup.
+  optional bool bookmark_client_tags_in_protocol_enabled = 5;
 }

components/sync_bookmarks/bookmark_local_changes_builder.cc

@@ -47,6 +47,19 @@ syncer::CommitRequestDataList BookmarkLocalChangesBuilder::BuildCommitRequests(
     data->creation_time = syncer::ProtoTimeToTime(metadata->creation_time());
     data->modification_time =
         syncer::ProtoTimeToTime(metadata->modification_time());
+
+    if (entity->has_final_guid() &&
+        bookmark_tracker_->bookmark_client_tags_in_protocol_enabled()) {
+      DCHECK(!metadata->client_tag_hash().empty());
+      data->client_tag_hash =
+          syncer::ClientTagHash::FromHashed(metadata->client_tag_hash());
+      DCHECK(metadata->is_deleted() ||
+             data->client_tag_hash ==
+                 syncer::ClientTagHash::FromUnhashed(
+                     syncer::BOOKMARKS,
+                     entity->bookmark_node()->guid().AsLowercaseString()));
+    }
+
     if (!metadata->is_deleted()) {
       const bookmarks::BookmarkNode* node = entity->bookmark_node();
       // Skip current entity if its favicon is not loaded yet. It will be

components/sync_bookmarks/bookmark_model_merger.cc

@@ -331,6 +331,7 @@ bool IsValidUpdate(const UpdateResponseData& update) {
     return false;
   }
   if (!HasExpectedBookmarkGuid(update_entity.specifics.bookmark(),
+                               update_entity.client_tag_hash,
                                update_entity.originator_cache_guid,
                                update_entity.originator_client_item_id)) {
     // Ignore updates with an unexpected GUID.

components/sync_bookmarks/bookmark_remote_updates_handler.cc

@@ -240,6 +240,7 @@ void BookmarkRemoteUpdatesHandler::Process(
         continue;
       }
       if (!HasExpectedBookmarkGuid(update_entity.specifics.bookmark(),
+                                   update_entity.client_tag_hash,
                                    update_entity.originator_cache_guid,
                                    update_entity.originator_client_item_id)) {
         // Ignore updates with an unexpected GUID.
@@ -292,13 +293,25 @@ void BookmarkRemoteUpdatesHandler::Process(
     // assume that it was never committed. The server will track the client that
     // sent up the original commit and return this in a get updates response. We
     // need to check if we have an entry that didn't get its server id updated
-    // correctly. The server sends down a |originator_cache_guid| and an
-    // |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.
+    // correctly. The server sends down |original_client_item_id| (regular case)
+    // or |client_provided_unique_tag| (experimental). If the tracker contains
+    // a matching entry, it should be treated as update.
     const SyncedBookmarkTracker::Entity* old_tracked_entity =
         bookmark_tracker_->GetEntityForSyncId(
             update_entity.originator_client_item_id);
+    if (!old_tracked_entity && !update_entity.client_tag_hash.value().empty()) {
+      // There's currently no way to perform a lookup by client tag hash. As an
+      // approximation, the bookmark node's GUID can be used, which is the same
+      // as the temporary sync ID assigned upon local creation (just like
+      // originator client ID). This doesn't work for remote deletions, which
+      // don't include a GUID in specifics, but the existing UMA data for
+      // DuplicateBookmarkEntityOnRemoteUpdateCondition::kServerIdTombstone
+      // indicates that users don't in practice run into this.
+      // TODO(crbug.com/1143246): Adopt proper lookups by client tag hash once
+      // the tracker supports this.
+      old_tracked_entity = bookmark_tracker_->GetEntityForSyncId(
+          remote_guid.AsLowercaseString());
+    }
     if (old_tracked_entity) {
       if (tracked_entity) {
         DCHECK_NE(tracked_entity, old_tracked_entity);

components/sync_bookmarks/bookmark_remote_updates_handler_unittest.cc

@@ -1004,6 +1004,8 @@ TEST_F(BookmarkRemoteUpdatesHandlerWithInitialMergeTest,
   bookmark_specifics->set_legacy_canonicalized_title(kTitle);
   bookmark_specifics->set_url(kUrl.spec());
   data.is_folder = false;
+  data.originator_client_item_id = bookmark_specifics->guid();
+
   syncer::UpdateResponseData response_data;
   response_data.entity = std::move(data);
   // Similar to what's done in the loopback_server.
@@ -1055,6 +1057,8 @@ TEST_F(BookmarkRemoteUpdatesHandlerWithInitialMergeTest,
   bookmark_specifics->set_icon_url(kIconUrl.spec());
   bookmark_specifics->set_favicon("PNG");
   data.is_folder = false;
+  data.originator_client_item_id = bookmark_specifics->guid();
+
   syncer::UpdateResponseData response_data;
   response_data.entity = std::move(data);
   // Similar to what's done in the loopback_server.
@@ -1093,6 +1097,8 @@ TEST_F(BookmarkRemoteUpdatesHandlerWithInitialMergeTest,
   bookmark_specifics->set_legacy_canonicalized_title(kTitle);
   bookmark_specifics->set_url(kUrl.spec());
   data.is_folder = false;
+  data.originator_client_item_id = bookmark_specifics->guid();
+
   syncer::UpdateResponseData response_data;
   response_data.entity = std::move(data);
   // Similar to what's done in the loopback_server.
@@ -1111,7 +1117,7 @@ TEST_F(BookmarkRemoteUpdatesHandlerWithInitialMergeTest,
 // server but the commit respone isn't received for some reason. Further updates
 // to that entity should update the sync id in the tracker.
 TEST_F(BookmarkRemoteUpdatesHandlerWithInitialMergeTest,
-       ShouldUpdateSyncIdWhenRecevingAnUpdateForNewlyCreatedLocalNode) {
+       ShouldUpdateSyncIdWhenRecevingUpdateForNewlyCreatedLocalNode) {
   const std::string kCacheGuid = "generated_id";
   const std::string kOriginatorClientItemId =
       base::GUID::GenerateRandomV4().AsLowercaseString();
@@ -1121,7 +1127,6 @@ TEST_F(BookmarkRemoteUpdatesHandlerWithInitialMergeTest,
                                      base::TimeDelta::FromSeconds(1));
 
   sync_pb::ModelTypeState model_type_state;
-  model_type_state.set_cache_guid(kCacheGuid);
   model_type_state.set_initial_sync_done(true);
 
   const sync_pb::UniquePosition unique_position;
@@ -1147,7 +1152,6 @@ TEST_F(BookmarkRemoteUpdatesHandlerWithInitialMergeTest,
   syncer::UpdateResponseDataList updates;
   syncer::EntityData data;
   data.id = kSyncId;
-  data.originator_cache_guid = kCacheGuid;
   data.originator_client_item_id = kOriginatorClientItemId;
   // Set the other required fields.
   data.unique_position = syncer::UniquePosition::InitialPosition(
@@ -1173,6 +1177,71 @@ TEST_F(BookmarkRemoteUpdatesHandlerWithInitialMergeTest,
   EXPECT_THAT(entity->bookmark_node(), Eq(&node));
 }
 
+// Same as above for bookmarks created with client tags.
+TEST_F(
+    BookmarkRemoteUpdatesHandlerWithInitialMergeTest,
+    ShouldUpdateSyncIdWhenRecevingUpdateForNewlyCreatedLocalNodeWithClientTag) {
+  base::test::ScopedFeatureList override_features;
+  override_features.InitAndEnableFeature(
+      switches::kSyncUseClientTagForBookmarkCommits);
+
+  const std::string kBookmarkGuid =
+      base::GUID::GenerateRandomV4().AsLowercaseString();
+  const std::string kSyncId = "server_id";
+  const int64_t kServerVersion = 1000;
+  const base::Time kModificationTime(base::Time::Now() -
+                                     base::TimeDelta::FromSeconds(1));
+
+  sync_pb::ModelTypeState model_type_state;
+  model_type_state.set_initial_sync_done(true);
+
+  const sync_pb::UniquePosition unique_position;
+  sync_pb::EntitySpecifics specifics;
+  sync_pb::BookmarkSpecifics* bookmark_specifics = specifics.mutable_bookmark();
+  bookmark_specifics->set_guid(kBookmarkGuid);
+  bookmark_specifics->set_legacy_canonicalized_title("Title");
+  bookmarks::BookmarkNode node(
+      /*id=*/1, base::GUID::ParseLowercase(kBookmarkGuid), GURL());
+  // Track a sync entity (similar to what happens after a local creation). The
+  // |originator_client_item_id| is used a temp sync id and mark the entity that
+  // it needs to be committed..
+  const SyncedBookmarkTracker::Entity* entity =
+      tracker()->Add(&node, /*sync_id=*/kBookmarkGuid, kServerVersion,
+                     kModificationTime, unique_position, specifics);
+  tracker()->IncrementSequenceNumber(entity);
+
+  ASSERT_THAT(tracker()->GetEntityForSyncId(kBookmarkGuid), Eq(entity));
+
+  // Now receive an update with the actual server id.
+  syncer::UpdateResponseDataList updates;
+  syncer::EntityData data;
+  data.id = kSyncId;
+  data.client_tag_hash =
+      syncer::ClientTagHash::FromUnhashed(syncer::BOOKMARKS, kBookmarkGuid);
+  // Set the other required fields.
+  data.unique_position = syncer::UniquePosition::InitialPosition(
+                             syncer::UniquePosition::RandomSuffix())
+                             .ToProto();
+  data.specifics = specifics;
+  data.specifics.mutable_bookmark()->set_guid(kBookmarkGuid);
+  data.is_folder = true;
+
+  syncer::UpdateResponseData response_data;
+  response_data.entity = std::move(data);
+  // Similar to what's done in the loopback_server.
+  response_data.response_version = 0;
+  updates.push_back(std::move(response_data));
+
+  updates_handler()->Process(updates,
+                             /*got_new_encryption_requirements=*/false);
+
+  // The sync id in the tracker should have been updated.
+  EXPECT_THAT(tracker()->GetEntityForSyncId(kBookmarkGuid), IsNull());
+  EXPECT_THAT(tracker()->GetEntityForSyncId(kSyncId), Eq(entity));
+  EXPECT_THAT(entity->metadata()->server_id(), Eq(kSyncId));
+  EXPECT_THAT(entity->bookmark_node(), Eq(&node));
+}
+
 TEST_F(BookmarkRemoteUpdatesHandlerWithInitialMergeTest,
        ShouldRecommitWhenEncryptionIsOutOfDate) {
   sync_pb::ModelTypeState model_type_state;

components/sync_bookmarks/bookmark_specifics_conversions.cc

@@ -394,14 +394,14 @@ bool IsValidBookmarkSpecifics(const sync_pb::BookmarkSpecifics& specifics,
 }
 
 bool HasExpectedBookmarkGuid(const sync_pb::BookmarkSpecifics& specifics,
+                             const syncer::ClientTagHash& client_tag_hash,
                              const std::string& originator_cache_guid,
                              const std::string& originator_client_item_id) {
   DCHECK(base::GUID::ParseLowercase(specifics.guid()).is_valid());
 
-  if (originator_client_item_id.empty()) {
-    // This could be a future bookmark with a client tag instead of an
-    // originator client item ID.
-    NOTIMPLEMENTED();
+  // If the client tag hash matches, that should already be good enough.
+  if (syncer::ClientTagHash::FromUnhashed(
+          syncer::BOOKMARKS, specifics.guid()) == client_tag_hash) {
     return true;
   }
 

components/sync_bookmarks/bookmark_specifics_conversions.h

@@ -24,6 +24,7 @@ class EntitySpecifics;
 }  // namespace sync_pb
 
 namespace syncer {
+class ClientTagHash;
 struct EntityData;
 }  // namespace syncer
 
@@ -89,9 +90,8 @@ bool IsValidBookmarkSpecifics(const sync_pb::BookmarkSpecifics& specifics,
 // Checks if bookmark specifics contain a GUID that matches the value that would
 // be inferred from other redundant fields. |specifics| must be valid as per
 // IsValidBookmarkSpecifics().
-// TODO(crbug.com/1032052): Replace this with an analogous function that
-// verifies that the bookmark's client tag hash matches the GUID.
 bool HasExpectedBookmarkGuid(const sync_pb::BookmarkSpecifics& specifics,
+                             const syncer::ClientTagHash& client_tag_hash,
                              const std::string& originator_cache_guid,
                              const std::string& originator_client_item_id);
 

components/sync_bookmarks/switches.cc

@@ -19,4 +19,7 @@ const base::Feature kSyncDeduplicateAllBookmarksWithSameGUID{
 const base::Feature kSyncIgnoreChangesInTouchIcons{
     "SyncIgnoreChangesInTouchIcons", base::FEATURE_ENABLED_BY_DEFAULT};
 
+const base::Feature kSyncUseClientTagForBookmarkCommits{
+    "SyncUseClientTagForBookmarkCommits", base::FEATURE_DISABLED_BY_DEFAULT};
+
 }  // namespace switches

components/sync_bookmarks/switches.h

@@ -17,6 +17,7 @@ extern const base::Feature kSyncReuploadBookmarkFullTitles;
 extern const base::Feature kSyncDeduplicateAllBookmarksWithSameGUID;
 // TODO(crbug.com/1075709): remove after launch.
 extern const base::Feature kSyncIgnoreChangesInTouchIcons;
+extern const base::Feature kSyncUseClientTagForBookmarkCommits;
 
 }  // namespace switches
 

components/sync_bookmarks/synced_bookmark_tracker.cc

@@ -222,6 +222,9 @@ std::unique_ptr<SyncedBookmarkTracker> SyncedBookmarkTracker::CreateEmpty(
   auto tracker = base::WrapUnique(new SyncedBookmarkTracker(
       std::move(model_type_state), /*bookmarks_full_title_reuploaded=*/false,
       /*last_sync_time=*/base::Time::Now()));
+  tracker->bookmark_client_tags_in_protocol_enabled_ =
+      base::FeatureList::IsEnabled(
+          switches::kSyncUseClientTagForBookmarkCommits);
   return tracker;
 }
 
@@ -236,23 +239,43 @@ SyncedBookmarkTracker::CreateFromBookmarkModelAndMetadata(
     return nullptr;
   }
 
-  auto tracker =
-      CreateEmpty(std::move(*model_metadata.mutable_model_type_state()));
-
   // When the reupload feature is enabled and disabled again, there may occur
   // new entities which weren't reuploaded.
   const bool bookmarks_full_title_reuploaded =
       model_metadata.bookmarks_full_title_reuploaded() &&
       base::FeatureList::IsEnabled(switches::kSyncReuploadBookmarkFullTitles);
-  if (bookmarks_full_title_reuploaded) {
-    tracker->SetBookmarksFullTitleReuploaded();
-  }
 
   // If the field is not present, |last_sync_time| will be initialized with the
   // Unix epoch.
-  tracker->last_sync_time_ =
+  const base::Time last_sync_time =
       syncer::ProtoTimeToTime(model_metadata.last_sync_time());
 
+  // base::WrapUnique() used because the constructor is private.
+  auto tracker = base::WrapUnique(new SyncedBookmarkTracker(
+      model_metadata.model_type_state(), bookmarks_full_title_reuploaded,
+      last_sync_time));
+
+  // Read |bookmark_client_tags_in_protocol_enabled_| while honoring the
+  // corresponding feature toggle too.
+  if (model_metadata.bookmark_client_tags_in_protocol_enabled()) {
+    // If the feature used to be enabled, it can continue to do so as long as
+    // the feature toggle is still enabled. If it becomes disabled, the boolean
+    // transitions to false immediately (independently of in-flight local
+    // changes) to guarantee that there is an effective kill switch.
+    tracker->bookmark_client_tags_in_protocol_enabled_ =
+        base::FeatureList::IsEnabled(
+            switches::kSyncUseClientTagForBookmarkCommits);
+  } else {
+    // If the feature used to be disabled, transitioning to true requires *NOT*
+    // having pending local changes (in-flight creations, strictly speaking, but
+    // that's too complex to implement), to avoid creating duplicates on the
+    // server (the same bookmark with and without a client tag).
+    tracker->bookmark_client_tags_in_protocol_enabled_ =
+        !tracker->HasLocalChanges() &&
+        base::FeatureList::IsEnabled(
+            switches::kSyncUseClientTagForBookmarkCommits);
+  }
+
   const CorruptionReason corruption_reason =
       tracker->InitEntitiesFromModelAndMetadata(model,
                                                 std::move(model_metadata));
@@ -447,6 +470,9 @@ SyncedBookmarkTracker::BuildBookmarkModelMetadata() const {
   model_metadata.set_bookmarks_full_title_reuploaded(
       bookmarks_full_title_reuploaded_);
   model_metadata.set_last_sync_time(syncer::TimeToProtoTime(last_sync_time_));
+  model_metadata.set_bookmark_client_tags_in_protocol_enabled(
+      bookmark_client_tags_in_protocol_enabled_);
+
   for (const std::pair<const std::string, std::unique_ptr<Entity>>& pair :
        sync_id_to_entities_map_) {
     DCHECK(pair.second) << " for ID " << pair.first;
@@ -758,6 +784,10 @@ bool SyncedBookmarkTracker::ReuploadBookmarksOnLoadIfNeeded() {
   return true;
 }
 
+bool SyncedBookmarkTracker::bookmark_client_tags_in_protocol_enabled() const {
+  return bookmark_client_tags_in_protocol_enabled_;
+}
+
 void SyncedBookmarkTracker::TraverseAndAppend(
     const bookmarks::BookmarkNode* node,
     std::vector<const SyncedBookmarkTracker::Entity*>* ordered_entities) const {

components/sync_bookmarks/synced_bookmark_tracker.h

@@ -301,6 +301,10 @@ class SyncedBookmarkTracker {
   // reuploaded.
   bool ReuploadBookmarksOnLoadIfNeeded();
 
+  // Returns whether bookmark commits sent to the server (most importantly
+  // creations) should populate client tags.
+  bool bookmark_client_tags_in_protocol_enabled() const;
+
  private:
   // Enumeration of possible reasons why persisted metadata are considered
   // corrupted and don't match the bookmark model. Used in UMA metrics. Do not
@@ -384,6 +388,12 @@ class SyncedBookmarkTracker {
   // required to populate the client tag (and be considered invalid otherwise).
   base::Time last_sync_time_;
 
+  // Represents whether bookmark commits sent to the server (most importantly
+  // creations) populate client tags.
+  // TODO(crbug.com/1032052): remove this code when the logic is enabled by
+  // default and enforced to true upon startup.
+  bool bookmark_client_tags_in_protocol_enabled_ = false;
+
   DISALLOW_COPY_AND_ASSIGN(SyncedBookmarkTracker);
 };