Fix DCHECK when upgrading from an old profile.

components/signin/core/browser/signin_manager_base.cc

 // Copyright 2014 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 "components/signin/core/browser/signin_manager_base.h"
 
 #include <string>
 #include <vector>
 
 #include "base/command_line.h"
@@ skipping 54 matching lines @@
     std::string pref_gaia_id =
         client_->GetPrefs()->GetString(prefs::kGoogleServicesUserAccountId);
 
     // If kGoogleServicesUserAccountId is empty, then this is either a chromeos
     // machine or a really old profile on one of the other platforms.  However
     // in this case the account tracker should have the gaia_id so fetch it
     // from there.
     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())
     SetAuthenticatedAccountId(account_id);
 }
 
 bool SigninManagerBase::IsInitialized() const { return initialized_; }
 
 bool SigninManagerBase::IsSigninAllowed() const {
   return client_->GetPrefs()->GetBoolean(prefs::kSigninAllowed);
 }
 
 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 {
   return authenticated_account_id_;
 }
 
 void SigninManagerBase::SetAuthenticatedAccountInfo(const std::string& gaia_id,
                                                     const std::string& email) {
   std::string account_id =
       account_tracker_service_->SeedAccountInfo(gaia_id, email);
@@ skipping 16 matching lines @@
   DCHECK(pref_account_id.empty() || pref_account_id == account_id)
       << "account_id=" << account_id
       << " pref_account_id=" << pref_account_id;
   authenticated_account_id_ = account_id;
   client_->GetPrefs()->SetString(prefs::kGoogleServicesAccountId, account_id);
 
   // This preference is set so that code on I/O thread has access to the
   // 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
   // opposed to on signin allows us to catch the upgrade scenario.
   client_->GetPrefs()->SetString(prefs::kGoogleServicesLastUsername,
                                  info.email);
 }
 
 bool SigninManagerBase::IsAuthenticated() const {
   return !authenticated_account_id_.empty();
@@ skipping 24 matching lines @@
   signin_diagnostics_observers_.RemoveObserver(observer);
 }
 
 void SigninManagerBase::NotifyDiagnosticsObservers(
     const TimedSigninStatusField& field,
     const std::string& value) {
   FOR_EACH_OBSERVER(SigninDiagnosticsObserver,
                     signin_diagnostics_observers_,
                     NotifySigninValueChanged(field, value));
 }