Force guest profiles to return default startup prefs.

Force guest profiles to return default startup prefs.

Crash reports for https://crbug.com/723414 show a crash in a CHECK()
of profile type being used to create a new browser window via
Browser::Browser(), specifically:

  CHECK(!profile_->IsGuestSession() || profile_->IsOffTheRecord())
      << "Only off the record browser may be opened in guest mode";

This cl forces SessionStartupPref::GetStartupPref() to return default
startup prefs for guest profiles.

R=sky@chromium.org
BUG=723414
Review-Url: https://codereview.chromium.org/2927853002
Cr-Commit-Position: refs/heads/master@{#480947}
Committed: https://chromium.googlesource.com/chromium/src/+/9262aba8fec57bed8ce2256574e0e38ad53ae917

[shrike, 2017-06-07 18:43:08]

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

[commit-bot: I haz the power, 2017-06-07 18:43:46]

Dry run: CQ is trying da patch.

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

[shrike, 2017-06-07 18:44:41]

Hello sky@ - you are listed as owner, but I'm not sure if someone else would be
the right person to sanity-check this change.

PTAL

[commit-bot: I haz the power, 2017-06-07 19:02:45]

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

[commit-bot: I haz the power, 2017-06-07 19:02:46]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[sky, 2017-06-07 19:21:41]

https://codereview.chromium.org/2927853002/diff/1/chrome/browser/sessions/ses...
File chrome/browser/sessions/session_restore.cc (right):

https://codereview.chromium.org/2927853002/diff/1/chrome/browser/sessions/ses...
chrome/browser/sessions/session_restore.cc:91: return profile->IsGuestSession()
? profile->GetOffTheRecordProfile()
I'm a little confused by this, and forgive me because I couldn't readily find
docs for guest mode. It's my understanding that when you exit guest mode we
throw everything away. If that's the case, why and how are we triggering session
restore of a guest session? Shouldn't all the data be temporary? If it's because
of a crash and parts of the guest session were left around, shouldn't we clean
them up and not attempt a restore?

[shrike, 2017-06-07 20:19:05]

https://codereview.chromium.org/2927853002/diff/1/chrome/browser/sessions/ses...
File chrome/browser/sessions/session_restore.cc (right):

https://codereview.chromium.org/2927853002/diff/1/chrome/browser/sessions/ses...
chrome/browser/sessions/session_restore.cc:91: return profile->IsGuestSession()
? profile->GetOffTheRecordProfile()
On 2017/06/07 19:21:40, sky wrote:
> I'm a little confused by this, and forgive me because I couldn't readily find
> docs for guest mode. It's my understanding that when you exit guest mode we
> throw everything away. If that's the case, why and how are we triggering
session
> restore of a guest session? Shouldn't all the data be temporary? If it's
because
> of a crash and parts of the guest session were left around, shouldn't we clean
> them up and not attempt a restore?

Gosh, I wish could even start to comment. I know nothing about all this stuff,
but was mirroring a similar change I made in
https://codereview.chromium.org/2899313005 to fix a different set of crashers.

The following crash seems to show that somehow the condition I'm trying to guard
against can happen (which it sounds like you're saying should not happen):

https://crash.corp.google.com/browse?q=product.name%3D%27Chrome_Mac%27%20AND%...

[shrike, 2017-06-07 21:53:33]

shrike@chromium.org changed reviewers:
+ avi@chromium.org

[shrike, 2017-06-07 21:53:34]

avi@ - are you someone who can comment on the correctness of this fix? I don't
understand enough about profiles, guest sessions, or session restore to know if
my change makes any sense (however the crash I link to in my reply to sky@
suggests I'm addressing a situation that can occur).

[Avi (use Gerrit), 2017-06-07 21:58:53]

On 2017/06/07 21:53:34, shrike wrote:
> avi@ - are you someone who can comment on the correctness of this fix? I don't
> understand enough about profiles, guest sessions, or session restore to know
if
> my change makes any sense (however the crash I link to in my reply to sky@
> suggests I'm addressing a situation that can occur).

I don't know about the correctness here. Scott has a really valid point, so I'd
suggest digging here. Why _are_ we trying to session restore a guest session?

[sky, 2017-06-07 23:40:14]

Description was changed from

==========
Check profile type passed to new browser window in Session Restore.

Crash reports for https://crbug.com/723414 show a crash in a CHECK()
of profile type being used to create a new browser window via
Browser::Browser(), specifically:

  CHECK(!profile_->IsGuestSession() || profile_->IsOffTheRecord())
      << "Only off the record browser may be opened in guest mode";

Thi cl modifies Session Restore to pass a profile safe for
Browser::Browser() to use.

R=sky@chromium.org
BUG=723414
==========

to

==========
Check profile type passed to new browser window in Session Restore.

Crash reports for https://crbug.com/723414 show a crash in a CHECK()
of profile type being used to create a new browser window via
Browser::Browser(), specifically:

  CHECK(!profile_->IsGuestSession() || profile_->IsOffTheRecord())
      << "Only off the record browser may be opened in guest mode";

Thi cl modifies Session Restore to pass a profile safe for
Browser::Browser() to use.

R=sky@chromium.org
BUG=723414
==========

[sky, 2017-06-07 23:40:14]

sky@chromium.org changed reviewers:
+ mlerman@chromium.org

[sky, 2017-06-07 23:41:35]

+merlman: Anthony suggested we contact you. We're trying to figure out
expectations around guest sessions on desktops (not chromeos). It seems we may
try to restore a previous guest session during startup. Is that expected? Is
there any documentation you might be able to point us at as far as how guest
sessions are suppose to work?

Thanks.

[shrike, 2017-06-08 00:34:43]

On 2017/06/07 23:41:35, sky wrote:
> +merlman: Anthony suggested we contact you. We're trying to figure out
> expectations around guest sessions on desktops (not chromeos). It seems we may
> try to restore a previous guest session during startup. Is that expected? Is
> there any documentation you might be able to point us at as far as how guest
> sessions are suppose to work?
> 
> Thanks.

What you and avi@ say about restoring a guest mode profile makes sense (now) and
I've been looking through the code to try to understand how it could be
happening. At first I was wondering if the guest profile was storing tab history
to disk but that doesn't appear to be the case (those files don't contain
anything). It seems like the code paths may flow through
BrowserContextKeyedBaseFactory::GetBrowserContextToUse(), which does check for
OTR profiles.

What I can't find is the place where the profile is getting created at start-up,
and told that it's a guest profile. It seems like that should not happen (it
also seems like the launch-after-a-crash path also prevents OTR profile
restoration). TabRestoreServiceFactory::BuildServiceInstanceFor() in
tab_restore_service_factory.cc has a DCHECK(!profile->IsOffTheRecord()), but
perhaps that condition is occurring out in the wild.

[Mike Lerman, 2017-06-08 13:35:10]

On 2017/06/08 00:34:43, shrike wrote:
> On 2017/06/07 23:41:35, sky wrote:
> > +merlman: Anthony suggested we contact you. We're trying to figure out
> > expectations around guest sessions on desktops (not chromeos). It seems we
may
> > try to restore a previous guest session during startup. Is that expected? Is
> > there any documentation you might be able to point us at as far as how guest
> > sessions are suppose to work?
> > 
> > Thanks.
> 
> What you and avi@ say about restoring a guest mode profile makes sense (now)
and
> I've been looking through the code to try to understand how it could be
> happening. At first I was wondering if the guest profile was storing tab
history
> to disk but that doesn't appear to be the case (those files don't contain
> anything). It seems like the code paths may flow through
> BrowserContextKeyedBaseFactory::GetBrowserContextToUse(), which does check for
> OTR profiles.
> 
> What I can't find is the place where the profile is getting created at
start-up,
> and told that it's a guest profile. It seems like that should not happen (it
> also seems like the launch-after-a-crash path also prevents OTR profile
> restoration). TabRestoreServiceFactory::BuildServiceInstanceFor() in
> tab_restore_service_factory.cc has a DCHECK(!profile->IsOffTheRecord()), but
> perhaps that condition is occurring out in the wild.

