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

components/invalidation/ticl_invalidation_service.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/invalidation/ticl_invalidation_service.h"
 
 #include "base/command_line.h"
 #include "base/metrics/histogram.h"
 #include "components/gcm_driver/gcm_driver.h"
 #include "components/invalidation/gcm_invalidation_bridge.h"
@@ skipping 90 matching lines @@
 void TiclInvalidationService::InitForTest(
     scoped_ptr<syncer::InvalidationStateTracker> invalidation_state_tracker,
     syncer::Invalidator* invalidator) {
   // Here we perform the equivalent of Init() and StartInvalidator(), but with
   // some minor changes to account for the fact that we're injecting the
   // invalidator.
   invalidation_state_tracker_ = invalidation_state_tracker.Pass();
   invalidator_.reset(invalidator);
 
   invalidator_->RegisterHandler(this);
-  invalidator_->UpdateRegisteredIds(
-      this,
-      invalidator_registrar_->GetAllRegisteredIds());
+  CHECK(invalidator_->UpdateRegisteredIds(
+      this, invalidator_registrar_->GetAllRegisteredIds()));
 }
 
 void TiclInvalidationService::RegisterInvalidationHandler(
     syncer::InvalidationHandler* handler) {
   DCHECK(CalledOnValidThread());
   DVLOG(2) << "Registering an invalidation handler";
   invalidator_registrar_->RegisterHandler(handler);
   logger_.OnRegistration(handler->GetOwnerName());
 }
 
-void TiclInvalidationService::UpdateRegisteredInvalidationIds(
+bool TiclInvalidationService::UpdateRegisteredInvalidationIds(
     syncer::InvalidationHandler* handler,
     const syncer::ObjectIdSet& ids) {
   DCHECK(CalledOnValidThread());
   DVLOG(2) << "Registering ids: " << ids.size();
-  invalidator_registrar_->UpdateRegisteredIds(handler, ids);
+  if (!invalidator_registrar_->UpdateRegisteredIds(handler, ids))
+    return false;
   if (invalidator_) {
-    invalidator_->UpdateRegisteredIds(
-        this,
-        invalidator_registrar_->GetAllRegisteredIds());
+    CHECK(invalidator_->UpdateRegisteredIds(

[maniscalco, 2015/05/18 15:42:28]

I see this method (TiclInvalidationService::UpdateRegisteredInvalidationIds) can
now return a boolean result.  Should we be returning false here instead of
CHECKing?

[pavely, 2015/05/18 18:38:04]

invalidator_ is private to TiclInvalidationService, the only handler possible
for it is TiclInvalidationService itself. Situation when there are two handlers
registering for the same ObjectId is invalid here. If it happens, we should
catch it early.

[maniscalco, 2015/05/18 18:56:05]

SGTM

+        this, invalidator_registrar_->GetAllRegisteredIds()));
   }
   logger_.OnUpdateIds(invalidator_registrar_->GetSanitizedHandlersIdsMap());
+  return true;
 }
 
 void TiclInvalidationService::UnregisterInvalidationHandler(
     syncer::InvalidationHandler* handler) {
   DCHECK(CalledOnValidThread());
   DVLOG(2) << "Unregistering";
   invalidator_registrar_->UnregisterHandler(handler);
   if (invalidator_) {
-    invalidator_->UpdateRegisteredIds(
-        this,
-        invalidator_registrar_->GetAllRegisteredIds());
+    CHECK(invalidator_->UpdateRegisteredIds(
+        this, invalidator_registrar_->GetAllRegisteredIds()));
   }
   logger_.OnUnregistration(handler->GetOwnerName());
 }
 
 syncer::InvalidatorState TiclInvalidationService::GetInvalidatorState() const {
   DCHECK(CalledOnValidThread());
   if (invalidator_) {
     DVLOG(2) << "GetInvalidatorState returning "
         << invalidator_->GetInvalidatorState();
     return invalidator_->GetInvalidatorState();
@@ skipping 238 matching lines @@
           invalidation_state_tracker_->GetInvalidatorClientId(),
           invalidation_state_tracker_->GetSavedInvalidations(),
           invalidation_state_tracker_->GetBootstrapData(),
           invalidation_state_tracker_.get(),
           user_agent_,
           request_context_));
 
   UpdateInvalidatorCredentials();
 
   invalidator_->RegisterHandler(this);
-  invalidator_->UpdateRegisteredIds(
-      this,
-      invalidator_registrar_->GetAllRegisteredIds());
+  CHECK(invalidator_->UpdateRegisteredIds(
+      this, invalidator_registrar_->GetAllRegisteredIds()));
 }
 
 void TiclInvalidationService::UpdateInvalidationNetworkChannel() {
   const InvalidationNetworkChannel network_channel_type =
       settings_provider_->UseGCMChannel() ? GCM_NETWORK_CHANNEL
                                           : PUSH_CLIENT_CHANNEL;
   if (network_channel_type_ == network_channel_type)
     return;
   network_channel_type_ = network_channel_type;
   if (IsStarted()) {
@@ skipping 12 matching lines @@
 }
 
 void TiclInvalidationService::StopInvalidator() {
   DCHECK(invalidator_);
   gcm_invalidation_bridge_.reset();
   invalidator_->UnregisterHandler(this);
   invalidator_.reset();
 }
 
 }  // namespace invalidation