[Sync] InvalidationService shouldn't CHECK when registering ObjectId for more than one handler

chrome/browser/policy/cloud/cloud_policy_invalidator.cc

 // Copyright 2013 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 "chrome/browser/policy/cloud/cloud_policy_invalidator.h"
 
 #include "base/bind.h"
 #include "base/hash.h"
 #include "base/location.h"
 #include "base/metrics/histogram.h"
@@ skipping 290 matching lines @@
   // Update internal state.
   if (invalid_)
     AcknowledgeInvalidation();
   is_registered_ = true;
   object_id_ = object_id;
   UpdateInvalidationsEnabled();
 
   // Update registration with the invalidation service.
   syncer::ObjectIdSet ids;
   ids.insert(object_id);
-  invalidation_service_->UpdateRegisteredInvalidationIds(this, ids);
+  CHECK(invalidation_service_->UpdateRegisteredInvalidationIds(this, ids));

[bartfab (slow), 2015/05/19 10:44:42]

Nit 1: #include "base/logging.h"
Nit 2: I think a LOG(ERROR) is more appropriate than a CHECK(). If we receive
invalid data from the cloud, invalidation will not work - no need to crash the
whole browser because of that.

[pavely, 2015/05/19 17:54:55]

#1: Done

#2: I wanted this change to preserve existing behavior, but allow call sites to
do something smarter than just crash. LOG(ERROR) would work but would not allow
us to see issues: invalid data would cause degraded experience and we'll never
find out about it.
You proposed earlier to report UMA histogram in such situations, this is the
place to do it. Let's land this change as it is and you'll add appropriate
reaction.

 }
 
 void CloudPolicyInvalidator::Unregister() {
   if (is_registered_) {
     if (invalid_)
       AcknowledgeInvalidation();
-    invalidation_service_->UpdateRegisteredInvalidationIds(
-        this,
-        syncer::ObjectIdSet());
+    CHECK(invalidation_service_->UpdateRegisteredInvalidationIds(
+        this, syncer::ObjectIdSet()));
     invalidation_service_->UnregisterInvalidationHandler(this);
     is_registered_ = false;
     UpdateInvalidationsEnabled();
   }
 }
 
 void CloudPolicyInvalidator::UpdateMaxFetchDelay(const PolicyMap& policy_map) {
   int delay;
 
   // Try reading the delay from the policy.
@@ skipping 107 matching lines @@
 bool CloudPolicyInvalidator::GetInvalidationsEnabled() {
   if (!invalidations_enabled_)
     return false;
   // If invalidations have been enabled for less than the grace period, then
   // consider invalidations to be disabled for metrics reporting.
   base::TimeDelta elapsed = clock_->Now() - invalidations_enabled_time_;
   return elapsed.InSeconds() >= kInvalidationGracePeriod;
 }
 
 }  // namespace policy