[Sync] Unsibscribe from topic-based invalidations

When standalone invalidations are enabled, we don't want to receive
topic-based invalidations. Updating interested topics after browser
startup doesn't unsubscribe from previously registered topics.

This CL extends invalidation service's interface to explicitly
unsubscribe from all topics which the handler is not interested in.

Bug: 1305524, 1051893
Change-Id: I995eac23c1808092399590fc14faa5ef8dff77d9
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3548296
Reviewed-by: Marc Treib <treib@chromium.org>
Commit-Queue: Rushan Suleymanov <rushans@google.com>
Cr-Commit-Position: refs/heads/main@{#985303}

components/invalidation/impl/fake_invalidation_service.cc

@@ -39,6 +39,11 @@ bool FakeInvalidationService::UpdateInterestedTopics(
   return invalidator_registrar_->UpdateRegisteredTopics(handler, topic_set);
 }
 
+void FakeInvalidationService::UnsubscribeFromUnregisteredTopics(
+    InvalidationHandler* handler) {
+  invalidator_registrar_->RemoveUnregisteredTopics(handler);
+}
+
 void FakeInvalidationService::UnregisterInvalidationHandler(
     InvalidationHandler* handler) {
   invalidator_registrar_->UnregisterHandler(handler);

components/invalidation/impl/fake_invalidation_service.h

@@ -32,6 +32,7 @@ class FakeInvalidationService : public InvalidationService {
   void RegisterInvalidationHandler(InvalidationHandler* handler) override;
   bool UpdateInterestedTopics(InvalidationHandler* handler,
                               const TopicSet& topics) override;
+  void UnsubscribeFromUnregisteredTopics(InvalidationHandler* handler) override;
   void UnregisterInvalidationHandler(InvalidationHandler* handler) override;
 
   InvalidatorState GetInvalidatorState() const override;

components/invalidation/impl/fcm_invalidation_service_base.cc

@@ -102,6 +102,15 @@ bool FCMInvalidationServiceBase::UpdateInterestedTopics(
   return true;
 }
 
+void FCMInvalidationServiceBase::UnsubscribeFromUnregisteredTopics(
+    InvalidationHandler* handler) {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  DVLOG(2) << "Unsubscribing from unregistered topics";
+  invalidator_registrar_.RemoveUnregisteredTopics(handler);
+  DoUpdateSubscribedTopicsIfNeeded();
+  logger_.OnUpdatedTopics(invalidator_registrar_.GetHandlerNameToTopicsMap());
+}
+
 void FCMInvalidationServiceBase::UnregisterInvalidationHandler(
     InvalidationHandler* handler) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);

components/invalidation/impl/fcm_invalidation_service_base.h

@@ -66,6 +66,7 @@ class FCMInvalidationServiceBase : public InvalidationService,
   void RegisterInvalidationHandler(InvalidationHandler* handler) override;
   bool UpdateInterestedTopics(InvalidationHandler* handler,
                               const TopicSet& topics) override;
+  void UnsubscribeFromUnregisteredTopics(InvalidationHandler* handler) override;
   void UnregisterInvalidationHandler(InvalidationHandler* handler) override;
   InvalidatorState GetInvalidatorState() const override;
   std::string GetInvalidatorClientId() const override;

components/invalidation/impl/invalidator_registrar_with_memory.cc

@@ -194,9 +194,6 @@ bool InvalidatorRegistrarWithMemory::UpdateRegisteredTopics(
     registered_handler_to_topics_map_[handler] = topics;
   }
 
-  DictionaryPrefUpdate update(prefs_, kTopicsToHandler);
-  base::Value* pref_data = update->FindDictKey(sender_id_);
-
   // This does *not* remove subscribed topics which are not registered. This
   // behaviour is used by some handlers to keep topic subscriptions after
   // browser startup even if they are not included in the first call of this
@@ -205,14 +202,12 @@ bool InvalidatorRegistrarWithMemory::UpdateRegisteredTopics(
   //
   // TODO(crbug.com/1051893): make the unsubscription behaviour consistent
   // regardless of browser restart in between.
-  auto to_unregister =
+  auto topics_to_unregister =
       base::STLSetDifference<std::set<TopicData>>(old_topics, topics);
-  for (const auto& topic : to_unregister) {
-    pref_data->RemoveKey(topic.name);
-    handler_name_to_subscribed_topics_map_[handler->GetOwnerName()].erase(
-        topic);
-  }
+  RemoveSubscribedTopics(handler, topics_to_unregister);
 
+  DictionaryPrefUpdate update(prefs_, kTopicsToHandler);
+  base::Value* pref_data = update->FindDictKey(sender_id_);
   for (const auto& topic : topics) {
     handler_name_to_subscribed_topics_map_[handler->GetOwnerName()].insert(
         topic);
@@ -224,6 +219,19 @@ bool InvalidatorRegistrarWithMemory::UpdateRegisteredTopics(
   return true;
 }
 
+void InvalidatorRegistrarWithMemory::RemoveUnregisteredTopics(
+    InvalidationHandler* handler) {
+  auto topics_to_unregister =
+      handler_name_to_subscribed_topics_map_[handler->GetOwnerName()];
+  if (registered_handler_to_topics_map_.find(handler) !=
+      registered_handler_to_topics_map_.end()) {
+    topics_to_unregister = base::STLSetDifference<std::set<TopicData>>(
+        topics_to_unregister, registered_handler_to_topics_map_[handler]);
+  }
+
+  RemoveSubscribedTopics(handler, std::move(topics_to_unregister));
+}
+
 Topics InvalidatorRegistrarWithMemory::GetRegisteredTopics(
     InvalidationHandler* handler) const {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
@@ -332,4 +340,17 @@ base::DictionaryValue InvalidatorRegistrarWithMemory::CollectDebugData() const {
   return return_value;
 }
 
+void InvalidatorRegistrarWithMemory::RemoveSubscribedTopics(
+    const InvalidationHandler* handler,
+    const std::set<TopicData>& topics_to_unsubscribe) {
+  DictionaryPrefUpdate update(prefs_, kTopicsToHandler);
+  base::Value* pref_data = update->FindDictKey(sender_id_);
+  DCHECK(pref_data);
+  for (const TopicData& topic : topics_to_unsubscribe) {
+    pref_data->RemoveKey(topic.name);
+    handler_name_to_subscribed_topics_map_[handler->GetOwnerName()].erase(
+        topic);
+  }
+}
+
 }  // namespace invalidation

components/invalidation/impl/invalidator_registrar_with_memory.h

@@ -69,6 +69,11 @@ class INVALIDATION_EXPORT InvalidatorRegistrarWithMemory {
   [[nodiscard]] bool UpdateRegisteredTopics(InvalidationHandler* handler,
                                             const std::set<TopicData>& topics);
 
+  // Unsubscribes from all topics which are associated with |handler| but were
+  // not added using UpdateRegisteredTopics(). It's useful to unsubscribe from
+  // all topics even if they were added before browser restart.
+  void RemoveUnregisteredTopics(InvalidationHandler* handler);
+
   // Returns all topics currently registered to |handler|.
   Topics GetRegisteredTopics(InvalidationHandler* handler) const;
 
@@ -118,6 +123,9 @@ class INVALIDATION_EXPORT InvalidatorRegistrarWithMemory {
   // Generate a Dictionary with all the debugging information.
   base::DictionaryValue CollectDebugData() const;
 
+  void RemoveSubscribedTopics(const InvalidationHandler* handler,
+                              const std::set<TopicData>& topics_to_unsubscribe);
+
   SEQUENCE_CHECKER(sequence_checker_);
 
   base::ObserverList<InvalidationHandler, true>::Unchecked handlers_;

components/invalidation/impl/invalidator_registrar_with_memory_unittest.cc

@@ -9,6 +9,7 @@
 #include "base/feature_list.h"
 #include "base/json/json_reader.h"
 #include "base/test/scoped_feature_list.h"
+#include "base/values.h"
 #include "components/invalidation/impl/fake_invalidation_handler.h"
 #include "components/invalidation/public/invalidation.h"
 #include "components/invalidation/public/invalidation_util.h"
@@ -28,6 +29,14 @@ namespace {
 
 constexpr char kTopicsToHandler[] = "invalidation.per_sender_topics_to_handler";
 
+base::Value MakeStoredTopicMetadata(const InvalidationHandler* handler,
+                                    TopicMetadata topic_metadata) {
+  base::Value::Dict stored_topic_metadata;
+  stored_topic_metadata.Set("handler", handler->GetOwnerName());
+  stored_topic_metadata.Set("is_public", topic_metadata.is_public);
+  return base::Value(std::move(stored_topic_metadata));
+}
+
 // Initialize the invalidator, register a handler, register some topics for that
 // handler, and then unregister the handler, dispatching invalidations in
 // between. The handler should only see invalidations when it's registered and
@@ -429,6 +438,91 @@ TEST(InvalidatorRegistrarWithMemoryTest, ShouldKeepSubscriptionsAfterRestart) {
   invalidator->UnregisterHandler(&handler);
 }
 
+TEST(InvalidatorRegistrarWithMemoryTest, ShouldRemoveAllTopics) {
+  const std::string kSenderId = "sender_id";
+  const TopicData kTopic1(/*name=*/"topic_1", /*is_public=*/true);
+  const TopicData kTopic2(/*name=*/"topic_2", /*is_public=*/true);
+
+  TestingPrefServiceSimple pref_service;
+  InvalidatorRegistrarWithMemory::RegisterProfilePrefs(pref_service.registry());
+
+  FakeInvalidationHandler handler("handler");
+
+  // Set up some previously-registered topics in the pref.
+  base::Value::Dict sender_id_topics;
+  sender_id_topics.Set(
+      kTopic1.name,
+      MakeStoredTopicMetadata(&handler, TopicMetadata{kTopic1.is_public}));
+  sender_id_topics.Set(
+      kTopic2.name,
+      MakeStoredTopicMetadata(&handler, TopicMetadata{kTopic2.is_public}));
+  base::Value::Dict stored_topics;
+  stored_topics.Set(kSenderId, std::move(sender_id_topics));
+  pref_service.Set(kTopicsToHandler, base::Value(std::move(stored_topics)));
+
+  auto invalidator = std::make_unique<InvalidatorRegistrarWithMemory>(
+      &pref_service, kSenderId, /*migrate_old_prefs=*/false);
+  invalidator->RegisterHandler(&handler);
+
+  // Verify that all topics are successfully subscribed but not registered by
+  // the |handler|.
+  ASSERT_THAT(invalidator->GetRegisteredTopics(&handler), IsEmpty());
+  ASSERT_THAT(invalidator->GetAllSubscribedTopics(),
+              UnorderedElementsAre(
+                  Pair(kTopic1.name, TopicMetadata{kTopic1.is_public}),
+                  Pair(kTopic2.name, TopicMetadata{kTopic2.is_public})));
+
+  // Unregister from all topics.
+  invalidator->RemoveUnregisteredTopics(&handler);
+  EXPECT_THAT(invalidator->GetAllSubscribedTopics(), IsEmpty());
+
+  invalidator->UnregisterHandler(&handler);
+}
+
+TEST(InvalidatorRegistrarWithMemoryTest, ShouldRemoveUnregisteredTopics) {
+  const std::string kSenderId = "sender_id";
+  const TopicData kTopic1(/*name=*/"topic_1", /*is_public=*/true);
+  const TopicData kTopic2(/*name=*/"topic_2", /*is_public=*/true);
+
+  TestingPrefServiceSimple pref_service;
+  InvalidatorRegistrarWithMemory::RegisterProfilePrefs(pref_service.registry());
+
+  FakeInvalidationHandler handler("handler");
+
+  // Set up some previously-registered topics in the pref.
+  base::Value::Dict sender_id_topics;
+  sender_id_topics.Set(
+      kTopic1.name,
+      MakeStoredTopicMetadata(&handler, TopicMetadata{kTopic1.is_public}));
+  sender_id_topics.Set(
+      kTopic2.name,
+      MakeStoredTopicMetadata(&handler, TopicMetadata{kTopic2.is_public}));
+  base::Value::Dict stored_topics;
+  stored_topics.Set(kSenderId, std::move(sender_id_topics));
+  pref_service.Set(kTopicsToHandler, base::Value(std::move(stored_topics)));
+
+  auto invalidator = std::make_unique<InvalidatorRegistrarWithMemory>(
+      &pref_service, kSenderId, /*migrate_old_prefs=*/false);
+  invalidator->RegisterHandler(&handler);
+
+  // Verify that all topics are successfully subscribed but not registered by
+  // the |handler|.
+  ASSERT_THAT(invalidator->GetRegisteredTopics(&handler), IsEmpty());
+  ASSERT_THAT(invalidator->GetAllSubscribedTopics(),
+              UnorderedElementsAre(
+                  Pair(kTopic1.name, TopicMetadata{kTopic1.is_public}),
+                  Pair(kTopic2.name, TopicMetadata{kTopic2.is_public})));
+
+  // Register to only one topic and unregister from another.
+  ASSERT_TRUE(invalidator->UpdateRegisteredTopics(&handler, {kTopic1}));
+  invalidator->RemoveUnregisteredTopics(&handler);
+  EXPECT_THAT(invalidator->GetAllSubscribedTopics(),
+              UnorderedElementsAre(
+                  Pair(kTopic1.name, TopicMetadata{kTopic1.is_public})));
+
+  invalidator->UnregisterHandler(&handler);
+}
+
 }  // namespace
 
 }  // namespace invalidation

components/invalidation/public/invalidation_service.h

@@ -46,6 +46,14 @@ class InvalidationLogger;
 //   service->UpdateInterestedTopics(client_handler, {});
 //   service->UnregisterInvalidationHandler(client_handler);
 //
+// Note that UpdateInterestedTopics() unsubscribes only from previously
+// registered topics during current browser session. To unsubscribe from *all*
+// topics, UnsubscribeFromUnregisteredTopics() should be used:
+//
+//   service->UpdateInterestedTopics(client_handler, {});
+//   service->UnsubscribeFromUnregisteredTopics(client_handler);
+//   service->UnregisterInvalidationHandler(client_handler);
+//
 // If an invalidation handler cares about the invalidator state, it should also
 // do the following when starting the client:
 //
@@ -73,24 +81,32 @@ class InvalidationService {
   // Starts sending notifications to |handler|.  |handler| must not be NULL,
   // and it must not already be registered.
   //
-  // Handler registrations are persisted across restarts of sync.
+  // Handler must unregister before browser shutdown.
   virtual void RegisterInvalidationHandler(InvalidationHandler* handler) = 0;
 
   // Updates the set of topics associated with |handler|. |handler| must not be
   // nullptr, and must already be registered. A topic must be subscribed for at
   // most one handler. If topic is already subscribed for another
-  // InvalidationHandler function returns false.
+  // InvalidationHandler function returns false. Note that this method
+  // unsubscribes only from the topics which were previously added (and does
+  // *not* unsubscribe from topics which were added before browser restart). Use
+  // UnsubscribeFromUnregisteredTopics() to explicitly unsubscribe from all
+  // unregistered topics.
   //
   // Subscribed topics are persisted across restarts of sync.
   [[nodiscard]] virtual bool UpdateInterestedTopics(
       InvalidationHandler* handler,
       const TopicSet& topics) = 0;
 
+  // Unsubscribes from all topics associated with |handler| which were not added
+  // using UpdateInterestedTopics(), even from those which were registered
+  // before browser restart.
+  virtual void UnsubscribeFromUnregisteredTopics(
+      InvalidationHandler* handler) = 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
   // associated with |handler|.
-  //
-  // Handler registrations are persisted across restarts of sync.
   virtual void UnregisterInvalidationHandler(InvalidationHandler* handler) = 0;
 
   // Returns the current invalidator state.  When called from within

components/sync/driver/glue/sync_engine_impl.cc

@@ -184,6 +184,7 @@ void SyncEngineImpl::Initialize(InitParams params) {
     invalidator_->RegisterInvalidationHandler(this);
     bool success = invalidator_->UpdateInterestedTopics(this, /*topics=*/{});
     DCHECK(success);
+    invalidator_->UnsubscribeFromUnregisteredTopics(this);
     invalidator_->UnregisterInvalidationHandler(this);
     invalidator_ = nullptr;
   }

components/sync/driver/glue/sync_engine_impl_unittest.cc

@@ -134,6 +134,10 @@ class MockInvalidationService : public invalidation::InvalidationService {
               (invalidation::InvalidationHandler * handler,
                const invalidation::TopicSet& topics),
               (override));
+  MOCK_METHOD(void,
+              UnsubscribeFromUnregisteredTopics,
+              (invalidation::InvalidationHandler * handler),
+              (override));
   MOCK_METHOD(void,
               UnregisterInvalidationHandler,
               (invalidation::InvalidationHandler * handler),
@@ -752,6 +756,7 @@ TEST_F(SyncEngineImplWithSyncInvalidationsForWalletAndOfferTest,
   // an empty TopicSet upon initialization.
   EXPECT_CALL(invalidator_,
               UpdateInterestedTopics(_, invalidation::TopicSet()));
+  EXPECT_CALL(invalidator_, UnsubscribeFromUnregisteredTopics);
   InitializeBackend(/*expect_success=*/true);
 
   EXPECT_CALL(invalidator_, UpdateInterestedTopics).Times(0);