Revert "[Sync] Process commit failure in Nigori model type processor"

This reverts commit a1c5cc32.

Reason for revert: likely culprit for increasing server-side exceptions count.

Original change's description:
> [Sync] Process commit failure in Nigori model type processor
>
> In case when the server returns commit failure, it is possible that
> entities will be in the transient state. This CL resets this state of
> the entity for nigori data type on any commit failure.
>
> Bug: 1137817
> Change-Id: I042015e4f211351ad13469e510f22ae71be28c24
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2563302
> Commit-Queue: Rushan Suleymanov <rushans@google.com>
> Reviewed-by: Maksim Moskvitin <mmoskvitin@google.com>
> Cr-Commit-Position: refs/heads/master@{#831876}

TBR=mmoskvitin@google.com,rushans@google.com

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: 1137817
Change-Id: Ib4be790ff8b53b8c959b6bcd76849a78b9728812
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2577474Reviewed-by: Maksim Moskvitin <mmoskvitin@google.com>
Commit-Queue: Maksim Moskvitin <mmoskvitin@google.com>
Cr-Commit-Position: refs/heads/master@{#834681}

components/sync/nigori/nigori_model_type_processor.cc

@@ -111,13 +111,6 @@ void NigoriModelTypeProcessor::OnCommitCompleted(
   bridge_->ApplySyncChanges(/*data=*/base::nullopt);
 }
 
-void NigoriModelTypeProcessor::OnCommitFailed(SyncCommitError commit_error) {
-  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-  DCHECK(entity_);
-
-  entity_->ClearTransientSyncState();
-}
-
 void NigoriModelTypeProcessor::OnUpdateReceived(
     const sync_pb::ModelTypeState& type_state,
     UpdateResponseDataList updates) {

components/sync/nigori/nigori_model_type_processor.h

@@ -37,7 +37,6 @@ class NigoriModelTypeProcessor : public ModelTypeProcessor,
       const sync_pb::ModelTypeState& type_state,
       const CommitResponseDataList& committed_response_list,
       const FailedCommitResponseDataList& error_response_list) override;
-  void OnCommitFailed(SyncCommitError commit_error) override;
   void OnUpdateReceived(const sync_pb::ModelTypeState& type_state,
                         UpdateResponseDataList updates) override;
 

components/sync/nigori/nigori_model_type_processor_unittest.cc

@@ -25,11 +25,9 @@ namespace syncer {
 namespace {
 
 using testing::_;
-using testing::ByMove;
 using testing::Eq;
 using testing::Ne;
 using testing::NotNull;
-using testing::Return;
 
 // TODO(mamir): remove those and adjust the code accordingly.
 const char kRawNigoriClientTagHash[] = "NigoriClientTagHash";
@@ -56,29 +54,22 @@ sync_pb::ModelTypeState CreateDummyModelTypeState() {
   return model_type_state;
 }
 
-std::unique_ptr<EntityData> CreateDummyNigoriEntityData() {
-  auto entity_data = std::make_unique<EntityData>();
-  // Set empty nigori specifics.
-  entity_data->specifics.mutable_nigori();
-  entity_data->name = kNigoriNonUniqueName;
-  entity_data->is_folder = true;
-  return entity_data;
-}
-
 // Creates a dummy Nigori UpdateResponseData that has the keystore decryptor
 // token key name set.
 syncer::UpdateResponseData CreateDummyNigoriUpdateResponseData(
     const std::string keystore_decryptor_token_key_name,
     int response_version) {
-  std::unique_ptr<EntityData> entity_data = CreateDummyNigoriEntityData();
-  entity_data->id = kNigoriServerId;
+  syncer::EntityData entity_data;
+  entity_data.is_folder = true;
+  entity_data.id = kNigoriServerId;
   sync_pb::NigoriSpecifics* nigori_specifics =
-      entity_data->specifics.mutable_nigori();
+      entity_data.specifics.mutable_nigori();
   nigori_specifics->mutable_keystore_decryptor_token()->set_key_name(
       keystore_decryptor_token_key_name);
+  entity_data.name = kNigoriNonUniqueName;
 
   syncer::UpdateResponseData response_data;
-  response_data.entity = std::move(*entity_data);
+  response_data.entity = std::move(entity_data);
   response_data.response_version = response_version;
   return response_data;
 }
@@ -211,7 +202,12 @@ TEST_F(NigoriModelTypeProcessorTest, ShouldIncrementSequenceNumberWhenPut) {
       processor()->GetMetadata().entity_metadata;
   ASSERT_TRUE(entity_metadata1);
 
-  processor()->Put(CreateDummyNigoriEntityData());
+  auto entity_data = std::make_unique<syncer::EntityData>();
+  entity_data->specifics.mutable_nigori();
+  entity_data->name = kNigoriNonUniqueName;
+  entity_data->is_folder = true;
+
+  processor()->Put(std::move(entity_data));
 
   base::Optional<sync_pb::EntityMetadata> entity_metadata2 =
       processor()->GetMetadata().entity_metadata;
@@ -233,7 +229,12 @@ TEST_F(NigoriModelTypeProcessorTest, ShouldGetEmptyLocalChanges) {
 TEST_F(NigoriModelTypeProcessorTest, ShouldGetLocalChangesWhenPut) {
   SimulateModelReadyToSync(/*initial_sync_done=*/true);
 
-  processor()->Put(CreateDummyNigoriEntityData());
+  auto entity_data = std::make_unique<syncer::EntityData>();
+  entity_data->specifics.mutable_nigori();
+  entity_data->name = kNigoriNonUniqueName;
+  entity_data->is_folder = true;
+
+  processor()->Put(std::move(entity_data));
   CommitRequestDataList commit_request;
   processor()->GetLocalChanges(
       /*max_entries=*/10,
@@ -246,7 +247,12 @@ TEST_F(NigoriModelTypeProcessorTest,
        ShouldSquashCommitRequestUponCommitCompleted) {
   SimulateModelReadyToSync(/*initial_sync_done=*/true);
 
-  processor()->Put(CreateDummyNigoriEntityData());
+  auto entity_data = std::make_unique<syncer::EntityData>();
+  entity_data->specifics.mutable_nigori();
+  entity_data->name = kNigoriNonUniqueName;
+  entity_data->is_folder = true;
+
+  processor()->Put(std::move(entity_data));
   CommitRequestDataList commit_request_list;
   processor()->GetLocalChanges(
       /*max_entries=*/10,
@@ -278,7 +284,12 @@ TEST_F(NigoriModelTypeProcessorTest,
        ShouldNotSquashCommitRequestUponEmptyCommitResponse) {
   SimulateModelReadyToSync(/*initial_sync_done=*/true);
 
-  processor()->Put(CreateDummyNigoriEntityData());
+  auto entity_data = std::make_unique<syncer::EntityData>();
+  entity_data->specifics.mutable_nigori();
+  entity_data->name = kNigoriNonUniqueName;
+  entity_data->is_folder = true;
+
+  processor()->Put(std::move(entity_data));
   CommitRequestDataList commit_request_list;
   processor()->GetLocalChanges(
       /*max_entries=*/10,
@@ -294,8 +305,13 @@ TEST_F(NigoriModelTypeProcessorTest,
 
   // Data has been moved into the previous request, so the processor will ask
   // for the commit data once more.
-  EXPECT_CALL(*mock_nigori_sync_bridge(), GetData)
-      .WillOnce(Return(ByMove(CreateDummyNigoriEntityData())));
+  ON_CALL(*mock_nigori_sync_bridge(), GetData()).WillByDefault([&]() {
+    auto entity_data = std::make_unique<syncer::EntityData>();
+    entity_data->specifics.mutable_nigori();
+    entity_data->name = kNigoriNonUniqueName;
+    entity_data->is_folder = true;
+    return entity_data;
+  });
 
   // The commit should still be pending.
   CommitResponseDataList commit_response_list;
@@ -309,10 +325,12 @@ TEST_F(NigoriModelTypeProcessorTest,
        ShouldKeepAnotherCommitRequestUponCommitCompleted) {
   SimulateModelReadyToSync(/*initial_sync_done=*/true);
 
-  std::unique_ptr<EntityData> entity_data = CreateDummyNigoriEntityData();
+  auto entity_data = std::make_unique<syncer::EntityData>();
   sync_pb::NigoriSpecifics* nigori_specifics =
       entity_data->specifics.mutable_nigori();
   nigori_specifics->set_encrypt_bookmarks(true);
+  entity_data->name = kNigoriNonUniqueName;
+  entity_data->is_folder = true;
 
   processor()->Put(std::move(entity_data));
   CommitRequestDataList commit_request_list;
@@ -329,8 +347,10 @@ TEST_F(NigoriModelTypeProcessorTest,
                                    1));
 
   // Make another local change before the commit response is received.
-  entity_data = CreateDummyNigoriEntityData();
+  entity_data = std::make_unique<syncer::EntityData>();
   nigori_specifics = entity_data->specifics.mutable_nigori();
+  entity_data->name = kNigoriNonUniqueName;
+  entity_data->is_folder = true;
   nigori_specifics->set_encrypt_preferences(true);
   processor()->Put(std::move(entity_data));
 
@@ -349,44 +369,16 @@ TEST_F(NigoriModelTypeProcessorTest,
   EXPECT_EQ(1U, commit_request_list.size());
 }
 
-TEST_F(NigoriModelTypeProcessorTest, ShouldRetryCommitAfterCommitFailed) {
-  SimulateModelReadyToSync(/*initial_sync_done=*/true);
-
-  std::unique_ptr<EntityData> entity_data = CreateDummyNigoriEntityData();
-  entity_data->specifics.mutable_nigori()->set_encrypt_everything(true);
-
-  processor()->Put(std::move(entity_data));
-  CommitRequestDataList commit_request_list;
-  processor()->GetLocalChanges(
-      /*max_entries=*/10,
-      base::BindOnce(&CaptureCommitRequest, &commit_request_list));
-  ASSERT_EQ(1U, commit_request_list.size());
-
-  // Next call for local changes should return nothing.
-  commit_request_list.clear();
-  processor()->GetLocalChanges(
-      /*max_entries=*/10,
-      base::BindOnce(&CaptureCommitRequest, &commit_request_list));
-  ASSERT_EQ(0U, commit_request_list.size());
-
-  processor()->OnCommitFailed(SyncCommitError::kBadServerResponse);
-
-  // Data has been moved into the previous request, so the processor will ask
-  // for the commit data once more.
-  EXPECT_CALL(*mock_nigori_sync_bridge(), GetData)
-      .WillOnce(Return(ByMove(CreateDummyNigoriEntityData())));
-
-  processor()->GetLocalChanges(
-      /*max_entries=*/10,
-      base::BindOnce(&CaptureCommitRequest, &commit_request_list));
-  EXPECT_EQ(1U, commit_request_list.size());
-}
-
 TEST_F(NigoriModelTypeProcessorTest,
        ShouldNudgeForCommitUponConnectSyncIfReadyToSyncAndLocalChanges) {
   SimulateModelReadyToSync(/*initial_sync_done=*/true);
 
-  processor()->Put(CreateDummyNigoriEntityData());
+  auto entity_data = std::make_unique<syncer::EntityData>();
+  entity_data->specifics.mutable_nigori();
+  entity_data->name = kNigoriNonUniqueName;
+  entity_data->is_folder = true;
+
+  processor()->Put(std::move(entity_data));
 
   EXPECT_CALL(*mock_commit_queue(), NudgeForCommit());
   SimulateConnectSync();
@@ -396,8 +388,13 @@ TEST_F(NigoriModelTypeProcessorTest, ShouldNudgeForCommitUponPutIfReadyToSync) {
   SimulateModelReadyToSync(/*initial_sync_done=*/true);
   SimulateConnectSync();
 
+  auto entity_data = std::make_unique<syncer::EntityData>();
+  entity_data->specifics.mutable_nigori();
+  entity_data->name = kNigoriNonUniqueName;
+  entity_data->is_folder = true;
+
   EXPECT_CALL(*mock_commit_queue(), NudgeForCommit());
-  processor()->Put(CreateDummyNigoriEntityData());
+  processor()->Put(std::move(entity_data));
 }
 
 TEST_F(NigoriModelTypeProcessorTest, ShouldInvokeSyncStartCallback) {