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

components/invalidation/ticl_invalidation_service.cc

Index: components/invalidation/ticl_invalidation_service.cc
diff --git a/components/invalidation/ticl_invalidation_service.cc b/components/invalidation/ticl_invalidation_service.cc
index e60902c0764dfc961593ceff0297cde1e92e9590..14a4e06390d4ac52b78b8e145f70a17c1adeae14 100644
--- a/components/invalidation/ticl_invalidation_service.cc
+++ b/components/invalidation/ticl_invalidation_service.cc
@@ -108,9 +108,8 @@ void TiclInvalidationService::InitForTest(
   invalidator_.reset(invalidator);
 
   invalidator_->RegisterHandler(this);
-  invalidator_->UpdateRegisteredIds(
-      this,
-      invalidator_registrar_->GetAllRegisteredIds());
+  CHECK(invalidator_->UpdateRegisteredIds(
+      this, invalidator_registrar_->GetAllRegisteredIds()));
 }
 
 void TiclInvalidationService::RegisterInvalidationHandler(
@@ -121,18 +120,19 @@ void TiclInvalidationService::RegisterInvalidationHandler(
   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(
@@ -141,9 +141,8 @@ void TiclInvalidationService::UnregisterInvalidationHandler(
   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());
 }
@@ -402,9 +401,8 @@ void TiclInvalidationService::StartInvalidator(
   UpdateInvalidatorCredentials();
 
   invalidator_->RegisterHandler(this);
-  invalidator_->UpdateRegisteredIds(
-      this,
-      invalidator_registrar_->GetAllRegisteredIds());
+  CHECK(invalidator_->UpdateRegisteredIds(
+      this, invalidator_registrar_->GetAllRegisteredIds()));
 }
 
 void TiclInvalidationService::UpdateInvalidationNetworkChannel() {