Fix the false-positive detection of commit errors

chrome/browser/sync/engine/sync_scheduler.cc

Index: chrome/browser/sync/engine/sync_scheduler.cc
diff --git a/chrome/browser/sync/engine/sync_scheduler.cc b/chrome/browser/sync/engine/sync_scheduler.cc
index d7d68abd73e6c7a41f0efdd76c473e8f44d69de3..320c6a04c150efaa039b662b7b91449ac8d34d81 100644
--- a/chrome/browser/sync/engine/sync_scheduler.cc
+++ b/chrome/browser/sync/engine/sync_scheduler.cc
@@ -813,74 +813,42 @@ void SyncScheduler::FinishSyncSessionJob(const SyncSessionJob& job) {
 void SyncScheduler::ScheduleNextSync(const SyncSessionJob& old_job) {
   DCHECK_EQ(MessageLoop::current(), sync_loop_);
   DCHECK(!old_job.session->HasMoreToSync());
-  // Note: |num_server_changes_remaining| > 0 here implies that we received a
-  // broken response while trying to download all updates, because the Syncer
-  // will loop until this value is exhausted. Also, if unsynced_handles exist
-  // but HasMoreToSync is false, this implies that the Syncer determined no
-  // forward progress was possible at this time (an error, such as an HTTP
-  // 500, is likely to have occurred during commit).
-  int num_server_changes_remaining =
-      old_job.session->status_controller()->num_server_changes_remaining();
-  size_t num_unsynced_handles =
-      old_job.session->status_controller()->unsynced_handles().size();
-  const bool work_to_do =
-      num_server_changes_remaining > 0 || num_unsynced_handles > 0;
-  SVLOG(2) << "num server changes remaining: " << num_server_changes_remaining
-           << ", num unsynced handles: " << num_unsynced_handles
-           << ", syncer has work to do: " << work_to_do;
 
   AdjustPolling(&old_job);
 
   // TODO(tim): Old impl had special code if notifications disabled. Needed?
-  if (!work_to_do) {
-    // Success implies backoff relief.  Note that if this was a
-    // "one-off" job (i.e. purpose ==
-    // SyncSessionJob::{CLEAR_USER_DATA,CLEANUP_DISABLED_TYPES}), if
-    // there was work_to_do before it ran this wont have changed, as
-    // jobs like this don't run a full sync cycle.  So we don't need
-    // special code here.
+  if (old_job.session->FailedToMakeProgress()) {
+      if (IsBackingOff() && wait_interval_->timer.IsRunning() &&
+          mode_ == NORMAL_MODE) {
+        // We are in backoff mode and our time did not run out. That means we
+        // had a local change, notification from server or a network connection
+        // change notification. In any case set had_nudge = true so we dont
+        // retry next nudge.
+        // Note: we will keep retrying network connection changes though as
+        // they are treated as canary jobs. Also we check the mode here because
+        // we want to do this only in normal mode. For config mode jobs we dont
+        // have anything similar to had_nudge.
+        SVLOG(2) << "A nudge during backoff failed";
+        // We weren't continuing but we're in backoff; must have been a nudge.
+        DCHECK_EQ(SyncSessionJob::NUDGE, old_job.purpose);
+        DCHECK(!wait_interval_->had_nudge);
+        wait_interval_->had_nudge = true;
+        // Old job did not finish. So make it the pending job.
+        InitOrCoalescePendingJob(old_job);
+        // Resume waiting.
+        RestartWaiting();
+      } else {

[lipalani1, 2011/09/22 20:22:46]

Can you add the vlog stmt back here?

+        // We seem to have not made forward progress. Start or extend backoff.
+        HandleConsecutiveContinuationError(old_job);
+      }
+  } else {
+    // Success implies backoff relief.  Note that if this was a "one-off" job
+    // (i.e. purpose ==
+    // SyncSessionJob::{CLEAR_USER_DATA,CLEANUP_DISABLED_TYPES}), if there was
+    // work to do before it ran this wont have changed, as jobs like this don't
+    // run a full sync cycle.  So we don't need special code here.
     wait_interval_.reset();
     SVLOG(2) << "Job succeeded so not scheduling more jobs";
-    return;
-  }
-
-  // We are in backoff mode and our time did not run out. That means we had
-  // a local change, notification from server or a network connection change
-  // notification. In any case set had_nudge = true so we dont retry next
-  // nudge. Note: we will keep retrying network connection changes though as
-  // they are treated as canary jobs. Also we check the mode here because
-  // we want to do this only in normal mode. For config mode jobs we dont
-  // have anything similar to had_nudge.
-  if (IsBackingOff() && wait_interval_->timer.IsRunning() &&
-      mode_ == NORMAL_MODE) {
-    SVLOG(2) << "A nudge during backoff failed";
-    // We weren't continuing but we're in backoff; must have been a nudge.
-    DCHECK_EQ(SyncSessionJob::NUDGE, old_job.purpose);
-    DCHECK(!wait_interval_->had_nudge);
-    wait_interval_->had_nudge = true;
-    // Old job did not finish. So make it the pending job.
-    InitOrCoalescePendingJob(old_job);
-    // Resume waiting.
-    RestartWaiting();
-  } else if (old_job.session->source().updates_source ==
-             GetUpdatesCallerInfo::SYNC_CYCLE_CONTINUATION) {
-    SVLOG(2) << "Job failed with source continuation";
-    // We don't seem to have made forward progress. Start or extend backoff.
-    HandleConsecutiveContinuationError(old_job);
-  } else {
-    SVLOG(2) << "Failed. Schedule a job with continuation as source";
-    // We weren't continuing and we aren't in backoff.  Schedule a normal
-    // continuation.
-    if (old_job.purpose == SyncSessionJob::CONFIGURATION) {
-      ScheduleConfigImpl(old_job.session->routing_info(),
-          old_job.session->workers(),
-          GetUpdatesFromNudgeSource(NUDGE_SOURCE_CONTINUATION));
-    } else  {
-      // For all other purposes(nudge and poll) we schedule a retry nudge.
-      ScheduleNudgeImpl(TimeDelta::FromSeconds(0),
-                        GetUpdatesFromNudgeSource(NUDGE_SOURCE_CONTINUATION),
-                        old_job.session->source().types, false, FROM_HERE);
-    }
   }
 }