cros: Fix loading user profile w/o UserSessionManager

cros: Fix loading user profile w/o UserSessionManager

The fallback logic in GetActiveUserOrOffTheRecordProfileFromPath
should cover the case when the user profile loading has not started.
So that it does not creates/loads user profile accidentally without
going through UserSessionManager.

BUG=728683, 718734
TEST=ProfileManagerTest.UserProfileLoading

Review-Url: https://codereview.chromium.org/2918203002
Cr-Commit-Position: refs/heads/master@{#478735}
Committed: https://chromium.googlesource.com/chromium/src/+/41d32a3c1bd0153aa660378b3b2a2c59e25c5f31

[xiyuan, 2017-06-02 20:25:13]

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

[commit-bot: I haz the power, 2017-06-02 20:25:46]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[xiyuan, 2017-06-02 20:27:37]

xiyuan@chromium.org changed reviewers:
+ emaxx@chromium.org, msarda@chromium.org

[xiyuan, 2017-06-02 20:27:38]

WDYT?

[commit-bot: I haz the power, 2017-06-02 21:54:04]

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

[commit-bot: I haz the power, 2017-06-02 21:54:05]

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

[msarda, 2017-06-05 16:41:47]

I think this is subtle change and I am afraid I cannot evaluate the risk as my
current knowledge of the profile code is limited (unfortunately all the people
that knew well the profile code changes projects).
emaxx@ / xiyuan@: Do you know this code well? Are you confident this change is
low-risk?

[xiyuan, 2017-06-05 17:05:32]

On 2017/06/05 16:41:47, msarda wrote:
> I think this is subtle change and I am afraid I cannot evaluate the risk as my
> current knowledge of the profile code is limited (unfortunately all the people
> that knew well the profile code changes projects).
> emaxx@ / xiyuan@: Do you know this code well? Are you confident this change is
> low-risk?

I think it is relatively low risk. Fixing the broken tests now and there are not
many of them. The affected code are those try to access user profile before it
is properly loaded (via UserSessionManager). As long as they don't cache the
profile pointer, we should be good.

My next step would be changing the LOG(WARNING) into a LOG(FATAL) or DCHECK in
GetLastUsedProfile() so that we can fix the offending code and find out future
issues earlier (not as P0 bugs).

[msarda, 2017-06-05 17:43:41]

OK, thank you for the explanation. I'll wait for your next patch for review
then.

[xiyuan, 2017-06-05 17:58:50]

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

[commit-bot: I haz the power, 2017-06-05 17:59:14]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-05 19:28:14]

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

[commit-bot: I haz the power, 2017-06-05 19:28:15]

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

[xiyuan, 2017-06-05 20:13:07]

Patchset #2 (id:20001) has been deleted

[xiyuan, 2017-06-05 20:13:40]

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

[commit-bot: I haz the power, 2017-06-05 20:15:00]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-06 00:30:19]

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

[commit-bot: I haz the power, 2017-06-06 00:30:20]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[emaxx, 2017-06-06 12:11:23]

https://codereview.chromium.org/2918203002/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/accessibility/accessibility_manager_browsertest.cc
(right):

