Server directed error handling backend code.

chrome/browser/sync/engine/syncer_proto_util.cc

Index: chrome/browser/sync/engine/syncer_proto_util.cc
diff --git a/chrome/browser/sync/engine/syncer_proto_util.cc b/chrome/browser/sync/engine/syncer_proto_util.cc
index 61f64fad5a9f6272793c20f521563d604aee6d78..7e87ef76d9729179e42927476cb27ca659430497 100644
--- a/chrome/browser/sync/engine/syncer_proto_util.cc
+++ b/chrome/browser/sync/engine/syncer_proto_util.cc
@@ -100,6 +100,9 @@ bool SyncerProtoUtil::VerifyResponseBirthday(syncable::Directory* dir,
 
   std::string local_birthday = dir->store_birthday();
 
+  // TODO(lipalani) : Remove this check here. When the new implementation
+  // becomes the default this check should go away. This is handled by the
+  // switch case in the new implementation.
   if (response->error_code() == ClientToServerResponse::CLEAR_PENDING ||
       response->error_code() == ClientToServerResponse::NOT_MY_BIRTHDAY) {
     // Birthday verification failures result in stopping sync and deleting
@@ -211,8 +214,90 @@ bool IsVeryFirstGetUpdates(const ClientToServerMessage& message) {
   return true;
 }
 
+sync_api::SyncErrorType ConvertSyncErrorTypePBToLocalType(
+    const sync_pb::ClientToServerResponse::ErrorType& error_type) {
+  switch (error_type) {
+    case ClientToServerResponse::SUCCESS:
+      return sync_api::SUCCESS;
+    case ClientToServerResponse::NOT_MY_BIRTHDAY:
+      return sync_api::NOT_MY_BIRTHDAY;
+    case ClientToServerResponse::THROTTLED:
+      return sync_api::THROTTLED;
+    case ClientToServerResponse::CLEAR_PENDING:
+      return sync_api::CLEAR_PENDING;
+    case ClientToServerResponse::TRANSIENT_ERROR:
+      return sync_api::TRANSIENT_ERROR;
+    case ClientToServerResponse::MIGRATION_DONE:
+      return sync_api::MIGRATION_DONE;
+    case ClientToServerResponse::UNKNOWN:
+      return sync_api::UNKNOWN_ERROR;
+    case ClientToServerResponse::USER_NOT_ACTIVATED:
+    case ClientToServerResponse::AUTH_INVALID:
+    case ClientToServerResponse::ACCESS_DENIED:
+      return sync_api::INVALID_CREDENTIAL;
+    default:
+      NOTREACHED();
+      return sync_api::UNKNOWN_ERROR;
+  }
+}
+
+sync_api::ClientAction ConvertClientActionPBToLocalClientAction(
+    const sync_pb::ClientToServerResponse::Error::Action& action) {
+  switch (action) {
+    case ClientToServerResponse::Error::UPGRADE_CLIENT:
+      return sync_api::UPGRADE_CLIENT;
+    case ClientToServerResponse::Error::CLEAR_USER_DATA_AND_RESYNC:
+      return sync_api::CLEAR_USER_DATA_AND_RESYNC;
+    case ClientToServerResponse::Error::ENABLE_SYNC_ON_ACCOUNT:
+      return sync_api::ENABLE_SYNC_ON_ACCOUNT;
+    case ClientToServerResponse::Error::STOP_AND_RESTART_SYNC:
+      return sync_api::STOP_AND_RESTART_SYNC;
+    case ClientToServerResponse::Error::DISABLE_SYNC_ON_CLIENT:
+      return sync_api::DISABLE_SYNC_ON_CLIENT;
+    case ClientToServerResponse::Error::UNKNOWN_ACTION:
+      return sync_api::UNKNOWN_ACTION;
+    default:
+      NOTREACHED();
+      return sync_api::UNKNOWN_ACTION;
+  }
+}
+
+sync_api::SyncError ConvertErrorPBToLocalType(
+    const sync_pb::ClientToServerResponse::Error& error) {
+  sync_api::SyncError sync_error;
+  sync_error.error_type = ConvertSyncErrorTypePBToLocalType(error.error_type());
+  sync_error.error_description = error.error_description();
+  sync_error.url = error.url();
+  sync_error.action = ConvertClientActionPBToLocalClientAction(error.action());
+
+  return sync_error;
+}
+
 }  // namespace
 
+bool ShouldRequestEarlyExit(const sync_api::SyncError& error) {

[tim (not reviewing), 2011/08/22 15:14:57]

Having this method here is a layering violation in my mind.  I feel like this is
something the SyncSession::Delegate should figure out. 

[lipalani1, 2011/08/22 21:43:53]

hmm... This is how it is currently implemented and I think it is right as well.
In the current impl if we get a birthday error SyncerProtoUtil figures that out
and asks the delegate to stop syncing. Same for throttled. I think that is the
correct layering as well.
On 2011/08/22 15:14:57, timsteele wrote:

[tim (not reviewing), 2011/08/23 14:17:07]

That's because we used an existing error ("birthday error" in this case) for a
new feature "stop syncing permanently", which wasn't ideal. And moreover, "stop
syncing permanently" in that case means a user-level "stop sync" operation --
not a low level thing like stopping the loop in sync scheduler.  Nothing should
know about that except the scheduler.

And it doesn't work that way for throttled, because we don't stop syncing. We
just tell the delegate "server said not to sync again until x" and the scheduler
figures out what to do internally. 

SyncerProtoUtil should be as uncoupled as possible from any implementation
details like the fact that the scheduler has to tell the Syncer to exit a loop;
that's why the delegate interface exists. 

+  switch (error.error_type) {
+    case sync_api::SUCCESS:
+    case sync_api::MIGRATION_DONE:
+      return false;
+    case sync_api::NOT_MY_BIRTHDAY:
+    case sync_api::THROTTLED:

[tim (not reviewing), 2011/08/22 15:14:57]

We shouldn't be requesting early exit on throttled...

[lipalani1, 2011/08/22 21:43:53]

Done.

+    case sync_api::CLEAR_PENDING:
+    case sync_api::TRANSIENT_ERROR:

[tim (not reviewing), 2011/08/22 15:14:57]

We definitely should not be requesting early exit on transient error.  In
general too, let's try to minimize the changes in behavior with the current
impl. 

[lipalani1, 2011/08/22 21:43:53]

Agree. it has already caused grievance for me :) With throttled and temporary
error the server might not send us any action.(there is nothing the user can
do). So the PSS would receive neither the sync_cycle_ended notification nor the
sync_error notification. if this is a first time sync we would end up with
infinite spin!
I am rewriting this. 
On 2011/08/22 15:14:57, timsteele wrote:
Done.

+    case sync_api::INVALID_CREDENTIAL:

[tim (not reviewing), 2011/08/22 15:14:57]

I think (as we discussed in review meetings) invalid credentials is a separate
case?  I'm not sure what the difference between this, an RC_UNAUTHORIZED server
connection manager code, or browser_sync::SYNC_AUTH_ERROR.  Two was too many to
keep track of, I'd rather not add a 3rd auth state.

[lipalani1, 2011/08/22 21:43:53]

It is a seperate case. The event for 401 error is handled by
|PostAndprocessHeader|. Ideally we should not have the invalid auth error in the
protocol but only in the http header. But just in case server sends the auth
error we need to handle it.(currently as per the client code if the server ever
sends the error in protobuf we will hit a notreached).
On 2011/08/22 15:14:57, timsteele wrote:

[tim (not reviewing), 2011/08/23 14:17:07]

I agree with what you say here, but I still don't see why we are now adding
another error code if you agree we already have too many :)

+      return true;
+    // Make the default a NOTREACHED. So if a new error is introduced we
+    // think about its expected functionality.
+    default:
+      NOTREACHED();
+      return false;
+  }
+}
+
+bool IsActionableError(const sync_api::SyncError& error) {
+  return (error.action != sync_api::UNKNOWN_ACTION);
+}
+
 // static
 bool SyncerProtoUtil::PostClientToServerMessage(
     const ClientToServerMessage& msg,
@@ -234,6 +319,60 @@ bool SyncerProtoUtil::PostClientToServerMessage(
                              msg, response))
     return false;
 
