[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}

9 files changed, 92 insertions, 44 deletions

diff --git a/chrome/browser/sync/test/integration/sync_exponential_backoff_test.cc b/chrome/browser/sync/test/integration/sync_exponential_backoff_test.cc
index 51a07cc..1910811 100644
--- a/chrome/browser/sync/test/integration/sync_exponential_backoff_test.cc
+++ b/chrome/browser/sync/test/integration/sync_exponential_backoff_test.cc
@@ -11,6 +11,7 @@
 #include "chrome/browser/sync/test/integration/sync_integration_test_util.h"
 #include "chrome/browser/sync/test/integration/sync_test.h"
 #include "components/browser_sync/browser/profile_sync_service.h"
+#include "net/base/network_change_notifier.h"
 
 namespace {
 
@@ -81,11 +82,24 @@ IN_PROC_BROWSER_TEST_F(SyncExponentialBackoffTest, OfflineToOnline) {
   exponential_backoff_checker.Wait();
   ASSERT_FALSE(exponential_backoff_checker.TimedOut());
 
+  // Trigger network change notification and remember time when it happened.
+  // Ensure that scheduler runs canary job immediately.
   GetFakeServer()->EnableNetwork();
+  net::NetworkChangeNotifier::NotifyObserversOfConnectionTypeChangeForTests(
+      net::NetworkChangeNotifier::CONNECTION_ETHERNET);
+
+  base::Time network_notification_time = base::Time::Now();
 
   // Verify that sync was able to recover.
   ASSERT_TRUE(AwaitCommitActivityCompletion(GetSyncService((0))));
   ASSERT_TRUE(ModelMatchesVerifier(0));
+
+  // Verify that recovery time is short. Without canary job recovery time would
+  // be more than 5 seconds.
+  base::TimeDelta recovery_time =
+      GetSyncService(0)->GetLastSessionSnapshot().sync_start_time() -
+      network_notification_time;
+  ASSERT_LE(recovery_time, base::TimeDelta::FromSeconds(2));
 }
 
 IN_PROC_BROWSER_TEST_F(SyncExponentialBackoffTest, TransientErrorTest) {
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 {
diff --git a/sync/internal_api/http_bridge.cc b/sync/internal_api/http_bridge.cc
index 842adf3..b8d57cc 100644
--- a/sync/internal_api/http_bridge.cc
+++ b/sync/internal_api/http_bridge.cc
@@ -512,7 +512,7 @@ void HttpBridge::OnURLFetchTimedOut() {
   fetch_state_.request_completed = true;
   fetch_state_.request_succeeded = false;
   fetch_state_.http_response_code = -1;
-  fetch_state_.error_code = net::URLRequestStatus::FAILED;
+  fetch_state_.error_code = net::ERR_TIMED_OUT;
 
   // This method is called by the timer, not the url fetcher implementation,
   // so it's safe to delete the fetcher here.
diff --git a/sync/internal_api/syncapi_server_connection_manager.cc b/sync/internal_api/syncapi_server_connection_manager.cc
index 16646ec..a54fcbc 100644
--- a/sync/internal_api/syncapi_server_connection_manager.cc
+++ b/sync/internal_api/syncapi_server_connection_manager.cc
@@ -53,9 +53,9 @@ bool SyncAPIBridgedConnection::Init(const char* path,
   int error_code = 0;
   int response_code = 0;
   if (!http->MakeSynchronousPost(&error_code, &response_code)) {
+    DCHECK_NE(error_code, net::OK);
     DVLOG(1) << "Http POST failed, error returns: " << error_code;
-    response->server_status = HttpResponse::ServerConnectionCodeFromNetError(
-        error_code);
+    response->server_status = HttpResponse::CONNECTION_UNAVAILABLE;
     return false;
   }
 
diff --git a/sync/internal_api/syncapi_server_connection_manager.h b/sync/internal_api/syncapi_server_connection_manager.h
index aa1fe1e..225959b 100644
--- a/sync/internal_api/syncapi_server_connection_manager.h
+++ b/sync/internal_api/syncapi_server_connection_manager.h
@@ -67,6 +67,8 @@ class SYNC_EXPORT SyncAPIServerConnectionManager
                            VeryEarlyAbortPost);
   FRIEND_TEST_ALL_PREFIXES(SyncAPIServerConnectionManagerTest, EarlyAbortPost);
   FRIEND_TEST_ALL_PREFIXES(SyncAPIServerConnectionManagerTest, AbortPost);
+  FRIEND_TEST_ALL_PREFIXES(SyncAPIServerConnectionManagerTest,
+                           FailPostWithTimedOut);
 
   // A factory creating concrete HttpPostProviders for use whenever we need to
   // issue a POST to sync servers.
diff --git a/sync/internal_api/syncapi_server_connection_manager_unittest.cc b/sync/internal_api/syncapi_server_connection_manager_unittest.cc
index 208b3a2..9e67be8 100644
--- a/sync/internal_api/syncapi_server_connection_manager_unittest.cc
+++ b/sync/internal_api/syncapi_server_connection_manager_unittest.cc
@@ -120,4 +120,70 @@ TEST(SyncAPIServerConnectionManagerTest, AbortPost) {
   abort_thread.Stop();
 }
 
+namespace {
+
+class FailingHttpPost : public HttpPostProviderInterface {
+ public:
+  explicit FailingHttpPost(int error_code) : error_code_(error_code) {}
+  ~FailingHttpPost() override {}
+
+  void SetExtraRequestHeaders(const char* headers) override {}
+  void SetURL(const char* url, int port) override {}
+  void SetPostPayload(const char* content_type,
+                      int content_length,
+                      const char* content) override {}
+  bool MakeSynchronousPost(int* error_code, int* response_code) override {
+    *error_code = error_code_;
+    return false;
+  }
+  int GetResponseContentLength() const override { return 0; }
+  const char* GetResponseContent() const override { return ""; }
+  const std::string GetResponseHeaderValue(
+      const std::string& name) const override {
+    return std::string();
+  }
+  void Abort() override {}
+
+ private:
+  int error_code_;
+};
+
+class FailingHttpPostFactory : public HttpPostProviderFactory {
+ public:
+  explicit FailingHttpPostFactory(int error_code) : error_code_(error_code) {}
+  ~FailingHttpPostFactory() override {}
+  void Init(const std::string& user_agent,
+            const BindToTrackerCallback& bind_to_tracker_callback) override {}
+
+  HttpPostProviderInterface* Create() override {
+    return new FailingHttpPost(error_code_);
+  }
+  void Destroy(HttpPostProviderInterface* http) override {
+    delete static_cast<FailingHttpPost*>(http);
+  }
+
+ private:
+  int error_code_;
+};
+
+}  // namespace
+
+// Fail request with TIMED_OUT error. Make sure server status is
+// CONNECTION_UNAVAILABLE and therefore request will be retried after network
+// change.
+TEST(SyncAPIServerConnectionManagerTest, FailPostWithTimedOut) {
+  CancelationSignal signal;
+  SyncAPIServerConnectionManager server(
+      "server", 0, true, new FailingHttpPostFactory(net::ERR_TIMED_OUT),
+      &signal);
+
+  ServerConnectionManager::PostBufferParams params;
+
+  bool result = server.PostBufferToPath(&params, "/testpath", "testauth");
+
+  EXPECT_FALSE(result);
+  EXPECT_EQ(HttpResponse::CONNECTION_UNAVAILABLE,
+            params.response.server_status);
+}
+
 }  // namespace syncer