GetAccountIdFromProfile() should not return an empty account id if a valid profile is provided.

GetAccountIdFromProfile() should not return an empty account id if a valid profile is provided.

On Chrome OS, we don't end the session immediately if a refresh token is revoked in the session. Thus it's possible that we get an empty account id for a profile from Profile::GetProfileUserName(). We should avoid this scenario on Chrome OS.

BUG=602807, 532535
Committed: https://crrev.com/d2a0bb5307f8d0d09aff9d5d531dd9704fe85841
Cr-Commit-Position: refs/heads/master@{#392133}

[xdai1, 2016-04-12 23:25:28]

xdai@chromium.org changed reviewers:
+ skuhne@chromium.org

[xdai1, 2016-04-12 23:25:29]

skuhne@, could you help review this CL please? Thanks!

In my previous CL https://codereview.chromium.org/1704623002 I modified the
GetProfileFromAccountId() function to always return a valid profile for an valid
account id, however, it seems I should modify GetAccountIdFromProfile() function
instead since it's still possible for GetAccountIdFromProfile() to return an
empty account id for a valid profile, which might be the cause of the issue
602807.

[xdai1, 2016-04-13 20:24:26]

Patchset #2 (id:20001) has been deleted

[xdai1, 2016-04-14 16:23:52]

On 2016/04/12 23:25:29, xdai1 wrote:
> skuhne@, could you help review this CL please? Thanks!
> 
> In my previous CL https://codereview.chromium.org/1704623002 I modified the
> GetProfileFromAccountId() function to always return a valid profile for an
valid
> account id, however, it seems I should modify GetAccountIdFromProfile()
function
> instead since it's still possible for GetAccountIdFromProfile() to return an
> empty account id for a valid profile, which might be the cause of the issue
> 602807.

kindly ping?

[Mr4D (OOO till 08-26), 2016-04-14 17:44:25]

lgtm!

https://codereview.chromium.org/1880923003/diff/40001/chrome/browser/signin/s...
File chrome/browser/signin/signin_error_notifier_ash_unittest.cc (right):

https://codereview.chromium.org/1880923003/diff/40001/chrome/browser/signin/s...
chrome/browser/signin/signin_error_notifier_ash_unittest.cc:117:
chromeos::MockUserManager* mock_user_manager_;
You might want to add a comment that this is owned by the user_manager_enabler_.

[xdai1, 2016-04-14 18:18:25]

xdai@chromium.org changed reviewers:
+ thakis@chromium.org

[xdai1, 2016-04-14 18:18:26]

thakis@, could you help review this CL please? Thanks!

[xdai1, 2016-04-15 21:19:01]

On 2016/04/14 18:18:26, xdai1 wrote:
> thakis@, could you help review this CL please? Thanks!

kindly ping?

[xdai1, 2016-04-18 16:46:06]

ping again? Thanks!

[Nico, 2016-04-18 19:49:08]

Is there a test for the new behavior? (There were a few test changes, but it
wasn't clear to me if one of those checks that account id isn't empty)

[xdai1, 2016-04-18 20:11:35]

On 2016/04/18 19:49:08, Nico wrote:
> Is there a test for the new behavior? (There were a few test changes, but it
> wasn't clear to me if one of those checks that account id isn't empty)

No I don't think there is an existing test to cover this. I will try to work on
a test.

[xdai1, 2016-04-21 23:42:05]

Nico, please take another look, Thanks! I added the test file:
multi_user_util_chromeos_unittest.cc

[xdai1, 2016-04-25 16:16:00]

On 2016/04/21 23:42:05, xdai1 wrote:
> Nico, please take another look, Thanks! I added the test file:
> multi_user_util_chromeos_unittest.cc

Ping? Thanks!

[xdai1, 2016-04-26 23:23:06]

On 2016/04/25 16:16:00, xdai1 wrote:
> On 2016/04/21 23:42:05, xdai1 wrote:
> > Nico, please take another look, Thanks! I added the test file:
> > multi_user_util_chromeos_unittest.cc
> 
> Ping? Thanks!

Ping again? Thanks!

[Nico, 2016-05-02 23:11:07]

lgtm

[xdai1, 2016-05-04 17:05:02]

The CQ bit was checked by xdai@chromium.org

[xdai1, 2016-05-04 17:05:02]

The patchset sent to the CQ was uploaded after l-g-t-m from skuhne@chromium.org
Link to the patchset: https://codereview.chromium.org/1880923003/#ps80001
(title: "Add unittest. Rebase")

[commit-bot: I haz the power, 2016-05-04 17:05:20]

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

[commit-bot: I haz the power, 2016-05-04 17:12:13]

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

[commit-bot: I haz the power, 2016-05-04 17:12:14]

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

[xdai1, 2016-05-04 17:17:16]

xdai@chromium.org changed reviewers:
+ rogerta@chromium.org

[xdai1, 2016-05-04 17:17:17]

rogerta@, could you help do an owner review of the following files please?
Thanks!
    components/signin/core/browser/fake_signin_manager.cc
    components/signin/core/browser/fake_signin_manager.h

[Roger Tawa OOO till Jul 10th, 2016-05-05 14:27:21]

Hi Xiaoqian,

Can you explain the root cause of the problem?  The linked bugs describes the
effects and there are guesses as to the cause, but nothing definitive.

I ask because cros forces users to sign in before opening a profile (ignoring
guest profiles since they are not part of bug).  The account id of a profile is
independent of the whether the refresh token is valid or not, so I don't see any
reason how GetAccountIdFromProfile() would return an empty account id on cros
when given a valid profile.

Also, please talk to alemate@.  He is making cros work better with account-ids.

https://codereview.chromium.org/1880923003/diff/120001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/multi_user/multi_user_util.cc (right):

https://codereview.chromium.org/1880923003/diff/120001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/multi_user/multi_user_util.cc:29: // returns an empty
account id, which might cause wired behaviors or crash.
wired --> weird

https://codereview.chromium.org/1880923003/diff/120001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/multi_user/multi_user_util.cc:40:
profile->GetOriginalProfile()->GetProfileUserName());
Shouldn't lines 39-40 be wrapped in an #else?  This is now dead code on cros.

