Fix race condition in ArcAuthService

Please first check the attached bug for context.

The fix is to listen on AccountTrackerService and not AccountManager
for account removals.

We cannot listen on AccountTrackerService for account updates because
AccountTrackerService does not notify its observers when an account's
LST changes.

Bug: 904978
Test: browser_tests --gtest_filter="ArcAuthServiceTest.AccountRemovalsArePropagated"
Change-Id: I75aeda8dd280734e58e509f5a34a4b1e27b09498
Reviewed-on: https://chromium-review.googlesource.com/c/1334368Reviewed-by: Hidehiko Abe <hidehiko@chromium.org>
Reviewed-by: Yury Khmel <khmel@chromium.org>
Commit-Queue: Kush Sinha <sinhak@chromium.org>
Cr-Commit-Position: refs/heads/master@{#608853}

chrome/browser/chromeos/arc/auth/arc_auth_service.cc

@@ -196,6 +196,13 @@ ArcAuthService::ArcAuthService(content::BrowserContext* browser_context,
 
   ArcSessionManager::Get()->AddObserver(this);
 
+  // |ArcAuthService| needs to listen on |AccountTrackerService| for account
+  // removals (See crbug.com/904978) and |AccountManager| for account updates
+  // (|ArcAuthService| cannot rely on |AccountTrackerService| for account
+  // updates because |AccountTrackerService| does not notify its observers when
+  // an account's LST changes).
+  account_tracker_service_->AddObserver(this);
+
   if (chromeos::switches::IsAccountManagerEnabled()) {
     // TODO(sinhak): This will need to be independent of Profile, when
     // Multi-Profile on Chrome OS is launched.
@@ -210,6 +217,7 @@ ArcAuthService::~ArcAuthService() {
   if (chromeos::switches::IsAccountManagerEnabled())
     account_manager_->RemoveObserver(this);
 
+  account_tracker_service_->RemoveObserver(this);
   ArcSessionManager::Get()->RemoveObserver(this);
   arc_bridge_service_->auth()->RemoveObserver(this);
   arc_bridge_service_->auth()->SetHost(nullptr);
@@ -466,9 +474,18 @@ void ArcAuthService::OnTokenUpserted(
 
 void ArcAuthService::OnAccountRemoved(
     const chromeos::AccountManager::AccountKey& account_key) {
+  // Ignore this notification. We depend on
+  // |AccountTrackerService::Observer::OnAccountRemoved| for account removal
+  // notifications. See crbug.com/904978.
+}
+
+void ArcAuthService::OnAccountRemoved(const AccountInfo& account_info) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
 
-  DCHECK(!IsPrimaryAccount(account_key));
+  DCHECK(!account_info.gaia.empty());
+  DCHECK(!IsPrimaryAccount(chromeos::AccountManager::AccountKey{
+      account_info.gaia /* id */, chromeos::account_manager::AccountType::
+                                      ACCOUNT_TYPE_GAIA /* account_type */}));
 
   // Ignore the update if ARC has not been provisioned yet.
   if (!arc::IsArcProvisioned(profile_))
@@ -479,10 +496,9 @@ void ArcAuthService::OnAccountRemoved(
   if (!instance)
     return;
 
-  const std::string account_name =
-      account_mapper_util_.AccountKeyToGaiaAccountInfo(account_key).email;
-  DCHECK(!account_name.empty());
-  instance->OnAccountUpdated(account_name, mojom::AccountUpdateType::REMOVAL);
+  DCHECK(!account_info.email.empty());
+  instance->OnAccountUpdated(account_info.email,
+                             mojom::AccountUpdateType::REMOVAL);
 }
 
 void ArcAuthService::OnArcInitialStart() {

chrome/browser/chromeos/arc/auth/arc_auth_service.h

@@ -20,8 +20,8 @@
 #include "components/arc/common/auth.mojom.h"
 #include "components/arc/connection_observer.h"
 #include "components/keyed_service/core/keyed_service.h"
+#include "components/signin/core/browser/account_tracker_service.h"
 
-class AccountTrackerService;
 class Profile;
 
 namespace content {
@@ -44,6 +44,7 @@ class ArcAuthService : public KeyedService,
                        public mojom::AuthHost,
                        public ConnectionObserver<mojom::AuthInstance>,
                        public chromeos::AccountManager::Observer,
+                       public AccountTrackerService::Observer,
                        public ArcSessionManager::Observer {
  public:
   // Returns singleton instance for the given BrowserContext,
@@ -87,6 +88,9 @@ class ArcAuthService : public KeyedService,
   void OnAccountRemoved(
       const chromeos::AccountManager::AccountKey& account_key) override;
 
+  // AccountTrackerService::Observer:
+  void OnAccountRemoved(const AccountInfo& account_info) override;
+
   // ArcSessionManager::Observer:
   void OnArcInitialStart() override;
 

chrome/browser/chromeos/arc/auth/arc_auth_service_browsertest.cc

@@ -497,10 +497,12 @@ IN_PROC_BROWSER_TEST_F(ArcAuthServiceTest, AccountRemovalsArePropagated) {
 
   EXPECT_EQ(0, auth_instance().num_account_removed_calls_);
 
-  chromeos::AccountManager::AccountKey account_key{
-      kSecondaryAccountGaiaId,
-      chromeos::account_manager::AccountType::ACCOUNT_TYPE_GAIA};
-  auth_service().OnAccountRemoved(account_key);
+  AccountTrackerService* account_tracker_service =
+      AccountTrackerServiceFactory::GetInstance()->GetForProfile(profile());
+  const std::string account_id =
+      account_tracker_service->FindAccountInfoByEmail(kSecondaryAccountEmail)
+          .account_id;
+  account_tracker_service->RemoveAccount(account_id);
   base::RunLoop().RunUntilIdle();
 
   EXPECT_EQ(1, auth_instance().num_account_removed_calls_);