[Tango->FCM] Do not perform bulk unregistering on shutdown and on handler unregistering.

* The service shouldn't unregister from the topics, when handler
the Handler is unregistering from topic (as stated in the documentation
for for UnregisterHandler).
* Sync shouldn't unregister on browser shutdown

TBR=gab@chromium.org, pavely@chromium.org

Bug: 894752, 801985
Change-Id: If676d159ba7d7beeebf325bce2e78b5847da3f76
Reviewed-on: https://chromium-review.googlesource.com/c/1277908Reviewed-by: Tatiana Gornak <melandory@chromium.org>
Reviewed-by: Gabriel Charette <gab@chromium.org>
Commit-Queue: Tatiana Gornak <melandory@chromium.org>
Cr-Commit-Position: refs/heads/master@{#599206}

chrome/browser/prefs/browser_prefs.cc

@@ -86,6 +86,7 @@
 #include "components/flags_ui/pref_service_flags_storage.h"
 #include "components/gcm_driver/gcm_channel_status_syncer.h"
 #include "components/image_fetcher/core/storage/image_cache.h"
+#include "components/invalidation/impl/invalidator_registrar_with_memory.h"
 #include "components/invalidation/impl/per_user_topic_registration_manager.h"
 #include "components/language/content/browser/geo_language_provider.h"
 #include "components/metrics/metrics_pref_names.h"
@@ -598,6 +599,7 @@ void RegisterProfilePrefs(user_prefs::PrefRegistrySyncable* registry) {
   sync_sessions::SessionSyncPrefs::RegisterProfilePrefs(registry);
   syncer::SyncPrefs::RegisterProfilePrefs(registry);
   syncer::PerUserTopicRegistrationManager::RegisterProfilePrefs(registry);
+  syncer::InvalidatorRegistrarWithMemory::RegisterProfilePrefs(registry);
   TemplateURLPrepopulateData::RegisterProfilePrefs(registry);
   translate::TranslatePrefs::RegisterProfilePrefs(registry);
   UINetworkQualityEstimatorService::RegisterProfilePrefs(registry);

components/invalidation/impl/BUILD.gn

@@ -25,6 +25,8 @@ static_library("impl") {
     "invalidator.h",
     "invalidator_registrar.cc",
     "invalidator_registrar.h",
+    "invalidator_registrar_with_memory.cc",
+    "invalidator_registrar_with_memory.h",
     "invalidator_storage.cc",
     "invalidator_storage.h",
     "mock_ack_handler.cc",

components/invalidation/impl/fcm_fake_invalidator.cc

@@ -16,7 +16,7 @@ FCMFakeInvalidator::~FCMFakeInvalidator() {}
 
 bool FCMFakeInvalidator::IsHandlerRegistered(
     InvalidationHandler* handler) const {
-  return registrar_.IsHandlerRegisteredForTest(handler);
+  return registrar_.IsHandlerRegistered(handler);
 }
 
 ObjectIdSet FCMFakeInvalidator::GetRegisteredIds(

components/invalidation/impl/fcm_invalidation_listener.cc

@@ -52,11 +52,9 @@ void FCMInvalidationListener::Start(
 }
 
 void FCMInvalidationListener::UpdateRegisteredTopics(const TopicSet& topics) {
+  ids_update_requested_ = true;
   registered_topics_ = topics;
-  if (ticl_state_ == INVALIDATIONS_ENABLED &&
-      per_user_topic_registration_manager_ && !token_.empty()) {
-    DoRegistrationUpdate();
-  }
+  DoRegistrationUpdate();
 }
 
 void FCMInvalidationListener::Ready(InvalidationClient* client) {
@@ -141,6 +139,11 @@ void FCMInvalidationListener::Drop(const invalidation::ObjectId& id,
 }
 
 void FCMInvalidationListener::DoRegistrationUpdate() {
+  if (ticl_state_ != INVALIDATIONS_ENABLED ||
+      !per_user_topic_registration_manager_ || token_.empty() ||
+      !ids_update_requested_) {
+    return;
+  }
   per_user_topic_registration_manager_->UpdateRegisteredTopics(
       registered_topics_, token_);
 

components/invalidation/impl/fcm_invalidation_listener.h

@@ -129,13 +129,18 @@ class FCMInvalidationListener : public InvalidationListener,
   // Stored to pass to |per_user_topic_registration_manager_| on start.
   TopicSet registered_topics_;
 
-  // The states of the ticl and FCN channel.
+  // The states of the ticl and FCM channel.
   InvalidatorState ticl_state_;
   InvalidatorState fcm_network_state_;
 
   std::unique_ptr<PerUserTopicRegistrationManager>
       per_user_topic_registration_manager_;
   std::string token_;
+  // Prevents call to DoRegistrationUpdate in cases when
+  // UpdateRegisteredTopics wasn't called. For example, InformTokenRecieved
+  // can trigger DoRegistrationUpdate before any invalidation handler has
+  // requested registration for topics.
+  bool ids_update_requested_ = false;
 
   base::WeakPtrFactory<FCMInvalidationListener> weak_factory_;
 

components/invalidation/impl/fcm_invalidation_service.cc

@@ -29,12 +29,14 @@ FCMInvalidationService::FCMInvalidationService(
     PrefService* pref_service,
     const syncer::ParseJSONCallback& parse_json,
     network::mojom::URLLoaderFactory* loader_factory)
-    : gcm_driver_(gcm_driver),
+    : invalidator_registrar_(pref_service),
+      gcm_driver_(gcm_driver),
       instance_id_driver_(instance_id_driver),
       identity_provider_(identity_provider),
       pref_service_(pref_service),
       parse_json_(parse_json),
-      loader_factory_(loader_factory) {}
+      loader_factory_(loader_factory),
+      update_was_requested_(false) {}
 
 FCMInvalidationService::~FCMInvalidationService() {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
@@ -61,8 +63,7 @@ void FCMInvalidationService::InitForTest(syncer::Invalidator* invalidator) {
   invalidator_.reset(invalidator);
 
   invalidator_->RegisterHandler(this);
-  CHECK(invalidator_->UpdateRegisteredIds(
-      this, invalidator_registrar_.GetAllRegisteredIds()));
+  DoUpdateRegisteredIdsIfNeeded();
 }
 
 void FCMInvalidationService::RegisterInvalidationHandler(
@@ -77,14 +78,12 @@ bool FCMInvalidationService::UpdateRegisteredInvalidationIds(
     syncer::InvalidationHandler* handler,
     const syncer::ObjectIdSet& ids) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  update_was_requested_ = true;
   DVLOG(2) << "Registering ids: " << ids.size();
   syncer::TopicSet topics = ConvertIdsToTopics(ids);
   if (!invalidator_registrar_.UpdateRegisteredTopics(handler, topics))
     return false;
-  if (invalidator_) {
-    CHECK(invalidator_->UpdateRegisteredIds(
-        this, invalidator_registrar_.GetAllRegisteredIds()));
-  }
+  DoUpdateRegisteredIdsIfNeeded();
   logger_.OnUpdateTopics(invalidator_registrar_.GetSanitizedHandlersIdsMap());
   return true;
 }
@@ -94,10 +93,6 @@ void FCMInvalidationService::UnregisterInvalidationHandler(
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   DVLOG(2) << "Unregistering";
   invalidator_registrar_.UnregisterHandler(handler);
-  if (invalidator_) {
-    CHECK(invalidator_->UpdateRegisteredIds(
-        this, invalidator_registrar_.GetAllRegisteredIds()));
-  }
   logger_.OnUnregistration(handler->GetOwnerName());
 }
 
@@ -190,10 +185,8 @@ void FCMInvalidationService::StartInvalidator() {
   invalidator_ = std::make_unique<syncer::FCMInvalidator>(
       std::move(network), identity_provider_, pref_service_, loader_factory_,
       parse_json_);
-
   invalidator_->RegisterHandler(this);
-  CHECK(invalidator_->UpdateRegisteredIds(
-      this, invalidator_registrar_.GetAllRegisteredIds()));
+  DoUpdateRegisteredIdsIfNeeded();
 }
 
 void FCMInvalidationService::StopInvalidator() {
@@ -233,4 +226,12 @@ void FCMInvalidationService::OnDeleteIDCompleted(
   // TODO(meandory): report metric in case of unsucesfull deletion.
 }
 
+void FCMInvalidationService::DoUpdateRegisteredIdsIfNeeded() {
+  if (!invalidator_ || !update_was_requested_)
+    return;
+  auto registered_ids = invalidator_registrar_.GetAllRegisteredIds();
+  CHECK(invalidator_->UpdateRegisteredIds(this, registered_ids));
+  update_was_requested_ = false;
+}
+
 }  // namespace invalidation

components/invalidation/impl/fcm_invalidation_service.h

@@ -9,7 +9,7 @@
 #include "base/timer/timer.h"
 #include "components/gcm_driver/instance_id/instance_id.h"
 #include "components/invalidation/impl/invalidation_logger.h"
-#include "components/invalidation/impl/invalidator_registrar.h"
+#include "components/invalidation/impl/invalidator_registrar_with_memory.h"
 #include "components/invalidation/public/identity_provider.h"
 #include "components/invalidation/public/invalidation_handler.h"
 #include "components/invalidation/public/invalidation_service.h"
@@ -93,7 +93,9 @@ class FCMInvalidationService : public InvalidationService,
   void OnInstanceIdRecieved(const std::string& id);
   void OnDeleteIDCompleted(instance_id::InstanceID::Result);
 
-  syncer::InvalidatorRegistrar invalidator_registrar_;
+  void DoUpdateRegisteredIdsIfNeeded();
+
+  syncer::InvalidatorRegistrarWithMemory invalidator_registrar_;
   std::unique_ptr<syncer::Invalidator> invalidator_;
 
   // The invalidation logger object we use to record state changes
@@ -108,6 +110,7 @@ class FCMInvalidationService : public InvalidationService,
   PrefService* pref_service_;
   syncer::ParseJSONCallback parse_json_;
   network::mojom::URLLoaderFactory* loader_factory_;
+  bool update_was_requested_ = false;
 
   SEQUENCE_CHECKER(sequence_checker_);
 

components/invalidation/impl/fcm_invalidation_service_unittest.cc

@@ -90,6 +90,8 @@ class FCMInvalidationServiceTestDelegate {
     pref_service_.registry()->RegisterStringPref(
         prefs::kFCMInvalidationClientIDCache,
         /*default_value=*/std::string());
+    syncer::InvalidatorRegistrarWithMemory::RegisterProfilePrefs(
+        pref_service_.registry());
   }
 
   ~FCMInvalidationServiceTestDelegate() {}

components/invalidation/impl/invalidator_registrar.cc

@@ -134,7 +134,7 @@ InvalidatorRegistrar::GetSanitizedHandlersIdsMap() {
   return clean_handlers_to_topics;
 }
 
-bool InvalidatorRegistrar::IsHandlerRegisteredForTest(
+bool InvalidatorRegistrar::IsHandlerRegistered(
     const InvalidationHandler* handler) const {
   DCHECK(thread_checker_.CalledOnValidThread());
   return handlers_.HasObserver(handler);

components/invalidation/impl/invalidator_registrar.h

@@ -33,23 +33,24 @@ class INVALIDATION_EXPORT InvalidatorRegistrar {
   // and it must already be registered.
   void RegisterHandler(InvalidationHandler* handler);
 
-  // Updates the set of topics associated with |handler|.  |handler| must
-  // not be NULL, and must already be registered.  A topic must be registered
-  // for at most one handler. If topic is already registered function returns
-  // false.
-  bool UpdateRegisteredTopics(InvalidationHandler* handler,
-                              const TopicSet& topics) 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
   // associated with |handler|.
   void UnregisterHandler(InvalidationHandler* handler);
 
-  TopicSet GetRegisteredTopics(InvalidationHandler* handler) const;
+  // Updates the set of topics associated with |handler|.  |handler| must
+  // not be NULL, and must already be registered.  A topic must be registered
+  // for at most one handler. If topic is already registered function returns
+  // false.
+  virtual bool UpdateRegisteredTopics(InvalidationHandler* handler,
+                                      const TopicSet& topics)
+      WARN_UNUSED_RESULT;
+
+  virtual TopicSet GetRegisteredTopics(InvalidationHandler* handler) const;
 
   // Returns the set of all IDs that are registered to some handler (even
   // handlers that have been unregistered).
-  TopicSet GetAllRegisteredIds() const;
+  virtual TopicSet GetAllRegisteredIds() const;
 
   // Sorts incoming invalidations into a bucket for each handler and then
   // dispatches the batched invalidations to the corresponding handler.
@@ -73,7 +74,7 @@ class INVALIDATION_EXPORT InvalidatorRegistrar {
   // to display every registered handlers and its objectsIds.
   std::map<std::string, TopicSet> GetSanitizedHandlersIdsMap();
 
-  bool IsHandlerRegisteredForTest(const InvalidationHandler* handler) const;
+  bool IsHandlerRegistered(const InvalidationHandler* handler) const;
 
   // Needed for death tests.
   void DetachFromThreadForTest();

components/invalidation/impl/invalidator_registrar_with_memory.cc

+// Copyright 2018 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "components/invalidation/impl/invalidator_registrar_with_memory.h"
+
+#include <cstddef>
+#include <iterator>
+#include <utility>
+
+#include "base/logging.h"
+#include "components/invalidation/public/object_id_invalidation_map.h"
+#include "components/invalidation/public/topic_invalidation_map.h"
+#include "components/prefs/pref_registry_simple.h"
+#include "components/prefs/scoped_user_pref_update.h"
+
+namespace syncer {
+
+namespace {
+
+const char kTopicsToHandler[] = "invalidation.topics_to_handler";
+
+}  // namespace
+
+// static
+void InvalidatorRegistrarWithMemory::RegisterProfilePrefs(
+    PrefRegistrySimple* registry) {
+  registry->RegisterDictionaryPref(kTopicsToHandler);
+}
+
+InvalidatorRegistrarWithMemory::InvalidatorRegistrarWithMemory(
+    PrefService* local_state)
+    : InvalidatorRegistrar(), local_state_(local_state) {
+  const base::Value* pref_data = local_state_->Get(kTopicsToHandler);
+  for (const auto& it : pref_data->DictItems()) {
+    Topic topic = it.first;
+    std::string handler_name;
+    it.second.GetAsString(&handler_name);
+    handler_name_to_topics_map_[handler_name].insert(topic);
+  }
+}
+
+InvalidatorRegistrarWithMemory::~InvalidatorRegistrarWithMemory() {}
+
+bool InvalidatorRegistrarWithMemory::UpdateRegisteredTopics(
+    InvalidationHandler* handler,
+    const TopicSet& topics) {
+  TopicSet old_topics = GetRegisteredTopics(handler);
+  bool success = InvalidatorRegistrar::UpdateRegisteredTopics(handler, topics);
+  if (!InvalidatorRegistrar::IsHandlerRegistered(handler)) {
+    return success;
+  }
+
+  TopicSet to_unregister;
+  DictionaryPrefUpdate update(local_state_, kTopicsToHandler);
+  std::set_difference(old_topics.begin(), old_topics.end(), topics.begin(),
+                      topics.end(),
+                      std::inserter(to_unregister, to_unregister.begin()));
+  if (!to_unregister.empty()) {
+    for (const auto& topic : to_unregister) {
+      update->RemoveKey(topic);
+      handler_name_to_topics_map_[handler->GetOwnerName()].erase(topic);
+    }
+  }
+
+  for (const auto& topic : topics) {
+    handler_name_to_topics_map_[handler->GetOwnerName()].insert(topic);
+    update->SetKey(topic, base::Value(handler->GetOwnerName()));
+  }
+  return success;
+}
+
+TopicSet InvalidatorRegistrarWithMemory::GetAllRegisteredIds() const {
+  TopicSet registered_topics;
+  for (const auto& handler_to_topic : handler_name_to_topics_map_) {
+    registered_topics.insert(handler_to_topic.second.begin(),
+                             handler_to_topic.second.end());
+  }
+  return registered_topics;
+}
+
+}  // namespace syncer

components/invalidation/impl/invalidator_registrar_with_memory.h

+// Copyright 2018 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef COMPONENTS_INVALIDATION_IMPL_INVALIDATOR_REGISTRAR_WITH_MEMORY_H_
+#define COMPONENTS_INVALIDATION_IMPL_INVALIDATOR_REGISTRAR_WITH_MEMORY_H_
+
+#include <map>
+
+#include "base/macros.h"
+#include "base/observer_list.h"
+#include "base/threading/thread_checker.h"
+#include "components/invalidation/impl/invalidator_registrar.h"
+#include "components/invalidation/public/invalidation_export.h"
+#include "components/invalidation/public/invalidation_handler.h"
+#include "components/invalidation/public/invalidation_util.h"
+#include "components/prefs/pref_registry_simple.h"
+#include "components/prefs/pref_service.h"
+
+class PrefRegistrySimple;
+class PrefService;
+
+namespace syncer {
+
+using HandlerNameTopicsMap = std::map<std::string, TopicSet>;
+
+// A helper class for implementations of the Invalidator interface.  It helps
+// keep track of registered handlers and which object ID registrations are
+// associated with which handlers, so implementors can just reuse the logic
+// here to dispatch invalidations and other interesting notifications.
+class INVALIDATION_EXPORT InvalidatorRegistrarWithMemory
+    : public InvalidatorRegistrar {
+ public:
+  InvalidatorRegistrarWithMemory(PrefService* local_state);
+
+  // It is an error to have registered handlers on destruction.
+  ~InvalidatorRegistrarWithMemory();
+
+  static void RegisterProfilePrefs(PrefRegistrySimple* registry);
+
+  // Updates the set of topics associated with |handler|.  |handler| must
+  // not be NULL, and must already be registered.  A topic must be registered
+  // for at most one handler. If topic is already registered function returns
+  // false.
+  bool UpdateRegisteredTopics(InvalidationHandler* handler,
+                              const TopicSet& topics) override
+      WARN_UNUSED_RESULT;
+
+  // void UnregisterHandler(InvalidationHandler* handler) override;
+  // void RegisterHandler(InvalidationHandler* handler) override;
+
+  // Returns the set of all IDs that are registered to some handler (even
+  // handlers that have been unregistered).
+  TopicSet GetAllRegisteredIds() const override;
+
+ private:
+  std::unordered_map<std::string, InvalidationHandler*>
+      handler_name_to_handler_;
+  HandlerNameTopicsMap handler_name_to_topics_map_;
+  PrefService* local_state_;
+
+  DISALLOW_COPY_AND_ASSIGN(InvalidatorRegistrarWithMemory);
+};
+
+}  // namespace syncer
+
+#endif  // COMPONENTS_INVALIDATION_IMPL_INVALIDATOR_REGISTRAR_WITH_MEMORY_H_

components/invalidation/public/invalidation_util.h

@@ -68,7 +68,8 @@ typedef std::map<invalidation::ObjectId, int, ObjectIdLessThan>
     ObjectIdCountMap;
 
 using Topic = std::string;
-using TopicSet = std::unordered_set<std::string>;
+// It should be std::set, since std::set_difference is used for it.
+using TopicSet = std::set<std::string>;
 
 // Caller owns the returned DictionaryValue.
 std::unique_ptr<base::DictionaryValue> ObjectIdToValue(

components/sync/driver/glue/sync_backend_host_impl.cc

@@ -149,9 +149,11 @@ void SyncBackendHostImpl::Shutdown(ShutdownReason reason) {
   DCHECK(!host_);
 
   if (invalidation_handler_registered_) {
-    bool success =
-        invalidator_->UpdateRegisteredInvalidationIds(this, ObjectIdSet());
-    DCHECK(success);
+    if (reason != BROWSER_SHUTDOWN) {
+      bool success =
+          invalidator_->UpdateRegisteredInvalidationIds(this, ObjectIdSet());
+      DCHECK(success);
+    }
     invalidator_->UnregisterInvalidationHandler(this);
     invalidator_ = nullptr;
   }