A few facts for background, as to how guest profiles work, and how session
restore interacts with profiles.
  - Guest Sessions are profiles that are forced to be incognito. e.g. when you
ask the ProfileManager for a Profile who's path is the guest profile's, you
_always_ get the OffTheRecord version, and I'm pretty sure that trying to
GetOriginalProfile() won't get you anywhere fruitful.
  - Thanks to the interaction between OffTheRecord profiles and the CBD
services, there shouldn't be anything interesting to restore from session
restore.
  - Probably unrelated interaction between Profiles and Session Restore: If you
use Profile Lock, when you unlock a locked profile you _always_ perform a
session restore, even if that Profile doesn't have that setting set.
  - If on Chrome startup, you try to open Guest Profile, you will instead open
the User Manager (although the Guest Profile* will have been created). See
startup_browser_creator::ShowUserManagerOnStartupIfNeeded()

What I expect is that browser startup is occurring with the Guest Profile being
used as the initial profile. How can that happen?
  - Launch Chrome with the --profile_directory="Guest Profile"
  - Launch Chrome when all profiles are locked
  - Launch Chrome when all available Profiles are in some funky, un-openable
state (we then default to Guest). See
startup_browser_creator::GetFallbackStartupProfile(). This doesn't happen often,
although there have been some iterations and additional applications to this
function over the past year. To my knowledge (though I've very out of the loop),
the exact causes of this aren't understood, but this method does reduce crashes.

I expect the easiest solution is to ensure none of the Session Restore logic
ever executes for GuestProfile or SystemProfile. I would not force Session
Restore to try and apply on GetOriginalProfile() in these cases, I would instead
exit the session restore flow (or prevent it from starting in the first place).

I'm not familiar enough with Session Restore to understand if there's a way that
session restore would be invoked.

[sky, 2017-06-08 19:37:12]

Thanks for the details!
Shrike, the easy fix may be to force SessionStartupPref
<https://cs.chromium.org/chromium/src/chrome/browser/prefs/session_startup_pre...>
::GetStartupPref
<https://cs.chromium.org/chromium/src/chrome/browser/prefs/session_startup_pre...
to return DEFAULT for guest profiles.

  -Scott

On Thu, Jun 8, 2017 at 6:35 AM, <mlerman@chromium.org> wrote:

> On 2017/06/08 00:34:43, shrike wrote:
> > On 2017/06/07 23:41:35, sky wrote:
> > > +merlman: Anthony suggested we contact you. We're trying to figure out
> > > expectations around guest sessions on desktops (not chromeos). It
> seems we
> may
> > > try to restore a previous guest session during startup. Is that
> expected? Is
> > > there any documentation you might be able to point us at as far as how
> guest
> > > sessions are suppose to work?
> > >
> > > Thanks.
> >
> > What you and avi@ say about restoring a guest mode profile makes sense
> (now)
> and
> > I've been looking through the code to try to understand how it could be
> > happening. At first I was wondering if the guest profile was storing tab
> history
> > to disk but that doesn't appear to be the case (those files don't contain
> > anything). It seems like the code paths may flow through
> > BrowserContextKeyedBaseFactory::GetBrowserContextToUse(), which does
> check for
> > OTR profiles.
> >
> > What I can't find is the place where the profile is getting created at
> start-up,
> > and told that it's a guest profile. It seems like that should not happen
> (it
> > also seems like the launch-after-a-crash path also prevents OTR profile
> > restoration). TabRestoreServiceFactory::BuildServiceInstanceFor() in
> > tab_restore_service_factory.cc has a DCHECK(!profile->IsOffTheRecord()),
> but
> > perhaps that condition is occurring out in the wild.
>
> A few facts for background, as to how guest profiles work, and how session
> restore interacts with profiles.
> - Guest Sessions are profiles that are forced to be incognito. e.g. when
> you
> ask the ProfileManager for a Profile who's path is the guest profile's, you
> _always_ get the OffTheRecord version, and I'm pretty sure that trying to
> GetOriginalProfile() won't get you anywhere fruitful.
> - Thanks to the interaction between OffTheRecord profiles and the CBD
> services, there shouldn't be anything interesting to restore from session
> restore.
> - Probably unrelated interaction between Profiles and Session Restore: If
> you
> use Profile Lock, when you unlock a locked profile you _always_ perform a
> session restore, even if that Profile doesn't have that setting set.
> - If on Chrome startup, you try to open Guest Profile, you will instead
> open
> the User Manager (although the Guest Profile* will have been created). See
> startup_browser_creator::ShowUserManagerOnStartupIfNeeded()
>
> What I expect is that browser startup is occurring with the Guest Profile
> being
> used as the initial profile. How can that happen?
> - Launch Chrome with the --profile_directory="Guest Profile"
> - Launch Chrome when all profiles are locked
> - Launch Chrome when all available Profiles are in some funky, un-openable
> state (we then default to Guest). See
> startup_browser_creator::GetFallbackStartupProfile(). This doesn't happen
> often,
> although there have been some iterations and additional applications to
> this
> function over the past year. To my knowledge (though I've very out of the
> loop),
> the exact causes of this aren't understood, but this method does reduce
> crashes.
>
> I expect the easiest solution is to ensure none of the Session Restore
> logic
> ever executes for GuestProfile or SystemProfile. I would not force Session
> Restore to try and apply on GetOriginalProfile() in these cases, I would
> instead
> exit the session restore flow (or prevent it from starting in the first
> place).
>
> I'm not familiar enough with Session Restore to understand if there's a
> way that
> session restore would be invoked.
>
> https://codereview.chromium.org/2927853002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[shrike, 2017-06-13 22:04:13]

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

