[GCM] Reset store on checkin rejection and add checkin state to internals

components/gcm_driver/gcm_client_impl.cc

Index: components/gcm_driver/gcm_client_impl.cc
diff --git a/components/gcm_driver/gcm_client_impl.cc b/components/gcm_driver/gcm_client_impl.cc
index 62b58b61f72032c5747a22438db302981330f1be..0881f38ae29b63a4585cc80f6e6feeec079ec7f3 100644
--- a/components/gcm_driver/gcm_client_impl.cc
+++ b/components/gcm_driver/gcm_client_impl.cc
@@ -544,8 +544,6 @@ void GCMClientImpl::DestroyStoreWhenNotNeeded() {
 }
 
 void GCMClientImpl::ResetStore() {
-  DCHECK_EQ(LOADING, state_);
-
   // If already being reset, don't do it again. We want to prevent from
   // resetting and loading from the store again and again.
   if (gcm_store_reset_) {
@@ -706,8 +704,8 @@ void GCMClientImpl::OnCheckinCompleted(
 
   if (!checkin_response.has_android_id() ||
       !checkin_response.has_security_token()) {
-    // TODO(fgorski): I don't think a retry here will help, we should probably
-    // start over. By checking in with (0, 0).
+    LOG(ERROR) << "Checkin rejected. Resetting GCM Store.";
+    ResetStore();

[Peter Beverloo, 2016/10/28 15:52:30]

I'm a bit concerned that this opens up a path for a bug in server-side GCM to
cause mass destruction of client-side databases. (E.g. responding with
401/Unauthorized in prod.)

There are a fair number of authentication changes being considered and pushed
through in the GCM team right now, so it's not unlikely that we'll see some
bugs.

How impactful would it be to only ResetStore() when the failure persists for
more than a day or two?

[Nicolas Zea, 2016/11/01 00:18:16]

Updated to only perform a reset for 400/401 response codes.

Re: persistent failure, thinking about this I wonder how much such a check would
really accomplish. How confident are we such a bug to get caught within a day or
two? I suspect the main reason we'd notice a regression would be from this reset
store event, and adding such a delay might actually make it harder to detect.

Thoughts?

[Peter Beverloo, 2016/11/01 15:19:14]

Chirp (go/finch-chirp) could help for automated warnings of such a UMA. The idea
of a mass self destruct of subscriptions frightens me.

You're much more experienced with Chrome's server <--> client interaction,
however, so I'm OK deferring to your judgement here.

     return;
   }
 
@@ -719,7 +717,7 @@ void GCMClientImpl::OnCheckinCompleted(
     OnFirstTimeDeviceCheckinCompleted(checkin_info);
   } else {
     // checkin_info is not expected to change after a periodic checkin as it
-    // would invalidate the registratoin IDs.
+    // would invalidate the registration IDs.
     DCHECK_EQ(READY, state_);
     DCHECK_EQ(device_checkin_info_.android_id, checkin_info.android_id);
     DCHECK_EQ(device_checkin_info_.secret, checkin_info.secret);
@@ -1187,6 +1185,9 @@ GCMClient::GCMStatistics GCMClientImpl::GetStatistics() const {
   stats.is_recording = recorder_.is_recording();
   stats.gcm_client_state = GetStateString();
   stats.connection_client_created = mcs_client_.get() != NULL;
+  stats.last_checkin = last_checkin_time_;
+  stats.next_checkin =
+      last_checkin_time_ + gservices_settings_.GetCheckinInterval();
   if (connection_factory_.get())
     stats.connection_state = connection_factory_->GetConnectionStateString();
   if (mcs_client_.get()) {