diff --git a/components/invalidation/impl/fake_invalidation_service.cc b/components/invalidation/impl/fake_invalidation_service.cc index 65c60aca2a6a21..cabdc73b41e463 100644 --- a/components/invalidation/impl/fake_invalidation_service.cc +++ b/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); diff --git a/components/invalidation/impl/fake_invalidation_service.h b/components/invalidation/impl/fake_invalidation_service.h index c6d96103957acb..5b66bbbca5c815 100644 --- a/components/invalidation/impl/fake_invalidation_service.h +++ b/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; diff --git a/components/invalidation/impl/fcm_invalidation_service_base.cc b/components/invalidation/impl/fcm_invalidation_service_base.cc index 4791b6d98ecfe0..77ddd56f22bca1 100644 --- a/components/invalidation/impl/fcm_invalidation_service_base.cc +++ b/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_); diff --git a/components/invalidation/impl/fcm_invalidation_service_base.h b/components/invalidation/impl/fcm_invalidation_service_base.h index b1b4ab0db328fb..2ae0e11b9ee1fb 100644 --- a/components/invalidation/impl/fcm_invalidation_service_base.h +++ b/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; diff --git a/components/invalidation/impl/invalidator_registrar_with_memory.cc b/components/invalidation/impl/invalidator_registrar_with_memory.cc index eee48637c3c591..106dbf89182d27 100644 --- a/components/invalidation/impl/invalidator_registrar_with_memory.cc +++ b/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>(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>( + 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& 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 diff --git a/components/invalidation/impl/invalidator_registrar_with_memory.h b/components/invalidation/impl/invalidator_registrar_with_memory.h index 5896cac6caca62..3427722265909f 100644 --- a/components/invalidation/impl/invalidator_registrar_with_memory.h +++ b/components/invalidation/impl/invalidator_registrar_with_memory.h @@ -69,6 +69,11 @@ class INVALIDATION_EXPORT InvalidatorRegistrarWithMemory { [[nodiscard]] bool UpdateRegisteredTopics(InvalidationHandler* handler, const std::set& 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& topics_to_unsubscribe); + SEQUENCE_CHECKER(sequence_checker_); base::ObserverList::Unchecked handlers_; diff --git a/components/invalidation/impl/invalidator_registrar_with_memory_unittest.cc b/components/invalidation/impl/invalidator_registrar_with_memory_unittest.cc index 8299432efd2ec0..66e2db7e153f02 100644 --- a/components/invalidation/impl/invalidator_registrar_with_memory_unittest.cc +++ b/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( + &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( + &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 diff --git a/components/invalidation/public/invalidation_service.h b/components/invalidation/public/invalidation_service.h index 28b6098cbadf03..893ab68530fd60 100644 --- a/components/invalidation/public/invalidation_service.h +++ b/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 diff --git a/components/sync/driver/glue/sync_engine_impl.cc b/components/sync/driver/glue/sync_engine_impl.cc index 461a9deb8de10b..95b3473d1597c0 100644 --- a/components/sync/driver/glue/sync_engine_impl.cc +++ b/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; } diff --git a/components/sync/driver/glue/sync_engine_impl_unittest.cc b/components/sync/driver/glue/sync_engine_impl_unittest.cc index 4b7ed8455d7395..1e7cdf9b6fde6f 100644 --- a/components/sync/driver/glue/sync_engine_impl_unittest.cc +++ b/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);