+  if (response->has_error()) {
+    // We are talking to a server that is capable of sending the |error| tag.
+    sync_api::SyncError sync_error = ConvertErrorPBToLocalType(
+        response->error());
+    sessions::StatusController* status = session->status_controller();
+    status->set_sync_error(sync_error);
+
+    // Birthday mismatch overrides any error that is sent by the server.
+    if (!VerifyResponseBirthday(dir, response)) {
+       sync_error.error_type = sync_api::NOT_MY_BIRTHDAY;
+       sync_error.action = sync_api::DISABLE_SYNC_ON_CLIENT;
+    }
+
+    // Inform the syncer if we need to exit early.
+    if (ShouldRequestEarlyExit(sync_error)) {
+      session->status_controller()->set_syncer_stuck(true);
+      session->delegate()->OnRequestEarlyExit();
+    }
+
+    // Inform the syncer if there is an actionable error.
+    if (IsActionableError(sync_error)) {
+      session->delegate()->OnActionableError(session);
+    }
+
+    // Now do any special handling for the error type and decide on the return
+    // value.
+    switch (sync_error.error_type) {
+      case sync_api::UNKNOWN_ERROR:
+        LOG(WARNING) << "Sync protocol out-of-date. The server is using a more "
+                     << "recent version.";
+        return false;
+      case sync_api::SUCCESS:
+        LogResponseProfilingData(*response);
+        return true;
+      case sync_api::THROTTLED:
+        LOG(WARNING) << "Client silenced by server.";
+        session->delegate()->OnSilencedUntil(base::TimeTicks::Now() +
+            GetThrottleDelay(*response));
+        return false;
+      case sync_api::TRANSIENT_ERROR:
+        return false;
+      case sync_api::MIGRATION_DONE:
+        HandleMigrationDoneResponse(response, session);
+        return false;
+      default:
+        NOTREACHED();
+        return false;
+    }
+  }
+
+  // TODO(lipalani): Plug this legacy implementation to the new error framework.
+  // New implementation code would have returned before by now. This is waiting
+  // on the frontend code being implemented. Otherwise ripping this would break
+  // sync.
   if (!VerifyResponseBirthday(dir, response)) {
     session->status_controller()->set_syncer_stuck(true);
     session->delegate()->OnShouldStopSyncingPermanently();