Remove canary member from SyncSessionJob

This commit removes the 'canary' flag from the SyncSessionJob.  This
makes the code a bit simpler, and makes it slightly more difficult to
create bugs that would schedule too many canary jobs.

BUG=175024


Review URL: https://chromiumcodereview.appspot.com/12317104

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@186263 0039d316-1c4b-4281-b951-d872f2087c98

6 files changed, 44 insertions, 56 deletions

diff --git a/sync/engine/sync_scheduler_impl.cc b/sync/engine/sync_scheduler_impl.cc
index 09f28b2..8d23bc0 100644
--- a/sync/engine/sync_scheduler_impl.cc
+++ b/sync/engine/sync_scheduler_impl.cc
@@ -259,7 +259,7 @@ void SyncSchedulerImpl::Start(Mode mode) {
     scoped_ptr<SyncSessionJob> pending(TakePendingJobForCurrentMode());
     if (pending.get()) {
       SDVLOG(2) << "Executing pending job. Good luck!";
-      DoSyncSessionJob(pending.Pass());
+      DoSyncSessionJob(pending.Pass(), NORMAL_PRIORITY);
     }
   }
 }
@@ -328,7 +328,7 @@ bool SyncSchedulerImpl::ScheduleConfiguration(
         session.Pass(),
         params));
     job->set_destruction_observer(weak_ptr_factory_.GetWeakPtr());
-    bool succeeded = DoSyncSessionJob(job.Pass());
+    bool succeeded = DoSyncSessionJob(job.Pass(), NORMAL_PRIORITY);
 
     // If we failed, the job would have been saved as the pending configure
     // job and a wait interval would have been set.
@@ -345,14 +345,15 @@ bool SyncSchedulerImpl::ScheduleConfiguration(
 }
 
 SyncSchedulerImpl::JobProcessDecision
