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

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

When model type's syncable service adds entry, GenericChangeProcessor
first looks up if entry with given client tag hash already exists.
WriteNode::InitUniqueByCreationImpl looks up entry, but doesn't decrypt
if it is found. BaseNode::GetEntitySpecifics() expects cached decrypted
entry's specifics for encrytped entry which causes DCHECK.

The fix is to call DecryptIfNecessary when existing entry is found in
WriteNode::InitUniqueByCreationImpl.

BUG=601257
R=zea@chromium.org

Committed: https://crrev.com/194ec8ddbc3a77d4af0b7455cd9b85305a134e1e
Cr-Commit-Position: refs/heads/master@{#389801}

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

LGTM with two nits

https://codereview.chromium.org/1915983003/diff/1/components/sync_driver/gene...
File components/sync_driver/generic_change_processor.cc (right):

https://codereview.chromium.org/1915983003/diff/1/components/sync_driver/gene...
components/sync_driver/generic_change_processor.cc:540: case
syncer::WriteNode::INIT_FAILED_DECRYPT_EXISTING_ENTRY: {
nit: no need for curly braces around return

https://codereview.chromium.org/1915983003/diff/1/components/sync_driver/gene...
components/sync_driver/generic_change_processor.cc:632: if (!agreement &&
!can_decrypt) {
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:29:30]

Patchset #2 (id:20001) has been deleted

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

https://codereview.chromium.org/1915983003/diff/1/components/sync_driver/gene...
File components/sync_driver/generic_change_processor.cc (right):

https://codereview.chromium.org/1915983003/diff/1/components/sync_driver/gene...
components/sync_driver/generic_change_processor.cc:632: if (!agreement &&
!can_decrypt) {
On 2016/04/25 21:40:10, Nicolas Zea wrote:
> 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.

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

[pavely, 2016-04-26 16:22:24]

The CQ bit was checked by pavely@chromium.org

[pavely, 2016-04-26 16:22:25]

The patchset sent to the CQ was uploaded after l-g-t-m from zea@chromium.org
Link to the patchset: https://codereview.chromium.org/1915983003/#ps40001
(title: "Removed HandleDecryptError()")

[commit-bot: I haz the power, 2016-04-26 16:22:51]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1915983003/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1915983003/40001

[commit-bot: I haz the power, 2016-04-26 16:27:17]

Committed patchset #2 (id:40001)

[commit-bot: I haz the power, 2016-04-26 16:29:00]

Description was changed from

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

When model type's syncable service adds entry, GenericChangeProcessor
first looks up if entry with given client tag hash already exists.
WriteNode::InitUniqueByCreationImpl looks up entry, but doesn't decrypt
if it is found. BaseNode::GetEntitySpecifics() expects cached decrypted
entry's specifics for encrytped entry which causes DCHECK.

The fix is to call DecryptIfNecessary when existing entry is found in
WriteNode::InitUniqueByCreationImpl.

BUG=601257
R=zea@chromium.org
==========

to

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

When model type's syncable service adds entry, GenericChangeProcessor
first looks up if entry with given client tag hash already exists.
WriteNode::InitUniqueByCreationImpl looks up entry, but doesn't decrypt
if it is found. BaseNode::GetEntitySpecifics() expects cached decrypted
entry's specifics for encrytped entry which causes DCHECK.

The fix is to call DecryptIfNecessary when existing entry is found in
WriteNode::InitUniqueByCreationImpl.

BUG=601257
R=zea@chromium.org

Committed: https://crrev.com/194ec8ddbc3a77d4af0b7455cd9b85305a134e1e
Cr-Commit-Position: refs/heads/master@{#389801}
==========

[commit-bot: I haz the power, 2016-04-26 16:29:01]

Patchset 2 (id:??) landed as
https://crrev.com/194ec8ddbc3a77d4af0b7455cd9b85305a134e1e
Cr-Commit-Position: refs/heads/master@{#389801}