[commit-bot: I haz the power, 2017-06-13 22:04:47]

Dry run: CQ is trying da patch.

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

[shrike, 2017-06-13 22:07:15]

Description was changed from

==========
Check profile type passed to new browser window in Session Restore.

Crash reports for https://crbug.com/723414 show a crash in a CHECK()
of profile type being used to create a new browser window via
Browser::Browser(), specifically:

  CHECK(!profile_->IsGuestSession() || profile_->IsOffTheRecord())
      << "Only off the record browser may be opened in guest mode";

Thi cl modifies Session Restore to pass a profile safe for
Browser::Browser() to use.

R=sky@chromium.org
BUG=723414
==========

to

==========
Force guest profiles to return default startup prefs.

Crash reports for https://crbug.com/723414 show a crash in a CHECK()
of profile type being used to create a new browser window via
Browser::Browser(), specifically:

  CHECK(!profile_->IsGuestSession() || profile_->IsOffTheRecord())
      << "Only off the record browser may be opened in guest mode";

This cl forces SessionStartupPref::GetStartupPref() to return default
startup prefs for guest profiles.

R=sky@chromium.org
BUG=723414
==========

[commit-bot: I haz the power, 2017-06-13 23:19:57]

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

[commit-bot: I haz the power, 2017-06-13 23:19:59]

