sync: fix scheduler unthrottling when there is a valid nudge canary

BUG=165561, 154216


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

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

2 files changed, 80 insertions, 8 deletions

diff --git a/sync/engine/sync_scheduler_impl.cc b/sync/engine/sync_scheduler_impl.cc
index c2a5353..897c182 100644
--- a/sync/engine/sync_scheduler_impl.cc
+++ b/sync/engine/sync_scheduler_impl.cc
@@ -1102,16 +1102,24 @@ void SyncSchedulerImpl::PollTimerCallback() {
 void SyncSchedulerImpl::Unthrottle(scoped_ptr<SyncSessionJob> to_be_canary) {
   DCHECK_EQ(MessageLoop::current(), sync_loop_);
   DCHECK_EQ(WaitInterval::THROTTLED, wait_interval_->mode);
+  DCHECK(!to_be_canary.get() || pending_nudge_ == to_be_canary.get() ||
+         wait_interval_->pending_configure_job == to_be_canary.get());
   SDVLOG(2) << "Unthrottled " << (to_be_canary.get() ? "with " : "without ")
-            << "canary.";
-  if (to_be_canary.get())
-    DoCanaryJob(to_be_canary.Pass());
+           << "canary.";
 
-  // TODO(tim): The way DecideOnJob works today, canary privileges aren't
-  // enough to bypass a THROTTLED wait interval, which would suggest we need
-  // to reset before DoCanaryJob (though trusting canary in DecideOnJob is
-  // probably the "right" thing to do). Bug 154216.
+  // We're no longer throttled, so clear the wait interval.
   wait_interval_.reset();
+
+  // We treat this as a 'canary' in the sense that it was originally scheduled
+  // to run some time ago, failed, and we now want to retry, versus a job that
+  // was just created (e.g via ScheduleNudgeImpl). The main implication is
+  // that we're careful to update routing info (etc) with such potentially
+  // stale canary jobs.
+  if (to_be_canary.get()) {
+    DoCanaryJob(to_be_canary.Pass());
+  } else {
+    DCHECK(!unscheduled_nudge_storage_.get());
+  }
 }
 
 void SyncSchedulerImpl::Notify(SyncEngineEvent::EventCause cause) {
diff --git a/sync/engine/sync_scheduler_unittest.cc b/sync/engine/sync_scheduler_unittest.cc
index 601d3b4..b5b676c 100644
--- a/sync/engine/sync_scheduler_unittest.cc
+++ b/sync/engine/sync_scheduler_unittest.cc
@@ -730,7 +730,7 @@ TEST_F(SyncSchedulerTest, ThrottlingDoesThrottle) {
   ASSERT_EQ(0, counter.times_called());
 }
 
-TEST_F(SyncSchedulerTest, ThrottlingExpires) {
+TEST_F(SyncSchedulerTest, ThrottlingExpiresFromPoll) {
   SyncShareRecords records;
   TimeDelta poll(TimeDelta::FromMilliseconds(15));
   TimeDelta throttle1(TimeDelta::FromMilliseconds(150));
@@ -756,6 +756,70 @@ TEST_F(SyncSchedulerTest, ThrottlingExpires) {
   AnalyzePollRun(records, kMinNumSamples, optimal_start, poll);
 }
 
+TEST_F(SyncSchedulerTest, ThrottlingExpiresFromNudge) {
+  SyncShareRecords records;
+  TimeDelta poll(TimeDelta::FromDays(1));
+  TimeDelta throttle1(TimeDelta::FromMilliseconds(150));
+  scheduler()->OnReceivedLongPollIntervalUpdate(poll);
+
+  ::testing::InSequence seq;
+  EXPECT_CALL(*syncer(), SyncShare(_,_,_))
+      .WillOnce(DoAll(
+          WithArg<0>(sessions::test_util::SimulateThrottled(throttle1)),
+          Return(true)))
+      .RetiresOnSaturation();
+  EXPECT_CALL(*syncer(), SyncShare(_,_,_))
+      .WillOnce(DoAll(Invoke(sessions::test_util::SimulateSuccess),
+                      QuitLoopNowAction()));
+
+  const ModelTypeSet types(BOOKMARKS);
+  StartSyncScheduler(SyncScheduler::NORMAL_MODE);
+  scheduler()->ScheduleNudgeAsync(
+      zero(), NUDGE_SOURCE_LOCAL, types, FROM_HERE);
+
+  PumpLoop();
+  EXPECT_TRUE(scheduler()->IsSyncingCurrentlySilenced());
+  RunLoop();
+  EXPECT_FALSE(scheduler()->IsSyncingCurrentlySilenced());
+
+  StopSyncScheduler();
+}
+
+TEST_F(SyncSchedulerTest, ThrottlingExpiresFromConfigure) {
+  SyncShareRecords records;
+  TimeDelta poll(TimeDelta::FromDays(1));
+  TimeDelta throttle1(TimeDelta::FromMilliseconds(150));
+  scheduler()->OnReceivedLongPollIntervalUpdate(poll);
+
+  ::testing::InSequence seq;
+  EXPECT_CALL(*syncer(), SyncShare(_,_,_))
+      .WillOnce(DoAll(
+          WithArg<0>(sessions::test_util::SimulateThrottled(throttle1)),
+          Return(true)))
+      .RetiresOnSaturation();
+  EXPECT_CALL(*syncer(), SyncShare(_,_,_))
+      .WillOnce(DoAll(Invoke(sessions::test_util::SimulateSuccess),
+                      QuitLoopNowAction()));
+
+  const ModelTypeSet types(BOOKMARKS);
+  StartSyncScheduler(SyncScheduler::CONFIGURATION_MODE);
+
+  CallbackCounter counter;
+  ConfigurationParams params(
+      GetUpdatesCallerInfo::RECONFIGURATION,
+      types,
+      TypesToRoutingInfo(types),
+      base::Bind(&CallbackCounter::Callback, base::Unretained(&counter)));
+  EXPECT_FALSE(scheduler()->ScheduleConfiguration(params));
+  EXPECT_EQ(0, counter.times_called());
+  EXPECT_TRUE(scheduler()->IsSyncingCurrentlySilenced());
+
+  RunLoop();
+  EXPECT_FALSE(scheduler()->IsSyncingCurrentlySilenced());
+
+  StopSyncScheduler();
+}
+
 // Test nudges / polls don't run in config mode and config tasks do.
 TEST_F(SyncSchedulerTest, ConfigurationMode) {
   TimeDelta poll(TimeDelta::FromMilliseconds(15));