Invalidations: Propagate InstanceID deletions

Before this CL, when FCMInvalidationServiceBase triggered an InstanceID
(aka client ID) deletion, it didn't actually clear its |client_id_|
member, and it didn't propagate the deletion to its clients (via the
InvalidatorRegistrar).
This CL fixes these two things.

This *probably* didn't really have consequences in practice: Only the
SyncEngine actually used the InstanceID (for reflection blocking), and
in this case it anyway gets shut down as well. Still, let's not keep
obsolete IDs around.

Bug: 1028761
Change-Id: I5c09aff83845bebab1b4fb6b7d8fd963ae91501f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1954475
Commit-Queue: Marc Treib <treib@chromium.org>
Reviewed-by: Tim Schumann <tschumann@chromium.org>
Cr-Commit-Position: refs/heads/master@{#723413}

components/invalidation/impl/fcm_invalidation_service_base.cc

@@ -161,6 +161,7 @@ void FCMInvalidationServiceBase::InitForTest(
   invalidation_listener_ = std::move(invalidation_listener);
   invalidation_listener_->StartForTest(this);
 
+  PopulateClientID();
   DoUpdateSubscribedTopicsIfNeeded();
 }
 
@@ -173,6 +174,9 @@ base::DictionaryValue FCMInvalidationServiceBase::CollectDebugData() const {
   status.SetString(
       "InvalidationService.IID-received",
       base::TimeFormatShortDateAndTime(diagnostic_info_.instance_id_received));
+  status.SetString(
+      "InvalidationService.IID-cleared",
+      base::TimeFormatShortDateAndTime(diagnostic_info_.instance_id_cleared));
   status.SetString(
       "InvalidationService.Service-stopped",
       base::TimeFormatShortDateAndTime(diagnostic_info_.service_was_stopped));
@@ -249,8 +253,18 @@ void FCMInvalidationServiceBase::ResetClientID() {
   instance_id->DeleteID(
       base::Bind(&FCMInvalidationServiceBase::OnDeleteInstanceIDCompleted,
                  base::Unretained(this)));
+
+  // Immediately clear our cached values (before we get confirmation of the
+  // deletion), since they shouldn't be used anymore. Lower layers are the
+  // source of truth, and are responsible for ensuring that the deletion
+  // actually happens.
+  client_id_.clear();
   DictionaryPrefUpdate update(pref_service_, prefs::kInvalidationClientIDCache);
   update->RemoveKey(sender_id_);
+
+  // Also let the registrar (and its observers) know that the instance ID is
+  // gone.
+  invalidator_registrar_.UpdateInvalidatorInstanceId(std::string());
 }
 
 void FCMInvalidationServiceBase::OnInstanceIDReceived(
@@ -267,8 +281,16 @@ void FCMInvalidationServiceBase::OnInstanceIDReceived(
 
 void FCMInvalidationServiceBase::OnDeleteInstanceIDCompleted(
     instance_id::InstanceID::Result result) {
+  // Note: |client_id_| and the pref were already cleared when we initiated the
+  // deletion.
+
   UMA_HISTOGRAM_ENUMERATION("FCMInvalidations.ResetClientIDStatus", result,
                             instance_id::InstanceID::Result::LAST_RESULT + 1);
+
+  diagnostic_info_.instance_id_cleared = base::Time::Now();
+
+  // TODO(crbug.com/1028761,crbug.com/1023813): This also deleted all Instance
+  // ID *tokens*; we need to let the FCMInvalidationListener know.
 }
 
 void FCMInvalidationServiceBase::DoUpdateSubscribedTopicsIfNeeded() {

components/invalidation/impl/fcm_invalidation_service_base.h

@@ -86,7 +86,7 @@ class FCMInvalidationServiceBase
   void OnInvalidatorStateChange(syncer::InvalidatorState state) override;
 
  protected:
-  // Initializes with an injected invalidator.
+  // Initializes with an injected listener.
   void InitForTest(
       std::unique_ptr<syncer::FCMInvalidationListener> invalidation_listener);
 
@@ -113,6 +113,7 @@ class FCMInvalidationServiceBase
   struct Diagnostics {
     base::Time instance_id_requested;
     base::Time instance_id_received;
+    base::Time instance_id_cleared;
     base::Time service_was_stopped;
     base::Time service_was_started;
   };

components/invalidation/impl/fcm_invalidation_service_unittest.cc

@@ -94,45 +94,23 @@ class MockInstanceID : public InstanceID {
 
   MOCK_METHOD1(GetID, void(const GetIDCallback& callback));
   MOCK_METHOD1(GetCreationTime, void(const GetCreationTimeCallback& callback));
-  void GetToken(const std::string& authorized_entity,
-                const std::string& scope,
-                const std::map<std::string, std::string>& options,
-                std::set<Flags> flags,
-                GetTokenCallback callback) override {
-    GetToken_(authorized_entity, scope, options, std::move(flags), callback);
-  }
-  MOCK_METHOD5(GetToken_,
+  MOCK_METHOD5(GetToken,
                void(const std::string& authorized_entity,
                     const std::string& scope,
                     const std::map<std::string, std::string>& options,
                     std::set<Flags> flags,
-                    GetTokenCallback& callback));
-  void ValidateToken(const std::string& authorized_entity,
-                     const std::string& scope,
-                     const std::string& token,
-                     ValidateTokenCallback callback) override {
-    ValidateToken_(authorized_entity, scope, token, callback);
-  }
-  MOCK_METHOD4(ValidateToken_,
+                    GetTokenCallback callback));
+  MOCK_METHOD4(ValidateToken,
                void(const std::string& authorized_entity,
                     const std::string& scope,
                     const std::string& token,
-                    ValidateTokenCallback& callback));
+                    ValidateTokenCallback callback));
 
- protected:
-  void DeleteTokenImpl(const std::string& authorized_entity,
-                       const std::string& scope,
-                       DeleteTokenCallback callback) override {
-    DeleteTokenImpl_(authorized_entity, scope, callback);
-  }
-  MOCK_METHOD3(DeleteTokenImpl_,
+  MOCK_METHOD3(DeleteTokenImpl,
                void(const std::string& authorized_entity,
                     const std::string& scope,
-                    DeleteTokenCallback& callback));
-  void DeleteIDImpl(DeleteIDCallback callback) override {
-    DeleteIDImpl_(callback);
-  }
-  MOCK_METHOD1(DeleteIDImpl_, void(DeleteIDCallback& callback));
+                    DeleteTokenCallback callback));
+  MOCK_METHOD1(DeleteIDImpl, void(DeleteIDCallback callback));
 
  private:
   DISALLOW_COPY_AND_ASSIGN(MockInstanceID);
@@ -178,8 +156,13 @@ class FCMInvalidationServiceTestDelegate {
 
     mock_instance_id_driver_ = std::make_unique<MockInstanceIDDriver>();
     mock_instance_id_ = std::make_unique<MockInstanceID>();
-    EXPECT_CALL(*mock_instance_id_driver_, GetInstanceID(kApplicationName))
-        .WillRepeatedly(testing::Return(mock_instance_id_.get()));
+    ON_CALL(*mock_instance_id_driver_, GetInstanceID(kApplicationName))
+        .WillByDefault(testing::Return(mock_instance_id_.get()));
+    ON_CALL(*mock_instance_id_, GetID(_))
+        .WillByDefault(testing::WithArg<0>(
+            testing::Invoke([](const InstanceID::GetIDCallback& callback) {
+              callback.Run("FakeIID");
+            })));
 
     invalidation_service_ = std::make_unique<FCMInvalidationService>(
         identity_provider_.get(),
@@ -197,7 +180,7 @@ class FCMInvalidationServiceTestDelegate {
     invalidation_service_->InitForTest(base::WrapUnique(fake_listener_));
   }
 
-  InvalidationService* GetInvalidationService() {
+  FCMInvalidationService* GetInvalidationService() {
     return invalidation_service_.get();
   }
 
@@ -233,6 +216,30 @@ INSTANTIATE_TYPED_TEST_SUITE_P(FCMInvalidationServiceTest,
                                InvalidationServiceTest,
                                FCMInvalidationServiceTestDelegate);
 
+TEST(FCMInvalidationServiceTest, ClearsInstanceIDOnSignout) {
+  // Set up an invalidation service and make sure it generated a client ID (aka
+  // InstanceID).
+  auto delegate = std::make_unique<FCMInvalidationServiceTestDelegate>();
+  delegate->CreateInvalidationService();
+  FCMInvalidationService* invalidation_service =
+      delegate->GetInvalidationService();
+  ASSERT_FALSE(invalidation_service->GetInvalidatorClientId().empty());
+
+  // Remove the active account (in practice, this means disabling
+  // Sync-the-feature, or just signing out of the content are if only
+  // Sync-the-transport was running). This should trigger deleting the
+  // InstanceID.
+  EXPECT_CALL(*delegate->mock_instance_id_, DeleteIDImpl(_));
+  invalidation_service->OnActiveAccountLogout();
+
+  // Also the cached InstanceID (aka ClientID) in the invalidation service
+  // should be gone. (Right now, the invalidation service clears its cache
+  // immediately. In the future, it might be changed to first wait for the
+  // asynchronous DeleteID operation to complete, in which case this test will
+  // have to be updated.)
+  EXPECT_TRUE(invalidation_service->GetInvalidatorClientId().empty());
+}
+
 namespace internal {
 
 class FakeCallbackContainer {