Sync commit errors should temporarily re-enable

trigger pre-commit getupdates

Add a boolean variable in class DataTypeTracker, so every
time when a data type got conflict response from server
during commit, will set that boolean variable to true, then
next time sync, GetUpdate will check the boolean to see if
need to GetUpdate to resolve conflict locally.

BUG=324893

Committed: https://crrev.com/21f43c5af27e24c34565df26bb51fcc704c97597
Cr-Commit-Position: refs/heads/master@{#315339}

Review URL: https://codereview.chromium.org/905853002

Cr-Commit-Position: refs/heads/master@{#315828}

15 files changed, 111 insertions, 49 deletions

diff --git a/sync/engine/commit.cc b/sync/engine/commit.cc
index 509e08d..0561e9b 100644
--- a/sync/engine/commit.cc
+++ b/sync/engine/commit.cc
@@ -83,6 +83,7 @@ Commit* Commit::Init(
 }
 
 SyncerError Commit::PostAndProcessResponse(
+    sessions::NudgeTracker* nudge_tracker,
     sessions::SyncSession* session,
     sessions::StatusController* status,
     ExtensionsActivity* extensions_activity) {
@@ -153,6 +154,9 @@ SyncerError Commit::PostAndProcessResponse(
                  "type", ModelTypeToString(it->first));
     SyncerError type_result =
         it->second->ProcessCommitResponse(response_, status);
+    if (type_result == SERVER_RETURN_CONFLICT) {
+      nudge_tracker->RecordCommitConflict(it->first);
+    }
     if (processing_result == SYNCER_OK && type_result != SYNCER_OK) {
       processing_result = type_result;
     }
diff --git a/sync/engine/commit.h b/sync/engine/commit.h
index 993bd872..ee81577 100644
--- a/sync/engine/commit.h
+++ b/sync/engine/commit.h
@@ -14,6 +14,7 @@
 #include "sync/internal_api/public/engine/model_safe_worker.h"
 #include "sync/internal_api/public/util/syncer_error.h"
 #include "sync/protocol/sync.pb.h"
+#include "sync/sessions/nudge_tracker.h"
 #include "sync/util/extensions_activity.h"
 
 namespace syncer {
@@ -54,10 +55,10 @@ class SYNC_EXPORT_PRIVATE Commit {
       CommitProcessor* commit_processor,
       ExtensionsActivity* extensions_activity);
 
-  SyncerError PostAndProcessResponse(
-      sessions::SyncSession* session,
-      sessions::StatusController* status,
-      ExtensionsActivity* extensions_activity);
+  SyncerError PostAndProcessResponse(sessions::NudgeTracker* nudge_tracker,
+                                     sessions::SyncSession* session,
+                                     sessions::StatusController* status,
+                                     ExtensionsActivity* extensions_activity);
 
   // Cleans up state associated with this commit.  Must be called before the
   // destructor.
diff --git a/sync/engine/sync_scheduler_impl.cc b/sync/engine/sync_scheduler_impl.cc
index d279fd0..6f76be0 100644
--- a/sync/engine/sync_scheduler_impl.cc
+++ b/sync/engine/sync_scheduler_impl.cc
@@ -465,9 +465,7 @@ void SyncSchedulerImpl::DoNudgeSyncSessionJob(JobPriority priority) {
            << ModelTypeSetToString(session_context_->GetEnabledTypes());
   scoped_ptr<SyncSession> session(SyncSession::Build(session_context_, this));
   bool premature_exit = !syncer_->NormalSyncShare(
-      GetEnabledAndUnthrottledTypes(),
-      nudge_tracker_,
-      session.get());
+      GetEnabledAndUnthrottledTypes(), &nudge_tracker_, session.get());
   AdjustPolling(FORCE_RESET);
   // Don't run poll job till the next time poll timer fires.
   do_poll_after_credentials_updated_ = false;
diff --git a/sync/engine/sync_scheduler_unittest.cc b/sync/engine/sync_scheduler_unittest.cc
index 21d7f28..90b20a0 100644
--- a/sync/engine/sync_scheduler_unittest.cc
+++ b/sync/engine/sync_scheduler_unittest.cc
@@ -44,9 +44,10 @@ using sync_pb::GetUpdatesCallerInfo;
 class MockSyncer : public Syncer {
  public:
   MockSyncer();
-  MOCK_METHOD3(NormalSyncShare, bool(ModelTypeSet,
-                                     const sessions::NudgeTracker&,
-                                     sessions::SyncSession*));
+  MOCK_METHOD3(NormalSyncShare,
+               bool(ModelTypeSet,
+                    sessions::NudgeTracker*,
+                    sessions::SyncSession*));
   MOCK_METHOD3(ConfigureSyncShare,
                bool(ModelTypeSet,
                     sync_pb::GetUpdatesCallerInfo::GetUpdatesSource,
diff --git a/sync/engine/syncer.cc b/sync/engine/syncer.cc
index bae809e..ef759b8 100644
--- a/sync/engine/syncer.cc
+++ b/sync/engine/syncer.cc
@@ -55,13 +55,13 @@ bool Syncer::ExitRequested() {
 }
 
 bool Syncer::NormalSyncShare(ModelTypeSet request_types,
-                             const NudgeTracker& nudge_tracker,
+                             NudgeTracker* nudge_tracker,
                              SyncSession* session) {
   HandleCycleBegin(session);
-  if (nudge_tracker.IsGetUpdatesRequired() ||
+  if (nudge_tracker->IsGetUpdatesRequired() ||
       session->context()->ShouldFetchUpdatesBeforeCommit()) {
     VLOG(1) << "Downloading types " << ModelTypeSetToString(request_types);
-    NormalGetUpdatesDelegate normal_delegate(nudge_tracker);
+    NormalGetUpdatesDelegate normal_delegate(*nudge_tracker);
     GetUpdatesProcessor get_updates_processor(
         session->context()->model_type_registry()->update_handler_map(),
         normal_delegate);
@@ -70,18 +70,18 @@ bool Syncer::NormalSyncShare(ModelTypeSet request_types,
             session,
             &get_updates_processor,
             kCreateMobileBookmarksFolder)) {
-      return HandleCycleEnd(session, nudge_tracker.GetLegacySource());
+      return HandleCycleEnd(session, nudge_tracker->GetLegacySource());
     }
   }
 
   VLOG(1) << "Committing from types " << ModelTypeSetToString(request_types);
   CommitProcessor commit_processor(
       session->context()->model_type_registry()->commit_contributor_map());
-  SyncerError commit_result =
-      BuildAndPostCommits(request_types, session, &commit_processor);
+  SyncerError commit_result = BuildAndPostCommits(request_types, nudge_tracker,
+                                                  session, &commit_processor);
   session->mutable_status_controller()->set_commit_result(commit_result);
 
-  return HandleCycleEnd(session, nudge_tracker.GetLegacySource());
+  return HandleCycleEnd(session, nudge_tracker->GetLegacySource());
 }
 
 bool Syncer::ConfigureSyncShare(
@@ -160,6 +160,7 @@ bool Syncer::DownloadAndApplyUpdates(
 }
 
 SyncerError Syncer::BuildAndPostCommits(ModelTypeSet requested_types,
+                                        sessions::NudgeTracker* nudge_tracker,
                                         sessions::SyncSession* session,
                                         CommitProcessor* commit_processor) {
   // The ExitRequested() check is unnecessary, since we should start getting
@@ -180,8 +181,7 @@ SyncerError Syncer::BuildAndPostCommits(ModelTypeSet requested_types,
     }
 
     SyncerError error = commit->PostAndProcessResponse(
-        session,
-        session->mutable_status_controller(),
+        nudge_tracker, session, session->mutable_status_controller(),
         session->context()->extensions_activity());
     commit->CleanUp();
     if (error != SYNCER_OK) {
diff --git a/sync/engine/syncer.h b/sync/engine/syncer.h
index 02e66e3..ca93b3608 100644
--- a/sync/engine/syncer.h
+++ b/sync/engine/syncer.h
@@ -48,7 +48,7 @@ class SYNC_EXPORT_PRIVATE Syncer {
   // sync.  The |nudge_tracker| contains state that describes why the client is
   // out of sync and what must be done to bring it back into sync.
   virtual bool NormalSyncShare(ModelTypeSet request_types,
-                               const sessions::NudgeTracker& nudge_tracker,
+                               sessions::NudgeTracker* nudge_tracker,
                                sessions::SyncSession* session);
 
   // Performs an initial download for the |request_types|.  It is assumed that
@@ -79,10 +79,10 @@ class SYNC_EXPORT_PRIVATE Syncer {
   // number of unsynced and ready to commit items reaches zero or an error is
   // encountered.  A request to exit early will be treated as an error and will
   // abort any blocking operations.
-  SyncerError BuildAndPostCommits(
-      ModelTypeSet request_types,
-      sessions::SyncSession* session,
-      CommitProcessor* commit_processor);
+  SyncerError BuildAndPostCommits(ModelTypeSet request_types,
+                                  sessions::NudgeTracker* nudge_tracker,
+                                  sessions::SyncSession* session,
+                                  CommitProcessor* commit_processor);
 
   void HandleCycleBegin(sessions::SyncSession* session);
   bool HandleCycleEnd(
diff --git a/sync/engine/syncer_unittest.cc b/sync/engine/syncer_unittest.cc
index 471df65..5f049ec 100644
--- a/sync/engine/syncer_unittest.cc
+++ b/sync/engine/syncer_unittest.cc
@@ -262,11 +262,8 @@ class SyncerTest : public testing::Test,
     // Pretend we've seen a local change, to make the nudge_tracker look normal.
     nudge_tracker_.RecordLocalChange(ModelTypeSet(BOOKMARKS));
 
-    EXPECT_TRUE(
-        syncer_->NormalSyncShare(
-            context_->GetEnabledTypes(),
-            nudge_tracker_,
-            session_.get()));
+    EXPECT_TRUE(syncer_->NormalSyncShare(context_->GetEnabledTypes(),
+                                         &nudge_tracker_, session_.get()));
   }
 
   void SyncShareConfigure() {
@@ -642,8 +639,7 @@ TEST_F(SyncerTest, GetCommitIdsFiltersThrottledEntries) {
   ResetSession();
   syncer_->NormalSyncShare(
       Difference(context_->GetEnabledTypes(), ModelTypeSet(BOOKMARKS)),
-      nudge_tracker_,
-      session_.get());
+      &nudge_tracker_, session_.get());
 
   {
     // Nothing should have been committed as bookmarks is throttled.
@@ -2867,6 +2863,25 @@ TEST_F(SyncerTest, SendDebugInfoEventsOnGetUpdates_PostFailsDontDrop) {
   EXPECT_EQ(0, mock_server_->last_request().debug_info().events_size());
 }
 
+// Tests that commit failure with conflict will trigger GetUpdates for next
+// sycle of sync
+TEST_F(SyncerTest, CommitFailureWithConflict) {
+  ConfigureNoGetUpdatesRequired();
+  CreateUnsyncedDirectory("X", "id_X");
+  EXPECT_FALSE(nudge_tracker_.IsGetUpdatesRequired());
+
+  SyncShareNudge();
+  EXPECT_FALSE(nudge_tracker_.IsGetUpdatesRequired());
+
+  CreateUnsyncedDirectory("Y", "id_Y");
+  mock_server_->set_conflict_n_commits(1);
+  SyncShareNudge();
+  EXPECT_TRUE(nudge_tracker_.IsGetUpdatesRequired());
+
+  nudge_tracker_.RecordSuccessfulSyncCycle();
+  EXPECT_FALSE(nudge_tracker_.IsGetUpdatesRequired());
+}
+
 // Tests that sending debug info events on Commit works.
 TEST_F(SyncerTest, SendDebugInfoEventsOnCommit_HappyCase) {
   // Make sure GetUpdate isn't call as it would "steal" debug info events before
diff --git a/sync/protocol/sync.proto b/sync/protocol/sync.proto
index df302e3..2c79c22 100644
--- a/sync/protocol/sync.proto
+++ b/sync/protocol/sync.proto
@@ -492,6 +492,13 @@ message GetUpdateTriggers {
   //
   // Introduced in M38.
   optional bool initial_sync_in_progress = 7;
+
+  // This flag is set if this GetUpdate request is due to client receiving
+  // conflict response from server, so client needs to sync and then resolve
+  // conflict locally, and then commit again.
+  //
+  // Introduced in M42.
+  optional bool sync_for_resolve_conflict_in_progress = 8;
 }
 
 message GarbageCollectionDirective {
diff --git a/sync/sessions/data_type_tracker.cc b/sync/sessions/data_type_tracker.cc
index da50bbf..8103c00 100644
--- a/sync/sessions/data_type_tracker.cc
+++ b/sync/sessions/data_type_tracker.cc
@@ -15,7 +15,8 @@ DataTypeTracker::DataTypeTracker()
     : local_nudge_count_(0),
       local_refresh_request_count_(0),
       payload_buffer_size_(NudgeTracker::kDefaultMaxPayloadsPerType),
-      initial_sync_required_(false) {
+      initial_sync_required_(false),
+      sync_required_to_resolve_conflict_(false) {
 }
 
 DataTypeTracker::~DataTypeTracker() { }
@@ -92,6 +93,10 @@ void DataTypeTracker::RecordInitialSyncRequired() {
   initial_sync_required_ = true;
 }
 
+void DataTypeTracker::RecordCommitConflict() {
+  sync_required_to_resolve_conflict_ = true;
+}
+
 void DataTypeTracker::RecordSuccessfulSyncCycle() {
   // If we were throttled, then we would have been excluded from this cycle's
   // GetUpdates and Commit actions.  Our state remains unchanged.
@@ -119,6 +124,7 @@ void DataTypeTracker::RecordSuccessfulSyncCycle() {
   }
 
   initial_sync_required_ = false;
+  sync_required_to_resolve_conflict_ = false;
 }
 
 // This limit will take effect on all future invalidations received.
@@ -133,7 +139,7 @@ bool DataTypeTracker::IsSyncRequired() const {
 bool DataTypeTracker::IsGetUpdatesRequired() const {
   return !IsThrottled() &&
          (HasRefreshRequestPending() || HasPendingInvalidation() ||
-          IsInitialSyncRequired());
+          IsInitialSyncRequired() || IsSyncRequiredToResolveConflict());
 }
 
 bool DataTypeTracker::HasLocalChangePending() const {
@@ -152,6 +158,10 @@ bool DataTypeTracker::IsInitialSyncRequired() const {
   return initial_sync_required_;
 }
 
+bool DataTypeTracker::IsSyncRequiredToResolveConflict() const {
+  return sync_required_to_resolve_conflict_;
+}
+
 void DataTypeTracker::SetLegacyNotificationHint(
     sync_pb::DataTypeProgressMarker* progress) const {
   DCHECK(!IsThrottled())
@@ -192,6 +202,8 @@ void DataTypeTracker::FillGetUpdatesTriggersMessage(
   msg->set_local_modification_nudges(local_nudge_count_);
   msg->set_datatype_refresh_nudges(local_refresh_request_count_);
   msg->set_initial_sync_in_progress(initial_sync_required_);
+  msg->set_sync_for_resolve_conflict_in_progress(
+      sync_required_to_resolve_conflict_);
 }
 
 bool DataTypeTracker::IsThrottled() const {
diff --git a/sync/sessions/data_type_tracker.h b/sync/sessions/data_type_tracker.h
index c53f393..0842197 100644
--- a/sync/sessions/data_type_tracker.h
+++ b/sync/sessions/data_type_tracker.h
@@ -43,6 +43,10 @@ class DataTypeTracker {
   // Takes note that initial sync is pending for this type.
   void RecordInitialSyncRequired();
 
+  // Takes note that the conflict happended for this type, need to sync to
+  // resolve conflict locally.
+  void RecordCommitConflict();
+
   // Records that a sync cycle has been performed successfully.
   // Generally, this means that all local changes have been committed and all
   // remote changes have been downloaded, so we can clear any flags related to
@@ -74,6 +78,9 @@ class DataTypeTracker {
   // Returns true if this type is requesting an initial sync.
   bool IsInitialSyncRequired() const;
 
+  // Returns true if this type is requesting a sync to resolve conflict issue.
+  bool IsSyncRequiredToResolveConflict() const;
+
   // Fills in the legacy invalidaiton payload information fields.
   void SetLegacyNotificationHint(
       sync_pb::DataTypeProgressMarker* progress) const;
@@ -122,6 +129,9 @@ class DataTypeTracker {
   // sync.
   bool initial_sync_required_;
 
+  // Set to true if this type need to get update to resolve conflict issue.
+  bool sync_required_to_resolve_conflict_;
+
   // If !unthrottle_time_.is_null(), this type is throttled and may not download
   // or commit data until the specified time.
   base::TimeTicks unthrottle_time_;
diff --git a/sync/sessions/nudge_tracker.cc b/sync/sessions/nudge_tracker.cc
index 9c62b07..6af6993 100644
--- a/sync/sessions/nudge_tracker.cc
+++ b/sync/sessions/nudge_tracker.cc
@@ -168,6 +168,12 @@ void NudgeTracker::RecordInitialSyncRequired(syncer::ModelType type) {
   tracker_it->second->RecordInitialSyncRequired();
 }
 
+void NudgeTracker::RecordCommitConflict(syncer::ModelType type) {
+  TypeTrackerMap::iterator tracker_it = type_trackers_.find(type);
+  DCHECK(tracker_it != type_trackers_.end());
+  tracker_it->second->RecordCommitConflict();
+}
+
 void NudgeTracker::OnInvalidationsEnabled() {
   invalidations_enabled_ = true;
 }
diff --git a/sync/sessions/nudge_tracker.h b/sync/sessions/nudge_tracker.h
index 2bf9da1..ec2277c 100644
--- a/sync/sessions/nudge_tracker.h
+++ b/sync/sessions/nudge_tracker.h
@@ -70,6 +70,10 @@ class SYNC_EXPORT_PRIVATE NudgeTracker {
   // Take note that an initial sync is pending for this type.
   void RecordInitialSyncRequired(syncer::ModelType type);
 
+  // Takes note that the conflict happended for this type, need to sync to
+  // resolve conflict locally.
+  void RecordCommitConflict(syncer::ModelType type);
+
   // These functions should be called to keep this class informed of the status
   // of the connection to the invalidations server.
   void OnInvalidationsEnabled();
diff --git a/sync/sessions/nudge_tracker_unittest.cc b/sync/sessions/nudge_tracker_unittest.cc
index c6a9dfb8..db2bc3e 100644
--- a/sync/sessions/nudge_tracker_unittest.cc
+++ b/sync/sessions/nudge_tracker_unittest.cc
@@ -369,6 +369,12 @@ TEST_F(NudgeTrackerTest, IsSyncRequired) {
   nudge_tracker_.RecordSuccessfulSyncCycle();
   EXPECT_FALSE(nudge_tracker_.IsSyncRequired());
 
+  // Sync request for resolve conflict.
+  nudge_tracker_.RecordCommitConflict(BOOKMARKS);
+  EXPECT_TRUE(nudge_tracker_.IsSyncRequired());
+  nudge_tracker_.RecordSuccessfulSyncCycle();
+  EXPECT_FALSE(nudge_tracker_.IsSyncRequired());
+
   // Local changes.
   nudge_tracker_.RecordLocalChange(ModelTypeSet(SESSIONS));
   EXPECT_TRUE(nudge_tracker_.IsSyncRequired());
diff --git a/sync/sessions/test_util.cc b/sync/sessions/test_util.cc
index 1163ea7..c226a12 100644
--- a/sync/sessions/test_util.cc
+++ b/sync/sessions/test_util.cc
@@ -46,23 +46,22 @@ void SimulateConfigureConnectionFailure(
 }
 
 void SimulateNormalSuccess(ModelTypeSet requested_types,
-                           const sessions::NudgeTracker& nudge_tracker,
+                           sessions::NudgeTracker* nudge_tracker,
                            sessions::SyncSession* session) {
   session->mutable_status_controller()->set_commit_result(SYNCER_OK);
   session->mutable_status_controller()->set_last_download_updates_result(
       SYNCER_OK);
 }
 
-void SimulateDownloadUpdatesFailed(
-    ModelTypeSet requested_types,
-    const sessions::NudgeTracker& nudge_tracker,
-    sessions::SyncSession* session) {
+void SimulateDownloadUpdatesFailed(ModelTypeSet requested_types,
+                                   sessions::NudgeTracker* nudge_tracker,
+                                   sessions::SyncSession* session) {
   session->mutable_status_controller()->set_last_download_updates_result(
       SERVER_RETURN_TRANSIENT_ERROR);
 }
 
 void SimulateCommitFailed(ModelTypeSet requested_types,
-                          const sessions::NudgeTracker& nudge_tracker,
+                          sessions::NudgeTracker* nudge_tracker,
                           sessions::SyncSession* session) {
   session->mutable_status_controller()->set_last_get_key_result(SYNCER_OK);
   session->mutable_status_controller()->set_last_download_updates_result(
@@ -71,10 +70,9 @@ void SimulateCommitFailed(ModelTypeSet requested_types,
       SERVER_RETURN_TRANSIENT_ERROR);
 }
 
-void SimulateConnectionFailure(
-                           ModelTypeSet requested_types,
-                           const sessions::NudgeTracker& nudge_tracker,
-                           sessions::SyncSession* session) {
+void SimulateConnectionFailure(ModelTypeSet requested_types,
+                               sessions::NudgeTracker* nudge_tracker,
+                               sessions::SyncSession* session) {
   session->mutable_status_controller()->set_last_download_updates_result(
       NETWORK_CONNECTION_UNAVAILABLE);
 }
@@ -118,7 +116,7 @@ void SimulatePollIntervalUpdateImpl(
 
 void SimulateSessionsCommitDelayUpdateImpl(
     ModelTypeSet requested_types,
-    const sessions::NudgeTracker& nudge_tracker,
+    sessions::NudgeTracker* nudge_tracker,
     sessions::SyncSession* session,
     const base::TimeDelta& new_delay) {
   SimulateNormalSuccess(requested_types, nudge_tracker, session);
diff --git a/sync/sessions/test_util.h b/sync/sessions/test_util.h
index 357637e..3e905dd 100644
--- a/sync/sessions/test_util.h
+++ b/sync/sessions/test_util.h
@@ -35,16 +35,16 @@ void SimulateConfigureConnectionFailure(
 
 // Normal mode sync cycle successes and failures.
 void SimulateNormalSuccess(ModelTypeSet requested_types,
-                           const sessions::NudgeTracker& nudge_tracker,
+                           sessions::NudgeTracker* nudge_tracker,
                            sessions::SyncSession* session);
 void SimulateDownloadUpdatesFailed(ModelTypeSet requested_types,
-                                   const sessions::NudgeTracker& nudge_tracker,
+                                   sessions::NudgeTracker* nudge_tracker,
                                    sessions::SyncSession* session);
 void SimulateCommitFailed(ModelTypeSet requested_types,
-                          const sessions::NudgeTracker& nudge_tracker,
+                          sessions::NudgeTracker* nudge_tracker,
                           sessions::SyncSession* session);
 void SimulateConnectionFailure(ModelTypeSet requested_types,
-                               const sessions::NudgeTracker& nudge_tracker,
+                               sessions::NudgeTracker* nudge_tracker,
                                sessions::SyncSession* session);
 
 // Poll successes and failures.
@@ -73,7 +73,7 @@ void SimulatePollIntervalUpdateImpl(
 // Works with normal cycles.
 void SimulateSessionsCommitDelayUpdateImpl(
     ModelTypeSet requested_types,
-    const sessions::NudgeTracker& nudge_tracker,
+    sessions::NudgeTracker* nudge_tracker,
     sessions::SyncSession* session,
     const base::TimeDelta& new_delay);