Enforce lower-case use of GUIDs for sync

base::GenerateGUID() produces lower-case GUIDs and they should get
exposed to the sync protocol exclusively in this canonical form to avoid
running into issues.

This patch replaces all occurrences in sync codebase with:
git grep -l 'IsValidGUID' -- components/sync* | \
  xargs sed -i 's/IsValidGUID/IsValidGUIDOutputString/g'

In addition, the implicit GUIDs used for bookmark sync, in particular
for permanent nodes, are updated to adopt lower-case GUIDs. These are
never committed to the server so there are no backward-compatibility
considerations.

Change-Id: I0d0468f3475e1a27a161e2af15853c757f53e201
Bug: 978430
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2054154Reviewed-by: Marc Treib <treib@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#741381}

components/bookmarks/browser/bookmark_node.cc

@@ -48,15 +48,15 @@ std::string PermanentNodeTypeToGuid(BookmarkNode::Type type) {
 // static
 const int64_t BookmarkNode::kInvalidSyncTransactionVersion = -1;
 const char BookmarkNode::kRootNodeGuid[] =
-    "00000000-0000-4000-A000-000000000001";
+    "00000000-0000-4000-a000-000000000001";
 const char BookmarkNode::kBookmarkBarNodeGuid[] =
-    "00000000-0000-4000-A000-000000000002";
+    "00000000-0000-4000-a000-000000000002";
 const char BookmarkNode::kOtherBookmarksNodeGuid[] =
-    "00000000-0000-4000-A000-000000000003";
+    "00000000-0000-4000-a000-000000000003";
 const char BookmarkNode::kMobileBookmarksNodeGuid[] =
-    "00000000-0000-4000-A000-000000000004";
+    "00000000-0000-4000-a000-000000000004";
 const char BookmarkNode::kManagedNodeGuid[] =
-    "00000000-0000-4000-A000-000000000005";
+    "00000000-0000-4000-a000-000000000005";
 
 std::string BookmarkNode::RootNodeGuid() {
   return BookmarkNode::kRootNodeGuid;

components/sync/engine_impl/bookmark_update_preprocessing.cc

@@ -89,7 +89,7 @@ std::string ComputeGuidFromBytes(base::span<const uint8_t> bytes) {
 std::string InferGuidForLegacyBookmark(
     const std::string& originator_cache_guid,
     const std::string& originator_client_item_id) {
-  DCHECK(!base::IsValidGUID(originator_client_item_id));
+  DCHECK(!base::IsValidGUIDOutputString(originator_client_item_id));
 
   const std::string unique_tag =
       base::StrCat({originator_cache_guid, originator_client_item_id});
@@ -99,7 +99,7 @@ std::string InferGuidForLegacyBookmark(
   static_assert(base::kSHA1Length >= 16, "16 bytes needed to infer GUID");
 
   const std::string guid = ComputeGuidFromBytes(base::make_span(hash));
-  DCHECK(base::IsValidGUID(guid));
+  DCHECK(base::IsValidGUIDOutputString(guid));
   return guid;
 }
 
@@ -196,7 +196,8 @@ void AdaptGuidForBookmark(const sync_pb::SyncEntity& update_entity,
   // Otherwise, we leave the field empty.
   if (specifics->bookmark().has_guid()) {
     LogGuidSource(BookmarkGuidSource::kSpecifics);
-  } else if (base::IsValidGUID(update_entity.originator_client_item_id())) {
+  } else if (base::IsValidGUIDOutputString(
+                 update_entity.originator_client_item_id())) {
     specifics->mutable_bookmark()->set_guid(
         update_entity.originator_client_item_id());
     LogGuidSource(BookmarkGuidSource::kValidOCII);

components/sync/engine_impl/model_type_worker.cc

@@ -572,7 +572,8 @@ void ModelTypeWorker::DeduplicatePendingUpdatesBasedOnOriginatorClientItemId() {
     // without deduplication, which is the case for all datatypes except
     // bookmarks, as well as bookmarks created before 2015, when the item ID was
     // not globally unique across clients.
-    if (!base::IsValidGUID(candidate.entity.originator_client_item_id)) {
+    if (!base::IsValidGUIDOutputString(
+            candidate.entity.originator_client_item_id)) {
       pending_updates_.push_back(std::move(candidate));
       continue;
     }

components/sync_bookmarks/bookmark_model_merger.cc

@@ -203,7 +203,7 @@ void BookmarkModelMerger::RemoteTreeNode::EmplaceSelfAndDescendantsByGUID(
 
   if (entity().server_defined_unique_tag.empty()) {
     const std::string& guid = entity().specifics.bookmark().guid();
-    DCHECK(base::IsValidGUID(guid));
+    DCHECK(base::IsValidGUIDOutputString(guid));
 
     // Duplicate GUIDs have been sorted out before.
     bool success = guid_to_remote_node_map->emplace(guid, this).second;
@@ -358,7 +358,7 @@ BookmarkModelMerger::FindGuidMatchesOrReassignLocal(
       bookmark_model->root_node());
   while (iterator.has_next()) {
     const bookmarks::BookmarkNode* const node = iterator.Next();
-    DCHECK(base::IsValidGUID(node->guid()));
+    DCHECK(base::IsValidGUIDOutputString(node->guid()));
 
     const auto remote_it = guid_to_remote_node_map.find(node->guid());
     if (remote_it == guid_to_remote_node_map.end()) {
@@ -598,7 +598,7 @@ void BookmarkModelMerger::ProcessLocalCreation(
   // server id upon receiving commit response.
   const bookmarks::BookmarkNode* node = parent->children()[index].get();
   DCHECK(!FindMatchingRemoteNodeByGUID(node));
-  DCHECK(base::IsValidGUID(node->guid()));
+  DCHECK(base::IsValidGUIDOutputString(node->guid()));
 
   // The node's GUID cannot run into collisions because
   // FindGuidMatchesOrReassignLocal() takes care of reassigning local GUIDs if

components/sync_bookmarks/bookmark_model_merger_unittest.cc

@@ -1061,7 +1061,8 @@ TEST(BookmarkModelMergerTest, ShouldReplaceBookmarkGUIDWithConflictingURLs) {
   EXPECT_EQ(bookmark_bar_node->children()[0]->guid(), kGuid);
   EXPECT_EQ(bookmark_bar_node->children()[0]->url(), kUrl2);
   EXPECT_NE(bookmark_bar_node->children()[1]->guid(), kGuid);
-  EXPECT_TRUE(base::IsValidGUID(bookmark_bar_node->children()[1]->guid()));
+  EXPECT_TRUE(
+      base::IsValidGUIDOutputString(bookmark_bar_node->children()[1]->guid()));
   EXPECT_EQ(bookmark_bar_node->children()[1]->url(), kUrl1);
 }
 
@@ -1113,7 +1114,8 @@ TEST(BookmarkModelMergerTest, ShouldReplaceBookmarkGUIDWithConflictingTypes) {
   EXPECT_EQ(bookmark_bar_node->children()[0]->guid(), kGuid);
   EXPECT_TRUE(bookmark_bar_node->children()[0]->is_folder());
   EXPECT_NE(bookmark_bar_node->children()[1]->guid(), kGuid);
-  EXPECT_TRUE(base::IsValidGUID(bookmark_bar_node->children()[1]->guid()));
+  EXPECT_TRUE(
+      base::IsValidGUIDOutputString(bookmark_bar_node->children()[1]->guid()));
   EXPECT_FALSE(bookmark_bar_node->children()[1]->is_folder());
 }
 

components/sync_bookmarks/bookmark_model_observer_impl.cc

@@ -95,7 +95,7 @@ void BookmarkModelObserverImpl::BookmarkNodeAdded(
   // 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
   // server id upon receiving commit response.
-  DCHECK(base::IsValidGUID(node->guid()));
+  DCHECK(base::IsValidGUIDOutputString(node->guid()));
 
   // Local bookmark creations should have used a random GUID so it's safe to
   // use it as originator client item ID, without the risk for collision.

components/sync_bookmarks/bookmark_remote_updates_handler.cc

@@ -135,7 +135,8 @@ void ApplyRemoteUpdate(
   // 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())) {
+      base::IsValidGUIDOutputString(
+          update_entity.specifics.bookmark().guid())) {
     const bookmarks::BookmarkNode* old_node = node;
     node = ReplaceBookmarkNodeGUID(
         node, update_entity.specifics.bookmark().guid(), model);
@@ -233,7 +234,7 @@ void BookmarkRemoteUpdatesHandler::Process(
     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));
+      DCHECK(base::IsValidGUIDOutputString(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.

components/sync_bookmarks/bookmark_specifics_conversions.cc

@@ -165,7 +165,7 @@ std::string ComputeGuidFromBytes(base::span<const uint8_t> bytes) {
 std::string InferGuidForLegacyBookmark(
     const std::string& originator_cache_guid,
     const std::string& originator_client_item_id) {
-  DCHECK(!base::IsValidGUID(originator_client_item_id));
+  DCHECK(!base::IsValidGUIDOutputString(originator_client_item_id));
 
   const std::string unique_tag =
       base::StrCat({originator_cache_guid, originator_client_item_id});
@@ -175,7 +175,7 @@ std::string InferGuidForLegacyBookmark(
   static_assert(base::kSHA1Length >= 16, "16 bytes needed to infer GUID");
 
   const std::string guid = ComputeGuidFromBytes(base::make_span(hash));
-  DCHECK(base::IsValidGUID(guid));
+  DCHECK(base::IsValidGUIDOutputString(guid));
   return guid;
 }
 
@@ -193,7 +193,8 @@ sync_pb::EntitySpecifics CreateSpecificsFromBookmarkNode(
   }
 
   DCHECK(!node->guid().empty());
-  DCHECK(base::IsValidGUID(node->guid())) << "Actual: " << node->guid();
+  DCHECK(base::IsValidGUIDOutputString(node->guid()))
+      << "Actual: " << node->guid();
 
   if (include_guid) {
     bm_specifics->set_guid(node->guid());
@@ -247,7 +248,7 @@ const bookmarks::BookmarkNode* CreateBookmarkNodeFromSpecifics(
   DCHECK(parent);
   DCHECK(model);
   DCHECK(favicon_service);
-  DCHECK(base::IsValidGUID(specifics.guid()));
+  DCHECK(base::IsValidGUIDOutputString(specifics.guid()));
 
   bookmarks::BookmarkNode::MetaInfoMap metainfo =
       GetBookmarkMetaInfo(specifics);
@@ -284,7 +285,7 @@ void UpdateBookmarkNodeFromSpecifics(
   // resolving any conflict in GUID. Either GUIDs are the same, or the GUID in
   // specifics is invalid, and hence we can ignore it.
   DCHECK(specifics.guid() == node->guid() ||
-         !base::IsValidGUID(specifics.guid()) ||
+         !base::IsValidGUIDOutputString(specifics.guid()) ||
          !base::FeatureList::IsEnabled(
              switches::kUpdateBookmarkGUIDWithNodeReplacement));
 
@@ -308,7 +309,7 @@ const bookmarks::BookmarkNode* ReplaceBookmarkNodeGUID(
     return node;
   }
   const bookmarks::BookmarkNode* new_node;
-  DCHECK(base::IsValidGUID(guid));
+  DCHECK(base::IsValidGUIDOutputString(guid));
 
   if (node->guid() == guid) {
     // Nothing to do.
@@ -340,7 +341,7 @@ bool IsValidBookmarkSpecifics(const sync_pb::BookmarkSpecifics& specifics,
     LogInvalidSpecifics(InvalidBookmarkSpecificsError::kEmptySpecifics);
     is_valid = false;
   }
-  if (!base::IsValidGUID(specifics.guid())) {
+  if (!base::IsValidGUIDOutputString(specifics.guid())) {
     DLOG(ERROR) << "Invalid bookmark: invalid GUID in the specifics.";
     LogInvalidSpecifics(InvalidBookmarkSpecificsError::kInvalidGUID);
     is_valid = false;
@@ -382,7 +383,7 @@ bool IsValidBookmarkSpecifics(const sync_pb::BookmarkSpecifics& specifics,
 bool HasExpectedBookmarkGuid(const sync_pb::BookmarkSpecifics& specifics,
                              const std::string& originator_cache_guid,
                              const std::string& originator_client_item_id) {
-  DCHECK(base::IsValidGUID(specifics.guid()));
+  DCHECK(base::IsValidGUIDOutputString(specifics.guid()));
 
   if (originator_client_item_id.empty()) {
     // This could be a future bookmark with a client tag instead of an
@@ -391,7 +392,7 @@ bool HasExpectedBookmarkGuid(const sync_pb::BookmarkSpecifics& specifics,
     return true;
   }
 
-  if (base::IsValidGUID(originator_client_item_id)) {
+  if (base::IsValidGUIDOutputString(originator_client_item_id)) {
     return specifics.guid() == originator_client_item_id;
   }