Fix issues with token refresh in AccountReconcilor

The CL was change a bit as suggested by the reviewers.

This CL now introduces a worklist model in GoogleAutoLoginHelper. Instead of firing all the refresh token events in a row, it queues up all the login requests and insert GAIA cookies one-by-one. This would let accounts to be appended instead of overwriting one another.

This also introduces GoogleAutoLoginHelperTest for some unit testing.

BUG=305249

Review URL: https://codereview.chromium.org/63253003

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@238547 0039d316-1c4b-4281-b951-d872f2087c98

chrome/android/javatests/src/org/chromium/chrome/browser/signin/OAuth2TokenServiceIntegrationTest.java

@@ -212,8 +212,9 @@ public class OAuth2TokenServiceIntegrationTest extends ChromiumTestShellTestBase
         // Run test.
         mOAuth2TokenService.validateAccounts(mContext);
 
-        // Ensure no calls have been made to the observer.
-        assertEquals(1, mObserver.getAvailableCallCount());
+        // All accounts will be notified. It is up to the observer
+        // to design if any action is needed.
+        assertEquals(2, mObserver.getAvailableCallCount());
         assertEquals(0, mObserver.getRevokedCallCount());
         assertEquals(0, mObserver.getLoadedCallCount());
     }

chrome/browser/android/signin/signin_manager_android.cc

@@ -17,7 +17,10 @@
 #include "chrome/browser/browsing_data/browsing_data_helper.h"
 #include "chrome/browser/browsing_data/browsing_data_remover.h"
 #include "chrome/browser/profiles/profile_manager.h"
+#include "chrome/browser/profiles/profiles_state.h"
 #include "chrome/browser/signin/google_auto_login_helper.h"
+#include "chrome/browser/signin/profile_oauth2_token_service.h"
+#include "chrome/browser/signin/profile_oauth2_token_service_factory.h"
 #include "chrome/browser/signin/signin_manager.h"
 #include "chrome/browser/signin/signin_manager_factory.h"
 #include "chrome/common/pref_names.h"
