Fix GCMKeyStore interleaved request handling

Fix GCMKeyStore interleaved request handling

Consider calling CreateKeys immediately followed by RemoveKeys for the
same app_id. Previously, the following would happen:

1. CreateKeys posts to database
2. RemoveKeys immediately succeeds, since the app_id is missing from key_pairs_
3. The database callback DidStoreKeys runs and writes app_id to key_pairs_

Hence the keys would end up existing (in both the key_pairs_ cache and the
database) even though RemoveKeys had been called second and claimed to succeed!

This patch fixes this class of issues, by making key_pairs_ the source of truth,
which is asynchronously replicated to the database.

BUG=589461
Committed: https://crrev.com/85aed8cb807e0a556cfdf5554943a065409b28f3
Cr-Commit-Position: refs/heads/master@{#391856}

[johnme, 2016-05-05 15:20:44]

johnme@chromium.org changed reviewers:
+ peter@chromium.org

[Peter Beverloo, 2016-05-05 15:29:36]

lgtm, but please add a test for it

The sentence in your CL description is *very* long. Would you mind splitting it
up?

One interesting piece of data is that, in the past two weeks, 70 out of 18
million key removals failed. The consequence of setting the state to failed is
that GCM messages would stop working, which is much better than having a CHECK
or similar.

https://codereview.chromium.org/1948133003/diff/1/components/gcm_driver/crypt...
File components/gcm_driver/crypto/gcm_key_store.cc (right):

https://codereview.chromium.org/1948133003/diff/1/components/gcm_driver/crypt...
components/gcm_driver/crypto/gcm_key_store.cc:144: // Our cache is now
inconsistent. Reject requests until Chrome is restarted.
nit (1): Blank line above
nit (2): Do not mention Chrome here please

dito re: line 191

[johnme, 2016-05-05 17:30:40]

Description was changed from

==========
Fix GCMKeyStore interleaved request handling

Previously calling CreateKeys immediately followed by RemoveKeys for the
same app_id would result in the RemoveKeys immediately succeeding, since
DidStoreKeys hadn't yet written the keys to key_pairs_; then
DidStoreKeys would get run for the original CreateKeys call, and so the
key would end up existing (in both the key_pairs_ cache and the
database) even though RemoveKeys was called second and succeeded!

BUG=589461
==========

to

==========
Fix GCMKeyStore interleaved request handling

Consider calling CreateKeys immediately followed by RemoveKeys for the
same app_id. Previously, the following would happen:

1. CreateKeys posts to database
2. RemoveKeys immediately succeeds, since the app_id is missing from key_pairs_
3. The database callback DidStoreKeys runs and writes app_id to key_pairs_

Hence the keys would end up existing (in both the key_pairs_ cache and the
database) even though RemoveKeys had been called second and claimed to succeed!

This patch fixes this class of issues, by making key_pairs_ the source of truth,
which is asynchronously replicated to the database.

BUG=589461
==========

[johnme, 2016-05-05 17:32:46]

Addressed review comments - thanks!

> lgtm, but please add a test for it

Done.

> The sentence in your CL description is *very* long. Would
> you mind splitting it up?

Done.

> One interesting piece of data is that, in the past two weeks, 70 out of 18
> million key removals failed. The consequence of setting the state to failed is
> that GCM messages would stop working, which is much better than having a CHECK
> or similar.

Yup, that was my thinking too when I saw the number of failures (which are
presumably due to the disk being full or similar edge cases).

https://codereview.chromium.org/1948133003/diff/1/components/gcm_driver/crypt...
File components/gcm_driver/crypto/gcm_key_store.cc (right):

https://codereview.chromium.org/1948133003/diff/1/components/gcm_driver/crypt...
components/gcm_driver/crypto/gcm_key_store.cc:144: // Our cache is now
inconsistent. Reject requests until Chrome is restarted.
On 2016/05/05 15:29:35, Peter Beverloo wrote:
> nit (1): Blank line above
> nit (2): Do not mention Chrome here please
> 
> dito re: line 191

Done.

[johnme, 2016-05-05 17:32:50]

The CQ bit was checked by johnme@chromium.org

[johnme, 2016-05-05 17:32:50]

The patchset sent to the CQ was uploaded after l-g-t-m from peter@chromium.org
Link to the patchset: https://codereview.chromium.org/1948133003/#ps20001
(title: "Address review comments")

[commit-bot: I haz the power, 2016-05-05 17:33:19]

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

[commit-bot: I haz the power, 2016-05-05 18:48:44]

Description was changed from

==========
Fix GCMKeyStore interleaved request handling

Consider calling CreateKeys immediately followed by RemoveKeys for the
same app_id. Previously, the following would happen:

1. CreateKeys posts to database
2. RemoveKeys immediately succeeds, since the app_id is missing from key_pairs_
3. The database callback DidStoreKeys runs and writes app_id to key_pairs_

Hence the keys would end up existing (in both the key_pairs_ cache and the
database) even though RemoveKeys had been called second and claimed to succeed!

This patch fixes this class of issues, by making key_pairs_ the source of truth,
which is asynchronously replicated to the database.

BUG=589461
==========

to

==========
Fix GCMKeyStore interleaved request handling

Consider calling CreateKeys immediately followed by RemoveKeys for the
same app_id. Previously, the following would happen:

1. CreateKeys posts to database
2. RemoveKeys immediately succeeds, since the app_id is missing from key_pairs_
3. The database callback DidStoreKeys runs and writes app_id to key_pairs_

Hence the keys would end up existing (in both the key_pairs_ cache and the
database) even though RemoveKeys had been called second and claimed to succeed!

This patch fixes this class of issues, by making key_pairs_ the source of truth,
which is asynchronously replicated to the database.

BUG=589461
==========

[commit-bot: I haz the power, 2016-05-05 18:48:45]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-05-05 18:50:04]

Description was changed from

==========
Fix GCMKeyStore interleaved request handling

Consider calling CreateKeys immediately followed by RemoveKeys for the
same app_id. Previously, the following would happen:

1. CreateKeys posts to database
2. RemoveKeys immediately succeeds, since the app_id is missing from key_pairs_
3. The database callback DidStoreKeys runs and writes app_id to key_pairs_

Hence the keys would end up existing (in both the key_pairs_ cache and the
database) even though RemoveKeys had been called second and claimed to succeed!

This patch fixes this class of issues, by making key_pairs_ the source of truth,
which is asynchronously replicated to the database.

BUG=589461
==========

to

==========
Fix GCMKeyStore interleaved request handling

Consider calling CreateKeys immediately followed by RemoveKeys for the
same app_id. Previously, the following would happen:

1. CreateKeys posts to database
2. RemoveKeys immediately succeeds, since the app_id is missing from key_pairs_
3. The database callback DidStoreKeys runs and writes app_id to key_pairs_

Hence the keys would end up existing (in both the key_pairs_ cache and the
database) even though RemoveKeys had been called second and claimed to succeed!

This patch fixes this class of issues, by making key_pairs_ the source of truth,
which is asynchronously replicated to the database.

BUG=589461

Committed: https://crrev.com/85aed8cb807e0a556cfdf5554943a065409b28f3
Cr-Commit-Position: refs/heads/master@{#391856}
==========

[commit-bot: I haz the power, 2016-05-05 18:50:05]

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