Fixed app launcher crashing trying to load guest profile.

Fixed app launcher crashing trying to load guest profile.

Previously, if the user has never used the app launcher, and it loads
while in a guest mode browser window, the browser would crash. This was
particularly bad because it could happen during startup on Windows, even
without explicitly opening the launcher (due to app list warmup).

Now if the last used profile is the guest profile, just uses the
default.

Added DCHECKs on app list profile loads to ensure they are not guest
profiles.

BUG=468812
Committed: https://crrev.com/f5966937a7f7f8bc610ab2c86f3be0255371cf03
Cr-Commit-Position: refs/heads/master@{#330712}

[Matt Giuca, 2015-04-22 08:55:22]

mgiuca@chromium.org changed reviewers:
+ benwells@chromium.org

[Mike Lerman, 2015-04-22 13:27:39]

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

[Mike Lerman, 2015-04-22 13:27:39]

Sorry for doing a drive-by ;)

I see you've touched Windows and Views code. Is there not similar Mac code that
should verify the profile isn't guest or OTR?

https://codereview.chromium.org/1106453002/diff/1/chrome/browser/ui/app_list/...
File chrome/browser/ui/app_list/app_list_service_views.cc (right):

https://codereview.chromium.org/1106453002/diff/1/chrome/browser/ui/app_list/...
chrome/browser/ui/app_list/app_list_service_views.cc:34:
DCHECK(!requested_profile->IsOffTheRecord());
Also please check requested_profile->IsGuestSession()

https://codereview.chromium.org/1106453002/diff/1/chrome/browser/ui/views/app...
File chrome/browser/ui/views/app_list/win/app_list_service_win.cc (right):

https://codereview.chromium.org/1106453002/diff/1/chrome/browser/ui/views/app...
chrome/browser/ui/views/app_list/win/app_list_service_win.cc:275:
DCHECK(!initial_profile->IsOffTheRecord());
Can this also check !initial_profile->IsGuestSession()? The guest profile can be
accessed in strange ways from the ProfileManager, and I've seen the GuestProfile
(outside of Browsers) not be OffTheRecord.

[Matt Giuca, 2015-04-24 04:51:04]

Added a CHECK in Mac. Haven't tested on Mac but I assume the test suite will
confirm that this doesn't break things.

https://codereview.chromium.org/1106453002/diff/1/chrome/browser/ui/app_list/...
File chrome/browser/ui/app_list/app_list_service_views.cc (right):

https://codereview.chromium.org/1106453002/diff/1/chrome/browser/ui/app_list/...
chrome/browser/ui/app_list/app_list_service_views.cc:34:
DCHECK(!requested_profile->IsOffTheRecord());
On 2015/04/22 13:27:39, Mike Lerman wrote:
> Also please check requested_profile->IsGuestSession()

Done.

https://codereview.chromium.org/1106453002/diff/1/chrome/browser/ui/views/app...
File chrome/browser/ui/views/app_list/win/app_list_service_win.cc (right):

https://codereview.chromium.org/1106453002/diff/1/chrome/browser/ui/views/app...
chrome/browser/ui/views/app_list/win/app_list_service_win.cc:275:
DCHECK(!initial_profile->IsOffTheRecord());
On 2015/04/22 13:27:39, Mike Lerman wrote:
> Can this also check !initial_profile->IsGuestSession()? The guest profile can
be
> accessed in strange ways from the ProfileManager, and I've seen the
GuestProfile
> (outside of Browsers) not be OffTheRecord.

Done.

[Matt Giuca, 2015-04-24 04:51:13]

benwells: PTAL.

[Matt Giuca, 2015-04-24 04:51:17]

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

[commit-bot: I haz the power, 2015-04-24 04:51:30]

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

[commit-bot: I haz the power, 2015-04-24 05:28:53]

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

[commit-bot: I haz the power, 2015-04-24 05:28:54]

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

[Mike Lerman, 2015-04-24 12:27:11]

Thanks, non-owner LGTM :)

[Matt Giuca, 2015-04-27 00:40:29]

Damn, these tests are failing:

AppListControllerBrowserTest.Incognito
AppListControllerBrowserTest.RegularThenIncognito

Because they are explicitly testing an incognito version of the app list. Does
this actually make sense? I wrote this CL with the assumption that there should
never be a guest or incognito app list (other than on CrOS), but it seems that
there are tests expecting it to work.

+tapted?

[tapted, 2015-04-27 06:46:31]

On 2015/04/27 00:40:29, Matt Giuca wrote:
> Damn, these tests are failing:
> 
> AppListControllerBrowserTest.Incognito
> AppListControllerBrowserTest.RegularThenIncognito
> 
> Because they are explicitly testing an incognito version of the app list. Does
> this actually make sense? I wrote this CL with the assumption that there
should
> never be a guest or incognito app list (other than on CrOS), but it seems that
> there are tests expecting it to work.
> 
> +tapted?

