[Sync] Fix crash in GCP when adding encrypted entry that already exists.

components/sync_driver/generic_change_processor.cc

Index: components/sync_driver/generic_change_processor.cc
diff --git a/components/sync_driver/generic_change_processor.cc b/components/sync_driver/generic_change_processor.cc
index 2df982022d69edcf38f818ec0f6d81ac73548e6b..4cc4269d61d64fa76872780f3fe60a00fdae0e52 100644
--- a/components/sync_driver/generic_change_processor.cc
+++ b/components/sync_driver/generic_change_processor.cc
@@ -537,6 +537,9 @@ syncer::SyncError GenericChangeProcessor::HandleActionAdd(
         LOG(ERROR) << "Create: Bad predecessor.";
         return error;
       }
+      case syncer::WriteNode::INIT_FAILED_DECRYPT_EXISTING_ENTRY: {

[Nicolas Zea, 2016/04/25 21:40:10]

nit: no need for curly braces around return

+        return HandleDecryptionError(type_str, trans, sync_node);
+      }
       default: {
         syncer::SyncError error;
         error.Reset(FROM_HERE, error_prefix + "unknown error", type_);
@@ -595,54 +598,7 @@ syncer::SyncError GenericChangeProcessor::HandleActionUpdate(
       LOG(ERROR) << "Update: deleted entry.";
       return error;
     } else {
-      syncer::Cryptographer* crypto = trans.GetCryptographer();
-      syncer::ModelTypeSet encrypted_types(trans.GetEncryptedTypes());
-      const sync_pb::EntitySpecifics& specifics =
-          sync_node->GetEntitySpecifics();
-      CHECK(specifics.has_encrypted());
-      const bool can_decrypt = crypto->CanDecrypt(specifics.encrypted());
-      const bool agreement = encrypted_types.Has(type_);
-      if (!agreement && !can_decrypt) {
-        syncer::SyncError error;
-        error.Reset(FROM_HERE,
-                    "Failed to load encrypted entry, missing key and "
-                    "nigori mismatch for " +
-                        type_str + ".",
-                    type_);
-        error_handler()->OnSingleDataTypeUnrecoverableError(error);
-        LOG(ERROR) << "Update: encr case 1.";
-        return error;
-      } else if (agreement && can_decrypt) {
-        syncer::SyncError error;
-        error.Reset(FROM_HERE,
-                    "Failed to load encrypted entry, we have the key "
-                    "and the nigori matches (?!) for " +
-                        type_str + ".",
-                    type_);
-        error_handler()->OnSingleDataTypeUnrecoverableError(error);
-        LOG(ERROR) << "Update: encr case 2.";
-        return error;
-      } else if (agreement) {
-        syncer::SyncError error;
-        error.Reset(FROM_HERE,
-                    "Failed to load encrypted entry, missing key and "
-                    "the nigori matches for " +
-                        type_str + ".",
-                    type_);
-        error_handler()->OnSingleDataTypeUnrecoverableError(error);
-        LOG(ERROR) << "Update: encr case 3.";
-        return error;
-      } else {
-        syncer::SyncError error;
-        error.Reset(FROM_HERE,
-                    "Failed to load encrypted entry, we have the key"
-                    "(?!) and nigori mismatch for " +
-                        type_str + ".",
-                    type_);
-        error_handler()->OnSingleDataTypeUnrecoverableError(error);
-        LOG(ERROR) << "Update: encr case 4.";
-        return error;
-      }
+      return HandleDecryptionError(type_str, trans, sync_node);
     }
   }
 
@@ -663,6 +619,59 @@ syncer::SyncError GenericChangeProcessor::HandleActionUpdate(
   return syncer::SyncError();
 }
 
+syncer::SyncError GenericChangeProcessor::HandleDecryptionError(
+    const std::string& type_str,
+    const syncer::WriteTransaction& trans,
+    syncer::WriteNode* sync_node) {
+  syncer::Cryptographer* crypto = trans.GetCryptographer();
+  syncer::ModelTypeSet encrypted_types(trans.GetEncryptedTypes());
+  const sync_pb::EntitySpecifics& specifics = sync_node->GetEntitySpecifics();
+  CHECK(specifics.has_encrypted());
+  const bool can_decrypt = crypto->CanDecrypt(specifics.encrypted());
+  const bool agreement = encrypted_types.Has(type_);
+  if (!agreement && !can_decrypt) {

[Nicolas Zea, 2016/04/25 21:40:10]

Honestly, I think we don't need this level of verbosity anymore. These were
added at a time when we thought there was something broken with decryption. I
think your fix probably resolves that, and we can just trigger a generic error
here.

[pavely, 2016/04/26 00:33:25]

I removed this function and added simpler fragments in HandleAdd/HandleUpdate.

+    syncer::SyncError error;
+    error.Reset(FROM_HERE,
+                "Failed to load encrypted entry, missing key and "
+                "nigori mismatch for " +
+                    type_str + ".",
+                type_);
+    error_handler()->OnSingleDataTypeUnrecoverableError(error);
+    LOG(ERROR) << "Update: encr case 1.";
+    return error;
+  } else if (agreement && can_decrypt) {
+    syncer::SyncError error;
+    error.Reset(FROM_HERE,
+                "Failed to load encrypted entry, we have the key "
+                "and the nigori matches (?!) for " +
+                    type_str + ".",
+                type_);
+    error_handler()->OnSingleDataTypeUnrecoverableError(error);
+    LOG(ERROR) << "Update: encr case 2.";
+    return error;
+  } else if (agreement) {
+    syncer::SyncError error;
+    error.Reset(FROM_HERE,
+                "Failed to load encrypted entry, missing key and "
+                "the nigori matches for " +
+                    type_str + ".",
+                type_);
+    error_handler()->OnSingleDataTypeUnrecoverableError(error);
+    LOG(ERROR) << "Update: encr case 3.";
+    return error;
+  } else {
+    syncer::SyncError error;
+    error.Reset(FROM_HERE,
+                "Failed to load encrypted entry, we have the key"
+                "(?!) and nigori mismatch for " +
+                    type_str + ".",
+                type_);
+    error_handler()->OnSingleDataTypeUnrecoverableError(error);
+    LOG(ERROR) << "Update: encr case 4.";
+    return error;
+  }
+}
+
 bool GenericChangeProcessor::SyncModelHasUserCreatedNodes(bool* has_nodes) {
   DCHECK(CalledOnValidThread());
   DCHECK(has_nodes);