[xdai1, 2016-05-05 18:08:47]

rogerta@, please take another look, thanks for your review!

Actually the account id of a profile does depend on the refresh token is valid
or not. 

multi_user_util::GetAccountIdFromProfile() calls
ProfileImpl::GetProfileUserName() calls
SigninManagerBase::GetAuthenticatedAccountInfo() calls
AccountTrackerService::GetAccountInfo(). The AccountTrackerService will stop
tracking an account if the refresh token is revoked in the session (see
https://code.google.com/p/chromium/codesearch#chromium/src/components/signin/...),
which causes AccountTrackerService::GetAccountInfo() returns an empty
AccountInfo and then causes an empty Account id.

I have talked with alemate@ before in my previous CL
https://codereview.chromium.org/1704623002 and he agreed it's the right fix. +cc
alemate@

https://codereview.chromium.org/1880923003/diff/120001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/multi_user/multi_user_util.cc (right):

https://codereview.chromium.org/1880923003/diff/120001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/multi_user/multi_user_util.cc:29: // returns an empty
account id, which might cause wired behaviors or crash.
On 2016/05/05 14:27:20, Roger Tawa wrote:
> wired --> weird

Done.

https://codereview.chromium.org/1880923003/diff/120001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/multi_user/multi_user_util.cc:40:
profile->GetOriginalProfile()->GetProfileUserName());
On 2016/05/05 14:27:20, Roger Tawa wrote:
> Shouldn't lines 39-40 be wrapped in an #else?  This is now dead code on cros.

Done. I thought it should be fine to not use a #elif since it always returns on
line 36 on Chrome OS.

[xdai1, 2016-05-05 21:21:17]

Patchset #7 (id:140001) has been deleted

[Alexander Alekseev, 2016-05-05 21:33:49]

