ChromeOS: This CL fixes bug in UserManager::GetKnownUserAccountId .

ChromeOS: This CL fixes bug in UserManager::GetKnownUserAccountId .

The bug in GetKnownUserAccountId led to crash on user log on.

BUG=462823
TEST=manual

Committed: https://crrev.com/fdc3aee749470b6182f98c07a150e125d60e291c
Cr-Commit-Position: refs/heads/master@{#361286}

[Alexander Alekseev, 2015-11-20 09:44:25]

alemate@chromium.org changed reviewers:
+ dzhioev@chromium.org

[Alexander Alekseev, 2015-11-20 09:44:25]

Please review.

[Alexander Alekseev, 2015-11-20 09:44:31]

The CQ bit was checked by alemate@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-11-20 09:44:53]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1463753002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1463753002/1

[commit-bot: I haz the power, 2015-11-20 10:18:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-11-20 10:18:23]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Alexander Alekseev, 2015-11-20 10:21:18]

The CQ bit was checked by alemate@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-11-20 10:21:30]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1463753002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1463753002/20001

[commit-bot: I haz the power, 2015-11-20 10:54:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-11-20 10:54:35]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Alexander Alekseev, 2015-11-20 12:31:51]

The CQ bit was checked by alemate@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-11-20 12:32:54]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1463753002/30001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1463753002/30001

[commit-bot: I haz the power, 2015-11-20 12:57:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-11-20 12:57:47]

Dry run: This issue passed the CQ dry run.

[achuithb, 2015-11-20 18:39:09]

achuith@chromium.org changed reviewers:
+ achuith@chromium.org

[achuithb, 2015-11-20 18:39:09]

Is there a bug for the crash?

[achuithb, 2015-11-20 18:46:52]

https://codereview.chromium.org/1463753002/diff/30001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/screens/chrome_user_selection_screen.cc
(right):

https://codereview.chromium.org/1463753002/diff/30001/chrome/browser/chromeos...
chrome/browser/chromeos/login/screens/chrome_user_selection_screen.cc:80: const
AccountId& account_id =
Shouldn't this be AccountId account_id=

Why do you want a const reference to a temporary? I realize this works, but
isn't it confusing?

https://codereview.chromium.org/1463753002/diff/30001/chrome/browser/chromeos...
chrome/browser/chromeos/login/screens/chrome_user_selection_screen.cc:112: const
AccountId& account_id =
Shouldn't this be AccountId account_id =

https://codereview.chromium.org/1463753002/diff/30001/components/user_manager...
File components/user_manager/user_manager_base.cc (right):

https://codereview.chromium.org/1463753002/diff/30001/components/user_manager...
components/user_manager/user_manager_base.cc:1161: // The opposite case is not
possible.
I think we need more comments for the various cases.

