Bugfix: CrOsOAuth2TsDelegate calling FireRefreshTokensLoaded too early

|ChromeOSOAuth2TokenServiceDelegate| loads tokens at startup and calls
|FireRefreshTokensLoaded| before some token service observers may have
had a chance to register themselves. This causes them to miss the
|OnRefreshTokensLoaded| callback.

For now, the solution is to respect the temporal coupling between
|OAuth2TokenServiceDelegate::LoadCredentials| and
|OAuth2TokenService::AddObserver|, i.e. |LoadCredentials| is called
only when all observers have been added. Ideally, it should be an
observer's responsibility to check whether it has been initialized 'too
late' and may have missed the |OnRefreshTokensLoaded| notification.

Bug: 820046
Change-Id: I3ac67798d165afe230fa29e03ec7af75a9a093a9
Reviewed-on: https://chromium-review.googlesource.com/1069353
Commit-Queue: Kush Sinha <sinhak@chromium.org>
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Reviewed-by: Lutz Justen <ljusten@chromium.org>
Cr-Commit-Position: refs/heads/master@{#561422}

chrome/browser/chromeos/oauth2_token_service_delegate.cc

@@ -21,12 +21,6 @@ ChromeOSOAuth2TokenServiceDelegate::ChromeOSOAuth2TokenServiceDelegate(
     : account_tracker_service_(account_tracker_service),
       account_manager_(account_manager),
       weak_factory_(this) {
-  DCHECK(account_manager_);
-  load_credentials_state_ = LOAD_CREDENTIALS_IN_PROGRESS;
-  account_manager_->AddObserver(this);
-  account_manager_->GetAccounts(
-      base::BindOnce(&ChromeOSOAuth2TokenServiceDelegate::GetAccountsCallback,
-                     weak_factory_.GetWeakPtr()));
 }
 
 ChromeOSOAuth2TokenServiceDelegate::~ChromeOSOAuth2TokenServiceDelegate() {
@@ -109,10 +103,28 @@ std::vector<std::string> ChromeOSOAuth2TokenServiceDelegate::GetAccounts() {
   return accounts;
 }
 
+void ChromeOSOAuth2TokenServiceDelegate::LoadCredentials(
+    const std::string& primary_account_id) {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+
+  if (load_credentials_state_ != LOAD_CREDENTIALS_NOT_STARTED) {
+    return;
+  }
+
+  load_credentials_state_ = LOAD_CREDENTIALS_IN_PROGRESS;
+
+  DCHECK(account_manager_);
+  account_manager_->AddObserver(this);
+  account_manager_->GetAccounts(
+      base::BindOnce(&ChromeOSOAuth2TokenServiceDelegate::GetAccountsCallback,
+                     weak_factory_.GetWeakPtr()));
+}
+
 void ChromeOSOAuth2TokenServiceDelegate::UpdateCredentials(
     const std::string& account_id,
     const std::string& refresh_token) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  DCHECK_EQ(LOAD_CREDENTIALS_FINISHED_WITH_SUCCESS, load_credentials_state_);
   DCHECK(!account_id.empty());
   DCHECK(!refresh_token.empty());
 
@@ -146,6 +158,11 @@ void ChromeOSOAuth2TokenServiceDelegate::GetAccountsCallback(
     std::vector<AccountManager::AccountKey> account_keys) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 
+  // This callback should only be triggered during |LoadCredentials|, which
+  // implies that |load_credentials_state_| should in
+  // |LOAD_CREDENTIALS_IN_PROGRESS| state.
+  DCHECK_EQ(LOAD_CREDENTIALS_IN_PROGRESS, load_credentials_state_);
+
   load_credentials_state_ = LOAD_CREDENTIALS_FINISHED_WITH_SUCCESS;
 
   // The typical order of |OAuth2TokenService::Observer| callbacks is:

chrome/browser/chromeos/oauth2_token_service_delegate.h

@@ -43,6 +43,7 @@ class ChromeOSOAuth2TokenServiceDelegate : public OAuth2TokenServiceDelegate,
   GoogleServiceAuthError GetAuthError(
       const std::string& account_id) const override;
   std::vector<std::string> GetAccounts() override;
+  void LoadCredentials(const std::string& primary_account_id) override;
   void UpdateCredentials(const std::string& account_id,
                          const std::string& refresh_token) override;
   net::URLRequestContextGetter* GetRequestContext() const override;

chrome/browser/chromeos/oauth2_token_service_delegate_unittest.cc

@@ -100,6 +100,8 @@ class CrOSOAuthDelegateTest : public testing::Test {
 
     delegate_ = std::make_unique<ChromeOSOAuth2TokenServiceDelegate>(
         &account_tracker_service_, &account_manager_);
+    delegate_->LoadCredentials(
+        account_info_.account_id /* primary_account_id */);
   }
 
   AccountInfo CreateAccountInfoTestFixture(const std::string& gaia_id,
@@ -251,6 +253,7 @@ TEST_F(CrOSOAuthDelegateTest, BatchChangeObserversAreNotifiedOncePerBatch) {
   // Register callbacks before AccountManager has been fully initialized.
   auto delegate = std::make_unique<ChromeOSOAuth2TokenServiceDelegate>(
       &account_tracker_service_, &account_manager);
+  delegate->LoadCredentials(account1.account_id /* primary_account_id */);
   TokenServiceObserver observer;
   delegate->AddObserver(&observer);
   // Wait until AccountManager is fully initialized.