Make TrySyncSessionJob post task on sync thread

Instead of calling sync cycle directly TrySyncSessionJob posts task on
sync thread.

Rescheduling exponential backoff timer had to be moved from TryCanaryJob
into TrySyncSessionJobImpl because it analyzes result of sync cycle.
The same with setting do_poll_after_credentials_updated_ after auth_error.

Tests needed additional PumpLoop because of additional Post call.

BUG=259913

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

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

3 files changed, 62 insertions, 36 deletions

diff --git a/sync/engine/sync_scheduler_impl.cc b/sync/engine/sync_scheduler_impl.cc
index f3b96e4..0aaaba0 100644
--- a/sync/engine/sync_scheduler_impl.cc
+++ b/sync/engine/sync_scheduler_impl.cc
@@ -207,7 +207,7 @@ void SyncSchedulerImpl::OnServerConnectionErrorFixed() {
   // 4. A nudge was scheduled + saved while in configuration mode.
   //
   // In all cases except (2), we want to retry contacting the server. We
-  // call DoCanaryJob to achieve this, and note that nothing -- not even a
+  // call TryCanaryJob to achieve this, and note that nothing -- not even a
   // canary job -- can bypass a THROTTLED WaitInterval. The only thing that
   // has the authority to do that is the Unthrottle timer.
   TryCanaryJob();
@@ -417,12 +417,6 @@ void SyncSchedulerImpl::ScheduleNudgeImpl(
   if (!CanRunNudgeJobNow(NORMAL_PRIORITY))
     return;
 
-  if (!started_) {
-    SDVLOG_LOC(nudge_location, 2)
-        << "Schedule not started; not running a nudge.";
-    return;
-  }
-
   TimeTicks incoming_run_time = TimeTicks::Now() + delay;
   if (!scheduled_nudge_time_.is_null() &&
     (scheduled_nudge_time_ < incoming_run_time)) {
@@ -667,11 +661,18 @@ void SyncSchedulerImpl::Stop() {
 // privileges.  Everyone else should use NORMAL_PRIORITY.
 void SyncSchedulerImpl::TryCanaryJob() {
   TrySyncSessionJob(CANARY_PRIORITY);
-  // Don't run poll job till the next time poll timer fires.
-  do_poll_after_credentials_updated_ = false;
 }
 
 void SyncSchedulerImpl::TrySyncSessionJob(JobPriority priority) {
+  // Post call to TrySyncSessionJobImpl on current thread. Later request for
+  // access token will be here.
+  base::MessageLoop::current()->PostTask(FROM_HERE, base::Bind(
+      &SyncSchedulerImpl::TrySyncSessionJobImpl,
+      weak_ptr_factory_.GetWeakPtr(),
+      priority));
+}
+
+void SyncSchedulerImpl::TrySyncSessionJobImpl(JobPriority priority) {
   DCHECK(CalledOnValidThread());
   if (mode_ == CONFIGURATION_MODE) {
     if (pending_configure_params_) {
@@ -686,8 +687,34 @@ void SyncSchedulerImpl::TrySyncSessionJob(JobPriority priority) {
     } else if (do_poll_after_credentials_updated_ ||
         ((base::TimeTicks::Now() - last_poll_reset_) >= GetPollInterval())) {
       DoPollSyncSessionJob();
+      // Poll timer fires infrequently. Usually by this time access token is
+      // already expired and poll job will fail with auth error. Set flag to
+      // retry poll once ProfileSyncService gets new access token, TryCanaryJob
+      // will be called after access token is retrieved.
+      if (HttpResponse::SYNC_AUTH_ERROR ==
+          session_context_->connection_manager()->server_status()) {
+        do_poll_after_credentials_updated_ = true;
+      }
     }
   }
+
+  if (priority == CANARY_PRIORITY) {
+    // If this is canary job then whatever result was don't run poll job till
+    // the next time poll timer fires.
+    do_poll_after_credentials_updated_ = false;
+  }
+
+  if (IsBackingOff() && !pending_wakeup_timer_.IsRunning()) {
+    // If we succeeded, our wait interval would have been cleared.  If it hasn't
+    // been cleared, then we should increase our backoff interval and schedule
+    // another retry.
+    TimeDelta length = delay_provider_->GetDelay(wait_interval_->length);
+    wait_interval_.reset(
+      new WaitInterval(WaitInterval::EXPONENTIAL_BACKOFF, length));
+    SDVLOG(2) << "Sync cycle failed.  Will back off for "
+        << wait_interval_->length.InMilliseconds() << "ms.";
+    RestartWaiting();
+  }
 }
 
 void SyncSchedulerImpl::PollTimerCallback() {
@@ -704,14 +731,6 @@ void SyncSchedulerImpl::PollTimerCallback() {
   }
 
   TrySyncSessionJob(NORMAL_PRIORITY);
-  // Poll timer fires infrequently. Usually by this time access token is already
-  // expired and poll job will fail with auth error. Set flag to retry poll once
-  // ProfileSyncService gets new access token, TryCanaryJob will be called in
-  // this case.
-  if (HttpResponse::SYNC_AUTH_ERROR ==
-      session_context_->connection_manager()->server_status()) {
-    do_poll_after_credentials_updated_ = true;
-  }
 }
 
 void SyncSchedulerImpl::Unthrottle() {
@@ -766,18 +785,6 @@ void SyncSchedulerImpl::PerformDelayedNudge() {
 
 void SyncSchedulerImpl::ExponentialBackoffRetry() {
   TryCanaryJob();
-
-  if (IsBackingOff()) {
-    // If we succeeded, our wait interval would have been cleared.  If it hasn't
-    // been cleared, then we should increase our backoff interval and schedule
-    // another retry.
-    TimeDelta length = delay_provider_->GetDelay(wait_interval_->length);
-    wait_interval_.reset(
-      new WaitInterval(WaitInterval::EXPONENTIAL_BACKOFF, length));
-    SDVLOG(2) << "Sync cycle failed.  Will back off for "
-        << wait_interval_->length.InMilliseconds() << "ms.";
-    RestartWaiting();
-  }
 }
 
 void SyncSchedulerImpl::Notify(SyncEngineEvent::EventCause cause) {
diff --git a/sync/engine/sync_scheduler_impl.h b/sync/engine/sync_scheduler_impl.h
index 4ffbf6f..4c0dd57 100644
--- a/sync/engine/sync_scheduler_impl.h
+++ b/sync/engine/sync_scheduler_impl.h
@@ -207,7 +207,10 @@ class SYNC_EXPORT_PRIVATE SyncSchedulerImpl
   // priority.
   void TryCanaryJob();
 
+  // At the moment TrySyncSessionJob just posts call to TrySyncSessionJobImpl on
+  // current thread. In the future it will request access token here.
   void TrySyncSessionJob(JobPriority priority);
+  void TrySyncSessionJobImpl(JobPriority priority);
 
   // Transitions out of the THROTTLED WaitInterval then calls TryCanaryJob().
   void Unthrottle();
diff --git a/sync/engine/sync_scheduler_unittest.cc b/sync/engine/sync_scheduler_unittest.cc
index 1a60f86..e587655 100644
--- a/sync/engine/sync_scheduler_unittest.cc
+++ b/sync/engine/sync_scheduler_unittest.cc
@@ -320,6 +320,7 @@ TEST_F(SyncSchedulerTest, Config) {
       base::Bind(&CallbackCounter::Callback, base::Unretained(&ready_counter)),
       base::Bind(&CallbackCounter::Callback, base::Unretained(&retry_counter)));
   scheduler()->ScheduleConfiguration(params);
+  PumpLoop();
   ASSERT_EQ(1, ready_counter.times_called());
   ASSERT_EQ(0, retry_counter.times_called());
 }
@@ -349,6 +350,7 @@ TEST_F(SyncSchedulerTest, ConfigWithBackingOff) {
       base::Bind(&CallbackCounter::Callback, base::Unretained(&ready_counter)),
       base::Bind(&CallbackCounter::Callback, base::Unretained(&retry_counter)));
   scheduler()->ScheduleConfiguration(params);
+  RunLoop();
   ASSERT_EQ(0, ready_counter.times_called());
   ASSERT_EQ(1, retry_counter.times_called());
 
@@ -395,6 +397,7 @@ TEST_F(SyncSchedulerTest, ConfigWithStop) {
       base::Bind(&CallbackCounter::Callback, base::Unretained(&ready_counter)),
       base::Bind(&CallbackCounter::Callback, base::Unretained(&retry_counter)));
   scheduler()->ScheduleConfiguration(params);
+  PumpLoop();
   ASSERT_EQ(0, ready_counter.times_called());
   ASSERT_EQ(0, retry_counter.times_called());
 }
@@ -423,6 +426,7 @@ TEST_F(SyncSchedulerTest, NudgeWithConfigWithBackingOff) {
       base::Bind(&CallbackCounter::Callback, base::Unretained(&ready_counter)),
       base::Bind(&CallbackCounter::Callback, base::Unretained(&retry_counter)));
   scheduler()->ScheduleConfiguration(params);
+  RunLoop();
   ASSERT_EQ(0, ready_counter.times_called());
   ASSERT_EQ(1, retry_counter.times_called());
   Mock::VerifyAndClearExpectations(syncer());
@@ -450,6 +454,7 @@ TEST_F(SyncSchedulerTest, NudgeWithConfigWithBackingOff) {
       .WillOnce(DoAll(Invoke(sessions::test_util::SimulateNormalSuccess),
                       RecordSyncShare(&times)));
   StartSyncScheduler(SyncScheduler::NORMAL_MODE);
+  PumpLoop();
 }
 
 // Test that nudges are coalesced.
@@ -660,6 +665,7 @@ TEST_F(SyncSchedulerTest, ThrottlingDoesThrottle) {
       base::Bind(&CallbackCounter::Callback, base::Unretained(&ready_counter)),
       base::Bind(&CallbackCounter::Callback, base::Unretained(&retry_counter)));
   scheduler()->ScheduleConfiguration(params);
+  PumpLoop();
   ASSERT_EQ(0, ready_counter.times_called());
   ASSERT_EQ(1, retry_counter.times_called());
 
@@ -711,7 +717,8 @@ TEST_F(SyncSchedulerTest, ThrottlingExpiresFromNudge) {
   StartSyncScheduler(SyncScheduler::NORMAL_MODE);
   scheduler()->ScheduleLocalNudge(zero(), types, FROM_HERE);
 
-  PumpLoop();
+  PumpLoop(); // To get PerformDelayedNudge called.
+  PumpLoop(); // To get TrySyncSessionJob called
   EXPECT_TRUE(scheduler()->IsCurrentlyThrottled());
   RunLoop();
   EXPECT_FALSE(scheduler()->IsCurrentlyThrottled());
@@ -747,6 +754,7 @@ TEST_F(SyncSchedulerTest, ThrottlingExpiresFromConfigure) {
       base::Bind(&CallbackCounter::Callback, base::Unretained(&ready_counter)),
       base::Bind(&CallbackCounter::Callback, base::Unretained(&retry_counter)));
   scheduler()->ScheduleConfiguration(params);
+  PumpLoop();
   EXPECT_EQ(0, ready_counter.times_called());
   EXPECT_EQ(1, retry_counter.times_called());
   EXPECT_TRUE(scheduler()->IsCurrentlyThrottled());
@@ -778,7 +786,8 @@ TEST_F(SyncSchedulerTest, TypeThrottlingBlocksNudge) {
 
   StartSyncScheduler(SyncScheduler::NORMAL_MODE);
   scheduler()->ScheduleLocalNudge(zero(), types, FROM_HERE);
-  PumpLoop();
+  PumpLoop(); // To get PerformDelayedNudge called.
+  PumpLoop(); // To get TrySyncSessionJob called
   EXPECT_TRUE(GetThrottledTypes().HasAll(types));
 
   // This won't cause a sync cycle because the types are throttled.
@@ -812,7 +821,8 @@ TEST_F(SyncSchedulerTest, TypeThrottlingDoesBlockOtherSources) {
 
   StartSyncScheduler(SyncScheduler::NORMAL_MODE);
   scheduler()->ScheduleLocalNudge(zero(), throttled_types, FROM_HERE);
-  PumpLoop();
+  PumpLoop(); // To get PerformDelayedNudge called.
+  PumpLoop(); // To get TrySyncSessionJob called
   EXPECT_TRUE(GetThrottledTypes().HasAll(throttled_types));
 
   // Ignore invalidations for throttled types.
@@ -865,6 +875,7 @@ TEST_F(SyncSchedulerTest, ConfigurationMode) {
       base::Bind(&CallbackCounter::Callback, base::Unretained(&ready_counter)),
       base::Bind(&CallbackCounter::Callback, base::Unretained(&retry_counter)));
   scheduler()->ScheduleConfiguration(params);
+  RunLoop();
   ASSERT_EQ(1, ready_counter.times_called());
   ASSERT_EQ(0, retry_counter.times_called());
 
@@ -882,7 +893,8 @@ TEST_F(SyncSchedulerTest, ConfigurationMode) {
   context()->set_routing_info(routing_info());
   StartSyncScheduler(SyncScheduler::NORMAL_MODE);
 
-  PumpLoop();
+  RunLoop();
+  Mock::VerifyAndClearExpectations(syncer());
 }
 
 class BackoffTriggersSyncSchedulerTest : public SyncSchedulerTest {
@@ -1021,6 +1033,7 @@ TEST_F(SyncSchedulerTest, BackoffDropsJobs) {
       base::Bind(&CallbackCounter::Callback, base::Unretained(&ready_counter)),
       base::Bind(&CallbackCounter::Callback, base::Unretained(&retry_counter)));
   scheduler()->ScheduleConfiguration(params);
+  PumpLoop();
   ASSERT_EQ(0, ready_counter.times_called());
   ASSERT_EQ(1, retry_counter.times_called());
 
@@ -1181,8 +1194,8 @@ TEST_F(SyncSchedulerTest, ServerConnectionChangeDuringBackoff) {
                     Return(true)));
 
   scheduler()->ScheduleLocalNudge(zero(), ModelTypeSet(BOOKMARKS), FROM_HERE);
-
-  PumpLoop();  // Run the nudge, that will fail and schedule a quick retry.
+  PumpLoop(); // To get PerformDelayedNudge called.
+  PumpLoop(); // Run the nudge, that will fail and schedule a quick retry.
   ASSERT_TRUE(scheduler()->IsBackingOff());
 
   // Before we run the scheduled canary, trigger a server connection change.
@@ -1214,11 +1227,13 @@ TEST_F(SyncSchedulerTest, ConnectionChangeCanaryPreemptedByNudge) {
 
   scheduler()->ScheduleLocalNudge(zero(), ModelTypeSet(BOOKMARKS), FROM_HERE);
 
-  PumpLoop();  // Run the nudge, that will fail and schedule a quick retry.
+  PumpLoop(); // To get PerformDelayedNudge called.
+  PumpLoop(); // Run the nudge, that will fail and schedule a quick retry.
   ASSERT_TRUE(scheduler()->IsBackingOff());
 
   // Before we run the scheduled canary, trigger a server connection change.
   scheduler()->OnConnectionStatusChange();
+  PumpLoop();
   connection()->SetServerReachable();
   connection()->UpdateConnectionStatus();
   scheduler()->ScheduleLocalNudge(zero(), ModelTypeSet(BOOKMARKS), FROM_HERE);
@@ -1277,6 +1292,7 @@ TEST_F(SyncSchedulerTest, PollFromCanaryAfterAuthError) {
                       RecordSyncShare(&times)));
   scheduler()->OnCredentialsUpdated();
   connection()->SetServerStatus(HttpResponse::SERVER_CONNECTION_OK);
+  RunLoop();
   StopSyncScheduler();
 }