sync: Refactor job ownership in SyncScheduler

sync/engine/sync_scheduler_impl.cc

Index: sync/engine/sync_scheduler_impl.cc
diff --git a/sync/engine/sync_scheduler_impl.cc b/sync/engine/sync_scheduler_impl.cc
index da195369c991ef36342bbe89b2a517b1e3d5e846..bd2e3e9a31bf7fe84d9a14b83b77873846535550 100644
--- a/sync/engine/sync_scheduler_impl.cc
+++ b/sync/engine/sync_scheduler_impl.cc
@@ -83,11 +83,10 @@ ConfigurationParams::ConfigurationParams(
 ConfigurationParams::~ConfigurationParams() {}
 
 SyncSchedulerImpl::WaitInterval::WaitInterval()
-    : mode(UNKNOWN),
-      had_nudge(false) {}
+    : mode(UNKNOWN) {}
 
 SyncSchedulerImpl::WaitInterval::WaitInterval(Mode mode, TimeDelta length)
-    : mode(mode), had_nudge(false), length(length) {}
+    : mode(mode), length(length) {}
 
 SyncSchedulerImpl::WaitInterval::~WaitInterval() {}
 
@@ -323,7 +322,6 @@ SyncSchedulerImpl::DecideWhileInWaitInterval(const SyncSessionJob& job,
 
   SDVLOG(2) << "DecideWhileInWaitInterval with WaitInterval mode "
             << WaitInterval::GetModeString(wait_interval_->mode)
-            << (wait_interval_->had_nudge ? " (had nudge)" : "")
             << ((priority == CANARY_PRIORITY) ? " (canary)" : "");
 
   // If we save a job while in a WaitInterval, there is a well-defined moment
@@ -345,7 +343,7 @@ SyncSchedulerImpl::DecideWhileInWaitInterval(const SyncSessionJob& job,
     // If we already had one nudge then just drop this nudge. We will retry
     // later when the timer runs out.
     if (priority == NORMAL_PRIORITY)
-      return wait_interval_->had_nudge ? DROP : CONTINUE;
+      return DROP;
     else // We are here because timer ran out. So retry.

[tim (not reviewing), 2013/04/05 16:56:44]

In fact this comment isn't true, so we should fix it.  We can wind up here if we
get a network change notification during EXPONENTIAL_BACKOFF, for example.  In
that case, we should attempt to sync before the WaitInterval expires.  

[rlarocque, 2013/04/05 19:13:29]

Done.

       return CONTINUE;
   }
@@ -541,13 +539,14 @@ void SyncSchedulerImpl::ScheduleNudgeImpl(
       << "Scheduling a nudge with "
       << run_delay.InMilliseconds() << " ms delay";
 
-  PostDelayedTask(
-      nudge_location,
-      "DoSyncSessionJob",
-      base::Bind(base::IgnoreResult(&SyncSchedulerImpl::DoNudgeSyncSessionJob),
-                 weak_ptr_factory_.GetWeakPtr(),
-                 NORMAL_PRIORITY),
-      run_delay);
+  if (started_) {
+    pending_wakeup_timer_.Start(
+        nudge_location,
+        run_delay,
+        base::Bind(&SyncSchedulerImpl::DoNudgeSyncSessionJob,
+                   weak_ptr_factory_.GetWeakPtr(),
+                   NORMAL_PRIORITY));
+  }
 }
 
 const char* SyncSchedulerImpl::GetModeString(SyncScheduler::Mode mode) {
@@ -568,23 +567,6 @@ const char* SyncSchedulerImpl::GetDecisionString(
   return "";
 }
 
-void SyncSchedulerImpl::PostDelayedTask(
-    const tracked_objects::Location& from_here,
-    const char* name, const base::Closure& task, base::TimeDelta delay) {
-  SDVLOG_LOC(from_here, 3) << "Posting " << name << " task with "
-                           << delay.InMilliseconds() << " ms delay";
-  DCHECK_EQ(MessageLoop::current(), sync_loop_);
-  if (!started_) {
-    SDVLOG(1) << "Not posting task as scheduler is stopped.";
-    return;
-  }
-  // This cancels the previous task, if one existed.
-  pending_wakeup_event_.Reset(task);
-  sync_loop_->PostDelayedTask(from_here,
-                              pending_wakeup_event_.callback(),
-                              delay);
-}
-
 bool SyncSchedulerImpl::DoSyncSessionJobImpl(scoped_ptr<SyncSessionJob> job,
                                              JobPriority priority) {
   DCHECK_EQ(MessageLoop::current(), sync_loop_);
@@ -600,14 +582,6 @@ bool SyncSchedulerImpl::DoSyncSessionJobImpl(scoped_ptr<SyncSessionJob> job,
     if (decision == SAVE) {
       if (job->purpose() == SyncSessionJob::CONFIGURATION) {
         pending_configure_job_ = job.Pass();
-
-        // It's very unlikely, but possible, that the WaitInterval's wakeup task
-        // isn't actually active at this point.  Sometimes the WaitInterval gets
-        // preempted by a nudge-while-in-backoff.  We can't just assume that
-        // there's a task waiting to wake us up once the WaitInterval expires;
-        // we need to ensure it by rescheduling the WaitInterval (while taking
-        // into account any time already spent waiting, of course).
-        ResumeWaiting();
       } else {
         pending_nudge_job_ = job.Pass();
       }
@@ -652,8 +626,8 @@ bool SyncSchedulerImpl::DoSyncSessionJobImpl(scoped_ptr<SyncSessionJob> job,
   return success;
 }
 
-bool SyncSchedulerImpl::DoNudgeSyncSessionJob(JobPriority priority) {
-  return DoSyncSessionJobImpl(pending_nudge_job_.Pass(), priority);
+void SyncSchedulerImpl::DoNudgeSyncSessionJob(JobPriority priority) {
+  DoSyncSessionJobImpl(pending_nudge_job_.Pass(), priority);
 }
 
 bool SyncSchedulerImpl::DoConfigurationSyncSessionJob(JobPriority priority) {
@@ -782,29 +756,8 @@ void SyncSchedulerImpl::ScheduleNextSync(
   // we should be able to detect such errors and only retry when we detect
   // transient errors.
 
-  if (IsBackingOff() && wait_interval_->timer.IsRunning() &&
-      mode_ == NORMAL_MODE) {
-    // When in normal mode, we allow up to one nudge per backoff interval.  It
-    // appears that this was our nudge for this interval, and it failed.
-    //
-    // Note: This does not prevent us from running canary jobs.  For example,
-    // an IP address change might still result in another nudge being executed
-    // during this backoff interval.
-    SDVLOG(2) << "A nudge during backoff failed, creating new pending nudge.";
-    DCHECK_EQ(SyncSessionJob::NUDGE, finished_job->purpose());
-    DCHECK(!wait_interval_->had_nudge);
-
-    wait_interval_->had_nudge = true;
-    DCHECK(!pending_nudge_job_);
-
-    pending_nudge_job_ = finished_job.Pass();
-    RestartWaiting();
-  } else {
-    // Either this is the first failure or a consecutive failure after our
-    // backoff timer expired.  We handle it the same way in either case.
-    SDVLOG(2) << "Non-'backoff nudge' SyncShare job failed";
-    HandleContinuationError(finished_job.Pass(), session);
-  }
+  SDVLOG(2) << "SyncShare job failed; will start or update backoff";
+  HandleContinuationError(finished_job.Pass(), session);
 }
 
 void SyncSchedulerImpl::AdjustPolling(const SyncSessionJob* old_job) {
@@ -828,28 +781,22 @@ void SyncSchedulerImpl::AdjustPolling(const SyncSessionJob* old_job) {
                     &SyncSchedulerImpl::PollTimerCallback);
 }
 
-void SyncSchedulerImpl::ResumeWaiting() {
-  TimeDelta length =
-      wait_interval_->timer.desired_run_time() - TimeTicks::Now();
-  wait_interval_->length = length < TimeDelta::FromSeconds(0) ?
-      TimeDelta::FromSeconds(0) : length;
-  RestartWaiting();
-}
-
 void SyncSchedulerImpl::RestartWaiting() {
   CHECK(wait_interval_.get());
-  wait_interval_->timer.Stop();
   DCHECK(wait_interval_->length >= TimeDelta::FromSeconds(0));
   if (wait_interval_->mode == WaitInterval::THROTTLED) {
-    pending_wakeup_event_.Reset(base::Bind(&SyncSchedulerImpl::Unthrottle,
-                                           weak_ptr_factory_.GetWeakPtr()));
-
+    pending_wakeup_timer_.Start(
+        FROM_HERE,
+        wait_interval_->length,
+        base::Bind(&SyncSchedulerImpl::Unthrottle,
+                   weak_ptr_factory_.GetWeakPtr()));
   } else {
-    pending_wakeup_event_.Reset(base::Bind(&SyncSchedulerImpl::TryCanaryJob,
-                                           weak_ptr_factory_.GetWeakPtr()));
+    pending_wakeup_timer_.Start(
+        FROM_HERE,
+        wait_interval_->length,
+        base::Bind(&SyncSchedulerImpl::TryCanaryJob,
+                   weak_ptr_factory_.GetWeakPtr()));
   }
-  wait_interval_->timer.Start(FROM_HERE, wait_interval_->length,
-                              pending_wakeup_event_.callback());
 }
 
 void SyncSchedulerImpl::HandleContinuationError(
@@ -867,7 +814,6 @@ void SyncSchedulerImpl::HandleContinuationError(
             << " job. The time delta(ms) is "
             << length.InMilliseconds();
 
-  // This will reset the had_nudge variable as well.
   wait_interval_.reset(new WaitInterval(WaitInterval::EXPONENTIAL_BACKOFF,
                                         length));
   NotifyRetryTime(base::Time::Now() + length);
@@ -906,7 +852,7 @@ void SyncSchedulerImpl::StopImpl(const base::Closure& callback) {
   wait_interval_.reset();
   NotifyRetryTime(base::Time());
   poll_timer_.Stop();
-  pending_wakeup_event_.Cancel();
+  pending_wakeup_timer_.Stop();
   pending_nudge_job_.reset();
   pending_configure_job_.reset();
   if (started_) {