The Webstore API can pass an incognito profile to
AppListService::ShowForProfile.  That needs to do something sensible. Either
show the app list and "work" or, if we don't show the app list, we should block
the app install earlier.

[Mike Lerman, 2015-04-27 13:15:55]

On 2015/04/27 06:46:31, tapted wrote:
> On 2015/04/27 00:40:29, Matt Giuca wrote:
> > Damn, these tests are failing:
> > 
> > AppListControllerBrowserTest.Incognito
> > AppListControllerBrowserTest.RegularThenIncognito
> > 
> > Because they are explicitly testing an incognito version of the app list.
Does
> > this actually make sense? I wrote this CL with the assumption that there
> should
> > never be a guest or incognito app list (other than on CrOS), but it seems
that
> > there are tests expecting it to work.
> > 
> > +tapted?
> 
> The Webstore API can pass an incognito profile to
> AppListService::ShowForProfile.  That needs to do something sensible. Either
> show the app list and "work" or, if we don't show the app list, we should
block
> the app install earlier.

What does it means to show the AppList for an Incognito Profile? Will the apps
launch to the incognito profile, or to the original profile? I feel like this is
something that shouldn't work and should be blocked, but I'd be curious what
battre@'s thoughts are.

Also - where is that code for the Webstore API call? Most of the calls to
ShowForProfile come with a profile_path, which cannot specify the OTR of a
Profile.

[tapted, 2015-04-27 13:27:49]

On 2015/04/27 13:15:55, Mike Lerman wrote:
> On 2015/04/27 06:46:31, tapted wrote:
> > On 2015/04/27 00:40:29, Matt Giuca wrote:
> > > Damn, these tests are failing:
> > > 
> > > AppListControllerBrowserTest.Incognito
> > > AppListControllerBrowserTest.RegularThenIncognito
> > > 
> > > Because they are explicitly testing an incognito version of the app list.
> Does
> > > this actually make sense? I wrote this CL with the assumption that there
> > should
> > > never be a guest or incognito app list (other than on CrOS), but it seems
> that
> > > there are tests expecting it to work.
> > > 
> > > +tapted?
> > 
> > The Webstore API can pass an incognito profile to
> > AppListService::ShowForProfile.  That needs to do something sensible. Either
> > show the app list and "work" or, if we don't show the app list, we should
> block
> > the app install earlier.
> 
> What does it means to show the AppList for an Incognito Profile?

The app list will just show the original/non-incognito profile

> Will the apps
> launch to the incognito profile, or to the original profile? I feel like this
is
> something that shouldn't work and should be blocked, but I'd be curious what
> battre@'s thoughts are.
> 
> Also - where is that code for the Webstore API call? Most of the calls to
> ShowForProfile come with a profile_path, which cannot specify the OTR of a
> Profile.

It's this one:

https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext...

For some reason installing extensions/apps in incognito has always worked -- it
just installs to the original profile. If we prevent it, the UI needs to be in
the webstore for a proper UX - there is b/13795858 for this.

[Mike Lerman, 2015-04-27 13:37:38]

On 2015/04/27 13:27:49, tapted wrote:
> On 2015/04/27 13:15:55, Mike Lerman wrote:
> > On 2015/04/27 06:46:31, tapted wrote:
> > > On 2015/04/27 00:40:29, Matt Giuca wrote:
> > > > Damn, these tests are failing:
> > > > 
> > > > AppListControllerBrowserTest.Incognito
> > > > AppListControllerBrowserTest.RegularThenIncognito
> > > > 
> > > > Because they are explicitly testing an incognito version of the app
list.
> > Does
> > > > this actually make sense? I wrote this CL with the assumption that there
> > > should
> > > > never be a guest or incognito app list (other than on CrOS), but it
seems
> > that
> > > > there are tests expecting it to work.
> > > > 
> > > > +tapted?
> > > 
> > > The Webstore API can pass an incognito profile to
> > > AppListService::ShowForProfile.  That needs to do something sensible.
Either
> > > show the app list and "work" or, if we don't show the app list, we should
> > block
> > > the app install earlier.
> > 
> > What does it means to show the AppList for an Incognito Profile?
> 
> The app list will just show the original/non-incognito profile
> 
> > Will the apps
> > launch to the incognito profile, or to the original profile? I feel like
this
> is
> > something that shouldn't work and should be blocked, but I'd be curious what
> > battre@'s thoughts are.
> > 
> > Also - where is that code for the Webstore API call? Most of the calls to
> > ShowForProfile come with a profile_path, which cannot specify the OTR of a
> > Profile.
> 
> It's this one:
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext...
> 
> For some reason installing extensions/apps in incognito has always worked --
it
> just installs to the original profile. If we prevent it, the UI needs to be in
> the webstore for a proper UX - there is b/13795858 for this.

