Fix DCHECK when upgrading from an old profile.

components/signin/core/browser/signin_manager_base.cc

Index: components/signin/core/browser/signin_manager_base.cc
diff --git a/components/signin/core/browser/signin_manager_base.cc b/components/signin/core/browser/signin_manager_base.cc
index f6333a32f4ae7789186c3fdf0c4b62a1fda9e47a..a5adc6c13fc63302b1abedc101f90fa1f399f9ab 100644
--- a/components/signin/core/browser/signin_manager_base.cc
+++ b/components/signin/core/browser/signin_manager_base.cc
@@ -72,21 +72,33 @@ void SigninManagerBase::Initialize(PrefService* local_state) {
     if (!pref_account_username.empty() && pref_gaia_id.empty()) {
       AccountTrackerService::AccountInfo info =
           account_tracker_service_->GetAccountInfo(pref_account_username);
-      DCHECK(!info.gaia.empty());
       pref_gaia_id = info.gaia;
+
+      // If |pref_gaia_id| is still null, this means the profile has been in
+      // an auth error state for some time (since M39).  It could also mean
+      // a profile that has not been used since M33.
     }
 
     if (!pref_account_username.empty() && !pref_gaia_id.empty()) {
       account_id = account_tracker_service_->SeedAccountInfo(
           pref_gaia_id, pref_account_username);
+    } else {
+      // If chrome is still using email as the account id, use the value of the
+      // old pref directly.  Otherwise, there is no way to know what the account
+      // id should be.  The user is essentially signed out.
+      // TODO(rogerta): may want to show a toast or something.
+      if (account_tracker_service_->GetMigrationState() ==
+              AccountTrackerService::MIGRATION_NOT_STARTED) {
+        account_id = pref_account_username;

[Mike Lerman, 2015/04/22 13:46:37]

The control flow could lead here even if pref_account_username is empty. Is that
desired?

[Roger Tawa OOO till Jul 10th, 2015/04/22 17:59:29]

Yes it could.  That happens when the profile is not signed in at all.  In that
case, |account_id| stays empty and we don't call SetAuthenticatedAccountId() at
line 105 as expected.

However, this is not clear from the code flow.  I've added an if statement
around most of the code to make this clearer.

+      }
+    }
 
-      // Now remove obsolete preferences.
-      client_->GetPrefs()->ClearPref(prefs::kGoogleServicesUsername);
+    // Now remove obsolete preferences.
+    client_->GetPrefs()->ClearPref(prefs::kGoogleServicesUsername);

[Mike Lerman, 2015/04/22 13:46:37]

Wouldn't we only want to clear the kGoogleServicesUsername if we've used it to
successfully set the AuthenticatedAccountId? (e.g. !account_id.empty())

[Roger Tawa OOO till Jul 10th, 2015/04/22 17:59:28]

Moving forward, this pref is obsolete.  If we don't clear it even on failure,
then it may never get cleared.  Think of the case where the user signs back in
after the failure, in which case kGoogleServicesAccountId is now set.

If there is an failure during the upgrade process, keep the pref around won't
help for next time.  So I don't see how it would help.  Have I missed something?

 
-      // TODO(rogerta): once migration to gaia id is complete, remove
-      // kGoogleServicesUserAccountId and change all uses of that pref to
-      // kGoogleServicesAccountId.
-    }
+    // TODO(rogerta): once migration to gaia id is complete, remove
+    // kGoogleServicesUserAccountId and change all uses of that pref to
+    // kGoogleServicesAccountId.
   }
 
   if (!account_id.empty())
@@ -100,8 +112,18 @@ bool SigninManagerBase::IsSigninAllowed() const {
 }
 
 std::string SigninManagerBase::GetAuthenticatedUsername() const {
-  return account_tracker_service_->GetAccountInfo(
-      GetAuthenticatedAccountId()).email;
+  AccountTrackerService::AccountInfo info =

[Mike Lerman, 2015/04/22 13:46:37]

nit: Can this fit on 2 lines?

[Roger Tawa OOO till Jul 10th, 2015/04/22 17:59:28]

Done.

+      account_tracker_service_->GetAccountInfo(
+      GetAuthenticatedAccountId());
+
+  // Until we switch to gaia id as the account id, we'll assume we can use the
+  // account_id as an email.  This DCHECK makes sure this code is not forgotten
+  // during the migration.
+  DCHECK(!info.email.empty() ||
+      (account_tracker_service_->GetMigrationState() ==
+           AccountTrackerService::MIGRATION_NOT_STARTED));
+
+  return info.email.empty() ? GetAuthenticatedAccountId() : info.email;
 }
 
 const std::string& SigninManagerBase::GetAuthenticatedAccountId() const {
@@ -138,9 +160,13 @@ void SigninManagerBase::SetAuthenticatedAccountId(
   // Gaia id of the signed in user.
   AccountTrackerService::AccountInfo info =
       account_tracker_service_->GetAccountInfo(account_id);
-  DCHECK(!info.gaia.empty());
-  client_->GetPrefs()->SetString(prefs::kGoogleServicesUserAccountId,
-                                 info.gaia);
+
+  // When this function is called from Initialize(), it's possible for
+  // |info.gaia| to be empty when migrating from a really old profile.
+  if (!info.gaia.empty()) {
+    client_->GetPrefs()->SetString(prefs::kGoogleServicesUserAccountId,
+                                   info.gaia);
+  }
 
   // Go ahead and update the last signed in account info here as well. Once a
   // user is signed in the two preferences should match. Doing it here as