Sync periodic polls always fail because of expired access tokens.

The issue is that poll job runs after few hours of inactivity and 
therefore will always fail with auth error because of expired access 
token. Once fresh access token is requested poll job is not retried.
The change is to remember that poll timer just fired and retry poll job
after credentials are updated.

BUG=251307

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

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

5 files changed, 64 insertions, 1 deletions

diff --git a/sync/engine/sync_scheduler_impl.cc b/sync/engine/sync_scheduler_impl.cc
index f837853..bdeb722 100644
--- a/sync/engine/sync_scheduler_impl.cc
+++ b/sync/engine/sync_scheduler_impl.cc
@@ -168,7 +168,8 @@ SyncSchedulerImpl::SyncSchedulerImpl(const std::string& name,
       delay_provider_(delay_provider),
       syncer_(syncer),
       session_context_(context),
-      no_scheduling_allowed_(false) {
+      no_scheduling_allowed_(false),
+      do_poll_after_credentials_updated_(false) {
 }
 
 SyncSchedulerImpl::~SyncSchedulerImpl() {
@@ -478,6 +479,8 @@ void SyncSchedulerImpl::DoNudgeSyncSessionJob(JobPriority priority) {
       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;
 
   bool success = !premature_exit
       && !sessions::HasSyncerError(
@@ -519,6 +522,8 @@ bool SyncSchedulerImpl::DoConfigurationSyncSessionJob(JobPriority priority) {
       GetRoutingInfoTypes(session_context_->routing_info()),
       session.get());
   AdjustPolling(FORCE_RESET);
+  // Don't run poll job till the next time poll timer fires.
+  do_poll_after_credentials_updated_ = false;
 
   bool success = !premature_exit
       && !sessions::HasSyncerError(
@@ -688,9 +693,16 @@ void SyncSchedulerImpl::TryCanaryJob() {
              CanRunNudgeJobNow(CANARY_PRIORITY)) {
     SDVLOG(2) << "Found pending nudge job; will run as canary";
     DoNudgeSyncSessionJob(CANARY_PRIORITY);
+  } else if (mode_ == NORMAL_MODE && CanRunJobNow(CANARY_PRIORITY) &&
+             do_poll_after_credentials_updated_) {
+    // Retry poll if poll timer recently fired and ProfileSyncService received
+    // fresh access token.
+    DoPollSyncSessionJob();
   } else {
     SDVLOG(2) << "Found no work to do; will not run a canary";
   }
+  // Don't run poll job till the next time poll timer fires.
+  do_poll_after_credentials_updated_ = false;
 }
 
 void SyncSchedulerImpl::PollTimerCallback() {
@@ -707,6 +719,14 @@ void SyncSchedulerImpl::PollTimerCallback() {
   }
 
   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 in
+  // this case.
+  if (HttpResponse::SYNC_AUTH_ERROR ==
+      session_context_->connection_manager()->server_status()) {
+    do_poll_after_credentials_updated_ = true;
+  }
 }
 
 void SyncSchedulerImpl::Unthrottle() {
diff --git a/sync/engine/sync_scheduler_impl.h b/sync/engine/sync_scheduler_impl.h
index 5d350a4..59468e7 100644
--- a/sync/engine/sync_scheduler_impl.h
+++ b/sync/engine/sync_scheduler_impl.h
@@ -311,6 +311,15 @@ class SYNC_EXPORT_PRIVATE SyncSchedulerImpl
   // could result in tight sync loops hitting sync servers.
   bool no_scheduling_allowed_;
 
+  // crbug/251307. This is a workaround for M29. crbug/259913 tracks proper fix
+  // for M30.
+  // The issue is that poll job runs after few hours of inactivity and therefore
+  // will always fail with auth error because of expired access token. Once
+  // fresh access token is requested poll job is not retried.
+  // The change is to remember that poll timer just fired and retry poll job
+  // after credentials are updated.
+  bool do_poll_after_credentials_updated_;
+
   DISALLOW_COPY_AND_ASSIGN(SyncSchedulerImpl);
 };
 
diff --git a/sync/engine/sync_scheduler_unittest.cc b/sync/engine/sync_scheduler_unittest.cc
index 4f2c56e..a09bcb3 100644
--- a/sync/engine/sync_scheduler_unittest.cc
+++ b/sync/engine/sync_scheduler_unittest.cc
@@ -1283,4 +1283,31 @@ TEST_F(SyncSchedulerTest, DoubleCanaryInConfigure) {
   PumpLoop();  // Run the nudge, that will fail and schedule a quick retry.
 }
 
+TEST_F(SyncSchedulerTest, PollFromCanaryAfterAuthError) {
+  SyncShareRecords records;
+  TimeDelta poll(TimeDelta::FromMilliseconds(15));
+  scheduler()->OnReceivedLongPollIntervalUpdate(poll);
+
+  ::testing::InSequence seq;
+  EXPECT_CALL(*syncer(), PollSyncShare(_,_))
+      .WillRepeatedly(DoAll(Invoke(sessions::test_util::SimulatePollSuccess),
+           WithArg<1>(RecordSyncShareMultiple(&records, kMinNumSamples))));
+
+  connection()->SetServerStatus(HttpResponse::SYNC_AUTH_ERROR);
+  StartSyncScheduler(SyncScheduler::NORMAL_MODE);
+
+  // Run to wait for polling.
+  RunLoop();
+
+  // Normally OnCredentialsUpdated calls TryCanaryJob that doesn't run Poll,
+  // but after poll finished with auth error from poll timer it should retry
+  // poll once more
+  EXPECT_CALL(*syncer(), PollSyncShare(_,_))
+      .WillOnce(DoAll(Invoke(sessions::test_util::SimulatePollSuccess),
+                      WithArg<1>(RecordSyncShare(&records))));
+  scheduler()->OnCredentialsUpdated();
+  connection()->SetServerStatus(HttpResponse::SERVER_CONNECTION_OK);
+  StopSyncScheduler();
+}
+
 }  // namespace syncer
diff --git a/sync/test/engine/mock_connection_manager.cc b/sync/test/engine/mock_connection_manager.cc
index 073a3be..65998e955 100644
--- a/sync/test/engine/mock_connection_manager.cc
+++ b/sync/test/engine/mock_connection_manager.cc
@@ -733,4 +733,9 @@ void MockConnectionManager::UpdateConnectionStatus() {
   }
 }
 
+void MockConnectionManager::SetServerStatus(
+    HttpResponse::ServerConnectionCode server_status) {
+  server_status_ = server_status;
+}
+
 }  // namespace syncer
diff --git a/sync/test/engine/mock_connection_manager.h b/sync/test/engine/mock_connection_manager.h
index f4f9882..9f7cdb1 100644
--- a/sync/test/engine/mock_connection_manager.h
+++ b/sync/test/engine/mock_connection_manager.h
@@ -248,6 +248,8 @@ class MockConnectionManager : public ServerConnectionManager {
   // requests.
   void UpdateConnectionStatus();
 
+  void SetServerStatus(HttpResponse::ServerConnectionCode server_status);
+
   // Return by copy to be thread-safe.
   const std::string store_birthday() {
     base::AutoLock lock(store_birthday_lock_);