I imagine it always worked because of this:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...

I see you had a CL related to this topic already:
https://codereview.chromium.org/219383006

I'm uneasy about things that go noiselessly from an incognito profile to an
original profile. I see that b/13795858 hasn't been working on for a while. Is
there anything we can do to drive that along?

+battre

[Mike Lerman, 2015-04-27 13:38:05]

Hi Dominic,

Do you have thought about the App Launcher + Incognito profiles?

Thanks,
Mike

[battre, 2015-04-27 15:59:24]

On 2015/04/27 13:38:05, Mike Lerman wrote:
> Hi Dominic,
> 
> Do you have thought about the App Launcher + Incognito profiles?
> 
> Thanks,
> Mike

Hi.

I think that heuristic makes sense. LGTM.

Thanks,
Dominic

[Matt Giuca, 2015-05-12 06:58:31]

Patchset #3 (id:40001) has been deleted

[Matt Giuca, 2015-05-13 06:10:24]

Note to self: This code was cleaned up by bauerb's
https://codereview.chromium.org/1113333003/ (but I don't think it fixes the
issue).

Rebase and try again.

[Matt Giuca, 2015-05-14 03:50:50]

Confirmed that r329608 does not solve the issue (+bauerb for interest). Rebased
onto ToT and going to try again to fix that test.

[Matt Giuca, 2015-05-14 05:00:24]

Patchset #4 (id:80001) has been deleted

[Matt Giuca, 2015-05-14 05:02:30]

So those DCHECKs were failing tests; turns out you can also trigger them in the
real world by going to the web store in guest or incognito and installing an
app. That's a rabbit hole I don't immediately want to go into
(http://crbug.com/416380), so instead, I'm just taking the DCHECKs out and
replacing them with TODOs.

benwells: PTAL.

[Matt Giuca, 2015-05-14 05:02:48]

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

[Matt Giuca, 2015-05-14 05:02:49]

The patchset sent to the CQ was uploaded after l-g-t-m from
mlerman@chromium.org, battre@chromium.org
Link to the patchset: https://codereview.chromium.org/1106453002/#ps100001
(title: "Remove the DCHECKs; crbug 416380 means they aren't always true.")

[commit-bot: I haz the power, 2015-05-14 05:03:17]

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

[commit-bot: I haz the power, 2015-05-14 05:43:54]

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

[commit-bot: I haz the power, 2015-05-14 05:43:56]

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

[Matt Giuca, 2015-05-14 08:17:54]

Damn, I can't land this because AppListServiceTest is broken (just filed
http://crbug.com/487951).

RefusesToLoadGuestLastUsedProfile passes, but I don't think it's testing
anything any more. FakeAppListProfile is bypassing the logic we want to test
(that if kProfileLastUsed is a guest profile, it won't get picked... this test
won't even bother to look at prefs because it uses FakeAppListProfile).

RefusesToLoadGuestAppListProfile fails because it expects the code to look at
kProfileLastUsed, but it doesn't.

Too much fake... let's discuss on http://crbug.com/487951.

[Matt Giuca, 2015-05-15 09:29:40]

OK fixed! PTAL.

[Mike Lerman, 2015-05-15 13:02:41]

still lgtm

[Matt Giuca, 2015-05-19 03:57:34]

benwells: friendly ping

[benwells, 2015-05-19 22:00:57]

lgtm, apologies for slowness

[Matt Giuca, 2015-05-19 23:43:14]

The CQ bit was checked by mgiuca@chromium.org

[Matt Giuca, 2015-05-19 23:43:15]

The patchset sent to the CQ was uploaded after l-g-t-m from battre@chromium.org
Link to the patchset: https://codereview.chromium.org/1106453002/#ps140001
(title: "Rebase off https://codereview.chromium.org/1134533007.")

[commit-bot: I haz the power, 2015-05-20 00:20:45]

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

[commit-bot: I haz the power, 2015-05-20 01:18:19]

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

[commit-bot: I haz the power, 2015-05-20 01:18:20]

Try jobs failed on following builders:
  win8_chromium_rel on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)

[Matt Giuca, 2015-05-20 07:03:55]

The CQ bit was checked by mgiuca@chromium.org

[Matt Giuca, 2015-05-20 07:03:55]

The patchset sent to the CQ was uploaded after l-g-t-m from
mlerman@chromium.org, benwells@chromium.org, battre@chromium.org
Link to the patchset: https://codereview.chromium.org/1106453002/#ps160001
(title: "Fix Windows compile.")

[commit-bot: I haz the power, 2015-05-20 07:04:08]

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

[commit-bot: I haz the power, 2015-05-20 07:52:58]

Committed patchset #7 (id:160001)

[commit-bot: I haz the power, 2015-05-20 07:54:05]

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