Remove workaround for KeyedService lifetimes from IdentityAccessorImpl.

This logic is no longer needed after https://crrev.com/c/1372318.

Change-Id: I15db2bf94d54fd92c92cdd455d58fed9bc8421f6
Bug: 913400,933109
Reviewed-on: https://chromium-review.googlesource.com/c/1488871Reviewed-by: Sylvain Defresne <sdefresne@chromium.org>
Commit-Queue: Lowell Manners <lowell@chromium.org>
Cr-Commit-Position: refs/heads/master@{#636743}

components/signin/core/browser/signin_manager_base.cc

@@ -229,10 +229,6 @@ bool SigninManagerBase::IsAuthenticated() const {
   return !authenticated_account_id_.empty();
 }
 
-void SigninManagerBase::Shutdown() {
-  on_shutdown_callback_list_.Notify();
-}
-
 void SigninManagerBase::AddObserver(Observer* observer) {
   observer_list_.AddObserver(observer);
 }

components/signin/core/browser/signin_manager_base.h

@@ -125,22 +125,10 @@ class SigninManagerBase : public KeyedService {
   // Returns true if there is an authenticated user.
   bool IsAuthenticated() const;
 
-  // KeyedService implementation.
-  void Shutdown() override;
-
   // Methods to register or remove observers of signin.
   void AddObserver(Observer* observer);
   void RemoveObserver(Observer* observer);
 
-  // Adds a callback that will be called when this instance is shut down.Not
-  // intended for general usage, but rather for usage only by the Identity
-  // Service implementation during the time period of conversion of Chrome to
-  // use the Identity Service.
-  std::unique_ptr<base::CallbackList<void()>::Subscription>
-  RegisterOnShutdownCallback(const base::Closure& cb) {
-    return on_shutdown_callback_list_.Add(cb);
-  }
-
  protected:
   SigninClient* signin_client() const { return client_; }
 

services/identity/identity_accessor_impl.cc

@@ -74,10 +74,6 @@ IdentityAccessorImpl::IdentityAccessorImpl(
       account_tracker_(account_tracker),
       signin_manager_(signin_manager),
       token_service_(token_service) {
-  signin_manager_shutdown_subscription_ =
-      signin_manager_->RegisterOnShutdownCallback(
-          base::BindRepeating(&IdentityAccessorImpl::OnSigninManagerShutdown,
-                              base::Unretained(this)));
   binding_.set_connection_error_handler(base::BindRepeating(
       &IdentityAccessorImpl::OnConnectionError, base::Unretained(this)));
 
@@ -185,10 +181,6 @@ AccountState IdentityAccessorImpl::GetStateOfAccount(
   return account_state;
 }
 
-void IdentityAccessorImpl::OnSigninManagerShutdown() {
-  delete this;
-}
-
 void IdentityAccessorImpl::OnConnectionError() {
   delete this;
 }

services/identity/identity_accessor_impl.h

@@ -96,12 +96,6 @@ class IdentityAccessorImpl : public mojom::IdentityAccessor,
   // Gets the current state of the account represented by |account_info|.
   AccountState GetStateOfAccount(const AccountInfo& account_info);
 
-  // Called when |signin_manager_| is shutting down. Destroys this instance,
-  // since this instance can't outlive the signin classes that it is depending
-  // on. Note that once IdentityAccessorImpl manages the lifetime of its
-  // dependencies internally, this will no longer be necessary.
-  void OnSigninManagerShutdown();
-
   // Called when |binding_| hits a connection error. Destroys this instance,
   // since it's no longer needed.
   void OnConnectionError();
@@ -111,9 +105,6 @@ class IdentityAccessorImpl : public mojom::IdentityAccessor,
   SigninManagerBase* signin_manager_;
   ProfileOAuth2TokenService* token_service_;
 
-  std::unique_ptr<base::CallbackList<void()>::Subscription>
-      signin_manager_shutdown_subscription_;
-
   // The set of pending requests for access tokens.
   AccessTokenRequests access_token_requests_;
 

services/identity/identity_accessor_impl_unittest.cc

@@ -58,9 +58,13 @@ class IdentityAccessorImplTest : public testing::Test {
   }
 
   void TearDown() override {
-    // Shut down the SigninManager so that the IdentityAccessorImpl doesn't end
-    // up outliving it.
-    signin_manager_.Shutdown();
+    // Explicitly destruct IdentityAccessorImpl so that it doesn't outlive its
+    // dependencies.
+    ResetIdentityAccessorImpl();
+
+    // Wait for the IdentityAccessorImpl to be notified of disconnection and
+    // destroy itself.
+    base::RunLoop().RunUntilIdle();
   }
 
   void OnReceivedPrimaryAccountInfo(
@@ -160,77 +164,6 @@ class IdentityAccessorImplTest : public testing::Test {
   DISALLOW_COPY_AND_ASSIGN(IdentityAccessorImplTest);
 };
 
-// Tests that it is not possible to connect to the IdentityAccessor if
-// initiated after SigninManager shutdown.
-TEST_F(IdentityAccessorImplTest, SigninManagerShutdownBeforeConnection) {
-  AccountInfo sentinel;
-  sentinel.account_id = "sentinel";
-  primary_account_info_ = sentinel;
-
-  // Ensure that the Identity Service has actually been created before
-  // invoking SigninManagerBase::Shutdown(), since otherwise this test will
-  // spin forever. Then reset the IdentityAccessor so that the next request
-  // makes a fresh connection.
-  FlushIdentityAccessorImplForTesting();
-  ResetIdentityAccessorImpl();
-
-  // Make a call to connect to the IdentityAccessorImpl *after* SigninManager
-  // shutdown; it should get notified of an error when the Identity Service
-  // drops the connection.
-  signin_manager()->Shutdown();
-  base::RunLoop run_loop;
-  SetIdentityAccessorImplConnectionErrorHandler(run_loop.QuitClosure());
-
-  GetIdentityAccessorImpl()->GetPrimaryAccountInfo(base::BindRepeating(
-      &IdentityAccessorImplTest::OnReceivedPrimaryAccountInfo,
-      base::Unretained(this), run_loop.QuitClosure()));
-  run_loop.Run();
-
-  // Verify that the callback to GetPrimaryAccountInfo() was not invoked.
-  EXPECT_TRUE(primary_account_info_);
-  EXPECT_EQ("sentinel", primary_account_info_->account_id);
-}
-
-// Tests that the IdentityAccessor destroys itself on SigninManager shutdown.
-TEST_F(IdentityAccessorImplTest, SigninManagerShutdownAfterConnection) {
-  base::RunLoop run_loop;
-  SetIdentityAccessorImplConnectionErrorHandler(run_loop.QuitClosure());
-
-  // Ensure that the IdentityAccessorImpl instance has actually been created
-  // before invoking SigninManagerBase::Shutdown(), since otherwise this test
-  // will spin forever.
-  FlushIdentityAccessorImplForTesting();
-  signin_manager()->Shutdown();
-  run_loop.Run();
-}
-
-// Tests that the IdentityAccessor properly handles its own destruction in the
-// case where there is an active consumer request (i.e., a pending callback from
-// a Mojo call). In particular, this flow should not cause a DCHECK to fire in
-// debug mode.
-TEST_F(IdentityAccessorImplTest,
-       IdentityAccessorImplShutdownWithActiveRequest) {
-  base::RunLoop run_loop;
-  SetIdentityAccessorImplConnectionErrorHandler(run_loop.QuitClosure());
-
-  // Call a method on the IdentityAccessorImpl that will cause it to store a
-  // pending callback. This callback will never be invoked, so just pass dummy
-  // arguments to it.
-  GetIdentityAccessorImpl()->GetPrimaryAccountWhenAvailable(base::BindRepeating(
-      &IdentityAccessorImplTest::OnPrimaryAccountAvailable,
-      base::Unretained(this), base::RepeatingClosure(), nullptr, nullptr));
-
-  // Ensure that the IdentityAccessorImpl has received the above call before
-  // invoking SigninManagerBase::Shutdown(), as otherwise this test is
-  // pointless.
-  FlushIdentityAccessorImplForTesting();
-
-  // This flow is what would cause a DCHECK to fire if IdentityAccessorImpl is
-  // not properly closing its binding on shutdown.
-  signin_manager()->Shutdown();
-  run_loop.Run();
-}
-
 // Check that the primary account info is null if not signed in.
 TEST_F(IdentityAccessorImplTest, GetPrimaryAccountInfoNotSignedIn) {
   base::RunLoop run_loop;

services/identity/identity_service.cc

@@ -19,9 +19,6 @@ IdentityService::IdentityService(AccountTrackerService* account_tracker,
       token_service_(token_service) {
   registry_.AddInterface<mojom::IdentityAccessor>(
       base::Bind(&IdentityService::Create, base::Unretained(this)));
-  signin_manager_shutdown_subscription_ =
-      signin_manager_->RegisterOnShutdownCallback(
-          base::Bind(&IdentityService::ShutDown, base::Unretained(this)));
 }
 
 IdentityService::~IdentityService() {
@@ -40,7 +37,6 @@ void IdentityService::ShutDown() {
     return;
 
   signin_manager_ = nullptr;
-  signin_manager_shutdown_subscription_.reset();
   token_service_ = nullptr;
   account_tracker_ = nullptr;
 }

services/identity/identity_service.h

@@ -45,9 +45,6 @@ class IdentityService : public service_manager::Service {
   SigninManagerBase* signin_manager_;
   ProfileOAuth2TokenService* token_service_;
 
-  std::unique_ptr<base::CallbackList<void()>::Subscription>
-      signin_manager_shutdown_subscription_;
-
   service_manager::BinderRegistry registry_;
 
   DISALLOW_COPY_AND_ASSIGN(IdentityService);