Notify GCMAppHandlers when the store is reset, so they clear cached IDs

Notify GCMAppHandlers when the store is reset, so they clear cached IDs

The GCM Store can be reset due to corruption or GCM rejecting the
checkin (with a HTTP_UNAUTHORIZED or HTTP_BAD_REQUEST response status).

When this happens, the device ID changes and so all registrations are
invalidated, hence app handlers should clear any registration IDs they
store (locally and ideally remotely too). This patch does that for
locally cached registration IDs; it does not yet attempt to clear
remotely cached registration IDs.

It does so by adding an OnStoreReset method to the GCMAppHandler
interface, which suffers the known race condition that app handlers
that are not yet registered will not find out about the store being
reset (e.g. during startup); the comment on OnStoreReset warns about
this.

BUG=661660
TBR=rdevlin.cronin@chromium.org (based on jianli's review)

Committed: https://crrev.com/93ec793ac69a542b2213297737178a55d069fd0d
Cr-Commit-Position: refs/heads/master@{#432865}

[johnme, 2016-11-02 17:41:52]

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

[Peter Beverloo, 2016-11-03 17:28:55]

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

[Peter Beverloo, 2016-11-03 17:28:57]

+zea fyi

GCM and push lgtm

Please get OWNERS review for the features you're touching.

https://codereview.chromium.org/2473813002/diff/1/chrome/browser/chromeos/pol...
File chrome/browser/chromeos/policy/heartbeat_scheduler.cc (right):

https://codereview.chromium.org/2473813002/diff/1/chrome/browser/chromeos/pol...
chrome/browser/chromeos/policy/heartbeat_scheduler.cc:429: // registration ID,
instead it just re-registers.
What about CloudPolicyClient::UpdateGcmId()? There's a TODO on line 331 that
says we send the IDs to the server. It also says we re-send it each time anyway.

Up to the OWNERS, but what about mimicking that TODO?

https://codereview.chromium.org/2473813002/diff/1/chrome/browser/chromeos/pol...
chrome/browser/chromeos/policy/heartbeat_scheduler.cc:429: // registration ID,
instead it just re-registers.
registration_id_?

https://codereview.chromium.org/2473813002/diff/1/components/gcm_driver/gcm_a...
File components/gcm_driver/gcm_account_mapper.cc (right):

https://codereview.chromium.org/2473813002/diff/1/components/gcm_driver/gcm_a...
components/gcm_driver/gcm_account_mapper.cc:155: // registration ID, instead it
just re-registers.
registration_id_

https://codereview.chromium.org/2473813002/diff/1/components/gcm_driver/gcm_a...
File components/gcm_driver/gcm_app_handler.h (right):

https://codereview.chromium.org/2473813002/diff/1/components/gcm_driver/gcm_a...
components/gcm_driver/gcm_app_handler.h:28: // registration IDs should be
deleted.
Maybe broaden this to "Any stored state related to GCM registrations should be
deleted." ?

https://codereview.chromium.org/2473813002/diff/1/components/gcm_driver/gcm_d...
File components/gcm_driver/gcm_driver_desktop.cc (right):

https://codereview.chromium.org/2473813002/diff/1/components/gcm_driver/gcm_d...
components/gcm_driver/gcm_driver_desktop.cc:1310: for (const auto& key_value :
app_handlers())
I can't wait for the destructor-y C++17 syntax... (P0144R2)

for (const auto& [app_id, handler] : app_handlers()) { .. }

https://codereview.chromium.org/2473813002/diff/1/components/invalidation/imp...
File components/invalidation/impl/gcm_invalidation_bridge.cc (right):

https://codereview.chromium.org/2473813002/diff/1/components/invalidation/imp...
components/invalidation/impl/gcm_invalidation_bridge.cc:324: // registration ID,
instead it just re-registers on every startup.
What about GCMNetworkChannel::registration_id_?

[johnme, 2016-11-08 19:22:41]

Addressed review comments, and ended up rewriting most of the overrides of
GCMAppHandler::OnStoreReset to clear more state.

Also expanded the comment on GCMAppHandler::OnStoreReset to clarify that
OnStoreReset is only really a defense in depth, since it won't be called if the
app handler isn't yet registered, which during startup is rather racy.

https://codereview.chromium.org/2473813002/diff/1/chrome/browser/chromeos/pol...
File chrome/browser/chromeos/policy/heartbeat_scheduler.cc (right):

https://codereview.chromium.org/2473813002/diff/1/chrome/browser/chromeos/pol...
chrome/browser/chromeos/policy/heartbeat_scheduler.cc:429: // registration ID,
instead it just re-registers.
On 2016/11/03 17:28:56, Peter Beverloo wrote:
> What about CloudPolicyClient::UpdateGcmId()? There's a TODO on line 331 that
> says we send the IDs to the server. It also says we re-send it each time
anyway.
> 
> Up to the OWNERS, but what about mimicking that TODO?

Done.

https://codereview.chromium.org/2473813002/diff/1/chrome/browser/chromeos/pol...
chrome/browser/chromeos/policy/heartbeat_scheduler.cc:429: // registration ID,
instead it just re-registers.
On 2016/11/03 17:28:56, Peter Beverloo wrote:
> registration_id_?

Done - I now call ShutdownGCM.

https://codereview.chromium.org/2473813002/diff/1/components/gcm_driver/gcm_a...
File components/gcm_driver/gcm_account_mapper.cc (right):

https://codereview.chromium.org/2473813002/diff/1/components/gcm_driver/gcm_a...
components/gcm_driver/gcm_account_mapper.cc:155: // registration ID, instead it
just re-registers.
On 2016/11/03 17:28:56, Peter Beverloo wrote:
> registration_id_

Done (now calls ShutdownHandler).

https://codereview.chromium.org/2473813002/diff/1/components/gcm_driver/gcm_a...
File components/gcm_driver/gcm_app_handler.h (right):

https://codereview.chromium.org/2473813002/diff/1/components/gcm_driver/gcm_a...
components/gcm_driver/gcm_app_handler.h:28: // registration IDs should be
deleted.
On 2016/11/03 17:28:57, Peter Beverloo wrote:
> Maybe broaden this to "Any stored state related to GCM registrations should be
> deleted." ?

Done (also mentioned InstanceIDs).

https://codereview.chromium.org/2473813002/diff/1/components/gcm_driver/gcm_d...
File components/gcm_driver/gcm_driver_desktop.cc (right):

https://codereview.chromium.org/2473813002/diff/1/components/gcm_driver/gcm_d...
components/gcm_driver/gcm_driver_desktop.cc:1310: for (const auto& key_value :
app_handlers())
On 2016/11/03 17:28:57, Peter Beverloo wrote:
> I can't wait for the destructor-y C++17 syntax... (P0144R2)
> 
> for (const auto& [app_id, handler] : app_handlers()) { .. }

Acknowledged.

https://codereview.chromium.org/2473813002/diff/1/components/invalidation/imp...
File components/invalidation/impl/gcm_invalidation_bridge.cc (right):

https://codereview.chromium.org/2473813002/diff/1/components/invalidation/imp...
components/invalidation/impl/gcm_invalidation_bridge.cc:324: // registration ID,
instead it just re-registers on every startup.
On 2016/11/03 17:28:57, Peter Beverloo wrote:
> What about GCMNetworkChannel::registration_id_?

Done - this was a little more involved, as I had to add a method to the delegate
interface.

[johnme, 2016-11-08 19:25:42]

Description was changed from

==========
Notify GCMAppHandlers when the store is reset, so they clear cached IDs

The GCM Store can be reset due to corruption or GCM rejecting the
checkin (with a HTTP_UNAUTHORIZED or HTTP_BAD_REQUEST response status).

When this happens, the device ID changes and so all registrations are
invalidated, hence app handlers should clear any registration IDs they
store (locally and ideally remotely too). This patch does that for
locally cached registration IDs; it does not yet attempt to clear
remotely cached registration IDs.

BUG=661660
==========

to

==========
Notify GCMAppHandlers when the store is reset, so they clear cached IDs

The GCM Store can be reset due to corruption or GCM rejecting the
checkin (with a HTTP_UNAUTHORIZED or HTTP_BAD_REQUEST response status).

When this happens, the device ID changes and so all registrations are
invalidated, hence app handlers should clear any registration IDs they
store (locally and ideally remotely too). This patch does that for
locally cached registration IDs; it does not yet attempt to clear
remotely cached registration IDs.

It does so by adding an OnStoreReset method to the GCMAppHandler
interface, which suffers the known race condition that app handlers
that are not yet registered will not find out about the store being
reset (e.g. during startup); the comment on OnStoreReset warns about
this.

BUG=661660
==========

[johnme, 2016-11-08 19:48:36]

zea: it'd be helpful to get a 2nd pair of eyes on this patch, as it's somewhat
subtle (especially the unfortunate race condition around registering app
handlers).

jianli: please review chrome/browser/extensions/extension_gcm_app_handler.* - is
there any way we can notify the extension about this case? If not, I guess this
is fine.

pavely: please review components/invalidation/ - can you confirm that clearing
GCMNetworkChannel's registration_id_ leaves it in a valid state? And is there an
easy way to notify the server that our old registration ID is no longer valid?

tengs: please review
components/proximity_auth/cryptauth/cryptauth_gcm_manager_impl.* - can you
confirm that merely clearing the pref leaves CryptAuthGCMManagerImpl in a valid
state? And is there an easy way to notify the server that our old registration
ID is no longer valid?

atwilson: please review chrome/browser/chromeos/policy/heartbeat_scheduler.* (I
also cc'd binjin since this is related to https://crbug.com/516375). Can you
confirm that ShutdownGCM leaves HeartbeatScheduler in a valid state? And is
there an easy way to notify the server that our old registration ID is no longer
valid?

Thanks all!

[johnme, 2016-11-08 19:49:57]

johnme@chromium.org changed reviewers:
+ atwilson@chromium.org, jianli@chromium.org, pavely@chromium.org,
tengs@chromium.org

[johnme, 2016-11-08 19:49:58]

zea: it'd be helpful to get a 2nd pair of eyes on this patch, as it's somewhat
subtle (especially the unfortunate race condition around registering app
handlers).

jianli: please review chrome/browser/extensions/extension_gcm_app_handler.* - is
there any way we can notify the extension about this case? If not, I guess this
is fine.

pavely: please review components/invalidation/ - can you confirm that clearing
GCMNetworkChannel's registration_id_ leaves it in a valid state? And is there an
easy way to notify the server that our old registration ID is no longer valid?

tengs: please review
components/proximity_auth/cryptauth/cryptauth_gcm_manager_impl.* - can you
confirm that merely clearing the pref leaves CryptAuthGCMManagerImpl in a valid
state? And is there an easy way to notify the server that our old registration
ID is no longer valid?

atwilson: please review chrome/browser/chromeos/policy/heartbeat_scheduler.* (I
also cc'd binjin since this is related to https://crbug.com/516375). Can you
confirm that ShutdownGCM leaves HeartbeatScheduler in a valid state? And is
there an easy way to notify the server that our old registration ID is no longer
valid?

Thanks all!

[jianli, 2016-11-08 23:54:57]

https://codereview.chromium.org/2473813002/diff/40001/components/gcm_driver/g...
File components/gcm_driver/gcm_client_impl.cc (right):

https://codereview.chromium.org/2473813002/diff/40001/components/gcm_driver/g...
components/gcm_driver/gcm_client_impl.cc:815: delegate_->OnStoreReset();
Do we want to call OnStoreReset if |success| is false?

[pavely, 2016-11-09 00:19:00]

components/invalidation/ lgtm

This implementation doesn't have a way to invalidate registration_id with
server.

[Andrew T Wilson (Slow), 2016-11-09 08:19:39]

https://codereview.chromium.org/2473813002/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/heartbeat_scheduler.cc (right):

https://codereview.chromium.org/2473813002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/heartbeat_scheduler.cc:430: ShutdownGCM();
Problem is that this will shutdown our GCM connection permanently until the next
reboot. I think you need to invoke RefreshHeartbeatSettings() after calling
ShutdownGCM() to re-initialize the GCM connection.

Also, can you add a test to make sure we do the right thing here?

[johnme, 2016-11-09 19:33:48]

https://codereview.chromium.org/2473813002/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/heartbeat_scheduler.cc (right):

https://codereview.chromium.org/2473813002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/heartbeat_scheduler.cc:430: ShutdownGCM();
On 2016/11/09 08:19:39, Andrew T Wilson (Slow) wrote:
> Problem is that this will shutdown our GCM connection permanently until the
next
> reboot. I think you need to invoke RefreshHeartbeatSettings() after calling
> ShutdownGCM() to re-initialize the GCM connection.

Makes sense.

> Also, can you add a test to make sure we do the right thing here?

I've added a pair of tests to theoretically test HeartbeatScheduler's behavior
here. Gonna have to run them via the try bots since I don't have a ChromeOS dev
machine, so they might not even compile yet :)

https://codereview.chromium.org/2473813002/diff/40001/components/gcm_driver/g...
File components/gcm_driver/gcm_client_impl.cc (right):

https://codereview.chromium.org/2473813002/diff/40001/components/gcm_driver/g...
components/gcm_driver/gcm_client_impl.cc:815: delegate_->OnStoreReset();
On 2016/11/08 23:54:57, jianli wrote:
> Do we want to call OnStoreReset if |success| is false?

Yes, I think so. Looking at the source code of leveldb::DestroyDB, it can return
a non-OK success if it deletes some of the DB files but (due to I/O errors)
fails to delete all of them. In such cases the store will have suffered
sufficient dataloss that it is unlikely to be recoverable (on top of whatever
corruption triggered the attempted reset).

And if one of the successfully deleted files is the "CURRENT" file, then
GCMStoreImpl's DatabaseExists function will in fact consider the DB to no longer
exist (to avoid https://crbug.com/650254), and hence the next
GCMClientImpl::OnLoadCompleted will simply call
Load(GCMStore::CREATE_IF_MISSING, ...) to create a new store rather than calling
ResetStore (so in such cases the partial reset is the last chance to fire an
OnStoreReset event).

In the unlikely event that the GCM Store completely fails to be reset (e.g.
because leveldb::DestroyDB fails to obtain a lock) and turns out to be
recoverable, there's little harm from having app handlers unnecessarily cleared
their state - they'll just re-register in due course (and may even end up
getting the same registration id).

[johnme, 2016-11-09 19:33:57]

The CQ bit was checked by johnme@chromium.org

[johnme, 2016-11-09 19:33:57]

The patchset sent to the CQ was uploaded after l-g-t-m from peter@chromium.org,
pavely@chromium.org
Link to the patchset: https://codereview.chromium.org/2473813002/#ps60001
(title: "HeartbeatScheduler to call and test RefreshHeartbeatSettings")

[johnme, 2016-11-09 19:34:29]

The CQ bit was unchecked by johnme@chromium.org

[commit-bot: I haz the power, 2016-11-09 19:34:33]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[johnme, 2016-11-09 19:34:42]

The CQ bit was checked by johnme@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-09 19:35:13]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-09 19:46:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-09 19:46:05]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Nicolas Zea, 2016-11-09 20:57:53]

