ServerConnectionManager: store full HttpResponse

instead of just the ServerConnectionCode and the net error code. For
now, this makes sure that the connection code and the net error are
always updated together and thus remain consistent. Later, it will
also allow us to expose the http error code as well.

Bug: 842096
Change-Id: I41b2dfae80b55c69ae3693ac2a8c6d6b8fa2547e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1560195
Commit-Queue: Marc Treib <treib@chromium.org>
Reviewed-by: Jan Krcal <jkrcal@chromium.org>
Cr-Commit-Position: refs/heads/master@{#649793}

components/sync/engine_impl/net/server_connection_manager.cc

@@ -15,7 +15,6 @@
 #include "components/sync/engine_impl/syncer.h"
 #include "components/sync/protocol/sync.pb.h"
 #include "components/sync/syncable/directory.h"
-#include "net/base/net_errors.h"
 #include "net/http/http_status_code.h"
 #include "url/gurl.h"
 
@@ -51,11 +50,48 @@ const char* GetServerConnectionCodeString(
 }  // namespace
 
 HttpResponse::HttpResponse()
-    : net_error_code(-1),
+    : server_status(NONE),
+      net_error_code(-1),
       http_status_code(-1),
       content_length(-1),
-      payload_length(-1),
-      server_status(NONE) {}
+      payload_length(-1) {}
+
+// static
+HttpResponse HttpResponse::Uninitialized() {
+  return HttpResponse();
+}
+
+// static
+HttpResponse HttpResponse::ForNetError(int net_error_code) {
+  HttpResponse response;
+  response.server_status = CONNECTION_UNAVAILABLE;
+  response.net_error_code = net_error_code;
+  return response;
+}
+
+// static
+HttpResponse HttpResponse::ForIoError() {
+  HttpResponse response;
+  response.server_status = IO_ERROR;
+  return response;
+}
+
+// static
+HttpResponse HttpResponse::ForHttpError(int http_status_code) {
+  HttpResponse response;
+  response.server_status = http_status_code == net::HTTP_UNAUTHORIZED
+                               ? SYNC_AUTH_ERROR
+                               : SYNC_SERVER_ERROR;
+  response.http_status_code = http_status_code;
+  return response;
+}
+
+// static
+HttpResponse HttpResponse::ForSuccess() {
+  HttpResponse response;
+  response.server_status = SERVER_CONNECTION_OK;
+  return response;
+}
 
 ServerConnectionManager::Connection::Connection(ServerConnectionManager* scm)
     : scm_(scm) {}
@@ -111,7 +147,7 @@ string StripTrailingSlash(const string& s) {
 
 }  // namespace
 
-// TODO(chron): Use a GURL instead of string concatenation.
+// TODO(crbug.com/951350): Use a GURL instead of string concatenation.
 string ServerConnectionManager::Connection::MakeConnectionURL(
     const string& sync_server,
     const string& path,
@@ -141,7 +177,7 @@ ServerConnectionManager::ServerConnectionManager(
       sync_server_port_(port),
       use_ssl_(use_ssl),
       proto_sync_path_(kSyncServerSyncPath),
-      server_status_(HttpResponse::NONE),
+      server_response_(HttpResponse::Uninitialized()),
       cancelation_signal_(cancelation_signal) {}
 
 ServerConnectionManager::~ServerConnectionManager() = default;
@@ -170,7 +206,7 @@ bool ServerConnectionManager::SetAccessToken(const std::string& access_token) {
   // second request. Need to notify sync frontend again to request new token,
   // otherwise backend will stay in SYNC_AUTH_ERROR state while frontend thinks
   // everything is fine and takes no actions.
-  SetServerStatus(HttpResponse::SYNC_AUTH_ERROR);
+  SetServerResponse(HttpResponse::ForHttpError(net::HTTP_UNAUTHORIZED));
   return false;
 }
 
@@ -178,22 +214,25 @@ void ServerConnectionManager::ClearAccessToken() {
   access_token_.clear();
 }
 
-void ServerConnectionManager::SetServerStatus(
-    HttpResponse::ServerConnectionCode server_status) {
-  // SYNC_AUTH_ERROR is permanent error. Need to notify observer to take
-  // action externally to resolve.
-  if (server_status != HttpResponse::SYNC_AUTH_ERROR &&
-      server_status_ == server_status) {
-    return;
+void ServerConnectionManager::SetServerResponse(
+    const HttpResponse& server_response) {
+  // Notify only if the server status changed, except for SYNC_AUTH_ERROR: In
+  // that case, always notify in order to poke observers to do something about
+  // it.
+  bool notify =
+      (server_response.server_status == HttpResponse::SYNC_AUTH_ERROR ||
+       server_response_.server_status != server_response.server_status);
+  server_response_ = server_response;
+  if (notify) {
+    NotifyStatusChanged();
   }
-  server_status_ = server_status;
-  NotifyStatusChanged();
 }
 
 void ServerConnectionManager::NotifyStatusChanged() {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   for (auto& observer : listeners_)
-    observer.OnServerConnectionEvent(ServerConnectionEvent(server_status_));
+    observer.OnServerConnectionEvent(
+        ServerConnectionEvent(server_response_.server_status));
 }
 
 bool ServerConnectionManager::PostBufferWithCachedAuth(
@@ -202,8 +241,7 @@ bool ServerConnectionManager::PostBufferWithCachedAuth(
   string path =
       MakeSyncServerPath(proto_sync_path(), MakeSyncQueryString(client_id_));
   bool result = PostBufferToPath(params, path, access_token_);
-  SetServerStatus(params->response.server_status);
-  net_error_code_ = params->response.net_error_code;
+  SetServerResponse(params->response);
   return result;
 }
 

components/sync/engine_impl/net/server_connection_manager.h

@@ -28,7 +28,8 @@ struct HttpResponse {
     // For uninitialized state.
     NONE,
 
-    // CONNECTION_UNAVAILABLE is returned when InternetConnect() fails.
+    // CONNECTION_UNAVAILABLE means either the request got canceled or it
+    // encountered a network error.
     CONNECTION_UNAVAILABLE,
 
     // IO_ERROR is returned when reading/writing to a buffer has failed.
@@ -40,12 +41,17 @@ struct HttpResponse {
 
     // SYNC_AUTH_ERROR is returned when the HTTP status code indicates that an
     // auth error has occurred (i.e. a 401).
+    // TODO(crbug.com/842096, crbug.com/951350): Remove this and instead use
+    // SYNC_SERVER_ERROR plus |http_status_code| == 401.
     SYNC_AUTH_ERROR,
 
     // SERVER_CONNECTION_OK is returned when request was handled correctly.
     SERVER_CONNECTION_OK,
   };
 
+  // Identifies the type of failure, if any.
+  ServerConnectionCode server_status;
+
   // The network error code.
   int net_error_code;
 
@@ -58,9 +64,15 @@ struct HttpResponse {
   // The size of a download request's payload.
   int64_t payload_length;
 
-  // Identifies the type of failure, if any.
-  ServerConnectionCode server_status;
+  static HttpResponse Uninitialized();
+  static HttpResponse ForNetError(int net_error_code);
+  static HttpResponse ForIoError();
+  static HttpResponse ForHttpError(int http_status_code);
+  static HttpResponse ForSuccess();
 
+ private:
+  // Private to prevent accidental usage. Use Uninitialized() if you really need
+  // a "default" instance.
   HttpResponse();
 };
 
@@ -88,7 +100,7 @@ class ServerConnectionManager {
   struct PostBufferParams {
     std::string buffer_in;
     std::string buffer_out;
-    HttpResponse response;
+    HttpResponse response = HttpResponse::Uninitialized();
   };
 
   // Abstract class providing network-layer functionality to the
@@ -100,6 +112,9 @@ class ServerConnectionManager {
     virtual ~Connection();
 
     // Called to initialize and perform an HTTP POST.
+    // TODO(crbug.com/951350): Return the HttpResponse by value. It's not
+    // obvious what the boolean return value means. (True means success or HTTP
+    // error, false means canceled or network error.)
     virtual bool Init(const char* path,
                       const std::string& access_token,
                       const std::string& payload,
@@ -149,12 +164,12 @@ class ServerConnectionManager {
 
   inline HttpResponse::ServerConnectionCode server_status() const {
     DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-    return server_status_;
+    return server_response_.server_status;
   }
 
   inline int net_error_code() const {
     DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-    return net_error_code_;
+    return server_response_.net_error_code;
   }
 
   const std::string client_id() const { return client_id_; }
@@ -179,8 +194,9 @@ class ServerConnectionManager {
  protected:
   inline std::string proto_sync_path() const { return proto_sync_path_; }
 
-  // Updates server_status_ and notifies listeners if server_status_ changed
-  void SetServerStatus(HttpResponse::ServerConnectionCode server_status);
+  // Updates server_response_ and notifies listeners if the server status
+  // changed.
+  void SetServerResponse(const HttpResponse& server_response);
 
   // NOTE: Tests rely on this protected function being virtual.
   //
@@ -218,12 +234,7 @@ class ServerConnectionManager {
 
   base::ObserverList<ServerConnectionEventListener>::Unchecked listeners_;
 
-  HttpResponse::ServerConnectionCode server_status_;
-
-  // Contains the network error code if there is an error when making the
-  // connection with the server in which case |server_status_| is set to
-  // HttpResponse::CONNECTION_UNAVAILABLE.
-  int net_error_code_;
+  HttpResponse server_response_;
 
   SEQUENCE_CHECKER(sequence_checker_);
 

components/sync/engine_impl/net/sync_server_connection_manager.cc

@@ -60,6 +60,7 @@ bool SyncBridgedConnection::Init(const char* path,
   int http_status_code = 0;
   if (!cancelation_signal_->TryRegisterHandler(this)) {
     // Return early because cancelation signal was signaled.
+    // TODO(crbug.com/951350): Introduce an extra status code for canceled?
     response->server_status = HttpResponse::CONNECTION_UNAVAILABLE;
     return false;
   }

components/sync/engine_impl/sync_scheduler_impl_unittest.cc

@@ -29,6 +29,7 @@
 #include "components/sync/test/engine/mock_connection_manager.h"
 #include "components/sync/test/engine/mock_nudge_handler.h"
 #include "components/sync/test/mock_invalidation.h"
+#include "net/http/http_status_code.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
@@ -1657,7 +1658,8 @@ TEST_F(SyncSchedulerImplTest, PollFromCanaryAfterAuthError) {
           DoAll(Invoke(test_util::SimulatePollSuccess),
                 RecordSyncShareMultiple(&times, kMinNumSamples, true)));
 
-  connection()->SetServerStatus(HttpResponse::SYNC_AUTH_ERROR);
+  connection()->SetServerResponse(
+      HttpResponse::ForHttpError(net::HTTP_UNAUTHORIZED));
   StartSyncScheduler(base::Time());
 
   // Run to wait for polling.
@@ -1670,7 +1672,7 @@ TEST_F(SyncSchedulerImplTest, PollFromCanaryAfterAuthError) {
       .WillOnce(DoAll(Invoke(test_util::SimulatePollSuccess),
                       RecordSyncShare(&times, true)));
   scheduler()->OnCredentialsUpdated();
-  connection()->SetServerStatus(HttpResponse::SERVER_CONNECTION_OK);
+  connection()->SetServerResponse(HttpResponse::ForSuccess());
   RunLoop();
   StopSyncScheduler();
 }

components/sync/test/engine/mock_connection_manager.cc

@@ -14,6 +14,7 @@
 #include "components/sync/syncable/directory.h"
 #include "components/sync/syncable/syncable_write_transaction.h"
 #include "components/sync/test/engine/test_id_factory.h"
+#include "net/base/net_errors.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 using std::find;
@@ -792,11 +793,9 @@ void MockConnectionManager::SetServerNotReachable() {
 }
 
 void MockConnectionManager::UpdateConnectionStatus() {
-  if (!server_reachable_) {
-    SetServerStatus(HttpResponse::CONNECTION_UNAVAILABLE);
-  } else {
-    SetServerStatus(HttpResponse::SERVER_CONNECTION_OK);
-  }
+  SetServerResponse(server_reachable_
+                        ? HttpResponse::ForSuccess()
+                        : HttpResponse::ForNetError(net::ERR_FAILED));
 }
 
 }  // namespace syncer

components/sync/test/engine/mock_connection_manager.h

@@ -251,7 +251,7 @@ class MockConnectionManager : public ServerConnectionManager {
   // requests.
   void UpdateConnectionStatus();
 
-  using ServerConnectionManager::SetServerStatus;
+  using ServerConnectionManager::SetServerResponse;
 
   // Return by copy to be thread-safe.
   const std::string store_birthday() {