[Sync] Fix possible use-after-free while processing remote update

During processing conflict the local entity might be deleted, e.g. when
there is a remote update upon local tombstone.

Bug: 1093024
Change-Id: I29341f26b000771a012cbe23f0373e9cb8c00802
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2366742Reviewed-by: Marc Treib <treib@chromium.org>
Commit-Queue: Rushan Suleymanov <rushans@google.com>
Cr-Commit-Position: refs/heads/master@{#800134}

components/sync_bookmarks/bookmark_remote_updates_handler.cc

@@ -363,7 +363,9 @@ void BookmarkRemoteUpdatesHandler::Process(
 
     if (tracked_entity && tracked_entity->IsUnsynced()) {
       ProcessConflict(*update, tracked_entity);
-      if (!bookmark_tracker_->GetEntityForSyncId(update_entity.id)) {
+      // |tracked_entity| might be deleted during processing conflict.
+      tracked_entity = bookmark_tracker_->GetEntityForSyncId(update_entity.id);
+      if (!tracked_entity) {
         // During conflict resolution, the entity could be dropped in case of
         // a conflict between local and remote deletions. We shouldn't worry
         // about changes to the encryption in that case.

components/sync_bookmarks/bookmark_remote_updates_handler_unittest.cc

@@ -1179,6 +1179,58 @@ TEST_F(BookmarkRemoteUpdatesHandlerWithInitialMergeTest,
   EXPECT_THAT(tracker()->GetEntityForSyncId(kId0)->IsUnsynced(), Eq(true));
 }
 
+// Tests that recommit will be initiated in case when there is a local tombstone
+// and server's update has out of date encryption.
+TEST_F(BookmarkRemoteUpdatesHandlerWithInitialMergeTest,
+       ShouldRecommitWhenEncryptionIsOutOfDateOnConflict) {
+  const std::string kId0 = "id0";
+
+  sync_pb::ModelTypeState model_type_state;
+  model_type_state.set_encryption_key_name("encryption_key_name");
+  tracker()->set_model_type_state(model_type_state);
+
+  // Create a new node and remove it locally.
+  syncer::UpdateResponseDataList updates;
+  syncer::UpdateResponseData response_data =
+      CreateUpdateResponseData(/*server_id=*/kId0,
+                               /*parent_id=*/kBookmarkBarId,
+                               /*is_deletion=*/false,
+                               /*version=*/0);
+  response_data.encryption_key_name = "encryption_key_name";
+  updates.push_back(std::move(response_data));
+  updates_handler()->Process(updates,
+                             /*got_new_encryption_requirements=*/false);
+
+  const SyncedBookmarkTracker::Entity* entity =
+      tracker()->GetEntityForSyncId(kId0);
+  ASSERT_THAT(entity, NotNull());
+  ASSERT_THAT(entity->bookmark_node(), NotNull());
+
+  bookmark_model()->Remove(entity->bookmark_node());
+  tracker()->MarkDeleted(entity);
+  tracker()->IncrementSequenceNumber(entity);
+
+  // Process an update with outdated encryption. This should cause a conflict
+  // and the remote version must be applied. Local tombstone entity will be
+  // removed during processing conflict.
+  updates.clear();
+  response_data = CreateUpdateResponseData(/*server_id=*/kId0,
+                                           /*parent_id=*/kBookmarkBarId,
+                                           /*is_deletion=*/false,
+                                           /*version=*/1);
+  response_data.encryption_key_name = "out_of_date_encryption_key_name";
+  updates.push_back(std::move(response_data));
+
+  updates_handler()->Process(updates,
+                             /*got_new_encryption_requirements=*/false);
+  // |entity| may be deleted here while processing update during conflict
+  // resolution.
+  entity = tracker()->GetEntityForSyncId(kId0);
+  ASSERT_THAT(entity, NotNull());
+  EXPECT_THAT(entity->IsUnsynced(), Eq(true));
+  EXPECT_THAT(entity->bookmark_node(), NotNull());
+}
+
 TEST_F(BookmarkRemoteUpdatesHandlerWithInitialMergeTest,
        ShouldRecommitWhenGotNewEncryptionRequirements) {
   const std::string kId0 = "id0";

components/sync_bookmarks/synced_bookmark_tracker.cc

@@ -413,16 +413,17 @@ void SyncedBookmarkTracker::MarkDeleted(const Entity* entity) {
 
 void SyncedBookmarkTracker::Remove(const Entity* entity) {
   DCHECK(entity);
-  // TODO(rushans): erase only if entity is not a tombstone.
+  DCHECK_EQ(entity, GetEntityForSyncId(entity->metadata()->server_id()));
+
   if (entity->bookmark_node()) {
     DCHECK(!entity->metadata()->is_deleted());
     DCHECK_EQ(0, std::count(ordered_local_tombstones_.begin(),
                             ordered_local_tombstones_.end(), entity));
+    bookmark_node_to_entities_map_.erase(entity->bookmark_node());
   } else {
     DCHECK(entity->metadata()->is_deleted());
   }
 
-  bookmark_node_to_entities_map_.erase(entity->bookmark_node());
   base::Erase(ordered_local_tombstones_, entity);
   sync_id_to_entities_map_.erase(entity->metadata()->server_id());
 }
@@ -596,6 +597,7 @@ SyncedBookmarkTracker::InitEntitiesFromModelAndMetadata(
           /*node=*/nullptr, std::make_unique<sync_pb::EntityMetadata>(std::move(
                                 *bookmark_metadata.mutable_metadata())));
       ordered_local_tombstones_.push_back(tombstone_entity.get());
+      DCHECK_EQ(0U, sync_id_to_entities_map_.count(sync_id));
       sync_id_to_entities_map_[sync_id] = std::move(tombstone_entity);
       continue;
     }
@@ -653,6 +655,7 @@ SyncedBookmarkTracker::InitEntitiesFromModelAndMetadata(
     entity->set_commit_may_have_started(true);
     CHECK_EQ(0U, bookmark_node_to_entities_map_.count(node));
     bookmark_node_to_entities_map_[node] = entity.get();
+    DCHECK_EQ(0U, sync_id_to_entities_map_.count(sync_id));
     sync_id_to_entities_map_[sync_id] = std::move(entity);
   }