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

Before this change InvalidationService CHECKs that object id is only
registered with one handler. This causes browser crash when object id is
received from network already registered.

Solution is to let each component that uses InvalidationService decide
how to react to duplicate registration.
In this change:
- InvalidationService::UpdateRegisteredInvalidationIds doesn't CHECK
  inside, but returns boolean that indicates if update was successful.
- All places that call this function do CHECK to preserve existing
  behavior.
- Internal implementations and tests are updated accordingly.

BUG=475941
R=maniscalco@chromium.org

Review URL: https://codereview.chromium.org/1146533005

Cr-Commit-Position: refs/heads/master@{#330654}

chrome/browser/drive/drive_notification_manager.cc

@@ -135,7 +135,7 @@ void DriveNotificationManager::RegisterDriveNotifications() {
   ids.insert(invalidation::ObjectId(
       ipc::invalidation::ObjectSource::COSMO_CHANGELOG,
       kDriveInvalidationObjectId));
-  invalidation_service_->UpdateRegisteredInvalidationIds(this, ids);
+  CHECK(invalidation_service_->UpdateRegisteredInvalidationIds(this, ids));
   push_notification_registered_ = true;
   OnInvalidatorStateChange(invalidation_service_->GetInvalidatorState());
 

chrome/browser/invalidation/fake_invalidation_service.cc

@@ -26,10 +26,10 @@ void FakeInvalidationService::RegisterInvalidationHandler(
   invalidator_registrar_.RegisterHandler(handler);
 }
 
-void FakeInvalidationService::UpdateRegisteredInvalidationIds(
-      syncer::InvalidationHandler* handler,
-      const syncer::ObjectIdSet& ids) {
-  invalidator_registrar_.UpdateRegisteredIds(handler, ids);
+bool FakeInvalidationService::UpdateRegisteredInvalidationIds(
+    syncer::InvalidationHandler* handler,
+    const syncer::ObjectIdSet& ids) {
+  return invalidator_registrar_.UpdateRegisteredIds(handler, ids);
 }
 
 void FakeInvalidationService::UnregisterInvalidationHandler(

chrome/browser/invalidation/fake_invalidation_service.h

@@ -33,7 +33,7 @@ class FakeInvalidationService : public InvalidationService {
 
   void RegisterInvalidationHandler(
       syncer::InvalidationHandler* handler) override;
-  void UpdateRegisteredInvalidationIds(syncer::InvalidationHandler* handler,
+  bool UpdateRegisteredInvalidationIds(syncer::InvalidationHandler* handler,
                                        const syncer::ObjectIdSet& ids) override;
   void UnregisterInvalidationHandler(
       syncer::InvalidationHandler* handler) override;

chrome/browser/policy/cloud/cloud_policy_invalidator.cc

@@ -308,16 +308,15 @@ void CloudPolicyInvalidator::Register(const invalidation::ObjectId& object_id) {
   // Update registration with the invalidation service.
   syncer::ObjectIdSet ids;
   ids.insert(object_id);
-  invalidation_service_->UpdateRegisteredInvalidationIds(this, ids);
+  CHECK(invalidation_service_->UpdateRegisteredInvalidationIds(this, ids));
 }
 
 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();

chrome/browser/policy/cloud/cloud_policy_invalidator.h

@@ -10,6 +10,7 @@
 #include "base/basictypes.h"
 #include "base/callback.h"
 #include "base/compiler_specific.h"
+#include "base/logging.h"
 #include "base/memory/ref_counted.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/memory/weak_ptr.h"

chrome/browser/policy/cloud/remote_commands_invalidator.cc

@@ -138,13 +138,13 @@ void RemoteCommandsInvalidator::Register(
   // Update registration with the invalidation service.
   syncer::ObjectIdSet ids;
   ids.insert(object_id);
-  invalidation_service_->UpdateRegisteredInvalidationIds(this, ids);
+  CHECK(invalidation_service_->UpdateRegisteredInvalidationIds(this, ids));
 }
 
 void RemoteCommandsInvalidator::Unregister() {
   if (is_registered_) {
-    invalidation_service_->UpdateRegisteredInvalidationIds(
-        this, syncer::ObjectIdSet());
+    CHECK(invalidation_service_->UpdateRegisteredInvalidationIds(
+        this, syncer::ObjectIdSet()));
     invalidation_service_->UnregisterInvalidationHandler(this);
     is_registered_ = false;
     UpdateInvalidationsEnabled();

chrome/browser/sync/glue/sync_backend_host_impl.cc

@@ -318,9 +318,8 @@ scoped_ptr<base::Thread> SyncBackendHostImpl::Shutdown(
 
 void SyncBackendHostImpl::UnregisterInvalidationIds() {
   if (invalidation_handler_registered_) {
-    invalidator_->UpdateRegisteredInvalidationIds(
-        this,
-        syncer::ObjectIdSet());
+    CHECK(invalidator_->UpdateRegisteredInvalidationIds(this,
+                                                        syncer::ObjectIdSet()));
   }
 }
 
@@ -624,9 +623,8 @@ void SyncBackendHostImpl::FinishConfigureDataTypesOnFrontendLoop(
     return;
 
   if (invalidator_) {
-    invalidator_->UpdateRegisteredInvalidationIds(
-        this,
-        ModelTypeSetToObjectIdSet(enabled_types));
+    CHECK(invalidator_->UpdateRegisteredInvalidationIds(
+        this, ModelTypeSetToObjectIdSet(enabled_types)));
   }
 
   // TODO(erikchen): Remove ScopedTracker below once http://crbug.com/458406 is

chrome/browser/sync/test/integration/fake_server_invalidation_service.cc

@@ -30,10 +30,10 @@ void FakeServerInvalidationService::RegisterInvalidationHandler(
   invalidator_registrar_.RegisterHandler(handler);
 }
 
-void FakeServerInvalidationService::UpdateRegisteredInvalidationIds(
+bool FakeServerInvalidationService::UpdateRegisteredInvalidationIds(
       syncer::InvalidationHandler* handler,
       const syncer::ObjectIdSet& ids) {
-  invalidator_registrar_.UpdateRegisteredIds(handler, ids);
+  return invalidator_registrar_.UpdateRegisteredIds(handler, ids);
 }
 
 void FakeServerInvalidationService::UnregisterInvalidationHandler(

chrome/browser/sync/test/integration/fake_server_invalidation_service.h

@@ -31,7 +31,7 @@ class FakeServerInvalidationService : public invalidation::InvalidationService,
 
   void RegisterInvalidationHandler(
       syncer::InvalidationHandler* handler) override;
-  void UpdateRegisteredInvalidationIds(syncer::InvalidationHandler* handler,
+  bool UpdateRegisteredInvalidationIds(syncer::InvalidationHandler* handler,
                                        const syncer::ObjectIdSet& ids) override;
   void UnregisterInvalidationHandler(
       syncer::InvalidationHandler* handler) override;

components/invalidation/fake_invalidator.cc

@@ -42,9 +42,9 @@ void FakeInvalidator::RegisterHandler(InvalidationHandler* handler) {
   registrar_.RegisterHandler(handler);
 }
 
-void FakeInvalidator::UpdateRegisteredIds(InvalidationHandler* handler,
+bool FakeInvalidator::UpdateRegisteredIds(InvalidationHandler* handler,
                                           const ObjectIdSet& ids) {
-  registrar_.UpdateRegisteredIds(handler, ids);
+  return registrar_.UpdateRegisteredIds(handler, ids);
 }
 
 void FakeInvalidator::UnregisterHandler(InvalidationHandler* handler) {

components/invalidation/fake_invalidator.h

@@ -31,7 +31,7 @@ class FakeInvalidator : public Invalidator {
       const ObjectIdInvalidationMap& invalidation_map);
 
   void RegisterHandler(InvalidationHandler* handler) override;
-  void UpdateRegisteredIds(InvalidationHandler* handler,
+  bool UpdateRegisteredIds(InvalidationHandler* handler,
                            const ObjectIdSet& ids) override;
   void UnregisterHandler(InvalidationHandler* handler) override;
   InvalidatorState GetInvalidatorState() const override;

components/invalidation/invalidation_notifier.cc

@@ -44,11 +44,13 @@ void InvalidationNotifier::RegisterHandler(InvalidationHandler* handler) {
   registrar_.RegisterHandler(handler);
 }
 
-void InvalidationNotifier::UpdateRegisteredIds(InvalidationHandler* handler,
+bool InvalidationNotifier::UpdateRegisteredIds(InvalidationHandler* handler,
                                                const ObjectIdSet& ids) {
   DCHECK(CalledOnValidThread());
-  registrar_.UpdateRegisteredIds(handler, ids);
+  if (!registrar_.UpdateRegisteredIds(handler, ids))
+    return false;
   invalidation_listener_.UpdateRegisteredIds(registrar_.GetAllRegisteredIds());
+  return true;
 }
 
 void InvalidationNotifier::UnregisterHandler(InvalidationHandler* handler) {

components/invalidation/invalidation_notifier.h

@@ -54,7 +54,7 @@ class INVALIDATION_EXPORT_PRIVATE InvalidationNotifier
 
   // Invalidator implementation.
   void RegisterHandler(InvalidationHandler* handler) override;
-  void UpdateRegisteredIds(InvalidationHandler* handler,
+  bool UpdateRegisteredIds(InvalidationHandler* handler,
                            const ObjectIdSet& ids) override;
   void UnregisterHandler(InvalidationHandler* handler) override;
   InvalidatorState GetInvalidatorState() const override;

components/invalidation/invalidation_service.h

@@ -76,12 +76,12 @@ class InvalidationService {
 
   // Updates the set of ObjectIds associated with |handler|.  |handler| must
   // not be NULL, and must already be registered.  An ID must be registered for
-  // at most one handler.
+  // at most one handler.  If ID is already registered function returns false.
   //
   // Registered IDs are persisted across restarts of sync.
-  virtual void UpdateRegisteredInvalidationIds(
+  virtual bool UpdateRegisteredInvalidationIds(
       syncer::InvalidationHandler* handler,
-      const syncer::ObjectIdSet& ids) = 0;
+      const syncer::ObjectIdSet& ids) WARN_UNUSED_RESULT = 0;
 
   // Stops sending notifications to |handler|.  |handler| must not be NULL, and
   // it must already be registered.  Note that this doesn't unregister the IDs

components/invalidation/invalidation_service_android.cc

@@ -38,14 +38,15 @@ void InvalidationServiceAndroid::RegisterInvalidationHandler(
   logger_.OnRegistration(handler->GetOwnerName());
 }
 
-void InvalidationServiceAndroid::UpdateRegisteredInvalidationIds(
+bool InvalidationServiceAndroid::UpdateRegisteredInvalidationIds(
     syncer::InvalidationHandler* handler,
     const syncer::ObjectIdSet& ids) {
   DCHECK(CalledOnValidThread());
   JNIEnv* env = base::android::AttachCurrentThread();
   DCHECK(env);
 
-  invalidator_registrar_.UpdateRegisteredIds(handler, ids);
+  if (!invalidator_registrar_.UpdateRegisteredIds(handler, ids))
+    return false;
   const syncer::ObjectIdSet& registered_ids =
       invalidator_registrar_.GetAllRegisteredIds();
 
@@ -66,6 +67,7 @@ void InvalidationServiceAndroid::UpdateRegisteredInvalidationIds(
       base::android::ToJavaArrayOfStrings(env, names).obj());
 
   logger_.OnUpdateIds(invalidator_registrar_.GetSanitizedHandlersIdsMap());
+  return true;
 }
 
 void InvalidationServiceAndroid::UnregisterInvalidationHandler(

components/invalidation/invalidation_service_android.h

@@ -39,7 +39,7 @@ class InvalidationServiceAndroid
   // exposing the client ID should be available soon; see crbug.com/172391.
   void RegisterInvalidationHandler(
       syncer::InvalidationHandler* handler) override;
-  void UpdateRegisteredInvalidationIds(syncer::InvalidationHandler* handler,
+  bool UpdateRegisteredInvalidationIds(syncer::InvalidationHandler* handler,
                                        const syncer::ObjectIdSet& ids) override;
   void UnregisterInvalidationHandler(
       syncer::InvalidationHandler* handler) override;

components/invalidation/invalidation_service_test_template.h

@@ -134,7 +134,7 @@ TYPED_TEST_P(InvalidationServiceTest, Basic) {
   syncer::ObjectIdSet ids;
   ids.insert(this->id1);
   ids.insert(this->id2);
-  invalidator->UpdateRegisteredInvalidationIds(&handler, ids);
+  EXPECT_TRUE(invalidator->UpdateRegisteredInvalidationIds(&handler, ids));
 
   this->delegate_.TriggerOnInvalidatorStateChange(
       syncer::INVALIDATIONS_ENABLED);
@@ -150,7 +150,7 @@ TYPED_TEST_P(InvalidationServiceTest, Basic) {
 
   ids.erase(this->id1);
   ids.insert(this->id3);
-  invalidator->UpdateRegisteredInvalidationIds(&handler, ids);
+  EXPECT_TRUE(invalidator->UpdateRegisteredInvalidationIds(&handler, ids));
 
   expected_invalidations = syncer::ObjectIdInvalidationMap();
   expected_invalidations.Insert(syncer::Invalidation::Init(this->id2, 2, "2"));
@@ -201,13 +201,13 @@ TYPED_TEST_P(InvalidationServiceTest, MultipleHandlers) {
     syncer::ObjectIdSet ids;
     ids.insert(this->id1);
     ids.insert(this->id2);
-    invalidator->UpdateRegisteredInvalidationIds(&handler1, ids);
+    EXPECT_TRUE(invalidator->UpdateRegisteredInvalidationIds(&handler1, ids));
   }
 
   {
     syncer::ObjectIdSet ids;
     ids.insert(this->id3);
-    invalidator->UpdateRegisteredInvalidationIds(&handler2, ids);
+    EXPECT_TRUE(invalidator->UpdateRegisteredInvalidationIds(&handler2, ids));
   }
 
   // Don't register any IDs for handler3.
@@ -215,7 +215,7 @@ TYPED_TEST_P(InvalidationServiceTest, MultipleHandlers) {
   {
     syncer::ObjectIdSet ids;
     ids.insert(this->id4);
-    invalidator->UpdateRegisteredInvalidationIds(&handler4, ids);
+    EXPECT_TRUE(invalidator->UpdateRegisteredInvalidationIds(&handler4, ids));
   }
 
   invalidator->UnregisterInvalidationHandler(&handler4);
@@ -272,6 +272,29 @@ TYPED_TEST_P(InvalidationServiceTest, MultipleHandlers) {
   invalidator->UnregisterInvalidationHandler(&handler1);
 }
 
+// Multiple registrations by different handlers on the same object ID should
+// return false.
+TYPED_TEST_P(InvalidationServiceTest, MultipleRegistrations) {
+  invalidation::InvalidationService* const invalidator =
+      this->CreateAndInitializeInvalidationService();
+
+  syncer::FakeInvalidationHandler handler1;
+  syncer::FakeInvalidationHandler handler2;
+
+  invalidator->RegisterInvalidationHandler(&handler1);
+  invalidator->RegisterInvalidationHandler(&handler2);
+
+  // Registering both handlers for the same ObjectId. First call should succeed,
+  // second should fail.
+  syncer::ObjectIdSet ids;
+  ids.insert(this->id1);
+  EXPECT_TRUE(invalidator->UpdateRegisteredInvalidationIds(&handler1, ids));
+  EXPECT_FALSE(invalidator->UpdateRegisteredInvalidationIds(&handler2, ids));
+
+  invalidator->UnregisterInvalidationHandler(&handler2);
+  invalidator->UnregisterInvalidationHandler(&handler1);
+}
+
 // Make sure that passing an empty set to UpdateRegisteredInvalidationIds clears
 // the corresponding entries for the handler.
 TYPED_TEST_P(InvalidationServiceTest, EmptySetUnregisters) {
@@ -290,19 +313,19 @@ TYPED_TEST_P(InvalidationServiceTest, EmptySetUnregisters) {
     syncer::ObjectIdSet ids;
     ids.insert(this->id1);
     ids.insert(this->id2);
-    invalidator->UpdateRegisteredInvalidationIds(&handler1, ids);
+    EXPECT_TRUE(invalidator->UpdateRegisteredInvalidationIds(&handler1, ids));
   }
 
   {
     syncer::ObjectIdSet ids;
     ids.insert(this->id3);
-    invalidator->UpdateRegisteredInvalidationIds(&handler2, ids);
+    EXPECT_TRUE(invalidator->UpdateRegisteredInvalidationIds(&handler2, ids));
   }
 
   // Unregister the IDs for the first observer. It should not receive any
   // further invalidations.
-  invalidator->UpdateRegisteredInvalidationIds(&handler1,
-                                               syncer::ObjectIdSet());
+  EXPECT_TRUE(invalidator->UpdateRegisteredInvalidationIds(
+      &handler1, syncer::ObjectIdSet()));
 
   this->delegate_.TriggerOnInvalidatorStateChange(
       syncer::INVALIDATIONS_ENABLED);
@@ -381,7 +404,10 @@ TYPED_TEST_P(InvalidationServiceTest, GetInvalidatorStateAlwaysCurrent) {
 }
 
 REGISTER_TYPED_TEST_CASE_P(InvalidationServiceTest,
-                           Basic, MultipleHandlers, EmptySetUnregisters,
+                           Basic,
+                           MultipleHandlers,
+                           MultipleRegistrations,
+                           EmptySetUnregisters,
                            GetInvalidatorStateAlwaysCurrent);
 
 #endif  // COMPONENTS_INVALIDATION_INVALIDATION_SERVICE_TEST_TEMPLATE_H_

components/invalidation/invalidator.h

@@ -59,9 +59,10 @@ class INVALIDATION_EXPORT Invalidator {
 
   // Updates the set of ObjectIds associated with |handler|.  |handler| must
   // not be NULL, and must already be registered.  An ID must be registered for
-  // at most one handler.
-  virtual void UpdateRegisteredIds(InvalidationHandler* handler,
-                                   const ObjectIdSet& ids) = 0;
+  // at most one handler. If ID is already registered function returns false.
+  virtual bool UpdateRegisteredIds(InvalidationHandler* handler,
+                                   const ObjectIdSet& ids)
+      WARN_UNUSED_RESULT = 0;
 
   // Stops sending notifications to |handler|.  |handler| must not be NULL, and
   // it must already be registered.  Note that this doesn't unregister the IDs

components/invalidation/invalidator_registrar.cc

@@ -28,9 +28,8 @@ void InvalidatorRegistrar::RegisterHandler(InvalidationHandler* handler) {
   handlers_.AddObserver(handler);
 }
 
-void InvalidatorRegistrar::UpdateRegisteredIds(
-    InvalidationHandler* handler,
-    const ObjectIdSet& ids) {
+bool InvalidatorRegistrar::UpdateRegisteredIds(InvalidationHandler* handler,
+                                               const ObjectIdSet& ids) {
   DCHECK(thread_checker_.CalledOnValidThread());
   CHECK(handler);
   CHECK(handlers_.HasObserver(handler));
@@ -47,11 +46,13 @@ void InvalidatorRegistrar::UpdateRegisteredIds(
         ids.begin(), ids.end(),
         std::inserter(intersection, intersection.end()),
         ObjectIdLessThan());
-    CHECK(intersection.empty())
-        << "Duplicate registration: trying to register "
-        << ObjectIdToString(*intersection.begin()) << " for "
-        << handler << " when it's already registered for "
-        << it->first;
+    if (!intersection.empty()) {
+      LOG(ERROR) << "Duplicate registration: trying to register "
+                 << ObjectIdToString(*intersection.begin()) << " for "
+                 << handler << " when it's already registered for "
+                 << it->first;
+      return false;
+    }
   }
 
   if (ids.empty()) {
@@ -59,6 +60,7 @@ void InvalidatorRegistrar::UpdateRegisteredIds(
   } else {
     handler_to_ids_map_[handler] = ids;
   }
+  return true;
 }
 
 void InvalidatorRegistrar::UnregisterHandler(InvalidationHandler* handler) {

components/invalidation/invalidator_registrar.h

@@ -39,9 +39,9 @@ class INVALIDATION_EXPORT InvalidatorRegistrar {
 
   // Updates the set of ObjectIds associated with |handler|.  |handler| must
   // not be NULL, and must already be registered.  An ID must be registered for
-  // at most one handler.
-  void UpdateRegisteredIds(InvalidationHandler* handler,
-                           const ObjectIdSet& ids);
+  // at most one handler. If ID is already registered function returns false.
+  bool UpdateRegisteredIds(InvalidationHandler* handler,
+                           const ObjectIdSet& ids) WARN_UNUSED_RESULT;
 
   // Stops sending notifications to |handler|.  |handler| must not be NULL, and
   // it must already be registered.  Note that this doesn't unregister the IDs

components/invalidation/invalidator_registrar_unittest.cc

@@ -33,9 +33,9 @@ class RegistrarInvalidator : public Invalidator {
     registrar_.RegisterHandler(handler);
   }
 
-  void UpdateRegisteredIds(InvalidationHandler* handler,
+  bool UpdateRegisteredIds(InvalidationHandler* handler,
                            const ObjectIdSet& ids) override {
-    registrar_.UpdateRegisteredIds(handler, ids);
+    return registrar_.UpdateRegisteredIds(handler, ids);
   }
 
   void UnregisterHandler(InvalidationHandler* handler) override {
@@ -109,43 +109,6 @@ INSTANTIATE_TYPED_TEST_CASE_P(
     RegistrarInvalidatorTest, InvalidatorTest,
     RegistrarInvalidatorTestDelegate);
 
-class InvalidatorRegistrarTest : public testing::Test {};
-
-// Technically the test below can be part of InvalidatorTest, but we
-// want to keep the number of death tests down.
-
-// When we expect a death via CHECK(), we can't match against the
-// CHECK() message since they are removed in official builds.
-
-#if GTEST_HAS_DEATH_TEST
-
-// Multiple registrations by different handlers on the same object ID should
-// cause a CHECK.
-TEST_F(InvalidatorRegistrarTest, MultipleRegistration) {
-  const invalidation::ObjectId id1(ipc::invalidation::ObjectSource::TEST, "a");
-  const invalidation::ObjectId id2(ipc::invalidation::ObjectSource::TEST, "a");
-
-  InvalidatorRegistrar registrar;
-
-  FakeInvalidationHandler handler1;
-  registrar.RegisterHandler(&handler1);
-
-  FakeInvalidationHandler handler2;
-  registrar.RegisterHandler(&handler2);
-
-  ObjectIdSet ids;
-  ids.insert(id1);
-  ids.insert(id2);
-  registrar.UpdateRegisteredIds(&handler1, ids);
-
-  registrar.DetachFromThreadForTest();
-  EXPECT_DEATH({ registrar.UpdateRegisteredIds(&handler2, ids); }, "");
-
-  registrar.UnregisterHandler(&handler2);
-  registrar.UnregisterHandler(&handler1);
-}
-#endif  // GTEST_HAS_DEATH_TEST
-
 }  // namespace
 
 }  // namespace syncer

components/invalidation/invalidator_test_template.h

@@ -146,7 +146,7 @@ TYPED_TEST_P(InvalidatorTest, Basic) {
   ObjectIdSet ids;
   ids.insert(this->id1);
   ids.insert(this->id2);
-  invalidator->UpdateRegisteredIds(&handler, ids);
+  EXPECT_TRUE(invalidator->UpdateRegisteredIds(&handler, ids));
 
   this->delegate_.TriggerOnInvalidatorStateChange(INVALIDATIONS_ENABLED);
   EXPECT_EQ(INVALIDATIONS_ENABLED, handler.GetInvalidatorState());
@@ -161,7 +161,7 @@ TYPED_TEST_P(InvalidatorTest, Basic) {
 
   ids.erase(this->id1);
   ids.insert(this->id3);
-  invalidator->UpdateRegisteredIds(&handler, ids);
+  EXPECT_TRUE(invalidator->UpdateRegisteredIds(&handler, ids));
 
   expected_invalidations = ObjectIdInvalidationMap();
   expected_invalidations.Insert(Invalidation::Init(this->id2, 2, "2"));
@@ -210,13 +210,13 @@ TYPED_TEST_P(InvalidatorTest, MultipleHandlers) {
     ObjectIdSet ids;
     ids.insert(this->id1);
     ids.insert(this->id2);
-    invalidator->UpdateRegisteredIds(&handler1, ids);
+    EXPECT_TRUE(invalidator->UpdateRegisteredIds(&handler1, ids));
   }
 
   {
     ObjectIdSet ids;
     ids.insert(this->id3);
-    invalidator->UpdateRegisteredIds(&handler2, ids);
+    EXPECT_TRUE(invalidator->UpdateRegisteredIds(&handler2, ids));
   }
 
   // Don't register any IDs for handler3.
@@ -224,7 +224,7 @@ TYPED_TEST_P(InvalidatorTest, MultipleHandlers) {
   {
     ObjectIdSet ids;
     ids.insert(this->id4);
-    invalidator->UpdateRegisteredIds(&handler4, ids);
+    EXPECT_TRUE(invalidator->UpdateRegisteredIds(&handler4, ids));
   }
 
   invalidator->UnregisterHandler(&handler4);
@@ -272,6 +272,28 @@ TYPED_TEST_P(InvalidatorTest, MultipleHandlers) {
   invalidator->UnregisterHandler(&handler1);
 }
 
+// Multiple registrations by different handlers on the same object ID should
+// return false.
+TYPED_TEST_P(InvalidatorTest, MultipleRegistrations) {
+  Invalidator* const invalidator = this->CreateAndInitializeInvalidator();
+
+  FakeInvalidationHandler handler1;
+  FakeInvalidationHandler handler2;
+
+  invalidator->RegisterHandler(&handler1);
+  invalidator->RegisterHandler(&handler2);
+
+  // Registering both handlers for the same ObjectId. First call should succeed,
+  // second should fail.
+  ObjectIdSet ids;
+  ids.insert(this->id1);
+  EXPECT_TRUE(invalidator->UpdateRegisteredIds(&handler1, ids));
+  EXPECT_FALSE(invalidator->UpdateRegisteredIds(&handler2, ids));
+
+  invalidator->UnregisterHandler(&handler2);
+  invalidator->UnregisterHandler(&handler1);
+}
+
 // Make sure that passing an empty set to UpdateRegisteredIds clears the
 // corresponding entries for the handler.
 TYPED_TEST_P(InvalidatorTest, EmptySetUnregisters) {
@@ -289,18 +311,18 @@ TYPED_TEST_P(InvalidatorTest, EmptySetUnregisters) {
     ObjectIdSet ids;
     ids.insert(this->id1);
     ids.insert(this->id2);
-    invalidator->UpdateRegisteredIds(&handler1, ids);
+    EXPECT_TRUE(invalidator->UpdateRegisteredIds(&handler1, ids));
   }
 
   {
     ObjectIdSet ids;
     ids.insert(this->id3);
-    invalidator->UpdateRegisteredIds(&handler2, ids);
+    EXPECT_TRUE(invalidator->UpdateRegisteredIds(&handler2, ids));
   }
 
   // Unregister the IDs for the first observer. It should not receive any
   // further invalidations.
-  invalidator->UpdateRegisteredIds(&handler1, ObjectIdSet());
+  EXPECT_TRUE(invalidator->UpdateRegisteredIds(&handler1, ObjectIdSet()));
 
   this->delegate_.TriggerOnInvalidatorStateChange(INVALIDATIONS_ENABLED);
   EXPECT_EQ(INVALIDATIONS_ENABLED, handler1.GetInvalidatorState());
@@ -369,7 +391,10 @@ TYPED_TEST_P(InvalidatorTest, GetInvalidatorStateAlwaysCurrent) {
 }
 
 REGISTER_TYPED_TEST_CASE_P(InvalidatorTest,
-                           Basic, MultipleHandlers, EmptySetUnregisters,
+                           Basic,
+                           MultipleHandlers,
+                           MultipleRegistrations,
+                           EmptySetUnregisters,
                            GetInvalidatorStateAlwaysCurrent);
 
 }  // namespace syncer

components/invalidation/non_blocking_invalidator.cc

@@ -274,10 +274,11 @@ void NonBlockingInvalidator::RegisterHandler(InvalidationHandler* handler) {
   registrar_.RegisterHandler(handler);
 }
 
-void NonBlockingInvalidator::UpdateRegisteredIds(InvalidationHandler* handler,
+bool NonBlockingInvalidator::UpdateRegisteredIds(InvalidationHandler* handler,
                                                  const ObjectIdSet& ids) {
   DCHECK(parent_task_runner_->BelongsToCurrentThread());
-  registrar_.UpdateRegisteredIds(handler, ids);
+  if (!registrar_.UpdateRegisteredIds(handler, ids))
+    return false;
   if (!network_task_runner_->PostTask(
           FROM_HERE,
           base::Bind(
@@ -286,6 +287,7 @@ void NonBlockingInvalidator::UpdateRegisteredIds(InvalidationHandler* handler,
               registrar_.GetAllRegisteredIds()))) {
     NOTREACHED();
   }
+  return true;
 }
 
 void NonBlockingInvalidator::UnregisterHandler(InvalidationHandler* handler) {

components/invalidation/non_blocking_invalidator.h

@@ -55,7 +55,7 @@ class INVALIDATION_EXPORT_PRIVATE NonBlockingInvalidator
 
   // Invalidator implementation.
   void RegisterHandler(InvalidationHandler* handler) override;
-  void UpdateRegisteredIds(InvalidationHandler* handler,
+  bool UpdateRegisteredIds(InvalidationHandler* handler,
                            const ObjectIdSet& ids) override;
   void UnregisterHandler(InvalidationHandler* handler) override;
   InvalidatorState GetInvalidatorState() const override;

components/invalidation/p2p_invalidation_service.cc

@@ -46,10 +46,10 @@ void P2PInvalidationService::RegisterInvalidationHandler(
   invalidator_->RegisterHandler(handler);
 }
 
-void P2PInvalidationService::UpdateRegisteredInvalidationIds(
+bool P2PInvalidationService::UpdateRegisteredInvalidationIds(
     syncer::InvalidationHandler* handler,
     const syncer::ObjectIdSet& ids) {
-  invalidator_->UpdateRegisteredIds(handler, ids);
+  return invalidator_->UpdateRegisteredIds(handler, ids);
 }
 
 void P2PInvalidationService::UnregisterInvalidationHandler(

components/invalidation/p2p_invalidation_service.h

@@ -40,7 +40,7 @@ class P2PInvalidationService : public base::NonThreadSafe,
   // It is an error to have registered handlers when the service is destroyed.
   void RegisterInvalidationHandler(
       syncer::InvalidationHandler* handler) override;
-  void UpdateRegisteredInvalidationIds(syncer::InvalidationHandler* handler,
+  bool UpdateRegisteredInvalidationIds(syncer::InvalidationHandler* handler,
                                        const syncer::ObjectIdSet& ids) override;
   void UnregisterInvalidationHandler(
       syncer::InvalidationHandler* handler) override;

components/invalidation/p2p_invalidator.cc

@@ -158,7 +158,7 @@ void P2PInvalidator::RegisterHandler(InvalidationHandler* handler) {
   registrar_.RegisterHandler(handler);
 }
 
-void P2PInvalidator::UpdateRegisteredIds(InvalidationHandler* handler,
+bool P2PInvalidator::UpdateRegisteredIds(InvalidationHandler* handler,
                                          const ObjectIdSet& ids) {
   DCHECK(thread_checker_.CalledOnValidThread());
   ObjectIdSet new_ids;
@@ -167,12 +167,14 @@ void P2PInvalidator::UpdateRegisteredIds(InvalidationHandler* handler,
                       old_ids.begin(), old_ids.end(),
                       std::inserter(new_ids, new_ids.end()),
                       ObjectIdLessThan());
-  registrar_.UpdateRegisteredIds(handler, ids);
+  if (!registrar_.UpdateRegisteredIds(handler, ids))
+    return false;
   const P2PNotificationData notification_data(
       invalidator_client_id_,
       send_notification_target_,
       ObjectIdInvalidationMap::InvalidateAll(ids));
   SendNotificationData(notification_data);
+  return true;
 }
 
 void P2PInvalidator::UnregisterHandler(InvalidationHandler* handler) {

components/invalidation/p2p_invalidator.h

@@ -102,7 +102,7 @@ class INVALIDATION_EXPORT_PRIVATE P2PInvalidator
 
   // Invalidator implementation.
   void RegisterHandler(InvalidationHandler* handler) override;
-  void UpdateRegisteredIds(InvalidationHandler* handler,
+  bool UpdateRegisteredIds(InvalidationHandler* handler,
                            const ObjectIdSet& ids) override;
   void UnregisterHandler(InvalidationHandler* handler) override;
   InvalidatorState GetInvalidatorState() const override;

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(
+        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() {

components/invalidation/ticl_invalidation_service.h

@@ -74,7 +74,7 @@ class TiclInvalidationService : public base::NonThreadSafe,
   // It is an error to have registered handlers when the service is destroyed.
   void RegisterInvalidationHandler(
       syncer::InvalidationHandler* handler) override;
-  void UpdateRegisteredInvalidationIds(syncer::InvalidationHandler* handler,
+  bool UpdateRegisteredInvalidationIds(syncer::InvalidationHandler* handler,
                                        const syncer::ObjectIdSet& ids) override;
   void UnregisterInvalidationHandler(
       syncer::InvalidationHandler* handler) override;

sync/tools/sync_client.cc

@@ -445,8 +445,8 @@ int SyncClientMain(int argc, char* argv[]) {
   invalidator->UpdateCredentials(credentials.email, credentials.sync_token);
   scoped_ptr<InvalidatorShim> shim(new InvalidatorShim(sync_manager.get()));
   invalidator->RegisterHandler(shim.get());
-  invalidator->UpdateRegisteredIds(
-      shim.get(), ModelTypeSetToObjectIdSet(model_types));
+  CHECK(invalidator->UpdateRegisteredIds(
+      shim.get(), ModelTypeSetToObjectIdSet(model_types)));
   sync_manager->StartSyncingNormally(routing_info, base::Time());
 
   sync_loop.Run();

sync/tools/sync_listen_notifications.cc

@@ -201,8 +201,8 @@ int SyncListenNotificationsMain(int argc, char* argv[]) {
 
   // Listen for notifications for all known types.
   invalidator->RegisterHandler(&notification_printer);
-  invalidator->UpdateRegisteredIds(
-      &notification_printer, ModelTypeSetToObjectIdSet(ModelTypeSet::All()));
+  CHECK(invalidator->UpdateRegisteredIds(
+      &notification_printer, ModelTypeSetToObjectIdSet(ModelTypeSet::All())));
 
   ui_loop.Run();