[Sync] Don't crash for encryption errors

[Sync] Don't crash for encryption errors

Encryption errors can arise due to OS issues in some cases or simply directory
corruption. Rather than crashing trigger an unrecoverable error.

BUG=123223
Committed: https://crrev.com/dea3a922a415ef050c3a5fb0d2d19571fe578413
Cr-Commit-Position: refs/heads/master@{#333644}

[Nicolas Zea, 2015-06-08 21:29:43]

zea@chromium.org changed reviewers:
+ stanisc@chromium.org

[Nicolas Zea, 2015-06-08 21:29:43]

+Stan, PTAL.

[stanisc, 2015-06-09 00:04:03]

https://codereview.chromium.org/1161463005/diff/40001/sync/internal_api/sync_...
File sync/internal_api/sync_encryption_handler_impl.cc (right):

https://codereview.chromium.org/1161463005/diff/40001/sync/internal_api/sync_...
sync/internal_api/sync_encryption_handler_impl.cc:837: break;
It isn't clear how this could be possible. The ID lookup would fail only if
ParentChildIndex was inconsistent with the ID->Entry index. I'd say NOTREACHED()
is appropriate in this case.

https://codereview.chromium.org/1161463005/diff/40001/sync/internal_api/sync_...
File sync/internal_api/sync_manager_impl_unittest.cc (right):

https://codereview.chromium.org/1161463005/diff/40001/sync/internal_api/sync_...
sync/internal_api/sync_manager_impl_unittest.cc:1110:
MockUnrecoverableErrorHandler* mock_unrecoverable_error_handler_;
Consider adding a comment about the ownership.

https://codereview.chromium.org/1161463005/diff/40001/sync/internal_api/write...
File sync/internal_api/write_node.cc (right):

https://codereview.chromium.org/1161463005/diff/40001/sync/internal_api/write...
sync/internal_api/write_node.cc:171: LOG(ERROR) << "Failed to encrypt password,
possibly due to sync node "
Should this code also upload the dump to give this failure some visibility?

https://codereview.chromium.org/1161463005/diff/40001/sync/util/cryptographer.cc
File sync/util/cryptographer.cc (right):

https://codereview.chromium.org/1161463005/diff/40001/sync/util/cryptographer...
sync/util/cryptographer.cc:130: LOG(ERROR) << "Cannot decrypt message";
Consider adding a comment explaining how this could be possible.

[Nicolas Zea, 2015-06-09 19:42:03]

https://codereview.chromium.org/1161463005/diff/40001/sync/internal_api/sync_...
File sync/internal_api/sync_encryption_handler_impl.cc (right):

https://codereview.chromium.org/1161463005/diff/40001/sync/internal_api/sync_...
sync/internal_api/sync_encryption_handler_impl.cc:837: break;
On 2015/06/09 00:04:02, stanisc wrote:
> It isn't clear how this could be possible. The ID lookup would fail only if
> ParentChildIndex was inconsistent with the ID->Entry index. I'd say
NOTREACHED()
> is appropriate in this case.

Lookup fails if we fail to decrypt as well (see WriteNode::InitByIdLookup's call
to DecryptIfNecessary).

https://codereview.chromium.org/1161463005/diff/40001/sync/internal_api/sync_...
File sync/internal_api/sync_manager_impl_unittest.cc (right):

https://codereview.chromium.org/1161463005/diff/40001/sync/internal_api/sync_...
sync/internal_api/sync_manager_impl_unittest.cc:1110:
MockUnrecoverableErrorHandler* mock_unrecoverable_error_handler_;
On 2015/06/09 00:04:02, stanisc wrote:
> Consider adding a comment about the ownership.

Done.

https://codereview.chromium.org/1161463005/diff/40001/sync/internal_api/write...
File sync/internal_api/write_node.cc (right):

https://codereview.chromium.org/1161463005/diff/40001/sync/internal_api/write...
sync/internal_api/write_node.cc:171: LOG(ERROR) << "Failed to encrypt password,
possibly due to sync node "
On 2015/06/09 00:04:02, stanisc wrote:
> Should this code also upload the dump to give this failure some visibility?

The DecryptIfNecessary method's calls to OnUnrecoverableError will trigger an
upload.

https://codereview.chromium.org/1161463005/diff/40001/sync/util/cryptographer.cc
File sync/util/cryptographer.cc (right):

https://codereview.chromium.org/1161463005/diff/40001/sync/util/cryptographer...
sync/util/cryptographer.cc:130: LOG(ERROR) << "Cannot decrypt message";
On 2015/06/09 00:04:02, stanisc wrote:
> Consider adding a comment explaining how this could be possible.

Done.

[stanisc, 2015-06-09 23:02:11]

lgtm

https://codereview.chromium.org/1161463005/diff/60001/sync/internal_api/sync_...
File sync/internal_api/sync_encryption_handler_impl.cc (right):

https://codereview.chromium.org/1161463005/diff/60001/sync/internal_api/sync_...
sync/internal_api/sync_encryption_handler_impl.cc:837: break;
Consider adding a comment here and mentioning that this could be due to a
failure to decrypt.

[Nicolas Zea, 2015-06-09 23:31:15]

Done, thanks for the review. Committing.

https://codereview.chromium.org/1161463005/diff/60001/sync/internal_api/sync_...
File sync/internal_api/sync_encryption_handler_impl.cc (right):

https://codereview.chromium.org/1161463005/diff/60001/sync/internal_api/sync_...
sync/internal_api/sync_encryption_handler_impl.cc:837: break;
On 2015/06/09 23:02:11, stanisc wrote:
> Consider adding a comment here and mentioning that this could be due to a
> failure to decrypt.

Done.

[Nicolas Zea, 2015-06-09 23:31:23]

The CQ bit was checked by zea@chromium.org

[Nicolas Zea, 2015-06-09 23:31:23]

The patchset sent to the CQ was uploaded after l-g-t-m from stanisc@chromium.org
Link to the patchset: https://codereview.chromium.org/1161463005/#ps80001
(title: "Address final comments")

[commit-bot: I haz the power, 2015-06-09 23:37:17]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1161463005/80001

[commit-bot: I haz the power, 2015-06-10 01:51:40]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-06-10 01:52:30]

Patchset 5 (id:??) landed as
https://crrev.com/dea3a922a415ef050c3a5fb0d2d19571fe578413
Cr-Commit-Position: refs/heads/master@{#333644}