Adopt Entity pointer as handle in SyncedBookmarkTracker

This patch refactors the SyncedBookmarkTracker API to make it less
sync-ID-centric (aka server IDs). Instead, entity pointers are proposed
(const SyncedBookmarkTracker::Entity*) as handles, following the design
principle in BookmarkModel and const BookmarkNode*.

This spares a few lookups but otherwise introduces no behavioral
changes.

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

components/sync_bookmarks/bookmark_local_changes_builder.cc

@@ -18,7 +18,7 @@
 namespace sync_bookmarks {
 
 BookmarkLocalChangesBuilder::BookmarkLocalChangesBuilder(
-    const SyncedBookmarkTracker* const bookmark_tracker,
+    SyncedBookmarkTracker* const bookmark_tracker,
     bookmarks::BookmarkModel* bookmark_model)
     : bookmark_tracker_(bookmark_tracker), bookmark_model_(bookmark_model) {
   DCHECK(bookmark_tracker);
@@ -71,6 +71,7 @@ syncer::CommitRequestDataList BookmarkLocalChangesBuilder::BuildCommitRequests(
           entity->final_guid_matches(node->guid()));
       data->name = data->specifics.bookmark().title();
     }
+
     auto request = std::make_unique<syncer::CommitRequestData>();
     request->entity = std::move(data);
     request->sequence_number = metadata->sequence_number();
@@ -79,6 +80,8 @@ syncer::CommitRequestDataList BookmarkLocalChangesBuilder::BuildCommitRequests(
     // added/updated.
     request->specifics_hash = metadata->specifics_hash();
 
+    bookmark_tracker_->MarkCommitMayHaveStarted(entity);
+
     commit_requests.push_back(std::move(request));
   }
   return commit_requests;

components/sync_bookmarks/bookmark_local_changes_builder.h

@@ -21,13 +21,13 @@ class BookmarkLocalChangesBuilder {
  public:
   // |bookmark_tracker| and |bookmark_model| must not be null and must outlive
   // this object.
-  BookmarkLocalChangesBuilder(const SyncedBookmarkTracker* bookmark_tracker,
+  BookmarkLocalChangesBuilder(SyncedBookmarkTracker* bookmark_tracker,
                               bookmarks::BookmarkModel* bookmark_model);
   // Builds the commit requests list.
   syncer::CommitRequestDataList BuildCommitRequests(size_t max_entries) const;
 
  private:
-  const SyncedBookmarkTracker* const bookmark_tracker_;
+  SyncedBookmarkTracker* const bookmark_tracker_;
   bookmarks::BookmarkModel* const bookmark_model_;
 
   DISALLOW_COPY_AND_ASSIGN(BookmarkLocalChangesBuilder);

components/sync_bookmarks/bookmark_model_merger.cc

@@ -425,7 +425,7 @@ void BookmarkModelMerger::MergeSubtree(
     const RemoteTreeNode& remote_node) {
   const EntityData& remote_update_entity = remote_node.entity();
   bookmark_tracker_->Add(
-      remote_update_entity.id, local_subtree_root,
+      local_subtree_root, remote_update_entity.id,
       remote_node.response_version(), remote_update_entity.creation_time,
       remote_update_entity.unique_position, remote_update_entity.specifics);
 
@@ -570,7 +570,7 @@ void BookmarkModelMerger::ProcessRemoteCreation(
                                       remote_update_entity.is_folder,
                                       bookmark_model_, favicon_service_);
   DCHECK(bookmark_node);
-  bookmark_tracker_->Add(remote_update_entity.id, bookmark_node,
+  bookmark_tracker_->Add(bookmark_node, remote_update_entity.id,
                          remote_node.response_version(),
                          remote_update_entity.creation_time,
                          remote_update_entity.unique_position, specifics);
@@ -640,10 +640,10 @@ void BookmarkModelMerger::ProcessLocalCreation(
   const sync_pb::EntitySpecifics specifics = CreateSpecificsFromBookmarkNode(
       node, bookmark_model_, /*force_favicon_load=*/true,
       /*include_guid=*/true);
-  bookmark_tracker_->Add(sync_id, node, server_version, creation_time,
-                         pos.ToProto(), specifics);
+  const SyncedBookmarkTracker::Entity* entity = bookmark_tracker_->Add(
+      node, sync_id, server_version, creation_time, pos.ToProto(), specifics);
   // Mark the entity that it needs to be committed.
-  bookmark_tracker_->IncrementSequenceNumber(sync_id);
+  bookmark_tracker_->IncrementSequenceNumber(entity);
   for (size_t i = 0; i < node->children().size(); ++i) {
     // If a local node hasn't matched with any remote entity, its descendants
     // will neither, unless they have been or will be matched by GUID, in which

components/sync_bookmarks/bookmark_model_observer_impl.cc

@@ -62,17 +62,16 @@ void BookmarkModelObserverImpl::BookmarkNodeMoved(
 
   const std::string& sync_id = entity->metadata()->server_id();
   const base::Time modification_time = base::Time::Now();
-
   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, entity->has_final_guid());
 
-  bookmark_tracker_->Update(sync_id, entity->metadata()->server_version(),
+  bookmark_tracker_->Update(entity, entity->metadata()->server_version(),
                             modification_time, unique_position, specifics);
   // Mark the entity that it needs to be committed.
-  bookmark_tracker_->IncrementSequenceNumber(sync_id);
+  bookmark_tracker_->IncrementSequenceNumber(entity);
   nudge_for_commit_closure_.Run();
 }
 
@@ -108,10 +107,10 @@ void BookmarkModelObserverImpl::BookmarkNodeAdded(
   sync_pb::EntitySpecifics specifics =
       CreateSpecificsFromBookmarkNode(node, model, /*force_favicon_load=*/true,
                                       /*include_guid=*/true);
-  bookmark_tracker_->Add(sync_id, node, server_version, creation_time,
-                         unique_position, specifics);
+  const SyncedBookmarkTracker::Entity* entity = bookmark_tracker_->Add(
+      node, sync_id, server_version, creation_time, unique_position, specifics);
   // Mark the entity that it needs to be committed.
-  bookmark_tracker_->IncrementSequenceNumber(sync_id);
+  bookmark_tracker_->IncrementSequenceNumber(entity);
   nudge_for_commit_closure_.Run();
 }
 
@@ -198,12 +197,11 @@ void BookmarkModelObserverImpl::BookmarkNodeChanged(
     // (e.g.upon a favicon load).
     return;
   }
-  const std::string& sync_id = entity->metadata()->server_id();
-  bookmark_tracker_->Update(sync_id, entity->metadata()->server_version(),
+  bookmark_tracker_->Update(entity, entity->metadata()->server_version(),
                             modification_time,
                             entity->metadata()->unique_position(), specifics);
   // Mark the entity that it needs to be committed.
-  bookmark_tracker_->IncrementSequenceNumber(sync_id);
+  bookmark_tracker_->IncrementSequenceNumber(entity);
   nudge_for_commit_closure_.Run();
 }
 
@@ -271,10 +269,10 @@ void BookmarkModelObserverImpl::BookmarkNodeChildrenReordered(
         child.get(), model, /*force_favicon_load=*/true,
         entity->has_final_guid());
 
-    bookmark_tracker_->Update(sync_id, entity->metadata()->server_version(),
+    bookmark_tracker_->Update(entity, entity->metadata()->server_version(),
                               modification_time, position.ToProto(), specifics);
     // Mark the entity that it needs to be committed.
-    bookmark_tracker_->IncrementSequenceNumber(sync_id);
+    bookmark_tracker_->IncrementSequenceNumber(entity);
   }
   nudge_for_commit_closure_.Run();
 }
@@ -352,17 +350,16 @@ void BookmarkModelObserverImpl::ProcessDelete(
       bookmark_tracker_->GetEntityForBookmarkNode(node);
   // Shouldn't try to delete untracked entities.
   DCHECK(entity);
-  const std::string& sync_id = entity->metadata()->server_id();
   // If the entity hasn't been committed and doesn't have an inflight commit
   // request, simply remove it from the tracker.
   if (entity->metadata()->server_version() == syncer::kUncommittedVersion &&
       !entity->commit_may_have_started()) {
-    bookmark_tracker_->Remove(sync_id);
+    bookmark_tracker_->Remove(entity);
     return;
   }
-  bookmark_tracker_->MarkDeleted(sync_id);
+  bookmark_tracker_->MarkDeleted(entity);
   // Mark the entity that it needs to be committed.
-  bookmark_tracker_->IncrementSequenceNumber(sync_id);
+  bookmark_tracker_->IncrementSequenceNumber(entity);
 }
 
 }  // namespace sync_bookmarks

components/sync_bookmarks/bookmark_model_observer_impl_unittest.cc

@@ -51,8 +51,8 @@ class BookmarkModelObserverImplTest : public testing::Test {
     sync_pb::EntitySpecifics specifics;
     specifics.mutable_bookmark()->set_title(kBookmarkBarTag);
     bookmark_tracker_->Add(
-        /*sync_id=*/kBookmarkBarId,
         /*bookmark_node=*/bookmark_model()->bookmark_bar_node(),
+        /*sync_id=*/kBookmarkBarId,
         /*server_version=*/0, /*creation_time=*/base::Time::Now(),
         syncer::UniquePosition::InitialPosition(
             syncer::UniquePosition::RandomSuffix())
@@ -60,8 +60,8 @@ class BookmarkModelObserverImplTest : public testing::Test {
         specifics);
     specifics.mutable_bookmark()->set_title(kOtherBookmarksTag);
     bookmark_tracker_->Add(
-        /*sync_id=*/kOtherBookmarksId,
         /*bookmark_node=*/bookmark_model()->other_node(),
+        /*sync_id=*/kOtherBookmarksId,
         /*server_version=*/0, /*creation_time=*/base::Time::Now(),
         syncer::UniquePosition::InitialPosition(
             syncer::UniquePosition::RandomSuffix())
@@ -69,8 +69,8 @@ class BookmarkModelObserverImplTest : public testing::Test {
         specifics);
     specifics.mutable_bookmark()->set_title(kMobileBookmarksTag);
     bookmark_tracker_->Add(
-        /*sync_id=*/kMobileBookmarksId,
         /*bookmark_node=*/bookmark_model()->mobile_node(),
+        /*sync_id=*/kMobileBookmarksId,
         /*server_version=*/0, /*creation_time=*/base::Time::Now(),
         syncer::UniquePosition::InitialPosition(
             syncer::UniquePosition::RandomSuffix())
@@ -87,9 +87,9 @@ class BookmarkModelObserverImplTest : public testing::Test {
          bookmark_tracker()->GetEntitiesWithLocalChanges(kMaxEntries)) {
       const std::string id = entity->metadata()->server_id();
       // Don't simulate change in id for simplicity.
-      bookmark_tracker()->UpdateUponCommitResponse(id, id,
-                                                   /*acked_sequence_number=*/1,
-                                                   /*server_version=*/1);
+      bookmark_tracker()->UpdateUponCommitResponse(entity, id,
+                                                   /*server_version=*/1,
+                                                   /*acked_sequence_number=*/1);
     }
   }
 
@@ -401,23 +401,22 @@ TEST_F(BookmarkModelObserverImplTest,
 
   // Node should be tracked now.
   ASSERT_THAT(bookmark_tracker()->TrackedEntitiesCountForTest(), 4U);
-  const std::string id = bookmark_tracker()
-                             ->GetEntityForBookmarkNode(folder_node)
-                             ->metadata()
-                             ->server_id();
+  const SyncedBookmarkTracker::Entity* entity =
+      bookmark_tracker()->GetEntityForBookmarkNode(folder_node);
+  const std::string id = entity->metadata()->server_id();
   ASSERT_THAT(
       bookmark_tracker()->GetEntitiesWithLocalChanges(kMaxEntries).size(), 1U);
 
-  bookmark_tracker()->MarkCommitMayHaveStarted(id);
+  bookmark_tracker()->MarkCommitMayHaveStarted(entity);
 
   // Remove the folder.
   bookmark_model()->Remove(folder_node);
 
   // Simulate a commit response for the first commit request (the creation).
   // Don't simulate change in id for simplicity.
-  bookmark_tracker()->UpdateUponCommitResponse(id, id,
-                                               /*acked_sequence_number=*/1,
-                                               /*server_version=*/1);
+  bookmark_tracker()->UpdateUponCommitResponse(entity, id,
+                                               /*server_version=*/1,
+                                               /*acked_sequence_number=*/1);
 
   // There should still be one local change (the deletion).
   EXPECT_THAT(
@@ -427,9 +426,9 @@ TEST_F(BookmarkModelObserverImplTest,
   EXPECT_THAT(bookmark_tracker()->TrackedEntitiesCountForTest(), 4U);
 
   // Commit the deletion.
-  bookmark_tracker()->UpdateUponCommitResponse(id, id,
-                                               /*acked_sequence_number=*/2,
-                                               /*server_version=*/2);
+  bookmark_tracker()->UpdateUponCommitResponse(entity, id,
+                                               /*server_version=*/2,
+                                               /*acked_sequence_number=*/2);
   // Entity should have been dropped.
   EXPECT_THAT(bookmark_tracker()->TrackedEntitiesCountForTest(), 3U);
 }
@@ -515,8 +514,8 @@ TEST_F(BookmarkModelObserverImplTest, ShouldNotSyncUnsyncableBookmarks) {
   sync_pb::EntitySpecifics specifics;
   specifics.mutable_bookmark()->set_title(kBookmarkBarTag);
   bookmark_tracker->Add(
-      /*sync_id=*/kBookmarkBarId,
       /*bookmark_node=*/model->bookmark_bar_node(),
+      /*sync_id=*/kBookmarkBarId,
       /*server_version=*/0, /*creation_time=*/base::Time::Now(),
       syncer::UniquePosition::InitialPosition(
           syncer::UniquePosition::RandomSuffix())
@@ -524,8 +523,8 @@ TEST_F(BookmarkModelObserverImplTest, ShouldNotSyncUnsyncableBookmarks) {
       specifics);
   specifics.mutable_bookmark()->set_title(kOtherBookmarksTag);
   bookmark_tracker->Add(
-      /*sync_id=*/kOtherBookmarksId,
       /*bookmark_node=*/model->other_node(),
+      /*sync_id=*/kOtherBookmarksId,
       /*server_version=*/0, /*creation_time=*/base::Time::Now(),
       syncer::UniquePosition::InitialPosition(
           syncer::UniquePosition::RandomSuffix())
@@ -533,8 +532,8 @@ TEST_F(BookmarkModelObserverImplTest, ShouldNotSyncUnsyncableBookmarks) {
       specifics);
   specifics.mutable_bookmark()->set_title(kMobileBookmarksTag);
   bookmark_tracker->Add(
-      /*sync_id=*/kMobileBookmarksId,
       /*bookmark_node=*/model->mobile_node(),
+      /*sync_id=*/kMobileBookmarksId,
       /*server_version=*/0, /*creation_time=*/base::Time::Now(),
       syncer::UniquePosition::InitialPosition(
           syncer::UniquePosition::RandomSuffix())
@@ -586,8 +585,8 @@ TEST_F(BookmarkModelObserverImplTest, ShouldAddChildrenInArbitraryOrder) {
   sync_pb::EntitySpecifics specifics;
   specifics.mutable_bookmark()->set_title(kBookmarkBarTag);
   bookmark_tracker->Add(
-      /*sync_id=*/kBookmarkBarId,
       /*bookmark_node=*/bookmark_model()->bookmark_bar_node(),
+      /*sync_id=*/kBookmarkBarId,
       /*server_version=*/0, /*creation_time=*/base::Time::Now(),
       syncer::UniquePosition::InitialPosition(
           syncer::UniquePosition::RandomSuffix())

components/sync_bookmarks/bookmark_model_type_processor.cc

@@ -185,13 +185,7 @@ void BookmarkModelTypeProcessor::GetLocalChanges(
     GetLocalChangesCallback callback) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   BookmarkLocalChangesBuilder builder(bookmark_tracker_.get(), bookmark_model_);
-  syncer::CommitRequestDataList local_changes =
-      builder.BuildCommitRequests(max_entries);
-  for (const std::unique_ptr<syncer::CommitRequestData>& local_change :
-       local_changes) {
-    bookmark_tracker_->MarkCommitMayHaveStarted(local_change->entity->id);
-  }
-  std::move(callback).Run(std::move(local_changes));
+  std::move(callback).Run(builder.BuildCommitRequests(max_entries));
 }
 
 void BookmarkModelTypeProcessor::OnCommitCompleted(
@@ -209,10 +203,19 @@ void BookmarkModelTypeProcessor::OnCommitCompleted(
     // during the commit, and |response.id| carries both the old and new ids.
     const std::string& old_sync_id =
         response.id_in_request.empty() ? response.id : response.id_in_request;
-    bookmark_tracker_->UpdateUponCommitResponse(old_sync_id, response.id,
-                                                response.sequence_number,
-                                                response.response_version);
+    const SyncedBookmarkTracker::Entity* entity =
+        bookmark_tracker_->GetEntityForSyncId(old_sync_id);
+    if (!entity) {
+      DLOG(WARNING) << "Received a commit response for an unknown entity: "
+                    << old_sync_id;
+      continue;
     }
+
+    bookmark_tracker_->UpdateUponCommitResponse(entity, response.id,
+                                                response.response_version,
+                                                response.sequence_number);
+  }
+
   bookmark_tracker_->set_model_type_state(type_state);
   schedule_save_closure_.Run();
 }

components/sync_bookmarks/bookmark_remote_updates_handler.h

@@ -63,9 +63,10 @@ class BookmarkRemoteUpdatesHandler {
   //    ignored.
   // 3. Otherwise, a new node is created in the local bookmark model and
   //    registered in |bookmark_tracker_|.
-  // Returns true if a new bookmark has been registered in the
-  // |bookmark_tracker_|, false otherwise.
-  bool ProcessCreate(const syncer::UpdateResponseData& update);
+  //
+  // Returns the newly tracked entity or null if the creation failed.
+  const SyncedBookmarkTracker::Entity* ProcessCreate(
+      const syncer::UpdateResponseData& update);
 
   // Processes a remote update of a bookmark node. |update| must not be a
   // deletion, and the server_id must be already tracked, otherwise, it is a

components/sync_bookmarks/synced_bookmark_tracker.cc

@@ -192,10 +192,14 @@ const SyncedBookmarkTracker::Entity* SyncedBookmarkTracker::GetEntityForSyncId(
   return it != sync_id_to_entities_map_.end() ? it->second.get() : nullptr;
 }
 
-SyncedBookmarkTracker::Entity* SyncedBookmarkTracker::GetMutableEntityForSyncId(
-    const std::string& sync_id) {
-  auto it = sync_id_to_entities_map_.find(sync_id);
-  return it != sync_id_to_entities_map_.end() ? it->second.get() : nullptr;
+SyncedBookmarkTracker::Entity* SyncedBookmarkTracker::AsMutableEntity(
+    const Entity* entity) {
+  DCHECK(entity);
+  DCHECK_EQ(entity, GetEntityForSyncId(entity->metadata()->server_id()));
+
+  // As per DCHECK above, this tracker owns |*entity|, so it's legitimate to
+  // return non-const pointer.
+  return const_cast<Entity*>(entity);
 }
 
 const SyncedBookmarkTracker::Entity*
@@ -205,14 +209,16 @@ SyncedBookmarkTracker::GetEntityForBookmarkNode(
   return it != bookmark_node_to_entities_map_.end() ? it->second : nullptr;
 }
 
-void SyncedBookmarkTracker::Add(const std::string& sync_id,
+const SyncedBookmarkTracker::Entity* SyncedBookmarkTracker::Add(
     const bookmarks::BookmarkNode* bookmark_node,
+    const std::string& sync_id,
     int64_t server_version,
     base::Time creation_time,
     const sync_pb::UniquePosition& unique_position,
     const sync_pb::EntitySpecifics& specifics) {
   DCHECK_GT(specifics.ByteSize(), 0);
   DCHECK(bookmark_node);
+
   auto metadata = std::make_unique<sync_pb::EntityMetadata>();
   metadata->set_is_deleted(false);
   metadata->set_server_id(sync_id);
@@ -234,70 +240,67 @@ void SyncedBookmarkTracker::Add(const std::string& sync_id,
   // TODO(crbug.com/516866): The below CHECK is added to debug some crashes.
   // Should be removed after figuring out the reason for the crash.
   CHECK_EQ(0U, sync_id_to_entities_map_.count(sync_id));
+  const Entity* raw_entity = entity.get();
   sync_id_to_entities_map_[sync_id] = std::move(entity);
+  return raw_entity;
 }
 
 void SyncedBookmarkTracker::Update(
-    const std::string& sync_id,
+    const Entity* entity,
     int64_t server_version,
     base::Time modification_time,
     const sync_pb::UniquePosition& unique_position,
     const sync_pb::EntitySpecifics& specifics) {
   DCHECK_GT(specifics.ByteSize(), 0);
-  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(
+  Entity* mutable_entity = AsMutableEntity(entity);
+  mutable_entity->metadata()->set_server_version(server_version);
+  mutable_entity->metadata()->set_modification_time(
       syncer::TimeToProtoTime(modification_time));
-  *entity->metadata()->mutable_unique_position() = unique_position;
-  HashSpecifics(specifics, entity->metadata()->mutable_specifics_hash());
+  *mutable_entity->metadata()->mutable_unique_position() = unique_position;
+  HashSpecifics(specifics,
+                mutable_entity->metadata()->mutable_specifics_hash());
   // TODO(crbug.com/516866): in case of conflict, the entity might exist in
   // |ordered_local_tombstones_| as well if it has been locally deleted.
 }
 
-void SyncedBookmarkTracker::UpdateServerVersion(const std::string& sync_id,
+void SyncedBookmarkTracker::UpdateServerVersion(const Entity* entity,
                                                 int64_t server_version) {
-  Entity* entity = GetMutableEntityForSyncId(sync_id);
   DCHECK(entity);
-  entity->metadata()->set_server_version(server_version);
+  AsMutableEntity(entity)->metadata()->set_server_version(server_version);
 }
 
-void SyncedBookmarkTracker::PopulateFinalGuid(const std::string& sync_id,
+void SyncedBookmarkTracker::PopulateFinalGuid(const Entity* entity,
                                               const std::string& guid) {
-  Entity* entity = GetMutableEntityForSyncId(sync_id);
   DCHECK(entity);
-  entity->set_final_guid(guid);
+  AsMutableEntity(entity)->set_final_guid(guid);
 }
 
-void SyncedBookmarkTracker::MarkCommitMayHaveStarted(
-    const std::string& sync_id) {
-  Entity* entity = GetMutableEntityForSyncId(sync_id);
+void SyncedBookmarkTracker::MarkCommitMayHaveStarted(const Entity* entity) {
   DCHECK(entity);
-  entity->set_commit_may_have_started(true);
+  AsMutableEntity(entity)->set_commit_may_have_started(true);
 }
 
-void SyncedBookmarkTracker::MarkDeleted(const std::string& sync_id) {
-  Entity* entity = GetMutableEntityForSyncId(sync_id);
+void SyncedBookmarkTracker::MarkDeleted(const Entity* entity) {
   DCHECK(entity);
   DCHECK(!entity->metadata()->is_deleted());
   DCHECK(entity->bookmark_node());
   DCHECK_EQ(1U, bookmark_node_to_entities_map_.count(entity->bookmark_node()));
 
-  entity->metadata()->set_is_deleted(true);
+  Entity* mutable_entity = AsMutableEntity(entity);
+  mutable_entity->metadata()->set_is_deleted(true);
   // Clear all references to the deleted bookmark node.
-  bookmark_node_to_entities_map_.erase(entity->bookmark_node());
-  entity->clear_bookmark_node();
+  bookmark_node_to_entities_map_.erase(mutable_entity->bookmark_node());
+  mutable_entity->clear_bookmark_node();
   // TODO(crbug.com/516866): The below CHECK is added to debug some crashes.
   // Should be removed after figuring out the reason for the crash.
   CHECK_EQ(0, std::count(ordered_local_tombstones_.begin(),
                          ordered_local_tombstones_.end(), entity));
-  ordered_local_tombstones_.push_back(entity);
+  ordered_local_tombstones_.push_back(mutable_entity);
 }
 
-void SyncedBookmarkTracker::Remove(const std::string& sync_id) {
-  const Entity* entity = GetEntityForSyncId(sync_id);
+void SyncedBookmarkTracker::Remove(const Entity* entity) {
   DCHECK(entity);
   // TODO(rushans): erase only if entity is not a tombstone.
   if (entity->bookmark_node()) {
@@ -310,19 +313,14 @@ void SyncedBookmarkTracker::Remove(const std::string& sync_id) {
 
   bookmark_node_to_entities_map_.erase(entity->bookmark_node());
   base::Erase(ordered_local_tombstones_, entity);
-  sync_id_to_entities_map_.erase(sync_id);
+  sync_id_to_entities_map_.erase(entity->metadata()->server_id());
 }
 
-void SyncedBookmarkTracker::IncrementSequenceNumber(
-    const std::string& sync_id) {
-  // TODO(crbug.com/516866): The below CHECK is added to debug some crashes.
-  // Should be switched to a DCHECK after figuring out the reason for the crash.
-  CHECK_NE(0U, sync_id_to_entities_map_.count(sync_id));
-  Entity* entity = GetMutableEntityForSyncId(sync_id);
+void SyncedBookmarkTracker::IncrementSequenceNumber(const Entity* entity) {
   DCHECK(entity);
   // TODO(crbug.com/516866): Update base hash specifics here if the entity is
   // not already out of sync.
-  entity->metadata()->set_sequence_number(
+  AsMutableEntity(entity)->metadata()->set_sequence_number(
       entity->metadata()->sequence_number() + 1);
 }
 
@@ -611,43 +609,45 @@ void SyncedBookmarkTracker::TraverseAndAppend(
 }
 
 void SyncedBookmarkTracker::UpdateUponCommitResponse(
-    const std::string& old_id,
-    const std::string& new_id,
-    int64_t acked_sequence_number,
-    int64_t server_version) {
+    const Entity* entity,
+    const std::string& sync_id,
+    int64_t server_version,
+    int64_t acked_sequence_number) {
   // TODO(crbug.com/516866): Update specifics if we decide to keep it.
-  Entity* entity = GetMutableEntityForSyncId(old_id);
-  if (!entity) {
-    DLOG(WARNING) << "Trying to update a non existing entity.";
-    return;
-  }
+  DCHECK(entity);
 
-  entity->metadata()->set_acked_sequence_number(acked_sequence_number);
-  entity->metadata()->set_server_version(server_version);
+  Entity* mutable_entity = AsMutableEntity(entity);
+  mutable_entity->metadata()->set_acked_sequence_number(acked_sequence_number);
+  mutable_entity->metadata()->set_server_version(server_version);
   // If there are no pending commits, remove tombstones.
-  if (!entity->IsUnsynced() && entity->metadata()->is_deleted()) {
-    Remove(old_id);
+  if (!mutable_entity->IsUnsynced() &&
+      mutable_entity->metadata()->is_deleted()) {
+    Remove(mutable_entity);
     return;
   }
 
-  UpdateSyncForLocalCreationIfNeeded(old_id, new_id);
+  UpdateSyncIdForLocalCreationIfNeeded(mutable_entity, sync_id);
 }
 
-void SyncedBookmarkTracker::UpdateSyncForLocalCreationIfNeeded(
-    const std::string& old_id,
-    const std::string& new_id) {
-  if (old_id == new_id) {
+void SyncedBookmarkTracker::UpdateSyncIdForLocalCreationIfNeeded(
+    const Entity* entity,
+    const std::string& sync_id) {
+  DCHECK(entity);
+
+  const std::string old_id = entity->metadata()->server_id();
+  if (old_id == sync_id) {
     return;
   }
   // TODO(crbug.com/516866): The below CHECK is added to debug some crashes.
   // Should be removed after figuring out the reason for the crash.
   CHECK_EQ(1U, sync_id_to_entities_map_.count(old_id));
-  CHECK_EQ(0U, sync_id_to_entities_map_.count(new_id));
+  CHECK_EQ(0U, sync_id_to_entities_map_.count(sync_id));
 
-  std::unique_ptr<Entity> entity =
+  std::unique_ptr<Entity> owned_entity =
       std::move(sync_id_to_entities_map_.at(old_id));
-  entity->metadata()->set_server_id(new_id);
-  sync_id_to_entities_map_[new_id] = std::move(entity);
+  DCHECK_EQ(entity, owned_entity.get());
+  owned_entity->metadata()->set_server_id(sync_id);
+  sync_id_to_entities_map_[sync_id] = std::move(owned_entity);
   sync_id_to_entities_map_.erase(old_id);
 }
 
@@ -667,10 +667,9 @@ void SyncedBookmarkTracker::UpdateBookmarkNodePointer(
   bookmark_node_to_entities_map_.erase(old_node);
 }
 
-void SyncedBookmarkTracker::AckSequenceNumber(const std::string& sync_id) {
-  Entity* entity = GetMutableEntityForSyncId(sync_id);
+void SyncedBookmarkTracker::AckSequenceNumber(const Entity* entity) {
   DCHECK(entity);
-  entity->metadata()->set_acked_sequence_number(
+  AsMutableEntity(entity)->metadata()->set_acked_sequence_number(
       entity->metadata()->sequence_number());
 }
 

components/sync_bookmarks/synced_bookmark_tracker.h

@@ -148,45 +148,48 @@ class SyncedBookmarkTracker {
   const SyncedBookmarkTracker::Entity* GetEntityForBookmarkNode(
       const bookmarks::BookmarkNode* node) const;
 
-  // Adds an entry for the |sync_id| and the corresponding local bookmark node
-  // and metadata in |sync_id_to_entities_map_|.
-  void Add(const std::string& sync_id,
-           const bookmarks::BookmarkNode* bookmark_node,
+  // Starts tracking local bookmark |bookmark_node|, which must not be tracked
+  // beforehand. The rest of the arguments represent the initial metadata.
+  // Returns the tracked entity.
+  const Entity* Add(const bookmarks::BookmarkNode* bookmark_node,
+                    const std::string& sync_id,
                     int64_t server_version,
                     base::Time creation_time,
                     const sync_pb::UniquePosition& unique_position,
                     const sync_pb::EntitySpecifics& specifics);
 
-  // Updates an existing entry for the |sync_id| and the corresponding metadata
-  // in |sync_id_to_entities_map_|.
-  void Update(const std::string& sync_id,
+  // Updates the sync metadata for a tracked entity. |entity| must be owned by
+  // this tracker.
+  void Update(const Entity* entity,
               int64_t server_version,
               base::Time modification_time,
               const sync_pb::UniquePosition& unique_position,
               const sync_pb::EntitySpecifics& specifics);
 
-  // Updates the server version of an existing entry for the |sync_id|.
-  void UpdateServerVersion(const std::string& sync_id, int64_t server_version);
+  // Updates the server version of an existing entity. |entity| must be owned by
+  // this tracker.
+  void UpdateServerVersion(const Entity* entity, int64_t server_version);
 
-  // Populates a bookmark's final GUID.
-  void PopulateFinalGuid(const std::string& sync_id, const std::string& guid);
+  // Populates a bookmark's final GUID. |entity| must be owned by this tracker.
+  void PopulateFinalGuid(const Entity* entity, 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);
+  // Marks an existing entry that a commit request might have been sent to the
+  // server. |entity| must be owned by this tracker.
+  void MarkCommitMayHaveStarted(const Entity* entity);
 
   // This class maintains the order of calls to this method and the same order
   // is guaranteed when returning local changes in
   // GetEntitiesWithLocalChanges() as well as in BuildBookmarkModelMetadata().
-  void MarkDeleted(const std::string& sync_id);
+  // |entity| must be owned by this tracker.
+  void MarkDeleted(const Entity* entity);
 
-  // Removes the entry coressponding to the |sync_id| from
-  // |sync_id_to_entities_map_|.
-  void Remove(const std::string& sync_id);
+  // Untracks an entity, which also invalidates the pointer. |entity| must be
+  // owned by this tracker.
+  void Remove(const Entity* entity);
 
-  // Increment sequence number in the metadata for the entity with |sync_id|.
-  // Tracker must contain a non-tombstone entity with server id = |sync_id|.
-  void IncrementSequenceNumber(const std::string& sync_id);
+  // Increment sequence number in the metadata for |entity|. |entity| must be
+  // owned by this tracker.
+  void IncrementSequenceNumber(const Entity* entity);
 
   sync_pb::BookmarkModelMetadata BuildBookmarkModelMetadata() const;
 
@@ -206,30 +209,30 @@ class SyncedBookmarkTracker {
   std::vector<const Entity*> GetEntitiesWithLocalChanges(
       size_t max_entries) const;
 
-  // Updates the tracker after receiving the commit response. |old_id| should be
-  // equal to |new_id| for all updates except the initial commit, where the
-  // temporary client-generated ID will be overriden by the server-provided
-  // final ID. In which case |sync_id_to_entities_map_| will be updated
-  // accordingly.
-  void UpdateUponCommitResponse(const std::string& old_id,
-                                const std::string& new_id,
-                                int64_t acked_sequence_number,
-                                int64_t server_version);
-
-  // Informs the tracker that the sync id for an entity has changed. It updates
-  // the internal state of the tracker accordingly.
-  void UpdateSyncForLocalCreationIfNeeded(const std::string& old_id,
-                                          const std::string& new_id);
+  // Updates the tracker after receiving the commit response. |sync_id| should
+  // match the already tracked sync ID for |entity|, with the exception of the
+  // initial commit, where the temporary client-generated ID will be overridden
+  // by the server-provided final ID. |entity| must be owned by this tracker.
+  void UpdateUponCommitResponse(const Entity* entity,
+                                const std::string& sync_id,
+                                int64_t server_version,
+                                int64_t acked_sequence_number);
+
+  // Informs the tracker that the sync ID for |entity| has changed. It updates
+  // the internal state of the tracker accordingly. |entity| must be owned by
+  // this tracker.
+  void UpdateSyncIdForLocalCreationIfNeeded(const Entity* entity,
+                                            const std::string& sync_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().
-  void AckSequenceNumber(const std::string& sync_id);
+  // Set the value of |EntityMetadata.acked_sequence_number| for |entity| to be
+  // equal to |EntityMetadata.sequence_number| such that it is not returned in
+  // GetEntitiesWithLocalChanges(). |entity| must be owned by this tracker.
+  void AckSequenceNumber(const Entity* entity);
 
   // Whether the tracker is empty or not.
   bool IsEmpty() const;
@@ -281,8 +284,10 @@ class SyncedBookmarkTracker {
       const bookmarks::BookmarkModel* model,
       sync_pb::BookmarkModelMetadata model_metadata);
 
-  // Returns null if no entity is found.
-  Entity* GetMutableEntityForSyncId(const std::string& sync_id);
+  // Conceptually, find a tracked entity that matches |entity| and returns a
+  // non-const pointer of it. The actual implementation is a const_cast.
+  // |entity| must be owned by this tracker.
+  Entity* AsMutableEntity(const Entity* entity);
 
   // Reorders |entities| that represents local non-deletions such that parent
   // creation/update is before child creation/update. Returns the ordered list.

components/sync_bookmarks/synced_bookmark_tracker_unittest.cc

@@ -110,10 +110,9 @@ TEST(SyncedBookmarkTrackerTest, ShouldGetAssociatedNodes) {
       GenerateSpecifics(/*title=*/std::string(), /*url=*/std::string());
 
   bookmarks::BookmarkNode node(kId, base::GenerateGUID(), kUrl);
-  tracker->Add(kSyncId, &node, kServerVersion, kCreationTime,
-               unique_position.ToProto(), specifics);
   const SyncedBookmarkTracker::Entity* entity =
-      tracker->GetEntityForSyncId(kSyncId);
+      tracker->Add(&node, kSyncId, kServerVersion, kCreationTime,
+                   unique_position.ToProto(), specifics);
   ASSERT_THAT(entity, NotNull());
   EXPECT_THAT(entity->bookmark_node(), Eq(&node));
   EXPECT_THAT(entity->metadata()->server_id(), Eq(kSyncId));
@@ -144,10 +143,12 @@ TEST(SyncedBookmarkTrackerTest, ShouldReturnNullForDisassociatedNodes) {
   const sync_pb::EntitySpecifics specifics =
       GenerateSpecifics(/*title=*/std::string(), /*url=*/std::string());
   bookmarks::BookmarkNode node(kId, base::GenerateGUID(), GURL());
-  tracker->Add(kSyncId, &node, kServerVersion, kModificationTime,
+  const SyncedBookmarkTracker::Entity* entity =
+      tracker->Add(&node, kSyncId, kServerVersion, kModificationTime,
                    unique_position, specifics);
-  ASSERT_THAT(tracker->GetEntityForSyncId(kSyncId), NotNull());
-  tracker->Remove(kSyncId);
+  ASSERT_THAT(entity, NotNull());
+  ASSERT_THAT(tracker->GetEntityForSyncId(kSyncId), Eq(entity));
+  tracker->Remove(entity);
   EXPECT_THAT(tracker->GetEntityForSyncId(kSyncId), IsNull());
 }
 
@@ -169,7 +170,7 @@ TEST(SyncedBookmarkTrackerTest, ShouldBuildBookmarkModelMetadata) {
       GenerateSpecifics(/*title=*/std::string(), /*url=*/std::string());
 
   bookmarks::BookmarkNode node(kId, base::GenerateGUID(), kUrl);
-  tracker->Add(kSyncId, &node, kServerVersion, kCreationTime,
+  tracker->Add(&node, kSyncId, kServerVersion, kCreationTime,
                unique_position.ToProto(), specifics);
 
   sync_pb::BookmarkModelMetadata bookmark_model_metadata =
@@ -195,11 +196,12 @@ TEST(SyncedBookmarkTrackerTest,
   const sync_pb::EntitySpecifics specifics =
       GenerateSpecifics(/*title=*/std::string(), /*url=*/std::string());
   bookmarks::BookmarkNode node(kId, base::GenerateGUID(), GURL());
-  tracker->Add(kSyncId, &node, kServerVersion, kModificationTime,
+  const SyncedBookmarkTracker::Entity* entity =
+      tracker->Add(&node, kSyncId, kServerVersion, kModificationTime,
                    unique_position, specifics);
 
   EXPECT_THAT(tracker->HasLocalChanges(), Eq(false));
-  tracker->IncrementSequenceNumber(kSyncId);
+  tracker->IncrementSequenceNumber(entity);
   EXPECT_THAT(tracker->HasLocalChanges(), Eq(true));
   // TODO(crbug.com/516866): Test HasLocalChanges after submitting commit
   // request in a separate test probably.
@@ -218,23 +220,24 @@ TEST(SyncedBookmarkTrackerTest, ShouldAckSequenceNumber) {
   const sync_pb::EntitySpecifics specifics =
       GenerateSpecifics(/*title=*/std::string(), /*url=*/std::string());
   bookmarks::BookmarkNode node(kId, base::GenerateGUID(), GURL());
-  tracker->Add(kSyncId, &node, kServerVersion, kModificationTime,
+  const SyncedBookmarkTracker::Entity* entity =
+      tracker->Add(&node, kSyncId, kServerVersion, kModificationTime,
                    unique_position, specifics);
 
   // Test simple scenario of ack'ing an incrememented sequence number.
   EXPECT_THAT(tracker->HasLocalChanges(), Eq(false));
-  tracker->IncrementSequenceNumber(kSyncId);
+  tracker->IncrementSequenceNumber(entity);
   EXPECT_THAT(tracker->HasLocalChanges(), Eq(true));
-  tracker->AckSequenceNumber(kSyncId);
+  tracker->AckSequenceNumber(entity);
   EXPECT_THAT(tracker->HasLocalChanges(), Eq(false));
 
   // Test ack'ing of a multiple times incremented sequence number.
-  tracker->IncrementSequenceNumber(kSyncId);
+  tracker->IncrementSequenceNumber(entity);
   EXPECT_THAT(tracker->HasLocalChanges(), Eq(true));
-  tracker->IncrementSequenceNumber(kSyncId);
-  tracker->IncrementSequenceNumber(kSyncId);
+  tracker->IncrementSequenceNumber(entity);
+  tracker->IncrementSequenceNumber(entity);
   EXPECT_THAT(tracker->HasLocalChanges(), Eq(true));
-  tracker->AckSequenceNumber(kSyncId);
+  tracker->AckSequenceNumber(entity);
   EXPECT_THAT(tracker->HasLocalChanges(), Eq(false));
 }
 
@@ -253,18 +256,23 @@ TEST(SyncedBookmarkTrackerTest, ShouldUpdateUponCommitResponseWithNewId) {
   const sync_pb::EntitySpecifics specifics =
       GenerateSpecifics(/*title=*/std::string(), /*url=*/std::string());
   bookmarks::BookmarkNode node(kId, base::GenerateGUID(), GURL());
-  tracker->Add(kSyncId, &node, kServerVersion, kModificationTime,
+  const SyncedBookmarkTracker::Entity* entity =
+      tracker->Add(&node, kSyncId, kServerVersion, kModificationTime,
                    unique_position, specifics);
-  ASSERT_THAT(tracker->GetEntityForSyncId(kSyncId), NotNull());
+  ASSERT_THAT(entity, NotNull());
+
+  // Initially only the old ID should be tracked.
+  ASSERT_THAT(tracker->GetEntityForSyncId(kSyncId), Eq(entity));
+  ASSERT_THAT(tracker->GetEntityForSyncId(kNewSyncId), IsNull());
+
   // Receive a commit response with a changed id.
-  tracker->UpdateUponCommitResponse(
-      kSyncId, kNewSyncId, /*acked_sequence_number=*/1, kNewServerVersion);
-  // Old id shouldn't be there.
+  tracker->UpdateUponCommitResponse(entity, kNewSyncId, kNewServerVersion,
+                                    /*acked_sequence_number=*/1);
+
+  // Old id shouldn't be there, but the new one should.
   EXPECT_THAT(tracker->GetEntityForSyncId(kSyncId), IsNull());
+  EXPECT_THAT(tracker->GetEntityForSyncId(kNewSyncId), Eq(entity));
 
-  const SyncedBookmarkTracker::Entity* entity =
-      tracker->GetEntityForSyncId(kNewSyncId);
-  ASSERT_THAT(entity, NotNull());
   EXPECT_THAT(entity->metadata()->server_id(), Eq(kNewSyncId));
   EXPECT_THAT(entity->bookmark_node(), Eq(&node));
   EXPECT_THAT(entity->metadata()->server_version(), Eq(kNewServerVersion));
@@ -284,18 +292,18 @@ TEST(SyncedBookmarkTrackerTest, ShouldUpdateId) {
       GenerateSpecifics(/*title=*/std::string(), /*url=*/std::string());
   bookmarks::BookmarkNode node(/*id=*/1, base::GenerateGUID(), GURL());
   // Track a sync entity.
-  tracker->Add(kSyncId, &node, kServerVersion, kModificationTime,
+  const SyncedBookmarkTracker::Entity* entity =
+      tracker->Add(&node, kSyncId, kServerVersion, kModificationTime,
                    unique_position, specifics);
 
-  ASSERT_THAT(tracker->GetEntityForSyncId(kSyncId), NotNull());
+  ASSERT_THAT(entity, NotNull());
   // Update the sync id.
-  tracker->UpdateSyncForLocalCreationIfNeeded(kSyncId, kNewSyncId);
-  // Old id shouldn't be there.
+  tracker->UpdateSyncIdForLocalCreationIfNeeded(entity, kNewSyncId);
+
+  // Old id shouldn't be there, but the new one should.
   EXPECT_THAT(tracker->GetEntityForSyncId(kSyncId), IsNull());
+  EXPECT_THAT(tracker->GetEntityForSyncId(kNewSyncId), Eq(entity));
 
-  const SyncedBookmarkTracker::Entity* entity =
-      tracker->GetEntityForSyncId(kNewSyncId);
-  ASSERT_THAT(entity, NotNull());
   EXPECT_THAT(entity->metadata()->server_id(), Eq(kNewSyncId));
   EXPECT_THAT(entity->bookmark_node(), Eq(&node));
   EXPECT_THAT(entity->metadata()->server_version(), Eq(kServerVersion));
@@ -406,9 +414,9 @@ TEST(SyncedBookmarkTrackerTest,
   ASSERT_THAT(tracker, NotNull());
 
   // Mark entities deleted in that order kId2, kId4, kId1
-  tracker->MarkDeleted(kId2);
-  tracker->MarkDeleted(kId4);
-  tracker->MarkDeleted(kId1);
+  tracker->MarkDeleted(tracker->GetEntityForSyncId(kId2));
+  tracker->MarkDeleted(tracker->GetEntityForSyncId(kId4));
+  tracker->MarkDeleted(tracker->GetEntityForSyncId(kId1));
 
   const sync_pb::BookmarkModelMetadata output_model_metadata =
       tracker->BuildBookmarkModelMetadata();
@@ -480,10 +488,10 @@ TEST(SyncedBookmarkTrackerTest,
   // Mark the entities that they have local changes. (in shuffled order just to
   // verify the tracker doesn't simply maintain the order of updates similar to
   // with deletions).
-  tracker->IncrementSequenceNumber(kId3);
-  tracker->IncrementSequenceNumber(kId1);
-  tracker->IncrementSequenceNumber(kId2);
-  tracker->IncrementSequenceNumber(kId0);
+  tracker->IncrementSequenceNumber(tracker->GetEntityForSyncId(kId3));
+  tracker->IncrementSequenceNumber(tracker->GetEntityForSyncId(kId1));
+  tracker->IncrementSequenceNumber(tracker->GetEntityForSyncId(kId2));
+  tracker->IncrementSequenceNumber(tracker->GetEntityForSyncId(kId0));
 
   std::vector<const SyncedBookmarkTracker::Entity*> entities_with_local_change =
       tracker->GetEntitiesWithLocalChanges(kMaxEntries);