[Sync] Client should prompt for passphrase after server data was cleared by different client

After server data was cleared as part of transition to passphrase encryption
other clients should prompt for passphrase. To achieve this server will return
error different from NOT_MY_BIRTHDAY (CLIENT_DATA_OBSOLETE). On receiving this
error client should reset local sync data and restart sync.

In this change:
- Add CLIENT_DATA_OBSOLETE error type and RESET_LOCAL_SYNC_DATA client action
- When server returns CLIENT_DATA_OBSOLETE notify ProfileSyncService with
  actionable error
- In PSS shutdown sync engine, clearing sync data, then restart sync engine.

Additionally:
- I moved code that prepares SyncProtocolError into separate
  function. This allows to test that different responses from server result in
  correct error types and actions.
- I renamed couple functions mentioned in TODO.

BUG=490836
R=zea@chromium.org
TEST=This change is covered by unittests (sync_unit_tests and unit_tests).
  Behavior is not accessible in chrome yet.

Review URL: https://codereview.chromium.org/1385323005

Cr-Commit-Position: refs/heads/master@{#353630}

4 files changed, 139 insertions, 86 deletions

diff --git a/sync/engine/sync_scheduler_impl.cc b/sync/engine/sync_scheduler_impl.cc
index 3e03114..f40158b 100644
--- a/sync/engine/sync_scheduler_impl.cc
+++ b/sync/engine/sync_scheduler_impl.cc
@@ -53,6 +53,7 @@ bool ShouldRequestEarlyExit(const SyncProtocolError& error) {
     case TRANSIENT_ERROR:
       return false;
     case NOT_MY_BIRTHDAY:
+    case CLIENT_DATA_OBSOLETE:
     case CLEAR_PENDING:
     case DISABLED_BY_ADMIN:
     case USER_ROLLBACK:
diff --git a/sync/engine/syncer_proto_util.cc b/sync/engine/syncer_proto_util.cc
index 4befc97..cd77e1e 100644
--- a/sync/engine/syncer_proto_util.cc
+++ b/sync/engine/syncer_proto_util.cc
@@ -105,7 +105,7 @@ SyncerError ServerConnectionErrorAsSyncerError(
   }
 }
 
-SyncProtocolErrorType ConvertSyncProtocolErrorTypePBToLocalType(
+SyncProtocolErrorType PBErrorTypeToSyncProtocolErrorType(
     const sync_pb::SyncEnums::ErrorType& error_type) {
   switch (error_type) {
     case sync_pb::SyncEnums::SUCCESS:
@@ -126,6 +126,8 @@ SyncProtocolErrorType ConvertSyncProtocolErrorTypePBToLocalType(
       return USER_ROLLBACK;
     case sync_pb::SyncEnums::PARTIAL_FAILURE:
       return PARTIAL_FAILURE;
+    case sync_pb::SyncEnums::CLIENT_DATA_OBSOLETE:
+      return CLIENT_DATA_OBSOLETE;
     case sync_pb::SyncEnums::UNKNOWN:
       return UNKNOWN_ERROR;
     case sync_pb::SyncEnums::USER_NOT_ACTIVATED:
@@ -138,8 +140,7 @@ SyncProtocolErrorType ConvertSyncProtocolErrorTypePBToLocalType(
   }
 }
 
-ClientAction ConvertClientActionPBToLocalClientAction(
-    const sync_pb::SyncEnums::Action& action) {
+ClientAction PBActionToClientAction(const sync_pb::SyncEnums::Action& action) {
   switch (action) {
     case sync_pb::SyncEnums::UPGRADE_CLIENT:
       return UPGRADE_CLIENT;
@@ -159,6 +160,47 @@ ClientAction ConvertClientActionPBToLocalClientAction(
   }
 }
 
+// Returns true iff |message| is an initial GetUpdates request.
+bool IsVeryFirstGetUpdates(const ClientToServerMessage& message) {
+  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;
+}
+
+// Returns true iff |message| should contain a store birthday.
+bool IsBirthdayRequired(const ClientToServerMessage& message) {
+  if (message.has_clear_server_data())
+    return false;
+  if (message.has_commit())
+    return true;
+  if (message.has_get_updates())
+    return !IsVeryFirstGetUpdates(message);
+  NOTIMPLEMENTED();
+  return true;
+}
+
+SyncProtocolError ErrorCodeToSyncProtocolError(
+    const sync_pb::SyncEnums::ErrorType& error_type) {
+  SyncProtocolError error;
+  error.error_type = PBErrorTypeToSyncProtocolErrorType(error_type);
+  if (error_type == sync_pb::SyncEnums::CLEAR_PENDING ||
+      error_type == sync_pb::SyncEnums::NOT_MY_BIRTHDAY) {
+    error.action = DISABLE_SYNC_ON_CLIENT;
+  } else if (error_type == sync_pb::SyncEnums::CLIENT_DATA_OBSOLETE) {
+    error.action = RESET_LOCAL_SYNC_DATA;
+  } else if (error_type == sync_pb::SyncEnums::DISABLED_BY_ADMIN) {
+    error.action = STOP_SYNC_FOR_DISABLED_ACCOUNT;
+  } else if (error_type == sync_pb::SyncEnums::USER_ROLLBACK) {
+    error.action = DISABLE_SYNC_AND_ROLLBACK;
+  }  // There is no other action we can compute for legacy server.
+  return error;
+}
+
 }  // namespace
 
 ModelTypeSet GetTypesToMigrate(const ClientToServerResponse& response) {
@@ -175,15 +217,14 @@ ModelTypeSet GetTypesToMigrate(const ClientToServerResponse& response) {
   return to_migrate;
 }
 
-SyncProtocolError ConvertErrorPBToLocalType(
+SyncProtocolError ConvertErrorPBToSyncProtocolError(
     const sync_pb::ClientToServerResponse_Error& error) {
   SyncProtocolError sync_protocol_error;
-  sync_protocol_error.error_type = ConvertSyncProtocolErrorTypePBToLocalType(
-      error.error_type());
+  sync_protocol_error.error_type =
+      PBErrorTypeToSyncProtocolErrorType(error.error_type());
   sync_protocol_error.error_description = error.error_description();
   sync_protocol_error.url = error.url();
-  sync_protocol_error.action = ConvertClientActionPBToLocalClientAction(
-      error.action());
+  sync_protocol_error.action = PBActionToClientAction(error.action());
 
   if (error.error_data_type_ids_size() > 0) {
     // THROTTLED and PARTIAL_FAILURE are currently the only error codes
@@ -244,6 +285,36 @@ bool SyncerProtoUtil::IsSyncDisabledByAdmin(
 }
 
 // static
+SyncProtocolError SyncerProtoUtil::GetProtocolErrorFromResponse(
+    const sync_pb::ClientToServerResponse& response,
+    syncable::Directory* dir) {
+  SyncProtocolError sync_protocol_error;
+
+  // The DISABLED_BY_ADMIN error overrides other errors sent by the server.
+  if (IsSyncDisabledByAdmin(response)) {
+    sync_protocol_error.error_type = DISABLED_BY_ADMIN;
+    sync_protocol_error.action = STOP_SYNC_FOR_DISABLED_ACCOUNT;
+  } else if (!VerifyResponseBirthday(response, dir)) {
+    // If sync isn't disabled, first check for a birthday mismatch error.
+    if (response.error_code() == sync_pb::SyncEnums::CLIENT_DATA_OBSOLETE) {
+      // Server indicates that client needs to reset sync data.
+      sync_protocol_error.error_type = CLIENT_DATA_OBSOLETE;
+      sync_protocol_error.action = RESET_LOCAL_SYNC_DATA;
+    } else {
+      sync_protocol_error.error_type = NOT_MY_BIRTHDAY;
+      sync_protocol_error.action = DISABLE_SYNC_ON_CLIENT;
+    }
+  } else if (response.has_error()) {
+    // This is a new server. Just get the error from the protocol.
+    sync_protocol_error = ConvertErrorPBToSyncProtocolError(response.error());
+  } else {
+    // Legacy server implementation. Compute the error based on |error_code|.
+    sync_protocol_error = ErrorCodeToSyncProtocolError(response.error_code());
+  }
+  return sync_protocol_error;
+}
+
+// static
 void SyncerProtoUtil::AddRequestBirthday(syncable::Directory* dir,
                                          ClientToServerMessage* msg) {
   if (!dir->store_birthday().empty())
@@ -312,50 +383,6 @@ base::TimeDelta SyncerProtoUtil::GetThrottleDelay(
   return throttle_delay;
 }
 
-namespace {
-
-// Returns true iff |message| is an initial GetUpdates request.
-bool IsVeryFirstGetUpdates(const ClientToServerMessage& message) {
-  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;
-}
-
-// Returns true iff |message| should contain a store birthday.
-bool IsBirthdayRequired(const ClientToServerMessage& message) {
-  if (message.has_clear_server_data())
-    return false;
-  if (message.has_commit())
-    return true;
-  if (message.has_get_updates())
-    return !IsVeryFirstGetUpdates(message);
-  NOTIMPLEMENTED();
-  return true;
-}
-
-// TODO(lipalani) : Rename these function names as per the CR for issue 7740067.
-SyncProtocolError ConvertLegacyErrorCodeToNewError(
-    const sync_pb::SyncEnums::ErrorType& error_type) {
-  SyncProtocolError error;
-  error.error_type = ConvertSyncProtocolErrorTypePBToLocalType(error_type);
-  if (error_type == sync_pb::SyncEnums::CLEAR_PENDING ||
-      error_type == sync_pb::SyncEnums::NOT_MY_BIRTHDAY) {
-    error.action = DISABLE_SYNC_ON_CLIENT;
-  } else if (error_type == sync_pb::SyncEnums::DISABLED_BY_ADMIN) {
-    error.action = STOP_SYNC_FOR_DISABLED_ACCOUNT;
-  } else if (error_type == sync_pb::SyncEnums::USER_ROLLBACK) {
-    error.action = DISABLE_SYNC_AND_ROLLBACK;
-  } // There is no other action we can compute for legacy server.
-  return error;
-}
-
-}  // namespace
-
 // static
 SyncerError SyncerProtoUtil::PostClientToServerMessage(
     ClientToServerMessage* msg,
@@ -395,24 +422,8 @@ SyncerError SyncerProtoUtil::PostClientToServerMessage(
   // Persist a bag of chips if it has been sent by the server.
   PersistBagOfChips(dir, *response);
 
-  SyncProtocolError sync_protocol_error;
-
-  // The DISABLED_BY_ADMIN error overrides other errors sent by the server.
-  if (IsSyncDisabledByAdmin(*response)) {
-    sync_protocol_error.error_type = DISABLED_BY_ADMIN;
-    sync_protocol_error.action = STOP_SYNC_FOR_DISABLED_ACCOUNT;
-  } else if (!VerifyResponseBirthday(*response, dir)) {
-    // If sync isn't disabled, first check for a birthday mismatch error.
-    sync_protocol_error.error_type = NOT_MY_BIRTHDAY;
-    sync_protocol_error.action = 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());
-  }
+  SyncProtocolError sync_protocol_error =
+      GetProtocolErrorFromResponse(*response, dir);
 
   // Inform the delegate of the error we got.
   session->delegate()->OnSyncProtocolError(sync_protocol_error);
@@ -517,6 +528,8 @@ SyncerError SyncerProtoUtil::PostClientToServerMessage(
         *partial_failure_data_types = sync_protocol_error.error_data_types;
       }
       return SERVER_RETURN_PARTIAL_FAILURE;
+    case CLIENT_DATA_OBSOLETE:
+      return SERVER_RETURN_CLIENT_DATA_OBSOLETE;
     default:
       NOTREACHED();
       return UNSET;
diff --git a/sync/engine/syncer_proto_util.h b/sync/engine/syncer_proto_util.h
index 2a7bd34..ae59cdf 100644
--- a/sync/engine/syncer_proto_util.h
+++ b/sync/engine/syncer_proto_util.h
@@ -42,7 +42,7 @@ SYNC_EXPORT_PRIVATE ModelTypeSet GetTypesToMigrate(
     const sync_pb::ClientToServerResponse& response);
 
 // Builds a SyncProtocolError from the data in |error|.
-SYNC_EXPORT_PRIVATE SyncProtocolError ConvertErrorPBToLocalType(
+SYNC_EXPORT_PRIVATE SyncProtocolError ConvertErrorPBToSyncProtocolError(
     const sync_pb::ClientToServerResponse_Error& error);
 
 class SYNC_EXPORT_PRIVATE SyncerProtoUtil {
@@ -110,6 +110,14 @@ class SYNC_EXPORT_PRIVATE SyncerProtoUtil {
 
   // Helper functions for PostClientToServerMessage.
 
+  // Analyzes error fields and store birthday in response message, compares
+  // store birthday with value in directory and returns corresponding
+  // SyncProtocolError. If needed updates store birthday in directory.
+  // This function makes it easier to test error handling.
+  static SyncProtocolError GetProtocolErrorFromResponse(
+      const sync_pb::ClientToServerResponse& response,
+      syncable::Directory* dir);
+
   // Verifies the store birthday, alerting/resetting as appropriate if there's a
   // mismatch. Return false if the syncer should be stuck.
   static bool VerifyResponseBirthday(
@@ -133,8 +141,6 @@ class SYNC_EXPORT_PRIVATE SyncerProtoUtil {
   friend class SyncerProtoUtilTest;
   FRIEND_TEST_ALL_PREFIXES(SyncerProtoUtilTest, AddRequestBirthday);
   FRIEND_TEST_ALL_PREFIXES(SyncerProtoUtilTest, PostAndProcessHeaders);
-  FRIEND_TEST_ALL_PREFIXES(SyncerProtoUtilTest, VerifyDisabledByAdmin);
-  FRIEND_TEST_ALL_PREFIXES(SyncerProtoUtilTest, VerifyResponseBirthday);
   FRIEND_TEST_ALL_PREFIXES(SyncerProtoUtilTest, HandleThrottlingNoDatatypes);
   FRIEND_TEST_ALL_PREFIXES(SyncerProtoUtilTest, HandleThrottlingWithDatatypes);
 
diff --git a/sync/engine/syncer_proto_util_unittest.cc b/sync/engine/syncer_proto_util_unittest.cc
index 35cc2c7..0bc745f 100644
--- a/sync/engine/syncer_proto_util_unittest.cc
+++ b/sync/engine/syncer_proto_util_unittest.cc
@@ -60,9 +60,9 @@ TEST(SyncerProtoUtil, GetTypesToMigrate) {
 }
 
 // Builds a ClientToServerResponse_Error with some error data type
-// ids, including invalid ones.  ConvertErrorPBToLocalType() should
+// ids, including invalid ones.  ConvertErrorPBToSyncProtocolError() should
 // return a SyncProtocolError with only the valid model types.
-TEST(SyncerProtoUtil, ConvertErrorPBToLocalType) {
+TEST(SyncerProtoUtil, ConvertErrorPBToSyncProtocolError) {
   sync_pb::ClientToServerResponse_Error error_pb;
   error_pb.set_error_type(sync_pb::SyncEnums::THROTTLED);
   error_pb.add_error_data_type_ids(
@@ -70,7 +70,7 @@ TEST(SyncerProtoUtil, ConvertErrorPBToLocalType) {
   error_pb.add_error_data_type_ids(
       GetSpecificsFieldNumberFromModelType(HISTORY_DELETE_DIRECTIVES));
   error_pb.add_error_data_type_ids(-1);
-  SyncProtocolError error = ConvertErrorPBToLocalType(error_pb);
+  SyncProtocolError error = ConvertErrorPBToSyncProtocolError(error_pb);
   EXPECT_TRUE(
       error.error_data_types.Equals(
           ModelTypeSet(BOOKMARKS, HISTORY_DELETE_DIRECTIVES)));
@@ -136,6 +136,14 @@ class SyncerProtoUtilTest : public testing::Test {
     return dir_maker_.directory();
   }
 
+  // Helper function to call GetProtocolErrorFromResponse. Allows not adding
+  // individual tests as friends to SyncerProtoUtil.
+  static SyncProtocolError CallGetProtocolErrorFromResponse(
+      const sync_pb::ClientToServerResponse& response,
+      syncable::Directory* directory) {
+    return SyncerProtoUtil::GetProtocolErrorFromResponse(response, directory);
+  }
+
  protected:
   base::MessageLoop message_loop_;
   TestDirectorySetterUpper dir_maker_;
@@ -145,38 +153,63 @@ TEST_F(SyncerProtoUtilTest, VerifyResponseBirthday) {
   // Both sides empty
   EXPECT_TRUE(directory()->store_birthday().empty());
   sync_pb::ClientToServerResponse response;
-  EXPECT_FALSE(SyncerProtoUtil::VerifyResponseBirthday(response, directory()));
+  SyncProtocolError sync_protocol_error;
+  response.set_error_code(sync_pb::SyncEnums::SUCCESS);
+
+  sync_protocol_error = CallGetProtocolErrorFromResponse(response, directory());
+  EXPECT_EQ(NOT_MY_BIRTHDAY, sync_protocol_error.error_type);
+  EXPECT_EQ(DISABLE_SYNC_ON_CLIENT, sync_protocol_error.action);
 
   // Remote set, local empty
   response.set_store_birthday("flan");
-  EXPECT_TRUE(SyncerProtoUtil::VerifyResponseBirthday(response, directory()));
+  sync_protocol_error = CallGetProtocolErrorFromResponse(response, directory());
+  EXPECT_EQ(SYNC_SUCCESS, sync_protocol_error.error_type);
+  EXPECT_EQ(UNKNOWN_ACTION, sync_protocol_error.action);
   EXPECT_EQ(directory()->store_birthday(), "flan");
 
   // Remote empty, local set.
   response.clear_store_birthday();
-  EXPECT_TRUE(SyncerProtoUtil::VerifyResponseBirthday(response, directory()));
+  sync_protocol_error = CallGetProtocolErrorFromResponse(response, directory());
+  EXPECT_EQ(SYNC_SUCCESS, sync_protocol_error.error_type);
+  EXPECT_EQ(UNKNOWN_ACTION, sync_protocol_error.action);
   EXPECT_EQ(directory()->store_birthday(), "flan");
 
   // Doesn't match
   response.set_store_birthday("meat");
-  EXPECT_FALSE(SyncerProtoUtil::VerifyResponseBirthday(response, directory()));
-
-  response.set_error_code(sync_pb::SyncEnums::CLEAR_PENDING);
-  EXPECT_FALSE(SyncerProtoUtil::VerifyResponseBirthday(response, directory()));
+  response.set_error_code(sync_pb::SyncEnums::NOT_MY_BIRTHDAY);
+  sync_protocol_error = CallGetProtocolErrorFromResponse(response, directory());
+  EXPECT_EQ(NOT_MY_BIRTHDAY, sync_protocol_error.error_type);
+  EXPECT_EQ(DISABLE_SYNC_ON_CLIENT, sync_protocol_error.action);
+
+  // Doesn't match. CLIENT_DATA_OBSOLETE error is set.
+  response.set_error_code(sync_pb::SyncEnums::CLIENT_DATA_OBSOLETE);
+  sync_protocol_error = CallGetProtocolErrorFromResponse(response, directory());
+  EXPECT_EQ(CLIENT_DATA_OBSOLETE, sync_protocol_error.error_type);
+  EXPECT_EQ(RESET_LOCAL_SYNC_DATA, sync_protocol_error.action);
 }
 
 TEST_F(SyncerProtoUtilTest, VerifyDisabledByAdmin) {
   // No error code
   sync_pb::ClientToServerResponse response;
-  EXPECT_FALSE(SyncerProtoUtil::IsSyncDisabledByAdmin(response));
+  SyncProtocolError sync_protocol_error;
+  directory()->set_store_birthday("flan");
+  response.set_error_code(sync_pb::SyncEnums::SUCCESS);
+
+  sync_protocol_error = CallGetProtocolErrorFromResponse(response, directory());
+  EXPECT_EQ(SYNC_SUCCESS, sync_protocol_error.error_type);
+  EXPECT_EQ(UNKNOWN_ACTION, sync_protocol_error.action);
 
   // Has error code, but not disabled
   response.set_error_code(sync_pb::SyncEnums::NOT_MY_BIRTHDAY);
-  EXPECT_FALSE(SyncerProtoUtil::IsSyncDisabledByAdmin(response));
+  sync_protocol_error = CallGetProtocolErrorFromResponse(response, directory());
+  EXPECT_EQ(NOT_MY_BIRTHDAY, sync_protocol_error.error_type);
+  EXPECT_NE(UNKNOWN_ACTION, sync_protocol_error.action);
 
   // Has error code, and is disabled by admin
   response.set_error_code(sync_pb::SyncEnums::DISABLED_BY_ADMIN);
-  EXPECT_TRUE(SyncerProtoUtil::IsSyncDisabledByAdmin(response));
+  sync_protocol_error = CallGetProtocolErrorFromResponse(response, directory());
+  EXPECT_EQ(DISABLED_BY_ADMIN, sync_protocol_error.error_type);
+  EXPECT_EQ(STOP_SYNC_FOR_DISABLED_ACCOUNT, sync_protocol_error.action);
 }
 
 TEST_F(SyncerProtoUtilTest, AddRequestBirthday) {