Fix the false-positive detection of commit errors

chrome/browser/sync/engine/syncer_proto_util.cc

 // Copyright (c) 2011 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.
 
 #include "chrome/browser/sync/engine/syncer_proto_util.h"
 
 #include "base/format_macros.h"
 #include "base/stringprintf.h"
 #include "chrome/browser/sync/engine/net/server_connection_manager.h"
 #include "chrome/browser/sync/engine/syncer.h"
 #include "chrome/browser/sync/engine/syncer_types.h"
 #include "chrome/browser/sync/protocol/service_constants.h"
 #include "chrome/browser/sync/protocol/sync.pb.h"
 #include "chrome/browser/sync/protocol/sync_protocol_error.h"
 #include "chrome/browser/sync/sessions/sync_session.h"
 #include "chrome/browser/sync/syncable/directory_manager.h"
 #include "chrome/browser/sync/syncable/model_type.h"
 #include "chrome/browser/sync/syncable/syncable-inl.h"
 #include "chrome/browser/sync/syncable/syncable.h"
 #include "chrome/browser/sync/util/time.h"
 
-using browser_sync::SyncProtocolErrorType;
+using browser_sync::SyncOperationResultType;
 using std::string;
 using std::stringstream;
 using syncable::BASE_VERSION;
 using syncable::CTIME;
 using syncable::ID;
 using syncable::IS_DEL;
 using syncable::IS_DIR;
 using syncable::IS_UNSYNCED;
 using syncable::MTIME;
 using syncable::PARENT_ID;
 using syncable::ScopedDirLookup;
 using syncable::SyncName;
 
 namespace browser_sync {
 using sessions::SyncSession;
 
 namespace {
 
+SyncOperationResult ServerConnectionErrorToSyncOperationResult(
+    const browser_sync::HttpResponse::ServerConnectionCode server_status)
+{
+  SyncOperationResult result;
+
+  switch (server_status) {
+    case browser_sync::HttpResponse::CONNECTION_UNAVAILABLE:
+      result.error_type =  NETWORK_CONNECTION_UNAVAILABLE;
+      break;
+    case browser_sync::HttpResponse::IO_ERROR:
+      result.error_type = NETWORK_IO_ERROR;
+      break;
+    case browser_sync::HttpResponse::SYNC_SERVER_ERROR:
+      // FIXME: how does this happen?  Can we map this to TRANSIENT_ERROR or
+      // NON_RETRIABLE_ERROR?
+      result.error_type = SYNC_SERVER_ERROR;
+      break;
+    case browser_sync::HttpResponse::SYNC_AUTH_ERROR:
+      // FIXME: how does this compare to invalid credential?
+      result.error_type = SYNC_AUTH_ERROR;
+      break;
+    case browser_sync::HttpResponse::RETRY:
+      result.error_type = TRANSIENT_ERROR;
+      break;
+
+    // If we succeeded in contacting the server, then the server will provide us
+    // with all the data we need to compute a SyncOperationResult.  We don't
+    // handle that here.
+    case browser_sync::HttpResponse::SERVER_CONNECTION_OK:
+    case browser_sync::HttpResponse::NONE:
+    default:
+      NOTREACHED();
+      result.error_type = INVALID;
+  }
+
+  return result;
+}
+
 // Time to backoff syncing after receiving a throttled response.
 static const int kSyncDelayAfterThrottled = 2 * 60 * 60;  // 2 hours
 void LogResponseProfilingData(const ClientToServerResponse& response) {
   if (response.has_profiling_data()) {
     stringstream response_trace;
     response_trace << "Server response trace:";
 
     if (response.profiling_data().has_user_lookup_time()) {
       response_trace << " user lookup: "
                      << response.profiling_data().user_lookup_time() << "ms";
@@ skipping 145 matching lines @@
   if (!message.has_get_updates())
     return false;
   DCHECK_LT(0, message.get_updates().from_progress_marker_size());
   for (int i = 0; i < message.get_updates().from_progress_marker_size(); ++i) {
     if (!message.get_updates().from_progress_marker(i).token().empty())
       return false;
   }
   return true;
 }
 
-SyncProtocolErrorType ConvertSyncProtocolErrorTypePBToLocalType(
+SyncOperationResultType ConvertSyncProtocolErrorTypePBToLocalType(
     const sync_pb::ClientToServerResponse::ErrorType& error_type) {
   switch (error_type) {
     case ClientToServerResponse::SUCCESS:
-      return browser_sync::SYNC_SUCCESS;
+      return browser_sync::OPERATION_SUCCESS;
     case ClientToServerResponse::NOT_MY_BIRTHDAY:
       return browser_sync::NOT_MY_BIRTHDAY;
     case ClientToServerResponse::THROTTLED:
       return browser_sync::THROTTLED;
     case ClientToServerResponse::CLEAR_PENDING:
       return browser_sync::CLEAR_PENDING;
     case ClientToServerResponse::TRANSIENT_ERROR:
       return browser_sync::TRANSIENT_ERROR;
     case ClientToServerResponse::MIGRATION_DONE:
       return browser_sync::MIGRATION_DONE;
@@ skipping 23 matching lines @@
     case ClientToServerResponse::Error::DISABLE_SYNC_ON_CLIENT:
       return browser_sync::DISABLE_SYNC_ON_CLIENT;
     case ClientToServerResponse::Error::UNKNOWN_ACTION:
       return browser_sync::UNKNOWN_ACTION;
     default:
       NOTREACHED();
       return browser_sync::UNKNOWN_ACTION;
   }
 }
 
-browser_sync::SyncProtocolError ConvertErrorPBToLocalType(
+browser_sync::SyncOperationResult ConvertErrorPBToLocalType(
     const sync_pb::ClientToServerResponse::Error& error) {
-  browser_sync::SyncProtocolError sync_protocol_error;
+  browser_sync::SyncOperationResult sync_protocol_error;
   sync_protocol_error.error_type = ConvertSyncProtocolErrorTypePBToLocalType(
       error.error_type());
   sync_protocol_error.error_description = error.error_description();
   sync_protocol_error.url = error.url();
   sync_protocol_error.action = ConvertClientActionPBToLocalClientAction(
       error.action());
   return sync_protocol_error;
 }
 
 // TODO(lipalani) : Rename these function names as per the CR for issue 7740067.
-browser_sync::SyncProtocolError ConvertLegacyErrorCodeToNewError(
+browser_sync::SyncOperationResult ConvertLegacyErrorCodeToNewError(
     const sync_pb::ClientToServerResponse::ErrorType& error_type) {
-  browser_sync::SyncProtocolError error;
+  browser_sync::SyncOperationResult error;
   error.error_type = ConvertSyncProtocolErrorTypePBToLocalType(error_type);
   if (error_type == ClientToServerResponse::CLEAR_PENDING ||
       error_type == ClientToServerResponse::NOT_MY_BIRTHDAY) {
       error.action = browser_sync::DISABLE_SYNC_ON_CLIENT;
   }  // There is no other action we can compute for legacy server.
   return error;
 }
 
 }  // namespace
 
 // static
-bool SyncerProtoUtil::PostClientToServerMessage(
+SyncOperationResult SyncerProtoUtil::PostClientToServerMessage(
     const ClientToServerMessage& msg,
     ClientToServerResponse* response,
     SyncSession* session) {
 
   CHECK(response);
   DCHECK(!msg.get_updates().has_from_timestamp());  // Deprecated.
   DCHECK(!msg.get_updates().has_requested_types());  // Deprecated.
   DCHECK(msg.has_store_birthday() || IsVeryFirstGetUpdates(msg))
       << "Must call AddRequestBirthday to set birthday.";
 
   ScopedDirLookup dir(session->context()->directory_manager(),
       session->context()->account_name());
-  if (!dir.good())
-    return false;
+  if (!dir.good()) {
+    SyncOperationResult result;
+    result.error_type = DIRECTORY_LOOKUP_FAILED;
+    return result;
+  }
 
   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();
 
-  browser_sync::SyncProtocolError sync_protocol_error;
+    DCHECK(server_status != browser_sync::HttpResponse::NONE);
+    DCHECK(server_status != browser_sync::HttpResponse::SERVER_CONNECTION_OK);
+
+    return ServerConnectionErrorToSyncOperationResult(server_status);
+  }
+
+  browser_sync::SyncOperationResult sync_protocol_error;
 
   // Birthday mismatch overrides any error that is sent by the server.
   if (!VerifyResponseBirthday(dir, response)) {
     sync_protocol_error.error_type = browser_sync::NOT_MY_BIRTHDAY;
      sync_protocol_error.action =
          browser_sync::DISABLE_SYNC_ON_CLIENT;
   } else if (response->has_error()) {
     // This is a new server. Just get the error from the protocol.
     sync_protocol_error = ConvertErrorPBToLocalType(response->error());
   } else {
     // Legacy server implementation. Compute the error based on |error_code|.
     sync_protocol_error = ConvertLegacyErrorCodeToNewError(
         response->error_code());
   }
 
   // Now set the error into the status so the layers above us could read it.
   sessions::StatusController* status = session->status_controller();
   status->set_sync_protocol_error(sync_protocol_error);
 
   // Inform the delegate of the error we got.
   session->delegate()->OnSyncProtocolError(session->TakeSnapshot());
 
   // Now do any special handling for the error type and decide on the return
   // value.
   switch (sync_protocol_error.error_type) {
     case browser_sync::UNKNOWN_ERROR:
       LOG(WARNING) << "Sync protocol out-of-date. The server is using a more "
                    << "recent version.";
-      return false;
-    case browser_sync::SYNC_SUCCESS:
+      break;
+    case browser_sync::OPERATION_SUCCESS:
       LogResponseProfilingData(*response);
-      return true;
+      break;
     case browser_sync::THROTTLED:
       LOG(WARNING) << "Client silenced by server.";
       session->delegate()->OnSilencedUntil(base::TimeTicks::Now() +
           GetThrottleDelay(*response));
-      return false;
-    case browser_sync::TRANSIENT_ERROR:
-      return false;
+      break;
     case browser_sync::MIGRATION_DONE:
       HandleMigrationDoneResponse(response, session);
-      return false;
-    case browser_sync::CLEAR_PENDING:
-    case browser_sync::NOT_MY_BIRTHDAY:
-      return false;
+      break;
+    case INVALID:
+      NOTREACHED();
     default:
-      NOTREACHED();
-      return false;
+      break;
   }
+
+  return sync_protocol_error;
 }
 
 // static
 bool SyncerProtoUtil::Compare(const syncable::Entry& local_entry,
                               const SyncEntity& server_entry) {
   const std::string name = NameFromSyncEntity(server_entry);
 
   CHECK(local_entry.Get(ID) == server_entry.id()) <<
       " SyncerProtoUtil::Compare precondition not met.";
   CHECK(server_entry.version() == local_entry.Get(BASE_VERSION)) <<
@@ skipping 112 matching lines @@
 std::string SyncerProtoUtil::ClientToServerResponseDebugString(
     const sync_pb::ClientToServerResponse& response) {
   // Add more handlers as needed.
   std::string output;
   if (response.has_get_updates())
     output.append(GetUpdatesResponseString(response.get_updates()));
   return output;
 }
 
 }  // namespace browser_sync