Dry run: This issue passed the CQ dry run.

[shrike, 2017-06-20 00:49:12]

On 2017/06/08 19:37:12, sky wrote:
> Thanks for the details!
> Shrike, the easy fix may be to force SessionStartupPref
>
<https://cs.chromium.org/chromium/src/chrome/browser/prefs/session_startup_pre...>
> ::GetStartupPref
>
<https://cs.chromium.org/chromium/src/chrome/browser/prefs/session_startup_pre...
> to return DEFAULT for guest profiles.

meriman@ - thank you for the info session restore and profiles.

sky@ - thank you for roping meriman@ in to get some clarity on this. I have
prepared a new cl along the lines of your suggestion - PTAL.

[sky, 2017-06-20 16:45:28]

https://codereview.chromium.org/2927853002/diff/20001/chrome/browser/prefs/se...
File chrome/browser/prefs/session_startup_pref.cc (right):

https://codereview.chromium.org/2927853002/diff/20001/chrome/browser/prefs/se...
chrome/browser/prefs/session_startup_pref.cc:105: // If a guest profile, force a
return of the DEFAULT startup prefs. See
When writing comments strive for comments that clarify 'why' the code is the way
it is. For example, here I think you want:

// Guest sessions should not store any state and such never trigger
// a restore during startup.

