Detect sync server communication errors

This is part 2 of 3 in a series of patches to remove the unnecessary
SYNC_CYCLE_CONTINUATION sync cycle that follows every commit.

This patch adds infrastructure to report errors involving communication
with the sync server back to the SyncSession and SyncScheduler.

It modifies SyncerProtoUtil::PostClientToServerMessage to have it return
an error code describing why it failed.  This function's callers (all of
which are SyncerCommands) use the infrastructure added in the first
patch of this series to return the error value to SyncShare().  From
there, the return values are placed into the session's StatusController
object.  The method SyncSession::Succeeded() method provides an easy way
to determine if the anything went wrong with the current sync session,
and its response is based on the stored return values.

This change has no effect on the client's behaviour.  Although it gives
the client access to lots more information, the client does not use it
for any decision making.

BUG=94670
TEST=


Review URL: http://codereview.chromium.org/9158004

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

chrome/browser/sync/engine/clear_data_command.cc

@@ -47,7 +47,7 @@ SyncerError ClearDataCommand::ExecuteImpl(SyncSession* session) {
 
   DVLOG(1) << "Clearing server data";
 
-  bool ok = SyncerProtoUtil::PostClientToServerMessage(
+  SyncerError result = SyncerProtoUtil::PostClientToServerMessage(
       client_to_server_message,
       &client_to_server_response,
       session);
@@ -61,7 +61,7 @@ SyncerError ClearDataCommand::ExecuteImpl(SyncSession* session) {
   // See also: crbug.com/71616.
   //
   // Clear pending indicates that the server has received our clear message
-  if (!ok || !client_to_server_response.has_error_code() ||
+  if (result != SYNCER_OK || !client_to_server_response.has_error_code() ||
       client_to_server_response.error_code() != sync_pb::SyncEnums::SUCCESS) {
     // On failure, subsequent requests to the server will cause it to attempt
     // to resume the clear.  The client will handle disabling of sync in
@@ -71,7 +71,7 @@ SyncerError ClearDataCommand::ExecuteImpl(SyncSession* session) {
 
     LOG(ERROR) << "Error posting ClearData.";
 
-    return SYNCER_OK;
+    return result;
   }
 
   SyncEngineEvent event(SyncEngineEvent::CLEAR_SERVER_DATA_SUCCEEDED);

chrome/browser/sync/engine/download_updates_command.cc

@@ -91,7 +91,7 @@ SyncerError DownloadUpdatesCommand::ExecuteImpl(SyncSession* session) {
 
   AppendClientDebugInfoIfNeeded(session, debug_info);
 
-  bool ok = SyncerProtoUtil::PostClientToServerMessage(
+  SyncerError result = SyncerProtoUtil::PostClientToServerMessage(
       client_to_server_message,
       &update_response,
       session);
@@ -101,11 +101,11 @@ SyncerError DownloadUpdatesCommand::ExecuteImpl(SyncSession* session) {
 
   StatusController* status = session->mutable_status_controller();
   status->set_updates_request_types(enabled_types);
-  if (!ok) {
+  if (result != SYNCER_OK) {
     status->increment_num_consecutive_errors();
     status->mutable_updates_response()->Clear();
     LOG(ERROR) << "PostClientToServerMessage() failed during GetUpdates";
-    return SYNCER_OK; // TODO(rlarocque): Return an error here.
+    return result;
   }
 
   status->mutable_updates_response()->CopyFrom(update_response);
@@ -115,7 +115,7 @@ SyncerError DownloadUpdatesCommand::ExecuteImpl(SyncSession* session) {
            << " updates and indicated "
            << update_response.get_updates().changes_remaining()
            << " updates left on server.";
-  return SYNCER_OK;
+  return result;
 }
 
 void DownloadUpdatesCommand::AppendClientDebugInfoIfNeeded(

chrome/browser/sync/engine/syncer.cc

@@ -1,4 +1,4 @@
-// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
@@ -144,7 +144,8 @@ void Syncer::SyncShare(sessions::SyncSession* session,
       }
       case DOWNLOAD_UPDATES: {
         DownloadUpdatesCommand download_updates;
-        download_updates.Execute(session);
+        session->mutable_status_controller()->set_last_download_updates_result(
+            download_updates.Execute(session));
         next_step = PROCESS_CLIENT_COMMAND;
         break;
       }
@@ -222,14 +223,17 @@ void Syncer::SyncShare(sessions::SyncSession* session,
       }
       case POST_COMMIT_MESSAGE: {
         PostCommitMessageCommand post_commit_command;
-        post_commit_command.Execute(session);
+        session->mutable_status_controller()->set_last_post_commit_result(
+            post_commit_command.Execute(session));
         next_step = PROCESS_COMMIT_RESPONSE;
         break;
       }
       case PROCESS_COMMIT_RESPONSE: {
         session->mutable_status_controller()->reset_num_conflicting_commits();
         ProcessCommitResponseCommand process_response_command;
-        process_response_command.Execute(session);
+        session->mutable_status_controller()->
+            set_last_process_commit_response_result(
+                process_response_command.Execute(session));
         next_step = BUILD_AND_PROCESS_CONFLICT_SETS;
         break;
       }

chrome/browser/sync/engine/syncer_proto_util.cc

@@ -80,6 +80,28 @@ void LogResponseProfilingData(const ClientToServerResponse& response) {
   }
 }
 
+SyncerError ServerConnectionErrorAsSyncerError(
+    const HttpResponse::ServerConnectionCode server_status) {
+  switch (server_status) {
+    case HttpResponse::CONNECTION_UNAVAILABLE:
+      return NETWORK_CONNECTION_UNAVAILABLE;
+    case HttpResponse::IO_ERROR:
+      return NETWORK_IO_ERROR;
+    case HttpResponse::SYNC_SERVER_ERROR:
+      // FIXME what does this mean?
+      return SYNC_SERVER_ERROR;
+    case HttpResponse::SYNC_AUTH_ERROR:
+      return SYNC_AUTH_ERROR;
+    case HttpResponse::RETRY:
+      return SERVER_RETURN_TRANSIENT_ERROR;
+    case HttpResponse::SERVER_CONNECTION_OK:
+    case HttpResponse::NONE:
+    default:
+      NOTREACHED();
+      return UNSET;
+  }
+}
+
 }  // namespace
 
 // static
@@ -303,7 +325,7 @@ browser_sync::SyncProtocolError ConvertLegacyErrorCodeToNewError(
 }  // namespace
 
 // static
-bool SyncerProtoUtil::PostClientToServerMessage(
+SyncerError SyncerProtoUtil::PostClientToServerMessage(
     const ClientToServerMessage& msg,
     ClientToServerResponse* response,
     SyncSession* session) {
@@ -317,11 +339,20 @@ bool SyncerProtoUtil::PostClientToServerMessage(
   ScopedDirLookup dir(session->context()->directory_manager(),
       session->context()->account_name());
   if (!dir.good())
-    return false;
+    return DIRECTORY_LOOKUP_FAILED;
 
   if (!PostAndProcessHeaders(session->context()->connection_manager(), session,
-                             msg, response))
-    return false;
+                             msg, response)) {
+    // There was an error establishing communication with the server.
+    // We can not proceed beyond this point.
+    const browser_sync::HttpResponse::ServerConnectionCode server_status =
+        session->context()->connection_manager()->server_status();
+
+    DCHECK_NE(server_status, browser_sync::HttpResponse::NONE);
+    DCHECK_NE(server_status, browser_sync::HttpResponse::SERVER_CONNECTION_OK);
+
+    return ServerConnectionErrorAsSyncerError(server_status);
+  }
 
   browser_sync::SyncProtocolError sync_protocol_error;
 
@@ -352,28 +383,29 @@ bool SyncerProtoUtil::PostClientToServerMessage(
     case browser_sync::UNKNOWN_ERROR:
       LOG(WARNING) << "Sync protocol out-of-date. The server is using a more "
                    << "recent version.";
-      return false;
+      return SERVER_RETURN_UNKNOWN_ERROR;
     case browser_sync::SYNC_SUCCESS:
       LogResponseProfilingData(*response);
-      return true;
+      return SYNCER_OK;
     case browser_sync::THROTTLED:
       LOG(WARNING) << "Client silenced by server.";
       HandleThrottleError(sync_protocol_error,
                           base::TimeTicks::Now() + GetThrottleDelay(*response),
                           session->context(),
                           session->delegate());
-      return false;
+      return SERVER_RETURN_THROTTLED;
     case browser_sync::TRANSIENT_ERROR:
-      return false;
+      return SERVER_RETURN_TRANSIENT_ERROR;
     case browser_sync::MIGRATION_DONE:
       HandleMigrationDoneResponse(response, session);
-      return false;
+      return SERVER_RETURN_MIGRATION_DONE;
     case browser_sync::CLEAR_PENDING:
+      return SERVER_RETURN_CLEAR_PENDING;
     case browser_sync::NOT_MY_BIRTHDAY:
-      return false;
+      return SERVER_RETURN_NOT_MY_BIRTHDAY;
     default:
       NOTREACHED();
-      return false;
+      return UNSET;
   }
 }
 

chrome/browser/sync/engine/syncer_proto_util.h

@@ -1,4 +1,4 @@
-// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
@@ -10,6 +10,7 @@
 
 #include "base/gtest_prod_util.h"
 #include "base/time.h"
+#include "chrome/browser/sync/internal_api/includes/syncer_error.h"
 #include "chrome/browser/sync/sessions/sync_session.h"
 #include "chrome/browser/sync/syncable/blob.h"
 #include "chrome/browser/sync/syncable/model_type.h"
@@ -42,7 +43,7 @@ class SyncerProtoUtil {
   // Returns true on success.  Also handles store birthday verification:
   // session->status()->syncer_stuck_ is set true if the birthday is
   // incorrect.  A false value will always be returned if birthday is bad.
-  static bool PostClientToServerMessage(
+  static SyncerError PostClientToServerMessage(
       const ClientToServerMessage& msg,
       sync_pb::ClientToServerResponse* response,
       sessions::SyncSession* session);

chrome/browser/sync/sessions/session_state.cc

@@ -1,4 +1,4 @@
-// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
@@ -95,7 +95,10 @@ DictionaryValue* DownloadProgressMarkersToValue(
 ErrorCounters::ErrorCounters()
     : num_conflicting_commits(0),
       consecutive_transient_error_commits(0),
-      consecutive_errors(0) {
+      consecutive_errors(0),
+      last_download_updates_result(UNSET),
+      last_post_commit_result(UNSET),
+      last_process_commit_response_result(UNSET) {
 }
 
 DictionaryValue* ErrorCounters::ToValue() const {

chrome/browser/sync/sessions/session_state.h

@@ -1,4 +1,4 @@
-// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
@@ -105,6 +105,11 @@ struct ErrorCounters {
 
   // Any protocol errors that we received during this sync session.
   SyncProtocolError sync_protocol_error;
+
+  // Records the most recent results of PostCommit and GetUpdates commands.
+  SyncerError last_download_updates_result;
+  SyncerError last_post_commit_result;
+  SyncerError last_process_commit_response_result;
 };
 
 // Caller takes ownership of the returned dictionary.

chrome/browser/sync/sessions/status_controller.cc

@@ -1,4 +1,4 @@
-// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
@@ -211,6 +211,20 @@ void StatusController::set_sync_protocol_error(
   shared_.error.mutate()->sync_protocol_error = error;
 }
 
+void StatusController::set_last_download_updates_result(
+    const SyncerError result) {
+  shared_.error.mutate()->last_download_updates_result = result;
+}
+
+void StatusController::set_last_post_commit_result(const SyncerError result) {
+  shared_.error.mutate()->last_post_commit_result = result;
+}
+
+void StatusController::set_last_process_commit_response_result(
+    const SyncerError result) {
+  shared_.error.mutate()->last_process_commit_response_result = result;
+}
+
 void StatusController::set_commit_set(const OrderedCommitSet& commit_set) {
   DCHECK(!group_restriction_in_effect_);
   shared_.commit_set = commit_set;

chrome/browser/sync/sessions/status_controller.h

@@ -1,4 +1,4 @@
-// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
@@ -228,6 +228,9 @@ class StatusController {
   void increment_num_local_overwrites();
   void increment_num_server_overwrites();
   void set_sync_protocol_error(const SyncProtocolError& error);
+  void set_last_download_updates_result(const SyncerError result);
+  void set_last_post_commit_result(const SyncerError result);
+  void set_last_process_commit_response_result(const SyncerError result);
 
   void set_commit_set(const OrderedCommitSet& commit_set);
   void update_conflict_sets_built(bool built);

chrome/browser/sync/sessions/status_controller_unittest.cc

@@ -1,4 +1,4 @@
-// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
@@ -130,6 +130,16 @@ TEST_F(StatusControllerTest, ReadYourWrites) {
   status.set_syncer_stuck(true);
   EXPECT_TRUE(status.syncer_status().syncer_stuck);
 
+  status.set_last_download_updates_result(SYNCER_OK);
+  EXPECT_EQ(SYNCER_OK, status.error().last_download_updates_result);
+
+  status.set_last_post_commit_result(SYNC_AUTH_ERROR);
+  EXPECT_EQ(SYNC_AUTH_ERROR, status.error().last_post_commit_result);
+
+  status.set_last_process_commit_response_result(SYNC_SERVER_ERROR);
+  EXPECT_EQ(SYNC_SERVER_ERROR,
+            status.error().last_process_commit_response_result);
+
   for (int i = 0; i < 14; i++)
     status.increment_num_successful_commits();
   EXPECT_EQ(14, status.syncer_status().num_successful_commits);

chrome/browser/sync/sessions/sync_session.cc

@@ -1,4 +1,4 @@
-// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
@@ -233,6 +233,26 @@ std::set<ModelSafeGroup>
   return enabled_groups_with_verified_updates;
 }
 
+namespace {
+// Return true if the command in question was attempted and did not complete
+// successfully.
+//
+bool IsError(SyncerError error) {
+  return error != UNSET && error != SYNCER_OK;
+}
+}  // namespace
+
+bool SyncSession::Succeeded() const {
+  const bool download_updates_error =
+      IsError(status_controller_->error().last_download_updates_result);
+  const bool post_commit_error =
+      IsError(status_controller_->error().last_post_commit_result);
+  const bool process_commit_response_error =
+      IsError(status_controller_->error().last_process_commit_response_result);
+  return !download_updates_error
+      && !post_commit_error
+      && !process_commit_response_error;
+}
 
 }  // namespace sessions
 }  // namespace browser_sync

chrome/browser/sync/sessions/sync_session.h

@@ -1,4 +1,4 @@
-// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
@@ -112,6 +112,22 @@ class SyncSession {
   // engine again.
   bool HasMoreToSync() const;
 
+  // Returns true if there we did not detect any errors in this session.
+  //
+  // There are many errors that could prevent a sync cycle from succeeding.
+  // These include invalid local state, inability to contact the server,
+  // inability to authenticate with the server, and server errors.  What they
+  // have in common is that the we either need to take some action and then
+  // retry the sync cycle or, in the case of transient errors, retry after some
+  // backoff timer has expired.  Most importantly, the SyncScheduler should not
+  // assume that the original action that triggered the sync cycle (ie. a nudge
+  // or a notification) has been properly serviced.
+  //
+  // This function also returns false if SyncShare has not been called on this
+  // session yet, or if ResetTransientState() has been called on this session
+  // since the last call to SyncShare.
+  bool Succeeded() const;
+
   // Collects all state pertaining to how and why |s| originated and unions it
   // with corresponding state in |this|, leaving |s| unchanged.  Allows |this|
   // to take on the responsibilities |s| had (e.g. certain data types) in the