[Invalidations] Do not update subscribed topics on duplicates

InvalidatorRegistarWithMemory didn't update registrations when there
are duplicates, but *did* update subscriptions. This behavior is
confusing and may lead to the future bugs.

This patch makes subscription/registration logic consistent.

Bug: 1020117
Change-Id: I6ddf5f84f35517570c8c246c415106e71038859b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2041470Reviewed-by: Marc Treib <treib@chromium.org>
Commit-Queue: Maksim Moskvitin <mmoskvitin@google.com>
Cr-Commit-Position: refs/heads/master@{#738964}

components/invalidation/impl/fake_invalidation_handler.cc

@@ -8,9 +8,15 @@ namespace syncer {
 
 FakeInvalidationHandler::FakeInvalidationHandler()
     : state_(DEFAULT_INVALIDATION_ERROR),
-      invalidation_count_(0) {}
+      invalidation_count_(0),
+      owner_name_("Fake") {}
 
-FakeInvalidationHandler::~FakeInvalidationHandler() {}
+FakeInvalidationHandler::FakeInvalidationHandler(const std::string& owner_name)
+    : FakeInvalidationHandler() {
+  owner_name_ = owner_name;
+}
+
+FakeInvalidationHandler::~FakeInvalidationHandler() = default;
 
 InvalidatorState FakeInvalidationHandler::GetInvalidatorState() const {
   return state_;
@@ -35,7 +41,9 @@ void FakeInvalidationHandler::OnIncomingInvalidation(
   ++invalidation_count_;
 }
 
-std::string FakeInvalidationHandler::GetOwnerName() const { return "Fake"; }
+std::string FakeInvalidationHandler::GetOwnerName() const {
+  return owner_name_;
+}
 
 bool FakeInvalidationHandler::IsPublicTopic(const syncer::Topic& topic) const {
   return topic == "PREFERENCE";

components/invalidation/impl/fake_invalidation_handler.h

@@ -17,6 +17,7 @@ namespace syncer {
 class FakeInvalidationHandler : public InvalidationHandler {
  public:
   FakeInvalidationHandler();
+  explicit FakeInvalidationHandler(const std::string& owner);
   ~FakeInvalidationHandler() override;
 
   InvalidatorState GetInvalidatorState() const;
@@ -34,6 +35,7 @@ class FakeInvalidationHandler : public InvalidationHandler {
   InvalidatorState state_;
   ObjectIdInvalidationMap last_invalidation_map_;
   int invalidation_count_;
+  std::string owner_name_;
 
   DISALLOW_COPY_AND_ASSIGN(FakeInvalidationHandler);
 };

components/invalidation/impl/invalidator_registrar_with_memory.cc

@@ -126,21 +126,16 @@ bool InvalidatorRegistrarWithMemory::UpdateRegisteredTopics(
   CHECK(handler);
   CHECK(handlers_.HasObserver(handler));
 
-  Topics old_topics = GetRegisteredTopics(handler);
-
-  bool success = !HasDuplicateTopicRegistration(handler, topics);
-
-  if (success) {
-    if (topics.empty()) {
-      registered_handler_to_topics_map_.erase(handler);
-    } else {
-      registered_handler_to_topics_map_[handler] = topics;
-    }
+  if (HasDuplicateTopicRegistration(handler, topics)) {
+    return false;
   }
 
-  // TODO(crbug.com/1020117): This seems inconsistent - if there's a duplicate,
-  // we don't update |registered_handler_to_topics_map_| but we *do* still
-  // update |handler_name_to_subscribed_topics_map_| and the prefs?!
+  Topics old_topics = GetRegisteredTopics(handler);
+  if (topics.empty()) {
+    registered_handler_to_topics_map_.erase(handler);
+  } else {
+    registered_handler_to_topics_map_[handler] = topics;
+  }
 
   DictionaryPrefUpdate update(prefs_, kTopicsToHandler);
   base::Value* pref_data = update->FindDictKey(sender_id_);
@@ -161,7 +156,7 @@ bool InvalidatorRegistrarWithMemory::UpdateRegisteredTopics(
     handler_pref.SetBoolKey(kIsPublic, topic.second.is_public);
     pref_data->SetKey(topic.first, std::move(handler_pref));
   }
-  return success;
+  return true;
 }
 
 Topics InvalidatorRegistrarWithMemory::GetRegisteredTopics(

components/invalidation/impl/invalidator_registrar_with_memory_unittest.cc

@@ -186,8 +186,8 @@ TEST(InvalidatorRegistrarWithMemoryTest, MultipleRegistrations) {
   auto invalidator = std::make_unique<InvalidatorRegistrarWithMemory>(
       &pref_service, "sender_id", /*migrate_old_prefs=*/false);
 
-  FakeInvalidationHandler handler1;
-  FakeInvalidationHandler handler2;
+  FakeInvalidationHandler handler1("owner1");
+  FakeInvalidationHandler handler2("owner2");
 
   invalidator->RegisterHandler(&handler1);
   invalidator->RegisterHandler(&handler2);
@@ -199,6 +199,11 @@ TEST(InvalidatorRegistrarWithMemoryTest, MultipleRegistrations) {
   EXPECT_FALSE(invalidator->UpdateRegisteredTopics(
       &handler2, ConvertIdsToTopics({id1}, &handler2)));
 
+  // |handler1| should still own subscription to the topic and deregistration
+  // of its topics should update subscriptions.
+  EXPECT_TRUE(invalidator->UpdateRegisteredTopics(&handler1, /*topics=*/{}));
+  EXPECT_TRUE(invalidator->GetAllSubscribedTopics().empty());
+
   invalidator->UnregisterHandler(&handler2);
   invalidator->UnregisterHandler(&handler1);
 }