sync: Fix handling of data type throttling

This change attempts to fix two semi-related issues in data type
throttling that were introduced in r194766.  Data type throttling has
always had a few rough edge cases, but that commit introduced some
issues that must be fixed.

1:
Prior to r194766, data type throttling would block sync cycles only if
the source was LOCAL and all types were throttled. This meant that a
refresh request or notification could override data type throttling, if
it arrived at the right time.  

That change made this behaviour much more deterministic.  It would fail
to perform any sync cycles as long as the only local nudges that were
currently tracked were for throttled types.  Even notificiations and
local refresh requests for these types will not be sufficient to force a
sync cycle.

This change tries to find a better balance.  It will allow sync cycles
in the presence of unthrottled data types if one of the following is
true:
- There is an unserviced invalidation for any type.
- There is an unservice refresh request for any type.
- There is a local nudge for any non-throttled type.

2:
The other problem has to do with exponential backoff in the presence of
per-type throttling.  In r194766, we made the assumption that we can
early-exit from attempted sync cycle only if there was some other reason
to perform a sync cycle later.  Common causes of early-exit include
throttling, exponential backoff, and network connectivity issues, all of
which should be able to get back into a normal state on their own.

However, one of those early-exit causes is data type throttling.  In
that case, there is no timer or notification that will restart sync once
the issue is resolved.  This is a problem because DoNudgeSyncSessionJob
assumes that it can return without scheduling a retry in the "if
(!CanRunNudgeJobNow())" case, while the code that manages the
exponential backoff assumes the function that invokes
DoNudgeSyncSessionJob, TryCanaryJob(), will always either clear or
restart the wait interval.

To fix this issue, this change adds a small function to invoke the
exponential backoff retry callback, and gives it responsibility for
making sure the WaitInterval is always managed correctly.

BUG=239693

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

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

4 files changed, 36 insertions, 3 deletions

diff --git a/sync/sessions/nudge_tracker.cc b/sync/sessions/nudge_tracker.cc
index 7eae5ec..b481a39 100644
--- a/sync/sessions/nudge_tracker.cc
+++ b/sync/sessions/nudge_tracker.cc
@@ -21,12 +21,20 @@ NudgeTracker::NudgeTracker()
 
 NudgeTracker::~NudgeTracker() { }
 
+bool NudgeTracker::IsGetUpdatesRequired() {
+  if (!refresh_requested_counts_.empty()) {
+    return true;
+  } else if (!payload_list_map_.empty()) {
+    return true;
+  } else {
+    return false;
+  }
+}
+
 bool NudgeTracker::IsSyncRequired() {
   if (!local_nudge_counts_.empty()) {
     return true;
-  } else if (!refresh_requested_counts_.empty()) {
-    return true;
-  } else if (!payload_list_map_.empty()) {
+  } else if (IsGetUpdatesRequired()) {
     return true;
   } else {
     return false;
diff --git a/sync/sessions/nudge_tracker.h b/sync/sessions/nudge_tracker.h
index c093250..0e557f3 100644
--- a/sync/sessions/nudge_tracker.h
+++ b/sync/sessions/nudge_tracker.h
@@ -28,6 +28,11 @@ class SYNC_EXPORT_PRIVATE NudgeTracker {
   NudgeTracker();
   ~NudgeTracker();
 
+  // Returns true if one of the main reasons for performing the sync cycle is to
+  // fetch updates.  This is true when we have pending invalidations or refresh
+  // requests.
+  bool IsGetUpdatesRequired();
+
   // Returns true if there is a good reason for performing a sync cycle.
   // This does not take into account whether or not this is a good *time* to
   // perform a sync cycle; that's the scheduler's job.
diff --git a/sync/sessions/test_util.cc b/sync/sessions/test_util.cc
index 3b4b6c9..03f6de2 100644
--- a/sync/sessions/test_util.cc
+++ b/sync/sessions/test_util.cc
@@ -4,6 +4,8 @@
 
 #include "sync/sessions/test_util.h"
 
+#include "sync/engine/throttled_data_type_tracker.h"
+
 namespace syncer {
 namespace sessions {
 namespace test_util {
@@ -64,6 +66,16 @@ void SimulateThrottledImpl(sessions::SyncSession* session,
   session->delegate()->OnSilencedUntil(base::TimeTicks::Now() + delta);
 }
 
+void SimulateTypesThrottledImpl(
+    sessions::SyncSession* session,
+    ModelTypeSet types,
+    const base::TimeDelta& delta) {
+  session->mutable_status_controller()->set_last_download_updates_result(
+      SERVER_RETURN_THROTTLED);
+  session->context()->throttled_data_type_tracker()->
+      SetUnthrottleTime(types, base::TimeTicks::Now() + delta);
+}
+
 void SimulatePollIntervalUpdateImpl(sessions::SyncSession* session,
     const base::TimeDelta& new_poll) {
   SimulateSuccess(session, SYNCER_BEGIN, SYNCER_END);
diff --git a/sync/sessions/test_util.h b/sync/sessions/test_util.h
index 38638e1..6ab32b1 100644
--- a/sync/sessions/test_util.h
+++ b/sync/sessions/test_util.h
@@ -27,6 +27,10 @@ void SimulateSuccess(sessions::SyncSession* session,
                      SyncerStep begin, SyncerStep end);
 void SimulateThrottledImpl(sessions::SyncSession* session,
     const base::TimeDelta& delta);
+void SimulateTypesThrottledImpl(
+    sessions::SyncSession* session,
+    ModelTypeSet types,
+    const base::TimeDelta& delta);
 void SimulatePollIntervalUpdateImpl(sessions::SyncSession* session,
     const base::TimeDelta& new_poll);
 void SimulateSessionsCommitDelayUpdateImpl(sessions::SyncSession* session,
@@ -36,6 +40,10 @@ ACTION_P(SimulateThrottled, throttle) {
   SimulateThrottledImpl(arg0, throttle);
 }
 
+ACTION_P2(SimulateTypesThrottled, types, throttle) {
+  SimulateTypesThrottledImpl(arg0, types, throttle);
+}
+
 ACTION_P(SimulatePollIntervalUpdate, poll) {
   SimulatePollIntervalUpdateImpl(arg0, poll);
 }