@@ -205,9 +208,25 @@ void SigninManagerAndroid::OnBrowsingDataRemoverDone() {
 }
 
 void SigninManagerAndroid::LogInSignedInUser(JNIEnv* env, jobject obj) {
+  if (profiles::IsNewProfileManagementEnabled()) {
+    // New Mirror code path that just fires the events and let the
+    // Account Reconcilor handles everything.
+    AndroidProfileOAuth2TokenService* token_service =
+        ProfileOAuth2TokenServiceFactory::GetForProfile(profile_);
+    const std::string& primary_acct = token_service->GetPrimaryAccountId();
+    const std::vector<std::string>& ids = token_service->GetAccounts();
+    token_service->ValidateAccounts(primary_acct, ids);
+
+  } else {
+    DVLOG(1) << "SigninManagerAndroid::LogInSignedInUser "
+        " Manually calling GoogleAutoLoginHelper";
+    // Old code path that doesn't depend on the new Account Reconcilor.
+    // We manually login.
+
     // AutoLogin deletes itself.
     GoogleAutoLoginHelper* autoLogin = new GoogleAutoLoginHelper(profile_);
     autoLogin->LogIn();
+  }
 }
 
 static jlong Init(JNIEnv* env, jobject obj) {

chrome/browser/signin/account_reconcilor.cc

@@ -24,6 +24,7 @@ AccountReconcilor::AccountReconcilor(Profile* profile)
     : profile_(profile),
       are_gaia_accounts_set_(false),
       requests_(NULL) {
+  DVLOG(1) << "AccountReconcilor::AccountReconcilor";
   RegisterWithSigninManager();
   RegisterWithCookieMonster();
 
@@ -80,6 +81,7 @@ void AccountReconcilor::UnregisterWithSigninManager() {
 }
 
 void AccountReconcilor::RegisterWithTokenService() {
+  DVLOG(1) << "AccountReconcilor::RegisterWithTokenService";
   ProfileOAuth2TokenService* token_service =
       ProfileOAuth2TokenServiceFactory::GetForProfile(profile_);
   token_service->AddObserver(this);

chrome/browser/signin/android_profile_oauth2_token_service.cc

@@ -159,7 +159,12 @@ void AndroidProfileOAuth2TokenService::ValidateAccounts(JNIEnv* env,
                                                      accounts,
                                                      &account_ids);
   std::string signed_in_account = ConvertJavaStringToUTF8(env, j_current_acc);
+  ValidateAccounts(signed_in_account, account_ids);
+}
 
+void AndroidProfileOAuth2TokenService::ValidateAccounts(
+    const std::string& signed_in_account,
+    const std::vector<std::string>& account_ids) {
   if (signed_in_account.empty())
     return;
 
@@ -167,7 +172,17 @@ void AndroidProfileOAuth2TokenService::ValidateAccounts(JNIEnv* env,
                 account_ids.end(),
                 signed_in_account) != account_ids.end()) {
     // Currently signed in account still exists among accounts on system.
+    std::vector<std::string> ids = GetAccounts();
+
+    // Always fire the primary signed in account first.
     FireRefreshTokenAvailable(signed_in_account);
+
+    for (std::vector<std::string>::iterator it = ids.begin();
+        it != ids.end(); it++) {
+      if (*it != signed_in_account) {
+        FireRefreshTokenAvailable(*it);
+      }
+    }
   } else {
     // Currently signed in account does not any longer exist among accounts on
     // system.

chrome/browser/signin/android_profile_oauth2_token_service.h

@@ -47,6 +47,11 @@ class AndroidProfileOAuth2TokenService : public ProfileOAuth2TokenService {
                         jobjectArray accounts,
                         jstring current_account);
 
+  // Takes a the signed in sync account as well as all the other
+  // android account ids and check the token status of each.
+  void ValidateAccounts(const std::string& signed_in_account,
+                        const std::vector<std::string>& account_ids);
+
   // Triggers a notification to all observers of the OAuth2TokenService that a
   // refresh token is now available. This may cause observers to retry
   // operations that require authentication.

chrome/browser/signin/google_auto_login_helper.cc

@@ -12,37 +12,79 @@
 GoogleAutoLoginHelper::GoogleAutoLoginHelper(Profile* profile)
     : profile_(profile) {}
 
-GoogleAutoLoginHelper::~GoogleAutoLoginHelper() {}
+GoogleAutoLoginHelper::~GoogleAutoLoginHelper() {
+  DCHECK(accounts_.empty());
+}
 
 void GoogleAutoLoginHelper::LogIn() {
-  uber_token_fetcher_.reset(new UbertokenFetcher(profile_, this));
+  // This is the code path for non-mirror world.
+  uber_token_fetcher_.reset(CreateNewUbertokenFetcher());
   uber_token_fetcher_->StartFetchingToken();
+  DCHECK(accounts_.empty());
 }
 
 void GoogleAutoLoginHelper::LogIn(const std::string& account_id) {
-  uber_token_fetcher_.reset(new UbertokenFetcher(profile_, this));
+  if (uber_token_fetcher_.get()) {
+    accounts_.push_back(account_id);
+  } else {
+    uber_token_fetcher_.reset(CreateNewUbertokenFetcher());
     uber_token_fetcher_->StartFetchingToken(account_id);
+  }
 }
 
 void GoogleAutoLoginHelper::OnUbertokenSuccess(const std::string& uber_token) {
-  gaia_auth_fetcher_.reset(new GaiaAuthFetcher(
-      this, GaiaConstants::kChromeSource, profile_->GetRequestContext()));
+  VLOG(1) << "GoogleAutoLoginHelper::OnUbertokenSuccess";
+  gaia_auth_fetcher_.reset(CreateNewGaiaAuthFetcher());
   gaia_auth_fetcher_->StartMergeSession(uber_token);
 }
 
 void GoogleAutoLoginHelper::OnUbertokenFailure(
     const GoogleServiceAuthError& error) {
   VLOG(1) << "Failed to retrieve ubertoken, error: " << error.ToString();
+  if (base::MessageLoop::current()) {
     base::MessageLoop::current()->DeleteSoon(FROM_HERE, this);
+  } else {
+    delete this;
+  }
 }
 
 void GoogleAutoLoginHelper::OnMergeSessionSuccess(const std::string& data) {
   DVLOG(1) << "MergeSession successful." << data;
+  if (accounts_.empty()) {
+    if (base::MessageLoop::current()) {
       base::MessageLoop::current()->DeleteSoon(FROM_HERE, this);
+    } else {
+      delete this;
+    }
+  } else {
+    uber_token_fetcher_.reset(CreateNewUbertokenFetcher());
+    uber_token_fetcher_->StartFetchingToken(accounts_.front());
+    accounts_.pop_front();
+  }
 }
 
 void GoogleAutoLoginHelper::OnMergeSessionFailure(
     const GoogleServiceAuthError& error) {
   VLOG(1) << "Failed MergeSession request, error: " << error.ToString();
+  // TODO(acleung): Depending on the return error we should probably
+  // take different actions instead of just throw our hands up.
+
+  // Clearning pending accounts for now.
+  accounts_.clear();
+
+  if (base::MessageLoop::current()) {
     base::MessageLoop::current()->DeleteSoon(FROM_HERE, this);
+  } else {
+    delete this;
+  }
+}
+
+UbertokenFetcher* GoogleAutoLoginHelper::CreateNewUbertokenFetcher() {
+  return new UbertokenFetcher(profile_, this);
+}
+
+GaiaAuthFetcher* GoogleAutoLoginHelper::CreateNewGaiaAuthFetcher() {
+ return new GaiaAuthFetcher(
+      this, GaiaConstants::kChromeSource, profile_->GetRequestContext());
 }
+

chrome/browser/signin/google_auto_login_helper.h

@@ -5,6 +5,8 @@
 #ifndef CHROME_BROWSER_ANDROID_SIGNIN_GOOGLE_AUTO_LOGIN_HELPER_H_
 #define CHROME_BROWSER_ANDROID_SIGNIN_GOOGLE_AUTO_LOGIN_HELPER_H_
 
+#include <deque>
+
 #include "chrome/browser/signin/ubertoken_fetcher.h"
 #include "google_apis/gaia/gaia_auth_consumer.h"
 
@@ -31,10 +33,15 @@ class GoogleAutoLoginHelper : public GaiaAuthConsumer,
   virtual void OnUbertokenSuccess(const std::string& token) OVERRIDE;
   virtual void OnUbertokenFailure(const GoogleServiceAuthError& error) OVERRIDE;
 
+  // For testing.
+  virtual UbertokenFetcher* CreateNewUbertokenFetcher();
+  virtual GaiaAuthFetcher* CreateNewGaiaAuthFetcher();
+
  private:
   Profile* profile_;
   scoped_ptr<GaiaAuthFetcher> gaia_auth_fetcher_;
   scoped_ptr<UbertokenFetcher> uber_token_fetcher_;
+  std::deque<std::string> accounts_;
 
   DISALLOW_COPY_AND_ASSIGN(GoogleAutoLoginHelper);
 };

chrome/browser/signin/google_auto_login_helper_unittest.cc

+// Copyright 2013 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include <algorithm>
+#include <string>
+#include <vector>
+
+#include "base/message_loop/message_loop.h"
+#include "chrome/browser/signin/google_auto_login_helper.h"
+#include "chrome/test/base/testing_profile.h"
+#include "testing/gmock/include/gmock/gmock.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace {
+
+class MockUbertokenFetcher : public UbertokenFetcher {
+ public:
+  MockUbertokenFetcher(Profile* profile, UbertokenConsumer* consumer) :
+      UbertokenFetcher(profile, consumer), account_id("") {}
+
+  void completePendingRequest() {
+    OnUberAuthTokenSuccess("mock token: " + account_id);
+  }
+
+  virtual void StartFetchingToken(const std::string& id) OVERRIDE {
+    account_id = id;
+  }
+
+  std::string account_id;
+};
+
+// Counts number of InstrumentedGoogleAutoLoginHelper created.
+// We can EXPECT_* to be zero at the end of our unit tests
+// to make sure everything is properly deleted.
+
+int total = 0;
+
+class InstrumentedGoogleAutoLoginHelper : public GoogleAutoLoginHelper {
+ public:
+  std::vector<MockUbertokenFetcher*>* mock_uber_fetchers_;
+  TestingProfile profile_;
+
+  InstrumentedGoogleAutoLoginHelper(
+    std::vector<MockUbertokenFetcher*>* fetchers) :
+    GoogleAutoLoginHelper(0), mock_uber_fetchers_(fetchers) {
+    total++;
+  }
+
+  virtual ~InstrumentedGoogleAutoLoginHelper() {
+    total--;
+  }
+
+  virtual void OnUbertokenSuccess(const std::string& token) OVERRIDE {
+    OnMergeSessionSuccess(token);
+  }
+
+  virtual UbertokenFetcher* CreateNewUbertokenFetcher() OVERRIDE {
+    MockUbertokenFetcher* m = new MockUbertokenFetcher(&profile_, this);
+    mock_uber_fetchers_->push_back(m);
+    return m;
+  }
+
+  void completePendingFetchers() {
+    while (!mock_uber_fetchers_->empty()) {
+      MockUbertokenFetcher* fetcher = mock_uber_fetchers_->back();
+      mock_uber_fetchers_->pop_back();
+      fetcher->completePendingRequest();
+    }
+  }
+
+  MOCK_METHOD1(OnMergeSessionSuccess, void(const std::string& uber_token));
+
+  void OnMergeSessionSuccessConcrete(const std::string& uber_token) {
+    GoogleAutoLoginHelper::OnMergeSessionSuccess(uber_token);
+  }
+};
+
+class GoogleAutoLoginHelperTest : public testing::Test {
+ public:
+  std::vector<MockUbertokenFetcher*> mock_uber_fetchers_;
+  base::MessageLoop message_loop_;
+};
+
+} // namespace
+
+using ::testing::Invoke;
+using ::testing::_;
+
+TEST_F(GoogleAutoLoginHelperTest, AllRequestsInOneGo) {
+  total = 0;
+
+  InstrumentedGoogleAutoLoginHelper* helper =
+      new InstrumentedGoogleAutoLoginHelper(&mock_uber_fetchers_);
+  EXPECT_CALL(*helper, OnMergeSessionSuccess("mock token: acc1@gmail.com"));
+  EXPECT_CALL(*helper, OnMergeSessionSuccess("mock token: acc2@gmail.com"));
+  EXPECT_CALL(*helper, OnMergeSessionSuccess("mock token: acc3@gmail.com"));
+  EXPECT_CALL(*helper, OnMergeSessionSuccess("mock token: acc4@gmail.com"));
+  EXPECT_CALL(*helper, OnMergeSessionSuccess("mock token: acc5@gmail.com"));
+
+  // Don't completely mock out OnMergeSessionSuccess, let GoogleAutoLoginHelper
+  // actually call OnMergeSessionSuccess to clean up it's work queue because
+  // it'll delete itself once the work queue becomes empty.
+  ON_CALL(*helper, OnMergeSessionSuccess(_)).WillByDefault(Invoke(helper,
+          &InstrumentedGoogleAutoLoginHelper::OnMergeSessionSuccessConcrete));
+  helper->LogIn("acc1@gmail.com");
+  helper->LogIn("acc2@gmail.com");
+  helper->LogIn("acc3@gmail.com");
+  helper->LogIn("acc4@gmail.com");
+  helper->LogIn("acc5@gmail.com");
+  helper->completePendingFetchers();
+
+  // Make sure GoogleAutoLoginHelper cleans itself up after everything is done.
+  message_loop_.RunUntilIdle();
+  EXPECT_EQ(0, total);
+}

chrome/browser/signin/ubertoken_fetcher.h

@@ -40,7 +40,7 @@ class UbertokenFetcher : public GaiaAuthConsumer,
 
   // Start fetching the token.
   void StartFetchingToken();
-  void StartFetchingToken(const std::string& account_id);
+  virtual void StartFetchingToken(const std::string& account_id);
 
   // Overriden from GaiaAuthConsumer
   virtual void OnUberAuthTokenSuccess(const std::string& token) OVERRIDE;

chrome/chrome_tests_unit.gypi

@@ -1242,6 +1242,7 @@
         'browser/signin/account_reconcilor_unittest.cc',
         'browser/signin/fake_auth_status_provider.cc',
         'browser/signin/fake_auth_status_provider.h',
+        'browser/signin/google_auto_login_helper_unittest.cc',
         'browser/signin/local_auth_unittest.cc',
         'browser/signin/profile_oauth2_token_service_request_unittest.cc',
         'browser/signin/profile_oauth2_token_service_unittest.cc',