gcm changes LGTM. You mention a race condition around app handler registration
though. Could you elaborate on that?

[jianli, 2016-11-09 21:53:52]

https://codereview.chromium.org/2473813002/diff/60001/components/gcm_driver/g...
File components/gcm_driver/gcm_driver_desktop.cc (right):

https://codereview.chromium.org/2473813002/diff/60001/components/gcm_driver/g...
components/gcm_driver/gcm_driver_desktop.cc:1311:
key_value.second->OnStoreReset();
There is a risk here when the called app_handler also tries to remove itself.
For example, HeartbeatScheduler::OnStoreReset.

[jianli, 2016-11-09 21:54:23]

https://codereview.chromium.org/2473813002/diff/40001/components/gcm_driver/g...
File components/gcm_driver/gcm_client_impl.cc (right):

https://codereview.chromium.org/2473813002/diff/40001/components/gcm_driver/g...
components/gcm_driver/gcm_client_impl.cc:815: delegate_->OnStoreReset();
On 2016/11/09 19:33:48, johnme wrote:
> On 2016/11/08 23:54:57, jianli wrote:
> > Do we want to call OnStoreReset if |success| is false?
> 
> Yes, I think so. Looking at the source code of leveldb::DestroyDB, it can
return
> a non-OK success if it deletes some of the DB files but (due to I/O errors)
> fails to delete all of them. In such cases the store will have suffered
> sufficient dataloss that it is unlikely to be recoverable (on top of whatever
> corruption triggered the attempted reset).
> 
> And if one of the successfully deleted files is the "CURRENT" file, then
> GCMStoreImpl's DatabaseExists function will in fact consider the DB to no
longer
> exist (to avoid https://crbug.com/650254), and hence the next
> GCMClientImpl::OnLoadCompleted will simply call
> Load(GCMStore::CREATE_IF_MISSING, ...) to create a new store rather than
calling
> ResetStore (so in such cases the partial reset is the last chance to fire an
> OnStoreReset event).
> 
> In the unlikely event that the GCM Store completely fails to be reset (e.g.
> because leveldb::DestroyDB fails to obtain a lock) and turns out to be
> recoverable, there's little harm from having app handlers unnecessarily
cleared
> their state - they'll just re-register in due course (and may even end up
> getting the same registration id).

