[Sync] Sync types never recovers from throttle

components/sync/engine_impl/sync_scheduler_impl.cc

Index: components/sync/engine_impl/sync_scheduler_impl.cc
diff --git a/components/sync/engine_impl/sync_scheduler_impl.cc b/components/sync/engine_impl/sync_scheduler_impl.cc
index d9d72533754ab6518a7f42bb63b25b7d3a7f23e3..eff39da742ef884a771bda9a4f51c3a8bab0dccd 100644
--- a/components/sync/engine_impl/sync_scheduler_impl.cc
+++ b/components/sync/engine_impl/sync_scheduler_impl.cc
@@ -639,6 +639,12 @@ void SyncSchedulerImpl::RestartWaiting() {
     // Per-datatype throttled or backed off.
     TimeDelta time_until_next_unblock =
         nudge_tracker_.GetTimeUntilNextUnblock();
+    TimeTicks incoming_run_time = TimeTicks::Now() + time_until_next_unblock;

[skym, 2017/04/21 16:06:34]

I think this logic already exists in this file, maybe refactor into a helper
method?

[Gang Wu, 2017/04/25 00:52:05]

Done.

+    if (pending_wakeup_timer_.IsRunning() &&
+        (pending_wakeup_timer_.desired_run_time() < incoming_run_time)) {
+      // Old job arrives sooner than this one.  Don't reschedule it.
+      return;
+    }
     pending_wakeup_timer_.Start(FROM_HERE, time_until_next_unblock,
                                 base::Bind(&SyncSchedulerImpl::OnTypesUnblocked,
                                            weak_ptr_factory_.GetWeakPtr()));
@@ -741,6 +747,10 @@ void SyncSchedulerImpl::Unthrottle() {
   NotifyRetryTime(base::Time());
   NotifyBlockedTypesChanged(nudge_tracker_.GetBlockedTypes());
 
+  // There may have some datatypes are in backoff or throttled before this
+  // global throttling, so rechedule for them.
+  RestartWaiting();

[skym, 2017/04/21 16:06:34]

Sprinkling RestartWaiting()s all over the place, and then when the next
developer comes in here and makes changes and needs to update things... seems
really error prone. I think it'd be better if the pending_wakeup_timer_ was
always scheduled with the same method, that has a priority curried into it, and
only it needs to call RestartWaiting() at the end.

I'd also like to see the whole block that calls RestartWaiting removed from
SyncSchedulerImpl::TrySyncCycleJobImpl(). Yes, changes can jump in between
posting and running TrySyncCycleJobImpl(), but it shouldn't be responsible for
reacting to these kinds of things.

[Gang Wu, 2017/04/25 00:52:05]

Done.

+
   // 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
@@ -767,15 +777,20 @@ void SyncSchedulerImpl::PerformDelayedNudge() {
   if (CanRunNudgeJobNow(NORMAL_PRIORITY))
     TrySyncCycleJob();
 
-  // We're not responsible for setting up any retries here.  The functions that
-  // first put us into a state that prevents successful sync cycles (eg. global
-  // throttling, type throttling, network errors, transient errors) will also
-  // setup the appropriate retry logic (eg. retry after timeout, exponential
-  // backoff, retry when the network changes).
+  // Since PerformDelayedNudge share pending_wakeup_timer_ with
+  // OnTypesUnblocked, Unthrottle and ExponentialBackoffRetry, we should check
+  // if there are any of those functions needed to be scheduled.
+  RestartWaiting();

[skym, 2017/04/21 16:06:34]

Why isn't RestartWaiting() the last thing done in this function?

[Gang Wu, 2017/04/25 00:52:05]

Where to run RestartWaiting() is not a matter, we just schedule timer
somewhere is fine.
Anyway, I remove RestartWaiting() from all place and put at the end of
TrySyncCycleJobImpl. 

 }
 
 void SyncSchedulerImpl::ExponentialBackoffRetry() {
   TryCanaryJob();
+
+  if (!IsBackingOff()) {
+    // There may have some datatypes are in backoff or throttled before this
+    // global backoff, so rechedule for them.
+    RestartWaiting();
+  }
 }
 
 void SyncSchedulerImpl::NotifyRetryTime(base::Time retry_time) {