https://codereview.chromium.org/1463753002/diff/30001/components/user_manager...
components/user_manager/user_manager_base.cc:1165: if (!user_email.empty() &&
!sanitized_email.empty() would be clearer

https://codereview.chromium.org/1463753002/diff/30001/components/user_manager...
components/user_manager/user_manager_base.cc:1171: return
AccountId::FromUserEmailGaiaId(sanitized_email, old_gaia_id);
Why is this right? SHouldn't we use the new gaia_id?

https://codereview.chromium.org/1463753002/diff/30001/components/user_manager...
components/user_manager/user_manager_base.cc:1174: std::string canonical_email;
maybe stored_email?

[achuithb, 2015-11-20 18:47:10]

Should probably add some tests since the logic is getting out of hand.

[Alexander Alekseev, 2015-11-20 23:46:55]

On 2015/11/20 18:39:09, achuithb wrote:
> Is there a bug for the crash?

No, but this problem was introduced in
https://codereview.chromium.org/1440583002/ , so this is "a fix to the larger
CL".

[Alexander Alekseev, 2015-11-21 01:06:29]

https://codereview.chromium.org/1463753002/diff/30001/components/user_manager...
File components/user_manager/user_manager_base.cc (right):

https://codereview.chromium.org/1463753002/diff/30001/components/user_manager...
components/user_manager/user_manager_base.cc:1165: if (!user_email.empty() &&
On 2015/11/20 18:46:51, achuithb wrote:
> !sanitized_email.empty() would be clearer

Done.

https://codereview.chromium.org/1463753002/diff/30001/components/user_manager...
components/user_manager/user_manager_base.cc:1174: std::string canonical_email;
On 2015/11/20 18:46:51, achuithb wrote:
> maybe stored_email?

Done.

[achuithb, 2015-11-23 22:20:50]

The CQ bit was checked by achuith@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-11-23 22:24:15]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1463753002/50001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1463753002/50001

[achuithb, 2015-11-23 22:26:10]

lgtm

https://codereview.chromium.org/1463753002/diff/50001/components/user_manager...
File components/user_manager/user_manager.cc (right):

https://codereview.chromium.org/1463753002/diff/50001/components/user_manager...
components/user_manager/user_manager.cc:108: return (gaia_id.empty()
Is this the right formatting? Shouldn't it be:
gaia_id.empty() ?
    AccountId::FromUserEmail(user_email) :
    AccountId::FromUserEmailGaiaId(...);

https://codereview.chromium.org/1463753002/diff/50001/components/user_manager...
File components/user_manager/user_manager_base.cc (right):

https://codereview.chromium.org/1463753002/diff/50001/components/user_manager...
components/user_manager/user_manager_base.cc:1178: // GetKnownUserStringPref()
retirns the first user record that matches
returns

https://codereview.chromium.org/1463753002/diff/50001/components/user_manager...
components/user_manager/user_manager_base.cc:1179: // given ID. So we will get
the first one in case of several possible.
So we will get the first one if there are multiples.

[commit-bot: I haz the power, 2015-11-23 23:08:31]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-11-23 23:08:32]

Dry run: This issue passed the CQ dry run.

[dzhioev (left Google), 2015-11-24 00:02:25]

LGTM

[Alexander Alekseev, 2015-11-24 04:26:55]

https://codereview.chromium.org/1463753002/diff/50001/components/user_manager...
File components/user_manager/user_manager.cc (right):

https://codereview.chromium.org/1463753002/diff/50001/components/user_manager...
components/user_manager/user_manager.cc:108: return (gaia_id.empty()
On 2015/11/23 22:26:10, achuithb wrote:
> Is this the right formatting? Shouldn't it be:
> gaia_id.empty() ?
>     AccountId::FromUserEmail(user_email) :
>     AccountId::FromUserEmailGaiaId(...);

This is the result of recent clang-format.

https://codereview.chromium.org/1463753002/diff/50001/components/user_manager...
File components/user_manager/user_manager_base.cc (right):

https://codereview.chromium.org/1463753002/diff/50001/components/user_manager...
components/user_manager/user_manager_base.cc:1178: // GetKnownUserStringPref()
retirns the first user record that matches
On 2015/11/23 22:26:10, achuithb wrote:
> returns

Done.

https://codereview.chromium.org/1463753002/diff/50001/components/user_manager...
components/user_manager/user_manager_base.cc:1179: // given ID. So we will get
the first one in case of several possible.
On 2015/11/23 22:26:10, achuithb wrote:
> So we will get the first one if there are multiples.

Done.

[Alexander Alekseev, 2015-11-24 04:32:39]

The CQ bit was checked by alemate@chromium.org

[Alexander Alekseev, 2015-11-24 04:32:39]

The patchset sent to the CQ was uploaded after l-g-t-m from
dzhioev@chromium.org, achuith@chromium.org
Link to the patchset: https://codereview.chromium.org/1463753002/#ps70001
(title: "Update after review.")

[commit-bot: I haz the power, 2015-11-24 04:36:19]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1463753002/70001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1463753002/70001

[commit-bot: I haz the power, 2015-11-24 06:05:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-11-24 06:05:35]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Alexander Alekseev, 2015-11-24 07:05:40]

The CQ bit was checked by alemate@chromium.org

[commit-bot: I haz the power, 2015-11-24 07:06:02]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1463753002/70001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1463753002/70001

[commit-bot: I haz the power, 2015-11-24 08:16:56]

Committed patchset #5 (id:70001)

[commit-bot: I haz the power, 2015-11-24 08:17:40]

Patchset 5 (id:??) landed as
https://crrev.com/fdc3aee749470b6182f98c07a150e125d60e291c
Cr-Commit-Position: refs/heads/master@{#361286}