-SyncSchedulerImpl::DecideWhileInWaitInterval(const SyncSessionJob& job) {
+SyncSchedulerImpl::DecideWhileInWaitInterval(const SyncSessionJob& job,
+                                             JobPriority priority) {
   DCHECK_EQ(MessageLoop::current(), sync_loop_);
   DCHECK(wait_interval_.get());
 
   SDVLOG(2) << "DecideWhileInWaitInterval with WaitInterval mode "
             << WaitInterval::GetModeString(wait_interval_->mode)
             << (wait_interval_->had_nudge ? " (had nudge)" : "")
-            << (job.is_canary() ? " (canary)" : "");
+            << ((priority == CANARY_PRIORITY) ? " (canary)" : "");
 
   if (job.purpose() == SyncSessionJob::POLL)
     return DROP;
@@ -375,16 +376,17 @@ 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 (!job.is_canary())
+    if (priority == NORMAL_PRIORITY)
       return wait_interval_->had_nudge ? DROP : CONTINUE;
     else // We are here because timer ran out. So retry.
       return CONTINUE;
   }
-  return job.is_canary() ? CONTINUE : SAVE;
+  return (priority == CANARY_PRIORITY) ? CONTINUE : SAVE;
 }
 
 SyncSchedulerImpl::JobProcessDecision SyncSchedulerImpl::DecideOnJob(
-    const SyncSessionJob& job) {
+    const SyncSessionJob& job,
+    JobPriority priority) {
   DCHECK_EQ(MessageLoop::current(), sync_loop_);
 
   // See if our type is throttled.
@@ -411,7 +413,7 @@ SyncSchedulerImpl::JobProcessDecision SyncSchedulerImpl::DecideOnJob(
   }
 
   if (wait_interval_.get())
-    return DecideWhileInWaitInterval(job);
+    return DecideWhileInWaitInterval(job, priority);
 
   if (mode_ == CONFIGURATION_MODE) {
     if (job.purpose() == SyncSessionJob::NUDGE)
@@ -473,7 +475,6 @@ SyncSchedulerImpl::JobProcessDecision SyncSchedulerImpl::DecideOnJob(
 }
 
 void SyncSchedulerImpl::HandleSaveJobDecision(scoped_ptr<SyncSessionJob> job) {
-  DCHECK_EQ(DecideOnJob(*job), SAVE);
   const bool is_nudge = job->purpose() == SyncSessionJob::NUDGE;
   if (is_nudge && pending_nudge_) {
     SDVLOG(2) << "Coalescing a pending nudge";
@@ -599,7 +600,7 @@ void SyncSchedulerImpl::ScheduleNudgeImpl(
       CreateSyncSession(info).Pass(),
       ConfigurationParams()));
   job->set_destruction_observer(weak_ptr_factory_.GetWeakPtr());
-  JobProcessDecision decision = DecideOnJob(*job);
+  JobProcessDecision decision = DecideOnJob(*job, NORMAL_PRIORITY);
   SDVLOG(2) << "Should run "
             << SyncSessionJob::GetPurposeString(job->purpose())
             << " job " << job->session()
@@ -714,11 +715,13 @@ void SyncSchedulerImpl::ScheduleSyncSessionJob(
   PostDelayedTask(loc, "DoSyncSessionJob",
       base::Bind(base::IgnoreResult(&SyncSchedulerImpl::DoSyncSessionJob),
                                     weak_ptr_factory_.GetWeakPtr(),
-                                    base::Passed(&job)),
+                                    base::Passed(&job),
+                                    NORMAL_PRIORITY),
       delay);
 }
 
-bool SyncSchedulerImpl::DoSyncSessionJob(scoped_ptr<SyncSessionJob> job) {
+bool SyncSchedulerImpl::DoSyncSessionJob(scoped_ptr<SyncSessionJob> job,
+                                         JobPriority priority) {
   DCHECK_EQ(MessageLoop::current(), sync_loop_);
   if (job->purpose() == SyncSessionJob::NUDGE) {
     if (pending_nudge_ == NULL ||
@@ -732,7 +735,7 @@ bool SyncSchedulerImpl::DoSyncSessionJob(scoped_ptr<SyncSessionJob> job) {
   }
 
   base::AutoReset<bool> protector(&no_scheduling_allowed_, true);
-  JobProcessDecision decision = DecideOnJob(*job);
+  JobProcessDecision decision = DecideOnJob(*job, priority);
   SDVLOG(2) << "Should run "
             << SyncSessionJob::GetPurposeString(job->purpose())
             << " job " << job->session()
@@ -914,11 +917,6 @@ void SyncSchedulerImpl::RestartWaiting(scoped_ptr<SyncSessionJob> job) {
 void SyncSchedulerImpl::HandleContinuationError(
     scoped_ptr<SyncSessionJob> old_job) {
   DCHECK_EQ(MessageLoop::current(), sync_loop_);
-  if (DCHECK_IS_ON()) {
-    if (IsBackingOff()) {
-      DCHECK(wait_interval_->timer.IsRunning() || old_job->is_canary());
-    }
-  }
 
   TimeDelta length = delay_provider_->GetDelay(
       IsBackingOff() ? wait_interval_->length :
@@ -983,12 +981,6 @@ void SyncSchedulerImpl::DoCanaryJob(scoped_ptr<SyncSessionJob> to_be_canary) {
   DCHECK_EQ(MessageLoop::current(), sync_loop_);
   SDVLOG(2) << "Do canary job";
 
-  // Only set canary privileges here, when we are about to run the job. This
-  // avoids confusion in managing canary bits during scheduling, when you
-  // consider that mode switches (e.g., to config) can "pre-empt" a NUDGE that
-  // was scheduled as canary, and send it to an "unscheduled" state.
-  to_be_canary->GrantCanaryPrivilege();
-
   if (to_be_canary->purpose() == SyncSessionJob::NUDGE) {
     // TODO(tim): Bug 158313.  Remove this check.
     if (pending_nudge_ == NULL ||
@@ -1000,7 +992,10 @@ void SyncSchedulerImpl::DoCanaryJob(scoped_ptr<SyncSessionJob> to_be_canary) {
     }
     DCHECK_EQ(pending_nudge_->session(), to_be_canary->session());
   }
-  DoSyncSessionJob(to_be_canary.Pass());
+
+  // This is the only place where we invoke DoSyncSessionJob with canary
+  // privileges.  Everyone else should use NORMAL_PRIORITY.
+  DoSyncSessionJob(to_be_canary.Pass(), CANARY_PRIORITY);
 }
 
 scoped_ptr<SyncSessionJob> SyncSchedulerImpl::TakePendingJobForCurrentMode() {
diff --git a/sync/engine/sync_scheduler_impl.h b/sync/engine/sync_scheduler_impl.h
index 66fd8d5..7eba048 100644
--- a/sync/engine/sync_scheduler_impl.h
+++ b/sync/engine/sync_scheduler_impl.h
@@ -94,6 +94,13 @@ class SYNC_EXPORT_PRIVATE SyncSchedulerImpl :
     DROP,
   };
 
+  enum JobPriority {
+    // Non-canary jobs respect exponential backoff.
+    NORMAL_PRIORITY,
+    // Canary jobs bypass exponential backoff, so use with extreme caution.
+    CANARY_PRIORITY
+  };
+
   friend class SyncSchedulerTest;
   friend class SyncSchedulerWhiteboxTest;
   friend class SyncerTest;
@@ -172,7 +179,8 @@ class SYNC_EXPORT_PRIVATE SyncSchedulerImpl :
                               scoped_ptr<SyncSessionJob> job);
 
   // Invoke the Syncer to perform a sync.
-  bool DoSyncSessionJob(scoped_ptr<SyncSessionJob> job);
+  bool DoSyncSessionJob(scoped_ptr<SyncSessionJob> job,
+                        JobPriority priority);
 
   // Called after the Syncer has performed the sync represented by |job|, to
   // reset our state.  |exited_prematurely| is true if the Syncer did not
@@ -196,7 +204,8 @@ class SYNC_EXPORT_PRIVATE SyncSchedulerImpl :
   void HandleContinuationError(scoped_ptr<SyncSessionJob> old_job);
 
   // Decide whether we should CONTINUE, SAVE or DROP the job.
-  JobProcessDecision DecideOnJob(const SyncSessionJob& job);
+  JobProcessDecision DecideOnJob(const SyncSessionJob& job,
+                                 JobPriority priority);
 
   // If DecideOnJob decides that |job| should be SAVEd, this function will
   // carry out the task of actually "saving" (or coalescing) the job.
@@ -204,7 +213,8 @@ class SYNC_EXPORT_PRIVATE SyncSchedulerImpl :
 
   // Decide on whether to CONTINUE, SAVE or DROP the job when we are in
   // backoff mode.
-  JobProcessDecision DecideWhileInWaitInterval(const SyncSessionJob& job);
+  JobProcessDecision DecideWhileInWaitInterval(const SyncSessionJob& job,
+                                               JobPriority priority);
 
   // 'Impl' here refers to real implementation of public functions, running on
   // |thread_|.
diff --git a/sync/engine/sync_scheduler_whitebox_unittest.cc b/sync/engine/sync_scheduler_whitebox_unittest.cc
index 335caff..01adb85 100644
--- a/sync/engine/sync_scheduler_whitebox_unittest.cc
+++ b/sync/engine/sync_scheduler_whitebox_unittest.cc
@@ -93,8 +93,9 @@ class SyncSchedulerWhiteboxTest : public testing::Test {
   }
 
   SyncSchedulerImpl::JobProcessDecision DecideOnJob(
-      const SyncSessionJob& job) {
-    return scheduler_->DecideOnJob(job);
+      const SyncSessionJob& job,
+      SyncSchedulerImpl::JobPriority priority) {
+    return scheduler_->DecideOnJob(job, priority);
   }
 
   void InitializeSyncerOnNormalMode() {
@@ -107,7 +108,7 @@ class SyncSchedulerWhiteboxTest : public testing::Test {
     scoped_ptr<SyncSession> s(scheduler_->CreateSyncSession(SyncSourceInfo()));
     SyncSessionJob job(purpose, TimeTicks::Now(), s.Pass(),
         ConfigurationParams());
-    return DecideOnJob(job);
+    return DecideOnJob(job, SyncSchedulerImpl::NORMAL_PRIORITY);
   }
 
   SyncSessionContext* context() { return context_.get(); }
@@ -158,7 +159,8 @@ TEST_F(SyncSchedulerWhiteboxTest, SaveNudgeWhileTypeThrottled) {
                      TimeTicks::Now(),
                      s.Pass(),
                      ConfigurationParams());
-  SyncSchedulerImpl::JobProcessDecision decision = DecideOnJob(job);
+  SyncSchedulerImpl::JobProcessDecision decision =
+      DecideOnJob(job, SyncSchedulerImpl::NORMAL_PRIORITY);
   // TODO(tim): This shouldn't drop. Bug 177659.
   EXPECT_EQ(decision, SyncSchedulerImpl::DROP);
 }
@@ -257,8 +259,8 @@ TEST_F(SyncSchedulerWhiteboxTest, ContinueCanaryJobConfig) {
                      TimeTicks::Now(), scoped_ptr<SyncSession>(),
                      ConfigurationParams());
 
-  job.GrantCanaryPrivilege();
-  SyncSchedulerImpl::JobProcessDecision decision = DecideOnJob(job);
+  SyncSchedulerImpl::JobProcessDecision decision =
+      DecideOnJob(job, SyncSchedulerImpl::CANARY_PRIORITY);
 
   EXPECT_EQ(decision, SyncSchedulerImpl::CONTINUE);
 }
diff --git a/sync/engine/sync_session_job.cc b/sync/engine/sync_session_job.cc
index adf898e..f18e010 100644
--- a/sync/engine/sync_session_job.cc
+++ b/sync/engine/sync_session_job.cc
@@ -20,7 +20,6 @@ SyncSessionJob::SyncSessionJob(
     : purpose_(purpose),
       scheduled_start_(start),
       session_(session.Pass()),
-      is_canary_(false),
       config_params_(config_params),
       finished_(NOT_FINISHED) {
 }
@@ -104,10 +103,6 @@ scoped_ptr<sessions::SyncSession> SyncSessionJob::CloneSession() const {
           session_->delegate(), session_->source()));
 }
 
-bool SyncSessionJob::is_canary() const {
-  return is_canary_;
-}
-
 SyncSessionJob::Purpose SyncSessionJob::purpose() const {
   return purpose_;
 }
@@ -132,12 +127,6 @@ ConfigurationParams SyncSessionJob::config_params() const {
   return config_params_;
 }
 
-void SyncSessionJob::GrantCanaryPrivilege() {
-  DCHECK_EQ(finished_, NOT_FINISHED);
-  DVLOG(2) << "Granting canary priviliege to " << session_.get();
-  is_canary_ = true;
-}
-
 SyncerStep SyncSessionJob::start_step() const {
   SyncerStep start, end;
   GetSyncerStepsForPurpose(purpose_, &start, &end);
diff --git a/sync/engine/sync_session_job.h b/sync/engine/sync_session_job.h
index 64ad1c6..b57f227 100644
--- a/sync/engine/sync_session_job.h
+++ b/sync/engine/sync_session_job.h
@@ -45,10 +45,9 @@ class SYNC_EXPORT_PRIVATE SyncSessionJob {
   ~SyncSessionJob();
 
   // Returns a new clone of the job, with a cloned SyncSession ready to be
-  // retried / rescheduled.  The returned job will *never* be a canary,
-  // regardless of |this|. A job can only be cloned once it has finished,
-  // to prevent bugs where multiple jobs are scheduled with the same session.
-  // Use CloneAndAbandon if you want to clone before finishing.
+  // retried / rescheduled.  A job can only be cloned once it has finished, to
+  // prevent bugs where multiple jobs are scheduled with the same session.  Use
+  // CloneAndAbandon if you want to clone before finishing.
   scoped_ptr<SyncSessionJob> Clone() const;
 
   // Same as Clone() above, but also ejects the SyncSession from this job,
@@ -71,15 +70,11 @@ class SYNC_EXPORT_PRIVATE SyncSessionJob {
   // notification) has been properly serviced.
   bool Finish(bool early_exit);
 
-  // Causes is_canary() to return true. Use with caution.
-  void GrantCanaryPrivilege();
-
   static const char* GetPurposeString(Purpose purpose);
   static void GetSyncerStepsForPurpose(Purpose purpose,
                                        SyncerStep* start,
                                        SyncerStep* end);
 
-  bool is_canary() const;
   Purpose purpose() const;
   base::TimeTicks scheduled_start() const;
   void set_scheduled_start(base::TimeTicks start);
@@ -106,7 +101,6 @@ class SYNC_EXPORT_PRIVATE SyncSessionJob {
 
   base::TimeTicks scheduled_start_;
   scoped_ptr<sessions::SyncSession> session_;
-  bool is_canary_;
 
   // Only used for purpose_ == CONFIGURATION.  This, and different Finish() and
   // Succeeded() behavior may be arguments to subclass in the future.
diff --git a/sync/engine/sync_session_job_unittest.cc b/sync/engine/sync_session_job_unittest.cc
index b55cc3d..66026ce 100644
--- a/sync/engine/sync_session_job_unittest.cc
+++ b/sync/engine/sync_session_job_unittest.cc
@@ -97,7 +97,6 @@ class SyncSessionJobTest : public testing::Test {
     EXPECT_EQ(job->scheduled_start(), clone->scheduled_start());
     EXPECT_EQ(job->start_step(), clone->start_step());
     EXPECT_EQ(job->end_step(), clone->end_step());
-    EXPECT_FALSE(clone->is_canary());
   }
 
  private:
@@ -125,7 +124,6 @@ TEST_F(SyncSessionJobTest, Clone) {
   EXPECT_NE(job1.session(), clone1->session());
 
   context()->set_routing_info(routes());
-  clone1->GrantCanaryPrivilege();
   sessions::test_util::SimulateSuccess(clone1->mutable_session(),
                                        clone1->start_step(),
                                        clone1->end_step());