Please add some comments to explain this.

[Andrew T Wilson (Slow), 2016-11-10 08:44:13]

FYI, you can build cros unittests by setting target_os = "chromeos" in a regular
chrome build. You don't have to sync down any of the chrome os sources or
anything like that.

When I type "gn args out/crosbuild" here's what I see:

is_component_build = true
enable_nacl = false
use_goma = true
target_os = "chromeos"

[johnme, 2016-11-10 19:56:54]

On 2016/11/09 20:57:53, Nicolas Zea wrote:
> You mention a race condition around app handler registration
> though. Could you elaborate on that?

I left the following comment on (pure virtual) GCMAppHandler::OnStoreReset:

// Called when the GCM store is reset (e.g. due to corruption), which changes
// the device ID, invalidating all prior registrations. Any stored state
// related to GCM registrations or InstanceIDs should be deleted. This should
// only be considered a defense in depth, as this method will not be called if
// the store is reset before this app handler is registered; hence it is
// recommended to regularly revalidate any stored registrations/InstanceIDs.
// TODO(johnme): GCMDriver doesn't yet provide an API for revalidating them.

Does that clarify things? The GCM component can't enumerate app handlers until
they are registered at runtime; if the store is reset early during startup
before some app handler has been registered (or even post-startup if there are
app handlers that are only conditionally registered and happen not to be
registered under the current conditions), there is no way for GCM to notify that
app handler about the reset.

