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

components/sync_driver/generic_change_processor.cc

 // Copyright 2014 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 "components/sync_driver/generic_change_processor.h"
 
 #include <stddef.h>
 #include <algorithm>
 #include <string>
 #include <utility>
@@ skipping 519 matching lines @@
         return error;
       }
       case syncer::WriteNode::INIT_FAILED_SET_PREDECESSOR: {
         syncer::SyncError error;
         error.Reset(
             FROM_HERE, error_prefix + "failed to set predecessor", type_);
         error_handler()->OnSingleDataTypeUnrecoverableError(error);
         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_);
         error_handler()->OnSingleDataTypeUnrecoverableError(error);
         LOG(ERROR) << "Create: Unknown error.";
         return error;
       }
     }
   }
   sync_node->SetTitle(change.sync_data().GetTitle());
@@ skipping 38 matching lines @@
       error_handler()->OnSingleDataTypeUnrecoverableError(error);
       LOG(ERROR) << "Update: bad entry.";
       return error;
     } else if (result == syncer::BaseNode::INIT_FAILED_ENTRY_IS_DEL) {
       syncer::SyncError error;
       error.Reset(FROM_HERE, error_prefix + "deleted entry", type_);
       error_handler()->OnSingleDataTypeUnrecoverableError(error);
       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);
     }
   }
 
   sync_node->SetTitle(change.sync_data().GetTitle());
   SetNodeSpecifics(sync_data_local.GetSpecifics(), sync_node);
   syncer::AttachmentIdList attachment_ids = sync_data_local.GetAttachmentIds();
   SetAttachmentMetadata(attachment_ids, sync_node);
 
   // Return any newly added attachments.
   new_attachments->insert(attachment_ids.begin(), attachment_ids.end());
 
   if (merge_result_.get()) {
     merge_result_->set_num_items_modified(merge_result_->num_items_modified() +
                                           1);
   }
   // TODO(sync): Support updating other parts of the sync node (title,
   // successor, parent, etc.).
   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);
   std::string type_name = syncer::ModelTypeToString(type_);
   std::string err_str = "Server did not create the top-level " + type_name +
       " node. We might be running against an out-of-date server.";
   *has_nodes = false;
   syncer::ReadTransaction trans(FROM_HERE, share_handle());
   syncer::ReadNode type_root_node(&trans);
   if (type_root_node.InitTypeRoot(type_) != syncer::BaseNode::INIT_OK) {
@@ skipping 36 matching lines @@
   }
 }
 
 scoped_ptr<syncer::AttachmentService>
 GenericChangeProcessor::GetAttachmentService() const {
   return scoped_ptr<syncer::AttachmentService>(
       new syncer::AttachmentServiceProxy(attachment_service_proxy_));
 }
 
 }  // namespace sync_driver