On 2016/05/05 18:08:47, xdai1 wrote:
> rogerta@, please take another look, thanks for your review!
> 
> Actually the account id of a profile does depend on the refresh token is valid
> or not. 
> 
> multi_user_util::GetAccountIdFromProfile() calls
> ProfileImpl::GetProfileUserName() calls
> SigninManagerBase::GetAuthenticatedAccountInfo() calls
> AccountTrackerService::GetAccountInfo(). The AccountTrackerService will stop
> tracking an account if the refresh token is revoked in the session (see
>
https://code.google.com/p/chromium/codesearch#chromium/src/components/signin/...),
> which causes AccountTrackerService::GetAccountInfo() returns an empty
> AccountInfo and then causes an empty Account id.
> 
> I have talked with alemate@ before in my previous CL
> https://codereview.chromium.org/1704623002 and he agreed it's the right fix.
+cc
> alemate@


Yes, I believe this is the right fix.
The problem is that we are experiencing crashes after token has been revoked.

[Roger Tawa OOO till Jul 10th, 2016-05-06 13:44:59]

lgtm

Thanks for the explanation guys.  One last question: this change is to fix some
strange run-after-crash scenario, is that right?  Because under normal
circumstances, cros always expects a signed in account and you never revoke
tokens from profiles.  (If you revoke the token, then it makes sense to return
empty from GetAccountIdFromProfile())

[xdai1, 2016-05-06 18:20:34]

On 2016/05/06 13:44:59, Roger Tawa wrote:
> lgtm
> 
> Thanks for the explanation guys.  One last question: this change is to fix
some
> strange run-after-crash scenario, is that right?  Because under normal
> circumstances, cros always expects a signed in account and you never revoke
> tokens from profiles.  (If you revoke the token, then it makes sense to return
> empty from GetAccountIdFromProfile())

Thanks for your review!

Yes, some strange run-after-crash scenario as seen in Issue 532553, or a window
lost its owner information which makes it get shown on both user's desktops as
seen Issue 602807. The refresh token will be revoked on one device if you change
your password on another device. Sometime it seems auto update is also one of
the reasons for the revoked refresh token (as seen in Issue 602807).

[xdai1, 2016-05-06 20:01:53]

The CQ bit was checked by xdai@chromium.org

[xdai1, 2016-05-06 20:01:54]

The patchset sent to the CQ was uploaded after l-g-t-m from thakis@chromium.org,
skuhne@chromium.org
Link to the patchset: https://codereview.chromium.org/1880923003/#ps160001
(title: "Address rogerta@'s comments. Rebase")

[commit-bot: I haz the power, 2016-05-06 20:02:18]

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

[commit-bot: I haz the power, 2016-05-06 20:07:34]

Committed patchset #7 (id:160001)

[commit-bot: I haz the power, 2016-05-06 20:08:33]

Description was changed from

==========
GetAccountIdFromProfile() should not return an empty account id if a valid
profile is provided.

On Chrome OS, we don't end the session immediately if a refresh token is revoked
in the session. Thus it's possible that we get an empty account id for a profile
from Profile::GetProfileUserName(). We should avoid this scenario on Chrome OS.

BUG=602807, 532535
==========

to

==========
GetAccountIdFromProfile() should not return an empty account id if a valid
profile is provided.

On Chrome OS, we don't end the session immediately if a refresh token is revoked
in the session. Thus it's possible that we get an empty account id for a profile
from Profile::GetProfileUserName(). We should avoid this scenario on Chrome OS.

BUG=602807, 532535

Committed: https://crrev.com/d2a0bb5307f8d0d09aff9d5d531dd9704fe85841
Cr-Commit-Position: refs/heads/master@{#392133}
==========

[commit-bot: I haz the power, 2016-05-06 20:08:34]

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

[Nico, 2016-05-07 14:12:54]

A revert of this CL (patchset #7 id:160001) has been created in
https://codereview.chromium.org/1962463002/ by thakis@chromium.org.

The reason for reverting is: Causes leaks:
https://build.chromium.org/p/chromium.memory/builders/Linux%20Chromium%20OS%2...

=================================================================
==5853==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 16 byte(s) in 1 object(s) allocated from:
    #0 0xa4f11b in operator new(unsigned long)
(/tmp/runJuOWov/out/Release/unit_tests+0xa4f11b)
    #1 0xf4375a6 in
chrome::MultiUserWindowManagerChromeOS::AddUser(content::BrowserContext*)
chrome/browser/ui/ash/multi_user/multi_user_window_manager_chromeos.cc:405:7
    #2 0x5654cde in
test::BrowserFinderChromeOSTest::CreateMultiUserProfile(AccountId const&)
chrome/browser/ui/browser_finder_chromeos_unittest.cc:43:5
    #3 0x237acd9 in BrowserWithTestWindowTest::SetUp()
chrome/test/base/browser_with_test_window_test.cc:67:14
    #4 0x56539c2 in test::BrowserFinderChromeOSTest::SetUp()
chrome/browser/ui/browser_finder_chromeos_unittest.cc:76:5
    #5 0x73422b6 in HandleExceptionsInMethodIfSupported<testing::Test, void>
testing/gtest/src/gtest.cc:2458:12
    #6 0x73422b6 in testing::Test::Run() testing/gtest/src/gtest.cc:2470
.

[Roger Tawa OOO till Jul 10th, 2016-05-09 14:14:15]

On 2016/05/06 18:20:34, xdai1 wrote:
> On 2016/05/06 13:44:59, Roger Tawa wrote:
> > lgtm
> > 
> > Thanks for the explanation guys.  One last question: this change is to fix
> some
> > strange run-after-crash scenario, is that right?  Because under normal
> > circumstances, cros always expects a signed in account and you never revoke
> > tokens from profiles.  (If you revoke the token, then it makes sense to
return
> > empty from GetAccountIdFromProfile())
> 
> Thanks for your review!
> 
> Yes, some strange run-after-crash scenario as seen in Issue 532553, or a
window
> lost its owner information which makes it get shown on both user's desktops as
> seen Issue 602807. The refresh token will be revoked on one device if you
change
> your password on another device. Sometime it seems auto update is also one of
> the reasons for the revoked refresh token (as seen in Issue 602807).

Hi Xiaoqian,

Note that changing a password on another device is "normal circumstances", so
this fix could be hiding a real bug somewhere else.  This causes the refresh
token to become invalid, which is different from saying that the token is
revoked.  The token should never be revoked. For my own info, can you point me
to the code that would revoke a token when chrome detects that it has become
invalid?

Similarly with 602807: if the refresh token becomes invalid, then chrome can't
refresh the account<-->email mapping which is cached on the device.  However,
this is only a problem if the user changes the email address associated with
their gaia account, which does not seem to be the case.

[xdai1, 2016-05-09 22:25:58]

On 2016/05/09 14:14:15, Roger Tawa wrote:
> 
> Hi Xiaoqian,
> 
> Note that changing a password on another device is "normal circumstances", so
> this fix could be hiding a real bug somewhere else.  This causes the refresh
> token to become invalid, which is different from saying that the token is
> revoked.  The token should never be revoked. For my own info, can you point me
> to the code that would revoke a token when chrome detects that it has become
> invalid?
> 
> Similarly with 602807: if the refresh token becomes invalid, then chrome can't
> refresh the account<-->email mapping which is cached on the device.  However,
> this is only a problem if the user changes the email address associated with
> their gaia account, which does not seem to be the case.

Hi Rogar,

Thanks for your info. I guess you're right. I originally thought the revoked
refresh token and invalid refresh token were the same thing... I clicked around
and could not find the code path that revokes a token when the token has become
invalid. However, from the bug reports (Issue 532553, Issue 602807 (related to
AU), Issue 602159 (related to AU), especially Issue 601525 (maybe related to
password changing?)), this CL is the best guess I can give... But maybe I was
wrong. Do you have any thoughts?

[xdai1, 2016-05-10 00:17:39]

Patchset #8 (id:180001) has been deleted

[Roger Tawa OOO till Jul 10th, 2016-05-11 12:50:50]

On 2016/05/09 22:25:58, xdai1 wrote:
> On 2016/05/09 14:14:15, Roger Tawa wrote:
> > 
> > Hi Xiaoqian,
> > 
> > Note that changing a password on another device is "normal circumstances",
so
> > this fix could be hiding a real bug somewhere else.  This causes the refresh
> > token to become invalid, which is different from saying that the token is
> > revoked.  The token should never be revoked. For my own info, can you point
me
> > to the code that would revoke a token when chrome detects that it has become
> > invalid?
> > 
> > Similarly with 602807: if the refresh token becomes invalid, then chrome
can't
> > refresh the account<-->email mapping which is cached on the device. 
However,
> > this is only a problem if the user changes the email address associated with
> > their gaia account, which does not seem to be the case.
> 
> Hi Rogar,
> 
> Thanks for your info. I guess you're right. I originally thought the revoked
> refresh token and invalid refresh token were the same thing... I clicked
around
> and could not find the code path that revokes a token when the token has
become
> invalid. However, from the bug reports (Issue 532553, Issue 602807 (related to
> AU), Issue 602159 (related to AU), especially Issue 601525 (maybe related to
> password changing?)), this CL is the best guess I can give... But maybe I was
> wrong. Do you have any thoughts?

Sorry Xiaoqian, I don't know the specifics of cros, only the more general sign
in related code.  Maybe you could fix the memory leak and re-land this CL as it
fixes a crash happening in the wild.  If you do, please open a bug for yourself
to investigate the true nature of why valid profiles don't have an associated
account id.