For many GCM consumers, missing a store reset isn't tragic, because they retry
registration on every startup anyway. But for the Push API (and to a lesser
extent extensions since that does less caching) store resets are really bad (for
example until this patch the Push API would continue to tell the website it was
subscribed even though it was not!). This patch will hopefully catch a lot of
future store resets, but we plan to later supplement this patch with explicitly
checking the GCM Store each time the website calls subscribe() or
getSubscription() (partly to fix users whose stores were reset before this
patch, but partly for any ongoing cases where we lose this race condition).

On 2016/11/10 08:44:13, Andrew T Wilson (Slow) wrote:
> FYI, you can build cros unittests by setting target_os = "chromeos" in a
regular
> chrome build

Ah, thanks! Fixed tests, and added a condition to
HeartbeatScheduler::OnStoreReset as a result (turns out you get
ASYNC_OPERATION_PENDING if you call ShutdownGCM and RefreshHeartbeatSettings
whilst a registration is ongoing - instead it should be fine to let the
registration finish normally one the store finishes loading after the reset).

https://codereview.chromium.org/2473813002/diff/40001/components/gcm_driver/g...
File components/gcm_driver/gcm_client_impl.cc (right):

https://codereview.chromium.org/2473813002/diff/40001/components/gcm_driver/g...
components/gcm_driver/gcm_client_impl.cc:815: delegate_->OnStoreReset();
On 2016/11/09 21:54:23, jianli wrote:
> On 2016/11/09 19:33:48, johnme wrote:
> > On 2016/11/08 23:54:57, jianli wrote:
> > > Do we want to call OnStoreReset if |success| is false?
> > 
> > Yes, I think so. Looking at the source code of leveldb::DestroyDB, it can
> return
> > a non-OK success if it deletes some of the DB files but (due to I/O errors)
> > fails to delete all of them. In such cases the store will have suffered
> > sufficient dataloss that it is unlikely to be recoverable (on top of
whatever
> > corruption triggered the attempted reset).
> > 
> > And if one of the successfully deleted files is the "CURRENT" file, then
> > GCMStoreImpl's DatabaseExists function will in fact consider the DB to no
> longer
> > exist (to avoid https://crbug.com/650254), and hence the next
> > GCMClientImpl::OnLoadCompleted will simply call
> > Load(GCMStore::CREATE_IF_MISSING, ...) to create a new store rather than
> calling
> > ResetStore (so in such cases the partial reset is the last chance to fire an
> > OnStoreReset event).
> > 
> > In the unlikely event that the GCM Store completely fails to be reset (e.g.
> > because leveldb::DestroyDB fails to obtain a lock) and turns out to be
> > recoverable, there's little harm from having app handlers unnecessarily
> cleared
> > their state - they'll just re-register in due course (and may even end up
> > getting the same registration id).
> 
> Please add some comments to explain this.

