FCMInvalidationServiceBase: Consistently notify about the client ID

FCMInvalidationServiceBase has a client ID aka InstanceID, which is
exposed to InvalidationHandlers via OnInvalidatorClientIdChange().
Before this CL, there was one sequence of events where handlers were
*not* informed about the client ID:
1) Handler registers itself (RegisterInvalidationHandler).
2) FCMInvalidationServiceBase gets initialized and restores the client
   ID from prefs, but does *not* notify handlers. <- This is the bug!
3) FCMInvalidationServiceBase receives a fresh (validated) client ID
   in OnInstanceIDReceived(), but it's (typically) identical to the
   previous, cached one, so it doesn't notify handlers.

This CL adds a notification to handlers in step 2, after the client ID
is read from prefs. It also adds a test about client ID notifications.

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

components/invalidation/impl/fake_invalidation_handler.cc

@@ -31,6 +31,10 @@ int FakeInvalidationHandler::GetInvalidationCount() const {
   return invalidation_count_;
 }
 
+const std::string& FakeInvalidationHandler::GetInvalidatorClientId() const {
+  return client_id_;
+}
+
 void FakeInvalidationHandler::OnInvalidatorStateChange(InvalidatorState state) {
   state_ = state;
 }
@@ -49,4 +53,9 @@ bool FakeInvalidationHandler::IsPublicTopic(const Topic& topic) const {
   return topic == "PREFERENCE";
 }
 
+void FakeInvalidationHandler::OnInvalidatorClientIdChange(
+    const std::string& client_id) {
+  client_id_ = client_id;
+}
+
 }  // namespace invalidation

components/invalidation/impl/fake_invalidation_handler.h

@@ -24,19 +24,22 @@ class FakeInvalidationHandler : public InvalidationHandler {
   InvalidatorState GetInvalidatorState() const;
   const TopicInvalidationMap& GetLastInvalidationMap() const;
   int GetInvalidationCount() const;
+  const std::string& GetInvalidatorClientId() const;
 
   // InvalidationHandler implementation.
   void OnInvalidatorStateChange(InvalidatorState state) override;
   void OnIncomingInvalidation(
       const TopicInvalidationMap& invalidation_map) override;
   std::string GetOwnerName() const override;
   bool IsPublicTopic(const Topic& topic) const override;
+  void OnInvalidatorClientIdChange(const std::string& client_id) override;
 
  private:
   InvalidatorState state_;
   TopicInvalidationMap last_invalidation_map_;
   int invalidation_count_;
   std::string owner_name_;
+  std::string client_id_;
 };
 
 }  // namespace invalidation

components/invalidation/impl/fcm_invalidation_service_base.cc

