Fix USS commit data's server ID not updated

We've recently received reports of DCHECK failures reflecting an
inconsistent state in the processor's tracker. The problematic scenario
is as follows:
1. There's a pending local creation, which starts with a temporary
   server ID. At the processor/tracker level, this means an empty ID.

2. A remote deletion is received for the same entity (same client tag
   hash), which is treated as conflict.

3. The default conflict resolution will choose to "undelete" the entity,
   that is, the creation should prevail over the deletion. However, the
   temporary server ID must be overriden with the real one as provided
   in the remote update (deletion).

Prior to this patch, ProcessorEntityTracker::RecordIgnoredUpdate() did
partially handle this scenario because the server ID in |metadata_| was
updated. However, the analogous ID in cached |commit_data_| also needs
to be updated.

The underlying issue has been around for very long but was reproduced
consistently with recent ongoing experiments that exercise local
deletions far more often.

Bug: 896138
Change-Id: I53a4e0447d2b809c5691a739a220046c8642156a
Reviewed-on: https://chromium-review.googlesource.com/c/1296169
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Reviewed-by: Jan Krcal <jkrcal@chromium.org>
Cr-Commit-Position: refs/heads/master@{#601891}

components/sync/model_impl/client_tag_based_model_type_processor_unittest.cc

@@ -1209,6 +1209,52 @@ TEST_F(ClientTagBasedModelTypeProcessorTest,
   worker()->VerifyNthPendingCommit(1, {kHash1}, {specifics2});
 }
 
+TEST_F(ClientTagBasedModelTypeProcessorTest,
+       ShouldResolveConflictToLocalUndeletion) {
+  InitializeToReadyState();
+  ASSERT_EQ(0U, worker()->GetNumPendingCommits());
+
+  bridge()->WriteItem(kKey1, kValue1);
+  ASSERT_EQ(1U, worker()->GetNumPendingCommits());
+  ASSERT_TRUE(
+      worker()->GetLatestPendingCommitForHash(kHash1).entity->id.empty());
+
+  // The update from the server should be mostly ignored because local wins, but
+  // the server ID should be updated.
+  bridge()->SetConflictResolution(ConflictResolution::UseLocal());
+  worker()->UpdateFromServer(kHash1, GenerateSpecifics(kKey1, kValue3));
+  // In this test setup, the processor's nudge for commit immediately pulls
+  // updates from the processor and list them as pending commits, so we should
+  // see two commits at this point.
+  EXPECT_EQ(2U, worker()->GetNumPendingCommits());
+
+  // Verify the commit request this operation has triggered.
+  const CommitRequestData& tag1_request_data =
+      worker()->GetLatestPendingCommitForHash(kHash1);
+  const EntityData& tag1_data = tag1_request_data.entity.value();
+
+  EXPECT_EQ(1, tag1_request_data.base_version);
+  EXPECT_FALSE(tag1_data.id.empty());
+  EXPECT_FALSE(tag1_data.creation_time.is_null());
+  EXPECT_FALSE(tag1_data.modification_time.is_null());
+  EXPECT_EQ(kKey1, tag1_data.non_unique_name);
+  EXPECT_FALSE(tag1_data.is_deleted());
+  EXPECT_EQ(kKey1, tag1_data.specifics.preference().name());
+  EXPECT_EQ(kValue1, tag1_data.specifics.preference().value());
+
+  EXPECT_EQ(1U, db().metadata_count());
+  const EntityMetadata metadata = db().GetMetadata(kKey1);
+  EXPECT_TRUE(metadata.has_client_tag_hash());
+  EXPECT_TRUE(metadata.has_server_id());
+  EXPECT_FALSE(metadata.is_deleted());
+  EXPECT_EQ(1, metadata.sequence_number());
+  EXPECT_EQ(0, metadata.acked_sequence_number());
+  EXPECT_EQ(1, metadata.server_version());
+  EXPECT_TRUE(metadata.has_creation_time());
+  EXPECT_TRUE(metadata.has_modification_time());
+  EXPECT_TRUE(metadata.has_specifics_hash());
+}
+
 TEST_F(ClientTagBasedModelTypeProcessorTest,
        ShouldResolveConflictToRemoteVersion) {
   InitializeToReadyState();

components/sync/model_impl/processor_entity_tracker.cc

@@ -138,6 +138,12 @@ void ProcessorEntityTracker::RecordIgnoredUpdate(
   // Either these already matched, acked was just bumped to squash a pending
   // commit and this should follow, or the pending commit needs to be requeued.
   commit_requested_sequence_number_ = metadata_.acked_sequence_number();
+  // If local change was made while server assigned a new id to the entity,
+  // update id in cached commit data.
+  if (HasCommitData() && commit_data_->id != metadata_.server_id()) {
+    DCHECK(commit_data_->id.empty());
+    commit_data_ = commit_data_->UpdateId(metadata_.server_id());
+  }
 }
 
 void ProcessorEntityTracker::RecordAcceptedUpdate(

components/sync/model_impl/processor_entity_tracker_unittest.cc

@@ -544,4 +544,46 @@ TEST_F(ProcessorEntityTrackerTest, RestoredLocalChangeWithUpdatedSpecifics) {
   // No verification is necessary. SetCommitData shouldn't DCHECK.
 }
 
+// Tests the scenario where a local creation conflicts with a remote deletion,
+// where usually (and in this test) local wins. In this case, the remote update
+// should be ignored but the server IDs should be updated.
+TEST_F(ProcessorEntityTrackerTest, LocalCreationConflictsWithServerTombstone) {
+  std::unique_ptr<ProcessorEntityTracker> entity = CreateNew();
+  entity->MakeLocalChange(GenerateEntityData(kHash, kName, kValue1));
+
+  ASSERT_TRUE(entity->IsUnsynced());
+  ASSERT_TRUE(entity->RequiresCommitRequest());
+  ASSERT_FALSE(entity->RequiresCommitData());
+  ASSERT_TRUE(entity->HasCommitData());
+  ASSERT_FALSE(entity->metadata().is_deleted());
+  ASSERT_TRUE(entity->metadata().server_id().empty());
+
+  {
+    // Local creation should use a temporary server ID (which in this tracker
+    // involves an empty string).
+    CommitRequestData request;
+    entity->InitializeCommitRequestData(&request);
+    EXPECT_TRUE(request.entity->id.empty());
+  }
+
+  // Before anything gets committed, we receive a remote tombstone, but local
+  // would usually win so the remote update is ignored.
+  entity->RecordIgnoredUpdate(
+      GenerateTombstone(*entity, kHash, kId, kName, base::Time::Now(), 2));
+
+  EXPECT_EQ(kId, entity->metadata().server_id());
+  EXPECT_TRUE(entity->IsUnsynced());
+  EXPECT_TRUE(entity->RequiresCommitRequest());
+  EXPECT_FALSE(entity->RequiresCommitData());
+  EXPECT_TRUE(entity->HasCommitData());
+  EXPECT_FALSE(entity->metadata().is_deleted());
+
+  // Generate a commit request. The server ID should have been reused from the
+  // otherwise ignored update.
+  const sync_pb::EntityMetadata metadata_v1 = entity->metadata();
+  CommitRequestData request;
+  entity->InitializeCommitRequestData(&request);
+  EXPECT_EQ(kId, request.entity->id);
+}
+
 }  // namespace syncer