[Sync] Sync should run canary cycle after any network related error

Today sync only runs canary cycle after small subset of network errors
that are related to connection/network failure. This leaves sync in
exponential backoff when unexpected error is returned from network
stack.

In this change I make sync translate all network errors to
CONNECTION_UNAVAILABLE therefore making it try canary job after network
change.

BUG=539943
R=zea@chromium.org

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

Cr-Commit-Position: refs/heads/master@{#367746}

4 files changed, 7 insertions, 41 deletions

diff --git a/sync/engine/backoff_delay_provider.cc b/sync/engine/backoff_delay_provider.cc
index cb8b0f1..864401c 100644
--- a/sync/engine/backoff_delay_provider.cc
+++ b/sync/engine/backoff_delay_provider.cc
@@ -65,27 +65,13 @@ TimeDelta BackoffDelayProvider::GetDelay(const base::TimeDelta& last_delay) {
 
 TimeDelta BackoffDelayProvider::GetInitialDelay(
     const sessions::ModelNeutralState& state) const {
-  // NETWORK_CONNECTION_UNAVAILABLE implies we did not even manage to hit the
-  // wire; the failure occurred locally. Note that if commit_result is *not*
-  // UNSET, this implies download_updates_result succeeded.  Also note that
-  // last_get_key_result is coupled to last_download_updates_result in that
-  // they are part of the same GetUpdates request, so we only check if
-  // the download request is CONNECTION_UNAVAILABLE.
-  //
-  // TODO(tim): Should we treat NETWORK_IO_ERROR similarly? It's different
-  // from CONNECTION_UNAVAILABLE in that a request may well have succeeded
-  // in contacting the server (e.g we got a 200 back), but we failed
-  // trying to parse the response (actual content length != HTTP response
-  // header content length value).  For now since we're considering
-  // merging this code to branches and I haven't audited all the
-  // NETWORK_IO_ERROR cases carefully, I'm going to target the fix
-  // very tightly (see bug chromium-os:35073). DIRECTORY_LOOKUP_FAILED is
-  // another example of something that shouldn't backoff, though the
-  // scheduler should probably be handling these cases differently. See
-  // the TODO(rlarocque) in ScheduleNextSync.
+  // NETWORK_CONNECTION_UNAVAILABLE implies we did not receive HTTP response
+  // from server because of some network error. If network is unavailable then
+  // on next connection type or address change scheduler will run canary job.
+  // Otherwise we'll retry in 30 seconds.
   if (state.commit_result == NETWORK_CONNECTION_UNAVAILABLE ||
       state.last_download_updates_result == NETWORK_CONNECTION_UNAVAILABLE) {
-    return short_initial_backoff_;
+    return default_initial_backoff_;
   }
 
   if (SyncerErrorIsError(state.last_get_key_result))
diff --git a/sync/engine/backoff_delay_provider_unittest.cc b/sync/engine/backoff_delay_provider_unittest.cc
index 2e58186..f1a78d8 100644
--- a/sync/engine/backoff_delay_provider_unittest.cc
+++ b/sync/engine/backoff_delay_provider_unittest.cc
@@ -46,7 +46,7 @@ TEST_F(BackoffDelayProviderTest, GetInitialDelay) {
             delay->GetInitialDelay(state).InSeconds());
 
   state.last_download_updates_result = NETWORK_CONNECTION_UNAVAILABLE;
-  EXPECT_EQ(kInitialBackoffImmediateRetrySeconds,
+  EXPECT_EQ(kInitialBackoffRetrySeconds,
             delay->GetInitialDelay(state).InSeconds());
 
   state.last_download_updates_result = SERVER_RETURN_TRANSIENT_ERROR;
@@ -74,7 +74,7 @@ TEST_F(BackoffDelayProviderTest, GetInitialDelay) {
             delay->GetInitialDelay(state).InSeconds());
 
   state.commit_result = NETWORK_CONNECTION_UNAVAILABLE;
-  EXPECT_EQ(kInitialBackoffImmediateRetrySeconds,
+  EXPECT_EQ(kInitialBackoffRetrySeconds,
             delay->GetInitialDelay(state).InSeconds());
 
   state.commit_result = SERVER_RETURN_CONFLICT;
diff --git a/sync/engine/net/server_connection_manager.cc b/sync/engine/net/server_connection_manager.cc
index 83fbafe..a5be7a5 100644
--- a/sync/engine/net/server_connection_manager.cc
+++ b/sync/engine/net/server_connection_manager.cc
@@ -55,23 +55,6 @@ const char* HttpResponse::GetServerConnectionCodeString(
 
 #undef ENUM_CASE
 
-// TODO(clamy): check if all errors are in the right category.
-HttpResponse::ServerConnectionCode
-HttpResponse::ServerConnectionCodeFromNetError(int error_code) {
-  switch (error_code) {
-    case net::ERR_ABORTED:
-    case net::ERR_SOCKET_NOT_CONNECTED:
-    case net::ERR_NETWORK_CHANGED:
-    case net::ERR_CONNECTION_FAILED:
-    case net::ERR_NAME_NOT_RESOLVED:
-    case net::ERR_INTERNET_DISCONNECTED:
-    case net::ERR_NETWORK_ACCESS_DENIED:
-    case net::ERR_NETWORK_IO_SUSPENDED:
-      return CONNECTION_UNAVAILABLE;
-  }
-  return IO_ERROR;
-}
-
 ServerConnectionManager::Connection::Connection(
     ServerConnectionManager* scm) : scm_(scm) {
 }
diff --git a/sync/engine/net/server_connection_manager.h b/sync/engine/net/server_connection_manager.h
index 6cb653f..62248ea 100644
--- a/sync/engine/net/server_connection_manager.h
+++ b/sync/engine/net/server_connection_manager.h
@@ -91,9 +91,6 @@ struct SYNC_EXPORT HttpResponse {
 
   static const char* GetServerConnectionCodeString(
       ServerConnectionCode code);
-
-  static ServerConnectionCode ServerConnectionCodeFromNetError(
-      int error_code);
 };
 
 struct ServerConnectionEvent {