Done.

https://codereview.chromium.org/2473813002/diff/60001/components/gcm_driver/g...
File components/gcm_driver/gcm_driver_desktop.cc (right):

https://codereview.chromium.org/2473813002/diff/60001/components/gcm_driver/g...
components/gcm_driver/gcm_driver_desktop.cc:1311:
key_value.second->OnStoreReset();
On 2016/11/09 21:53:51, jianli wrote:
> There is a risk here when the called app_handler also tries to remove itself.
> For example, HeartbeatScheduler::OnStoreReset.

Good spot! I now iterate over a copy of app_handlers().

[jianli, 2016-11-16 22:12:01]

lgtm

[Tim Song, 2016-11-16 22:28:09]

LGTM with nit.

https://codereview.chromium.org/2473813002/diff/80001/components/proximity_au...
File components/proximity_auth/cryptauth/cryptauth_gcm_manager_impl.cc (right):

https://codereview.chromium.org/2473813002/diff/80001/components/proximity_au...
components/proximity_auth/cryptauth/cryptauth_gcm_manager_impl.cc:87: //
TODO(crbug.com/661660): Tell server the registration ID is no longer valid.
Can you change this comment to:
"We will automatically re-register to GCM and re-enroll the new registration ID
to Cryptauth during the next scheduled sync."