https://codereview.chromium.org/2927853002/diff/20001/chrome/browser/prefs/se...
chrome/browser/prefs/session_startup_pref.cc:109:
prefs->SetInteger(prefs::kRestoreOnStartup,
Is it really necessary to modify the prefs here? Instead could you return a
SessionStartPref with the type explicitly set to default, e.g.:

return SessionStartPref(SessionStartPref::DEFAULT);
?

[shrike, 2017-06-20 18:01:18]

PTAL

https://codereview.chromium.org/2927853002/diff/20001/chrome/browser/prefs/se...
File chrome/browser/prefs/session_startup_pref.cc (right):

https://codereview.chromium.org/2927853002/diff/20001/chrome/browser/prefs/se...
chrome/browser/prefs/session_startup_pref.cc:105: // If a guest profile, force a
return of the DEFAULT startup prefs. See
On 2017/06/20 16:45:27, sky wrote:
> When writing comments strive for comments that clarify 'why' the code is the
way
> it is. For example, here I think you want:
> 
> // Guest sessions should not store any state and such never trigger
> // a restore during startup.
> 

Acknowledged.

https://codereview.chromium.org/2927853002/diff/20001/chrome/browser/prefs/se...
chrome/browser/prefs/session_startup_pref.cc:109:
prefs->SetInteger(prefs::kRestoreOnStartup,
On 2017/06/20 16:45:27, sky wrote:
> Is it really necessary to modify the prefs here? Instead could you return a
> SessionStartPref with the type explicitly set to default, e.g.:
> 
> return SessionStartPref(SessionStartPref::DEFAULT);
> ?

I was concerned about not changing the original code path too much, but I see
that SessionStartupPref::GetStartupPref() should not do much of anything with a
DEFAULT profile, so should be safe to bypass it.

[sky, 2017-06-20 18:15:01]

LGTM

[shrike, 2017-06-20 18:36:03]

The CQ bit was checked by shrike@chromium.org

[commit-bot: I haz the power, 2017-06-20 18:36:23]

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-20 18:49:21]

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

[commit-bot: I haz the power, 2017-06-20 18:49:22]

Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[shrike, 2017-06-20 19:16:02]

The CQ bit was checked by shrike@chromium.org

[shrike, 2017-06-20 19:16:03]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org
Link to the patchset: https://codereview.chromium.org/2927853002/#ps60001
(title: "Fix error.")

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

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-20 20:45:20]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1497986162130500,
"parent_rev": "455a29452a85a4de9603b0d4d99373930cb20bb3", "commit_rev":
"9262aba8fec57bed8ce2256574e0e38ad53ae917"}

[commit-bot: I haz the power, 2017-06-20 20:45:34]

Description was changed from

==========
Force guest profiles to return default startup prefs.

Crash reports for https://crbug.com/723414 show a crash in a CHECK()
of profile type being used to create a new browser window via
Browser::Browser(), specifically:

  CHECK(!profile_->IsGuestSession() || profile_->IsOffTheRecord())
      << "Only off the record browser may be opened in guest mode";

This cl forces SessionStartupPref::GetStartupPref() to return default
startup prefs for guest profiles.

R=sky@chromium.org
BUG=723414
==========

to

==========
Force guest profiles to return default startup prefs.

Crash reports for https://crbug.com/723414 show a crash in a CHECK()
of profile type being used to create a new browser window via
Browser::Browser(), specifically:

  CHECK(!profile_->IsGuestSession() || profile_->IsOffTheRecord())
      << "Only off the record browser may be opened in guest mode";

This cl forces SessionStartupPref::GetStartupPref() to return default
startup prefs for guest profiles.

R=sky@chromium.org
BUG=723414

Review-Url: https://codereview.chromium.org/2927853002
Cr-Commit-Position: refs/heads/master@{#480947}
Committed:
https://chromium.googlesource.com/chromium/src/+/9262aba8fec57bed8ce2256574e0...
==========

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

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/9262aba8fec57bed8ce2256574e0...