https://codereview.chromium.org/2918203002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/accessibility/accessibility_manager_browsertest.cc:165:
Profile* profile = g_browser_process->profile_manager()->GetProfile(
This construction - loading the profile for the active user - is repeated
several times in this CL. So maybe it's better to introduce the new method in
ProfileManager/ProfileHelper for doing this operation?

https://codereview.chromium.org/2918203002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/accessibility/accessibility_manager_browsertest.cc:181:
ProfileHelper::Get()->GetProfilePathByUserIdHash(
Is it really fine to pass UserEmail to this function instead of UserIdHash?
I think ProfileHelper::GetUserIdHashByUserIdForTesting() is used for this
transformation from UserEmail in other tests.

https://codereview.chromium.org/2918203002/diff/40001/chrome/browser/profiles...
File chrome/browser/profiles/profile_manager.cc (right):

https://codereview.chromium.org/2918203002/diff/40001/chrome/browser/profiles...
chrome/browser/profiles/profile_manager.cc:638: return
GetActiveUserOrOffTheRecordProfileFromPath(user_data_dir);
Not insisting (I'm not aware of the general guideline regarding this), but we
could add DumpWithoutCrashing() here and start collecting the data without
disrupting end users / developers with browser crashes.

https://codereview.chromium.org/2918203002/diff/40001/chrome/browser/profiles...
chrome/browser/profiles/profile_manager.cc:1332: const bool
allow_user_profile_loading =
I'm a bit unhappy with keeping the lazy profile loading here even when limited
to this sole case - this means the improvement only to the non-restarting half
of all cases. Also this complicates the behavior of GetActiveUserProfile() even
more than before.

Couldn't we load the active profile explicitly somewhere in the browser restart
code? For instance, one place for this could be CreatePrimaryProfile() in
chrome_browser_main.cc, where it currently loads the profile by calling
GetActiveUserProfile().

[xiyuan, 2017-06-06 20:29:24]

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

[xiyuan, 2017-06-06 20:29:39]

https://codereview.chromium.org/2918203002/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/accessibility/accessibility_manager_browsertest.cc
(right):

https://codereview.chromium.org/2918203002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/accessibility/accessibility_manager_browsertest.cc:165:
Profile* profile = g_browser_process->profile_manager()->GetProfile(
On 2017/06/06 12:11:23, emaxx wrote:
> This construction - loading the profile for the active user - is repeated
> several times in this CL. So maybe it's better to introduce the new method in
> ProfileManager/ProfileHelper for doing this operation?

Explicit user profile loading should be banned in production code. It skips
UserSessionManager and is the reason why we had problem now.

The code here is only for tests so that we can exclude UserSessionManager from
tests and keep them simple.

Found we have a ProfileHelper::GetProfileByUserIdHash call. Renamed to
GetProfileByUserIdHashForTest and changed to use it here.

https://codereview.chromium.org/2918203002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/accessibility/accessibility_manager_browsertest.cc:181:
ProfileHelper::Get()->GetProfilePathByUserIdHash(
On 2017/06/06 12:11:23, emaxx wrote:
> Is it really fine to pass UserEmail to this function instead of UserIdHash?
> I think ProfileHelper::GetUserIdHashByUserIdForTesting() is used for this
> transformation from UserEmail in other tests.

This is because the tests are using the email as hash when CreateSession.
Changed to look up the User and use the username_hash in User so that it looks
more correct.

https://codereview.chromium.org/2918203002/diff/40001/chrome/browser/profiles...
File chrome/browser/profiles/profile_manager.cc (right):

https://codereview.chromium.org/2918203002/diff/40001/chrome/browser/profiles...
chrome/browser/profiles/profile_manager.cc:638: return
GetActiveUserOrOffTheRecordProfileFromPath(user_data_dir);
On 2017/06/06 12:11:23, emaxx wrote:
> Not insisting (I'm not aware of the general guideline regarding this), but we
> could add DumpWithoutCrashing() here and start collecting the data without
> disrupting end users / developers with browser crashes.

I will fix the offending code and change this to a LOG(FATAL) in a follow up CL.
I intend to merge this CL back to M60 so do not want to have it here.

https://codereview.chromium.org/2918203002/diff/40001/chrome/browser/profiles...
chrome/browser/profiles/profile_manager.cc:1332: const bool
allow_user_profile_loading =
On 2017/06/06 12:11:23, emaxx wrote:
> I'm a bit unhappy with keeping the lazy profile loading here even when limited
> to this sole case - this means the improvement only to the non-restarting half
> of all cases. Also this complicates the behavior of GetActiveUserProfile()
even
> more than before.
> 
> Couldn't we load the active profile explicitly somewhere in the browser
restart
> code? For instance, one place for this could be CreatePrimaryProfile() in
> chrome_browser_main.cc, where it currently loads the profile by calling
> GetActiveUserProfile().

Added ProfileManager::CreateInitialProfile() and use it in
CreatePrimaryProfile(). And the |allow_user_profile_loading| check is removed
since CreateInitialProfile() loads the profile explicitly.

[commit-bot: I haz the power, 2017-06-06 20:29:41]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[xiyuan, 2017-06-06 20:36:05]

Patchset #3 (id:60001) has been deleted

[xiyuan, 2017-06-06 20:37:00]

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

[commit-bot: I haz the power, 2017-06-06 20:37:21]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-06 22:00:05]

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

[commit-bot: I haz the power, 2017-06-06 22:00:06]

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

[xiyuan, 2017-06-06 22:39:57]

emm, PS3 causes some new test failures. I am in middle of a bisect and probably
can only come back to this CL tomorrow.

[xiyuan, 2017-06-08 18:15:32]

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

[commit-bot: I haz the power, 2017-06-08 18:16:10]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-08 19:23:26]

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

[commit-bot: I haz the power, 2017-06-08 19:23:27]

Dry run: This issue passed the CQ dry run.

[xiyuan, 2017-06-08 19:24:01]

Bots are green now. PTAL. Thanks.

[emaxx, 2017-06-08 19:43:54]

LGTM (but admitting that I wasn't able to fully understand all the changes made
in tests)

https://codereview.chromium.org/2918203002/diff/100001/chrome/browser/chrome_...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/2918203002/diff/100001/chrome/browser/chrome_...
chrome/browser/chrome_browser_main.cc:429: // one we got passed.
GetActiveUserProfile will therefore use the correct path
nit: s/GetActiveUserProfile/CreateInitialProfile/ ?

https://codereview.chromium.org/2918203002/diff/100001/chrome/browser/chromeo...
File chrome/browser/chromeos/accessibility/accessibility_manager_browsertest.cc
(right):

https://codereview.chromium.org/2918203002/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/accessibility/accessibility_manager_browsertest.cc:178:
// profile and mark session as started.
nit: s/mark/marking/ ?

https://codereview.chromium.org/2918203002/diff/100001/chrome/browser/chromeo...
File chrome/browser/chromeos/profiles/profile_helper.h (right):

https://codereview.chromium.org/2918203002/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/profiles/profile_helper.h:80: // test. It should not be
used in production code because it could loads a
nit: s/loads/load/, s/skips/skip/

https://codereview.chromium.org/2918203002/diff/100001/chrome/browser/profile...
File chrome/browser/profiles/profile_manager.cc (right):

https://codereview.chromium.org/2918203002/diff/100001/chrome/browser/profile...
chrome/browser/profiles/profile_manager.cc:1339: // or has not fully loaded yet.
nit: "not loaded or has not fully loaded" sounds like a tautology - perhaps you
wanted to express something else here?

https://codereview.chromium.org/2918203002/diff/100001/chrome/browser/profile...
File chrome/browser/profiles/profile_manager.h (right):

https://codereview.chromium.org/2918203002/diff/100001/chrome/browser/profile...
chrome/browser/profiles/profile_manager.h:85: // browser is started normally or
is restarted after crash. On other
> depending on whether
> browser is started normally or is restarted after crash.
Hmm, does the function really behave like that? It calls GetInitialProfileDir()
which actually relies on logged_in_ to make the decision.

[xiyuan, 2017-06-08 20:28:21]

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

[commit-bot: I haz the power, 2017-06-08 20:28:42]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[xiyuan, 2017-06-08 20:30:41]

Most of the test changes are adding an explicit user profile load.

The exception is in CrashRestore*Test where policy code kicks in since the user
profile is loaded via UserSessionManager with the CL and the test does not setup
env for policy.

https://codereview.chromium.org/2918203002/diff/100001/chrome/browser/chrome_...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/2918203002/diff/100001/chrome/browser/chrome_...
chrome/browser/chrome_browser_main.cc:429: // one we got passed.
GetActiveUserProfile will therefore use the correct path
On 2017/06/08 19:43:53, emaxx wrote:
> nit: s/GetActiveUserProfile/CreateInitialProfile/ ?

Done.

https://codereview.chromium.org/2918203002/diff/100001/chrome/browser/chromeo...
File chrome/browser/chromeos/accessibility/accessibility_manager_browsertest.cc
(right):

https://codereview.chromium.org/2918203002/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/accessibility/accessibility_manager_browsertest.cc:178:
// profile and mark session as started.
On 2017/06/08 19:43:53, emaxx wrote:
> nit: s/mark/marking/ ?

Done.

https://codereview.chromium.org/2918203002/diff/100001/chrome/browser/chromeo...
File chrome/browser/chromeos/profiles/profile_helper.h (right):

https://codereview.chromium.org/2918203002/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/profiles/profile_helper.h:80: // test. It should not be
used in production code because it could loads a
On 2017/06/08 19:43:54, emaxx wrote:
> nit: s/loads/load/, s/skips/skip/

Done.

https://codereview.chromium.org/2918203002/diff/100001/chrome/browser/profile...
File chrome/browser/profiles/profile_manager.cc (right):

https://codereview.chromium.org/2918203002/diff/100001/chrome/browser/profile...
chrome/browser/profiles/profile_manager.cc:1339: // or has not fully loaded yet.
On 2017/06/08 19:43:54, emaxx wrote:
> nit: "not loaded or has not fully loaded" sounds like a tautology - perhaps
you
> wanted to express something else here?

Changed to "is not started loading or has not fully loaded".

https://codereview.chromium.org/2918203002/diff/100001/chrome/browser/profile...
File chrome/browser/profiles/profile_manager.h (right):

https://codereview.chromium.org/2918203002/diff/100001/chrome/browser/profile...
chrome/browser/profiles/profile_manager.h:85: // browser is started normally or
is restarted after crash. On other
On 2017/06/08 19:43:54, emaxx wrote:
> > depending on whether
> > browser is started normally or is restarted after crash.
> Hmm, does the function really behave like that? It calls
GetInitialProfileDir()
> which actually relies on logged_in_ to make the decision.

|logged_in_| is only set for ChromeOS. So for other platforms, this is always
the "Default" profile. On ChromeOS, this should only be called on browser
startup as the name implies. On the browser start up case, |logged_in_| is false
for normal browser start and set to true for browser start case.

[commit-bot: I haz the power, 2017-06-08 21:16:48]

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

[commit-bot: I haz the power, 2017-06-08 21:16:49]

Dry run: This issue passed the CQ dry run.

[msarda, 2017-06-08 23:49:19]

LGTM for chrome/browser/profiles/*

https://codereview.chromium.org/2918203002/diff/140001/chrome/browser/profile...
File chrome/browser/profiles/profile_manager.cc (right):

https://codereview.chromium.org/2918203002/diff/140001/chrome/browser/profile...
chrome/browser/profiles/profile_manager.cc:1332: // Fallback to default
off-the-record profile, if user profile is not started
ss/is/has

https://codereview.chromium.org/2918203002/diff/140001/chrome/browser/profile...
File chrome/browser/profiles/profile_manager_unittest.cc (right):

https://codereview.chromium.org/2918203002/diff/140001/chrome/browser/profile...
chrome/browser/profiles/profile_manager_unittest.cc:301: // otr version uses a
different temp path.
Maybe s/otr/OTR?

[xiyuan, 2017-06-12 15:32:54]

https://codereview.chromium.org/2918203002/diff/140001/chrome/browser/profile...
File chrome/browser/profiles/profile_manager.cc (right):

https://codereview.chromium.org/2918203002/diff/140001/chrome/browser/profile...
chrome/browser/profiles/profile_manager.cc:1332: // Fallback to default
off-the-record profile, if user profile is not started
On 2017/06/08 23:49:19, msarda wrote:
> ss/is/has

Done.

https://codereview.chromium.org/2918203002/diff/140001/chrome/browser/profile...
File chrome/browser/profiles/profile_manager_unittest.cc (right):

https://codereview.chromium.org/2918203002/diff/140001/chrome/browser/profile...
chrome/browser/profiles/profile_manager_unittest.cc:301: // otr version uses a
different temp path.
On 2017/06/08 23:49:19, msarda wrote:
> Maybe s/otr/OTR?

Done.

[xiyuan, 2017-06-12 15:45:46]

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

[xiyuan, 2017-06-12 15:45:46]

thakis@, could I get an owner approval for
 chrome/browser/chrome_browser_main.cc
?

Replace ProfileManager::GetActiveUserProfile() with a new
ProfileManager::CreateInitialProfile() there.

The CL makes GetActiveUser|PrimaryUser|LastUsedProfile() return only an already
loaded profile so that we have a single code path that do the actual profile
loading. And we now need to explicitly load the "Default" profile on browser
start up.

Thanks.

[xiyuan, 2017-06-12 15:45:52]

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

[commit-bot: I haz the power, 2017-06-12 15:46:29]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-12 17:17:50]

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

[commit-bot: I haz the power, 2017-06-12 17:17:51]

Dry run: This issue passed the CQ dry run.

[Nico, 2017-06-12 19:52:53]

lgtm

[xiyuan, 2017-06-12 20:00:02]

The CQ bit was checked by xiyuan@chromium.org

[xiyuan, 2017-06-12 20:00:02]

The patchset sent to the CQ was uploaded after l-g-t-m from emaxx@chromium.org,
msarda@chromium.org
Link to the patchset: https://codereview.chromium.org/2918203002/#ps180001
(title: "rebase")

[commit-bot: I haz the power, 2017-06-12 20:00:37]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-12 20:06:15]

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1497297602081630,
"parent_rev": "9010f6cbfa29ee8d337d0d236dd07ac2a823fa27", "commit_rev":
"41d32a3c1bd0153aa660378b3b2a2c59e25c5f31"}

[commit-bot: I haz the power, 2017-06-12 20:06:27]

Description was changed from

==========
cros: Fix loading user profile w/o UserSessionManager

The fallback logic in GetActiveUserOrOffTheRecordProfileFromPath
should cover the case when the user profile loading has not started.
So that it does not creates/loads user profile accidentally without
going through UserSessionManager.

BUG=728683,718734
TEST=ProfileManagerTest.UserProfileLoading
==========

to

==========
cros: Fix loading user profile w/o UserSessionManager

The fallback logic in GetActiveUserOrOffTheRecordProfileFromPath
should cover the case when the user profile loading has not started.
So that it does not creates/loads user profile accidentally without
going through UserSessionManager.

BUG=728683,718734
TEST=ProfileManagerTest.UserProfileLoading

Review-Url: https://codereview.chromium.org/2918203002
Cr-Commit-Position: refs/heads/master@{#478735}
Committed:
https://chromium.googlesource.com/chromium/src/+/41d32a3c1bd0153aa660378b3b2a...
==========

[commit-bot: I haz the power, 2017-06-12 20:06:29]

Committed patchset #8 (id:180001) as
https://chromium.googlesource.com/chromium/src/+/41d32a3c1bd0153aa660378b3b2a...