[Andrew T Wilson (Slow), 2016-11-17 12:19:30]

*policy/* LGTM

[johnme, 2016-11-17 12:57:25]

https://codereview.chromium.org/2473813002/diff/80001/components/proximity_au...
File components/proximity_auth/cryptauth/cryptauth_gcm_manager_impl.cc (right):

https://codereview.chromium.org/2473813002/diff/80001/components/proximity_au...
components/proximity_auth/cryptauth/cryptauth_gcm_manager_impl.cc:87: //
TODO(crbug.com/661660): Tell server the registration ID is no longer valid.
On 2016/11/16 22:28:09, Tim Song wrote:
> Can you change this comment to:
> "We will automatically re-register to GCM and re-enroll the new registration
ID
> to Cryptauth during the next scheduled sync." 

Done.

[johnme, 2016-11-17 12:57:31]

The CQ bit was checked by johnme@chromium.org

[johnme, 2016-11-17 12:57:32]

The patchset sent to the CQ was uploaded after l-g-t-m from pavely@chromium.org,
peter@chromium.org, zea@chromium.org, atwilson@chromium.org,
jianli@chromium.org, tengs@chromium.org
Link to the patchset: https://codereview.chromium.org/2473813002/#ps100001
(title: "Update Cryptauth comment")

[commit-bot: I haz the power, 2016-11-17 12:57:47]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-17 13:04:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-17 13:04:34]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[johnme, 2016-11-17 13:12:10]

Description was changed from

==========
Notify GCMAppHandlers when the store is reset, so they clear cached IDs

The GCM Store can be reset due to corruption or GCM rejecting the
checkin (with a HTTP_UNAUTHORIZED or HTTP_BAD_REQUEST response status).

When this happens, the device ID changes and so all registrations are
invalidated, hence app handlers should clear any registration IDs they
store (locally and ideally remotely too). This patch does that for
locally cached registration IDs; it does not yet attempt to clear
remotely cached registration IDs.

It does so by adding an OnStoreReset method to the GCMAppHandler
interface, which suffers the known race condition that app handlers
that are not yet registered will not find out about the store being
reset (e.g. during startup); the comment on OnStoreReset warns about
this.

BUG=661660
==========

to

==========
Notify GCMAppHandlers when the store is reset, so they clear cached IDs

The GCM Store can be reset due to corruption or GCM rejecting the
checkin (with a HTTP_UNAUTHORIZED or HTTP_BAD_REQUEST response status).

When this happens, the device ID changes and so all registrations are
invalidated, hence app handlers should clear any registration IDs they
store (locally and ideally remotely too). This patch does that for
locally cached registration IDs; it does not yet attempt to clear
remotely cached registration IDs.

It does so by adding an OnStoreReset method to the GCMAppHandler
interface, which suffers the known race condition that app handlers
that are not yet registered will not find out about the store being
reset (e.g. during startup); the comment on OnStoreReset warns about
this.

BUG=661660
TBR=rdevlin.cronin@chromium.org (based on jianli's review)
==========

[johnme, 2016-11-17 13:12:10]

johnme@chromium.org changed reviewers:
+ rdevlin.cronin@chromium.org

[johnme, 2016-11-17 13:12:20]

The CQ bit was checked by johnme@chromium.org

[commit-bot: I haz the power, 2016-11-17 13:12:36]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-17 14:27:26]

Description was changed from

==========
Notify GCMAppHandlers when the store is reset, so they clear cached IDs

The GCM Store can be reset due to corruption or GCM rejecting the
checkin (with a HTTP_UNAUTHORIZED or HTTP_BAD_REQUEST response status).

When this happens, the device ID changes and so all registrations are
invalidated, hence app handlers should clear any registration IDs they
store (locally and ideally remotely too). This patch does that for
locally cached registration IDs; it does not yet attempt to clear
remotely cached registration IDs.

It does so by adding an OnStoreReset method to the GCMAppHandler
interface, which suffers the known race condition that app handlers
that are not yet registered will not find out about the store being
reset (e.g. during startup); the comment on OnStoreReset warns about
this.

BUG=661660
TBR=rdevlin.cronin@chromium.org (based on jianli's review)
==========

to

==========
Notify GCMAppHandlers when the store is reset, so they clear cached IDs

The GCM Store can be reset due to corruption or GCM rejecting the
checkin (with a HTTP_UNAUTHORIZED or HTTP_BAD_REQUEST response status).

When this happens, the device ID changes and so all registrations are
invalidated, hence app handlers should clear any registration IDs they
store (locally and ideally remotely too). This patch does that for
locally cached registration IDs; it does not yet attempt to clear
remotely cached registration IDs.

It does so by adding an OnStoreReset method to the GCMAppHandler
interface, which suffers the known race condition that app handlers
that are not yet registered will not find out about the store being
reset (e.g. during startup); the comment on OnStoreReset warns about
this.

BUG=661660
TBR=rdevlin.cronin@chromium.org (based on jianli's review)
==========

[commit-bot: I haz the power, 2016-11-17 14:27:28]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-11-17 14:31:13]

Description was changed from

==========
Notify GCMAppHandlers when the store is reset, so they clear cached IDs

The GCM Store can be reset due to corruption or GCM rejecting the
checkin (with a HTTP_UNAUTHORIZED or HTTP_BAD_REQUEST response status).

When this happens, the device ID changes and so all registrations are
invalidated, hence app handlers should clear any registration IDs they
store (locally and ideally remotely too). This patch does that for
locally cached registration IDs; it does not yet attempt to clear
remotely cached registration IDs.

It does so by adding an OnStoreReset method to the GCMAppHandler
interface, which suffers the known race condition that app handlers
that are not yet registered will not find out about the store being
reset (e.g. during startup); the comment on OnStoreReset warns about
this.

BUG=661660
TBR=rdevlin.cronin@chromium.org (based on jianli's review)
==========

to

==========
Notify GCMAppHandlers when the store is reset, so they clear cached IDs

The GCM Store can be reset due to corruption or GCM rejecting the
checkin (with a HTTP_UNAUTHORIZED or HTTP_BAD_REQUEST response status).

When this happens, the device ID changes and so all registrations are
invalidated, hence app handlers should clear any registration IDs they
store (locally and ideally remotely too). This patch does that for
locally cached registration IDs; it does not yet attempt to clear
remotely cached registration IDs.

It does so by adding an OnStoreReset method to the GCMAppHandler
interface, which suffers the known race condition that app handlers
that are not yet registered will not find out about the store being
reset (e.g. during startup); the comment on OnStoreReset warns about
this.

BUG=661660
TBR=rdevlin.cronin@chromium.org (based on jianli's review)

Committed: https://crrev.com/93ec793ac69a542b2213297737178a55d069fd0d
Cr-Commit-Position: refs/heads/master@{#432865}
==========

[commit-bot: I haz the power, 2016-11-17 14:31:15]

Patchset 6 (id:??) landed as
https://crrev.com/93ec793ac69a542b2213297737178a55d069fd0d
Cr-Commit-Position: refs/heads/master@{#432865}