@@ -235,13 +235,27 @@ void FCMInvalidationServiceBase::StopInvalidatorPermanently() {
 
 void FCMInvalidationServiceBase::PopulateClientID() {
   diagnostic_info_.instance_id_requested = base::Time::Now();
+
   if (sender_id_ == kInvalidationGCMSenderId) {
     MigratePrefs(pref_service_, sender_id_);
   }
+
+  // Retrieve any client ID (aka Instance ID) from a previous run, which was
+  // cached in prefs.
   const std::string* client_id_pref =
       pref_service_->GetDictionary(prefs::kInvalidationClientIDCache)
           ->FindStringKey(sender_id_);
   client_id_ = client_id_pref ? *client_id_pref : "";
+
+  // There might already be clients (handlers) registered, so tell them about
+  // the client ID.
+  invalidator_registrar_.UpdateInvalidatorInstanceId(client_id_);
+
+  // Also retrieve a fresh (or validated) client ID. If the |client_id_| just
+  // retrieved from prefs is non-empty, then the fresh/validated one will
+  // typically be equal to it, but it's not completely guaranteed. OTOH, if
+  // |client_id_| is empty, i.e. we didn't have one previously, then this will
+  // generate/retrieve a new one.
   instance_id::InstanceID* instance_id =
       instance_id_driver_->GetInstanceID(GetApplicationName());
   instance_id->GetID(

components/invalidation/impl/fcm_invalidation_service_base.h

@@ -50,7 +50,7 @@ class FCMInvalidationServiceBase : public InvalidationService,
           per_user_topic_subscription_manager_callback,
       instance_id::InstanceIDDriver* instance_id_driver,
       PrefService* pref_service,
-      const std::string& sender_id = {});
+      const std::string& sender_id);
   FCMInvalidationServiceBase(const FCMInvalidationServiceBase& other) = delete;
   FCMInvalidationServiceBase& operator=(
       const FCMInvalidationServiceBase& other) = delete;

components/invalidation/impl/fcm_invalidation_service_unittest.cc

@@ -13,12 +13,14 @@
 #include "base/macros.h"
 #include "base/memory/ptr_util.h"
 #include "base/memory/weak_ptr.h"
+#include "base/strings/strcat.h"
 #include "base/test/task_environment.h"
 #include "base/values.h"
 #include "components/gcm_driver/fake_gcm_driver.h"
 #include "components/gcm_driver/gcm_driver.h"
 #include "components/gcm_driver/instance_id/instance_id.h"
 #include "components/gcm_driver/instance_id/instance_id_driver.h"
+#include "components/invalidation/impl/fake_invalidation_handler.h"
 #include "components/invalidation/impl/fcm_invalidation_listener.h"
 #include "components/invalidation/impl/fcm_network_handler.h"
 #include "components/invalidation/impl/fcm_sync_network_channel.h"
@@ -27,6 +29,7 @@
 #include "components/invalidation/impl/profile_identity_provider.h"
 #include "components/invalidation/public/topic_invalidation_map.h"
 #include "components/prefs/pref_registry_simple.h"
+#include "components/prefs/scoped_user_pref_update.h"
 #include "components/prefs/testing_pref_service.h"
 #include "components/signin/public/identity_manager/identity_test_environment.h"
 #include "services/data_decoder/public/cpp/test_support/in_process_data_decoder.h"
@@ -75,6 +78,7 @@ class FakeFCMInvalidationListener : public FCMInvalidationListener {
 }  // namespace
 
 const char kApplicationName[] = "com.google.chrome.fcm.invalidations";
+const char kSenderId[] = "invalidations-sender-id";
 
 class MockInstanceID : public InstanceID {
  public:
@@ -140,7 +144,8 @@ class FCMInvalidationServiceTestDelegate {
     mock_instance_id_driver_ =
         std::make_unique<testing::NiceMock<MockInstanceIDDriver>>();
     mock_instance_id_ = std::make_unique<testing::NiceMock<MockInstanceID>>();
-    ON_CALL(*mock_instance_id_driver_, GetInstanceID(kApplicationName))
+    ON_CALL(*mock_instance_id_driver_,
+            GetInstanceID(base::StrCat({kApplicationName, "-", kSenderId})))
         .WillByDefault(testing::Return(mock_instance_id_.get()));
     ON_CALL(*mock_instance_id_, GetID(_))
         .WillByDefault(testing::WithArg<0>(
@@ -155,7 +160,7 @@ class FCMInvalidationServiceTestDelegate {
         base::BindRepeating(&PerUserTopicSubscriptionManager::Create,
                             identity_provider_.get(), &pref_service_,
                             &url_loader_factory_),
-        mock_instance_id_driver_.get(), &pref_service_);
+        mock_instance_id_driver_.get(), &pref_service_, kSenderId);
   }
 
   void InitializeInvalidationService() {
@@ -199,6 +204,62 @@ INSTANTIATE_TYPED_TEST_SUITE_P(FCMInvalidationServiceTest,
                                InvalidationServiceTest,
                                FCMInvalidationServiceTestDelegate);
 
+TEST(FCMInvalidationServiceTest, NotifiesAboutInstanceID) {
+  auto delegate = std::make_unique<FCMInvalidationServiceTestDelegate>();
+
+  // Set up a cached InstanceID aka client ID stored in prefs.
+  {
+    DictionaryPrefUpdate update(&delegate->pref_service_,
+                                prefs::kInvalidationClientIDCache);
+    update->SetStringKey(kSenderId, "InstanceIDFromPrefs");
+  }
+
+  // Create the invalidation service, but do not initialize it yet.
+  delegate->CreateUninitializedInvalidationService();
+  FCMInvalidationService* invalidation_service =
+      delegate->GetInvalidationService();
+  ASSERT_TRUE(invalidation_service->GetInvalidatorClientId().empty());
+
+  // Register a handler *before* initializing the invalidation service.
+  FakeInvalidationHandler handler;
+  invalidation_service->RegisterInvalidationHandler(&handler);
+
+  // Because the invalidation service hasn't been initialized, the client ID is
+  // still empty.
+  EXPECT_TRUE(handler.GetInvalidatorClientId().empty());
+
+  // Make sure the MockInstanceID doesn't immediately provide a fresh client ID.
+  InstanceID::GetIDCallback get_id_callback;
+  EXPECT_CALL(*delegate->mock_instance_id_, GetID(_))
+      .WillOnce([&](InstanceID::GetIDCallback callback) {
+        get_id_callback = std::move(callback);
+      });
+
+  // Initialize the service. It should read the client ID from prefs.
+  delegate->InitializeInvalidationService();
+  // The invalidation service has requested a fresh client ID.
+  ASSERT_FALSE(get_id_callback.is_null());
+
+  // The invalidation service should have restored the client ID from prefs, and
+  // passed it on to the handler.
+  EXPECT_EQ(handler.GetInvalidatorClientId(), "InstanceIDFromPrefs");
+
+  // Once the invalidation service receives a fresh client ID, it should notify
+  // the handler again. (Note that in practice, the fresh ID will almost always
+  // be identical to the cached one.)
+  std::move(get_id_callback).Run("FreshInstanceID");
+  EXPECT_EQ(handler.GetInvalidatorClientId(), "FreshInstanceID");
+
+  // Another handler that gets registered should immediately be informed of the
+  // client ID.
+  FakeInvalidationHandler handler2;
+  invalidation_service->RegisterInvalidationHandler(&handler2);
+  EXPECT_EQ(handler2.GetInvalidatorClientId(), "FreshInstanceID");
+
+  invalidation_service->UnregisterInvalidationHandler(&handler2);
+  invalidation_service->UnregisterInvalidationHandler(&handler);
+}
+
 TEST(FCMInvalidationServiceTest, ClearsInstanceIDOnSignout) {
   // Set up an invalidation service and make sure it generated a client ID (aka
   // InstanceID).