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

components/gcm_driver/gcm_client_impl.cc

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "components/gcm_driver/gcm_client_impl.h"
 
 #include <stddef.h>
 
 #include <memory>
 #include <utility>
@@ skipping 526 matching lines @@
 
 void GCMClientImpl::DestroyStoreWhenNotNeeded() {
   if (state_ != LOADED || start_mode_ != DELAYED_START)
     return;
 
   gcm_store_->Destroy(base::Bind(&GCMClientImpl::DestroyStoreCallback,
                       weak_ptr_factory_.GetWeakPtr()));
 }
 
 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_) {
     RecordResetStoreErrorToUMA(INFINITE_STORE_RESET);
     state_ = UNINITIALIZED;
     return;
   }
   gcm_store_reset_ = true;
 
   // Destroy the GCM store to start over.
@@ skipping 140 matching lines @@
   device_checkin_info_.SnapshotCheckinAccounts();
   checkin_request_->Start();
 }
 
 void GCMClientImpl::OnCheckinCompleted(
     const checkin_proto::AndroidCheckinResponse& checkin_response) {
   checkin_request_.reset();
 
   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;
   }
 
   CheckinInfo checkin_info;
   checkin_info.android_id = checkin_response.android_id();
   checkin_info.secret = checkin_response.security_token();
 
   if (state_ == INITIAL_DEVICE_CHECKIN) {
     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);
   }
 
   if (device_checkin_info_.IsValid()) {
     // First update G-services settings, as something might have changed.
     if (gservices_settings_.UpdateFromCheckinResponse(checkin_response)) {
       gcm_store_->SetGServicesSettings(
           gservices_settings_.settings_map(),
@@ skipping 447 matching lines @@
 void GCMClientImpl::ClearActivityLogs() {
   recorder_.Clear();
 }
 
 GCMClient::GCMStatistics GCMClientImpl::GetStatistics() const {
   GCMClient::GCMStatistics stats;
   stats.gcm_client_created = true;
   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()) {
     stats.send_queue_size = mcs_client_->GetSendQueueSize();
     stats.resend_queue_size = mcs_client_->GetResendQueueSize();
   }
   if (device_checkin_info_.android_id > 0)
     stats.android_id = device_checkin_info_.android_id;
   recorder_.CollectActivities(&stats.recorded_activities);
 
@@ skipping 229 matching lines @@
 bool GCMClientImpl::HasStandaloneRegisteredApp() const {
   if (registrations_.empty())
     return false;
   // Note that account mapper is not counted as a standalone app since it is
   // automatically started when other app uses GCM.
   return registrations_.size() > 1 ||
          !ExistsGCMRegistrationInMap(registrations_, kGCMAccountMapperAppId);
 }
 
 }  // namespace gcm