Fix DCHECK when upgrading from an old profile.

Fix DCHECK when upgrading from an old profile.

BUG=478090
Committed: https://crrev.com/75aae4a64bdc138599c4e9d31f72670f34e4d16c
Cr-Commit-Position: refs/heads/master@{#326790}

[Roger Tawa OOO till Jul 10th, 2015-04-22 11:42:15]

rogerta@chromium.org changed reviewers:
+ mlerman@chromium.org, zea@chromium.org

[Roger Tawa OOO till Jul 10th, 2015-04-22 11:42:15]

Hi Mike, Nicolas,

Please take a look.

Nicolas: the changes in sync are not strictly necessary because of the temporary
code I added to signin manager, but are still worth doing in the long run, since
the code will continue to work even after the temporary code is removed.

Thanks.

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

You think you've caught everything in a big CL, and then there's always little
things...

It looks like calling ATS.GetAccountInfo(account_id).email isn't a generally
safe operation, since in two places you're wrapping the call to it a bit. A
search (
https://code.google.com/p/chromium/codesearch#search/&sq=package:chromium&typ...
) shows that there are other places where we also make the same call. Should
those call sites have the same guards?

e.g.
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/sig...
https://code.google.com/p/chromium/codesearch#chromium/src/components/autofil...

https://codereview.chromium.org/1086073006/diff/1/components/signin/core/brow...
File components/signin/core/browser/signin_manager_base.cc (right):

https://codereview.chromium.org/1086073006/diff/1/components/signin/core/brow...
components/signin/core/browser/signin_manager_base.cc:92: account_id =
pref_account_username;
The control flow could lead here even if pref_account_username is empty. Is that
desired?

https://codereview.chromium.org/1086073006/diff/1/components/signin/core/brow...
components/signin/core/browser/signin_manager_base.cc:97:
client_->GetPrefs()->ClearPref(prefs::kGoogleServicesUsername);
Wouldn't we only want to clear the kGoogleServicesUsername if we've used it to
successfully set the AuthenticatedAccountId? (e.g. !account_id.empty())

https://codereview.chromium.org/1086073006/diff/1/components/signin/core/brow...
components/signin/core/browser/signin_manager_base.cc:115:
AccountTrackerService::AccountInfo info =
nit: Can this fit on 2 lines?

[Nicolas Zea, 2015-04-22 17:20:43]

sync change LGTM

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

Thanks Mike.  Comments addressed.

https://codereview.chromium.org/1086073006/diff/1/components/signin/core/brow...
File components/signin/core/browser/signin_manager_base.cc (right):

https://codereview.chromium.org/1086073006/diff/1/components/signin/core/brow...
components/signin/core/browser/signin_manager_base.cc:92: account_id =
pref_account_username;
On 2015/04/22 13:46:37, Mike Lerman wrote:
> The control flow could lead here even if pref_account_username is empty. Is
that
> desired?

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.

https://codereview.chromium.org/1086073006/diff/1/components/signin/core/brow...
components/signin/core/browser/signin_manager_base.cc:97:
client_->GetPrefs()->ClearPref(prefs::kGoogleServicesUsername);
On 2015/04/22 13:46:37, Mike Lerman wrote:
> Wouldn't we only want to clear the kGoogleServicesUsername if we've used it to
> successfully set the AuthenticatedAccountId? (e.g. !account_id.empty())

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?

https://codereview.chromium.org/1086073006/diff/1/components/signin/core/brow...
components/signin/core/browser/signin_manager_base.cc:115:
AccountTrackerService::AccountInfo info =
On 2015/04/22 13:46:37, Mike Lerman wrote:
> nit: Can this fit on 2 lines?

Done.

[Roger Tawa OOO till Jul 10th, 2015-04-23 18:48:07]

Hi Mike,

I changed SeedAccountInfo() to allow seeding with email only.  Before this was
not really necessary, but now that is useful.  I think the CL is cleaner now. 
Please take another look.  Thanks.

[Mike Lerman, 2015-04-24 12:43:09]

This is getting much cleaner. Thanks!

https://codereview.chromium.org/1086073006/diff/80001/components/signin/core/...
File components/signin/core/browser/account_tracker_service.cc (right):

https://codereview.chromium.org/1086073006/diff/80001/components/signin/core/...
components/signin/core/browser/account_tracker_service.cc:258: if
(!gaia_id.empty()) {
nit: informal style seems to be that, rather than doing if (!condition) { <some
long block> } return, you instead do: if (condition) return; <some long block>

This (and FindAccountInfoByEmail) would turn into: if (gaia_id.empty()) return
AccountInfo(); for (...)

I don't feel at all strongly about this and you can leave this as is if you
prefer.

https://codereview.chromium.org/1086073006/diff/80001/components/signin/core/...
File components/signin/core/browser/signin_manager_base.cc (right):

https://codereview.chromium.org/1086073006/diff/80001/components/signin/core/...
components/signin/core/browser/signin_manager_base.cc:125:
account_tracker_service_->SeedAccountInfo(gaia_id, email);
Since we're making the assumption now that the ATS always is seeded with an
email, can you add a DCHECK to SeedAccountInfo and perhaps to LoadFromPrefs that
validates we always have an email?

[Roger Tawa OOO till Jul 10th, 2015-04-24 13:00:34]

https://codereview.chromium.org/1086073006/diff/80001/components/signin/core/...
File components/signin/core/browser/account_tracker_service.cc (right):

https://codereview.chromium.org/1086073006/diff/80001/components/signin/core/...
components/signin/core/browser/account_tracker_service.cc:258: if
(!gaia_id.empty()) {
On 2015/04/24 12:43:09, Mike Lerman wrote:
> nit: informal style seems to be that, rather than doing if (!condition) {
<some
> long block> } return, you instead do: if (condition) return; <some long block>
> 
> This (and FindAccountInfoByEmail) would turn into: if (gaia_id.empty()) return
> AccountInfo(); for (...)
> 
> I don't feel at all strongly about this and you can leave this as is if you
> prefer.

I think I'll leave.it as is.

https://codereview.chromium.org/1086073006/diff/80001/components/signin/core/...
File components/signin/core/browser/signin_manager_base.cc (right):

https://codereview.chromium.org/1086073006/diff/80001/components/signin/core/...
components/signin/core/browser/signin_manager_base.cc:125:
account_tracker_service_->SeedAccountInfo(gaia_id, email);
On 2015/04/24 12:43:09, Mike Lerman wrote:
> Since we're making the assumption now that the ATS always is seeded with an
> email, can you add a DCHECK to SeedAccountInfo and perhaps to LoadFromPrefs
that
> validates we always have an email?

There is a dcheck in seed by virtue of the call to pick, see function.

I don't think a dcheck in load is necessary, since the existing dcheck in seed
already guarantees that we always seed with an email.

[Mike Lerman, 2015-04-24 14:12:47]

Ok. LGTM.

[Roger Tawa OOO till Jul 10th, 2015-04-24 14:24:28]

The CQ bit was checked by rogerta@chromium.org

[Roger Tawa OOO till Jul 10th, 2015-04-24 14:24:29]

The patchset sent to the CQ was uploaded after l-g-t-m from zea@chromium.org
Link to the patchset: https://codereview.chromium.org/1086073006/#ps100001
(title: "rebased")

[commit-bot: I haz the power, 2015-04-24 14:24:37]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1086073006/100001

[commit-bot: I haz the power, 2015-04-24 14:28:11]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-04-24 14:29:02]

Patchset 6 (id:??) landed as
https://crrev.com/75aae4a64bdc138599c4e9d31f72670f34e4d16c
Cr-Commit-Position: refs/heads/master@{#326790}