Never create an app list for an incognito profile.

Never create an app list for an incognito profile.

Currently the webstore API can ask for an incognito profile to be loaded
into the app list, which isn't supported. This change makes the app list
service robust against that. Instead, if an incognito profile is
requested, initialize the app list with the original profile.

This same codepath could be used to trigger an "impossible" crash
without incognito being involved. This can happen when the app launcher
is "shown" without ever being "enabled" (i.e. if it is disabled via
command line flag or editing prefs). If it is then enabled by installing
an app to a profile different to the one that was shown, the "Create*"
codepaths were not checking for a current profile at all. This would
lead to the original app list "leaking", and a crash on browser shutdown
when 2 windows are observed to close instead of just one.

To fix this and to consolidate incognito profile checks in the right
place the create functions now check profiles. Show functions just
always "create" and assume it will be a no-op if it's already created.

BUG=358135, 373689
TEST=Added regression tests AppListControllerBrowserTest.* (or see
http://crbug.com/358135 for involved repro).

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

[tapted, 2014-04-01 02:09:35]

Hi Ben, does this seem like the right thing to do?

[benwells, 2014-04-01 02:16:21]

On 2014/04/01 02:09:35, tapted wrote:
> Hi Ben, does this seem like the right thing to do?

I'm not sure. In the original repro, what does installing an app into an
incognito profile actually do?

[tapted, 2014-04-01 02:34:20]

On 2014/04/01 02:16:21, benwells wrote:
> On 2014/04/01 02:09:35, tapted wrote:
> > Hi Ben, does this seem like the right thing to do?
> 
> I'm not sure. In the original repro, what does installing an app into an
> incognito profile actually do?

It works, for the most part. Webstore accepts a login, and webstore_installer.cc
has logic [1] to handle installing to the original profile. The only thing that
doesn't work is the 'Launch App' button that appears on the webstore app details
page after installing.

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

[benwells, 2014-04-01 02:41:57]

On 2014/04/01 02:34:20, tapted wrote:
> On 2014/04/01 02:16:21, benwells wrote:
> > On 2014/04/01 02:09:35, tapted wrote:
> > > Hi Ben, does this seem like the right thing to do?
> > 
> > I'm not sure. In the original repro, what does installing an app into an
> > incognito profile actually do?
> 
> It works, for the most part. Webstore accepts a login, and
webstore_installer.cc
> has logic [1] to handle installing to the original profile. The only thing
that
> doesn't work is the 'Launch App' button that appears on the webstore app
details
> page after installing.
> 
> [1]
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext...

I think we should get clarification from PMs about what the correct behavior is.
I'm personally not sure that isntalling an app in incognito should install the
app into the original profile. I'd be surprised by that, but maybe the privacy
and product experts have already thought about it.

[benwells, 2014-10-09 05:32:33]

lgtm

https://codereview.chromium.org/219383006/diff/50001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/app_list_shower.cc (right):

https://codereview.chromium.org/219383006/diff/50001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_shower.cc:32: DCHECK(profile_);
Are these DCHECKs related to this change or just good to adds?

[tapted, 2014-10-09 05:52:01]

eep I will need to rebase this. Things have probably changed in the last 6
months... But that's a pretty effective way to poke!

https://codereview.chromium.org/219383006/diff/50001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/app_list_shower.cc (right):

https://codereview.chromium.org/219383006/diff/50001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_shower.cc:32: DCHECK(profile_);
On 2014/10/09 05:32:33, benwells wrote:
> Are these DCHECKs related to this change or just good to adds?

semi-related. They're not being added because I tickled a bug or anything. And
It's more for the check below.

IsSameProfile(NULL) is a valid call and always returns `false`. Below that's
inverted so the `else if` there could be entered in that case, but it never will
be because `profile_ == NULL` \implies{} `app_list_ == NULL`, and the first
block will be taken.

The DCHECK is just to capture that invariant. i.e. to make it clear these blocks
are always *replacing* a profile (as opposed to the `CreateViewForProfile` path
which should always be taken if profile_ is non-NULL).

[tapted, 2014-10-20 05:18:45]

Patchset #8 (id:240001) has been deleted

[tapted, 2014-10-20 05:36:18]

Hi Ben, PTAL. While updating the new tests (yay tests) I stumbled upon the root
cause for http://crbug.com/373689 . Putting the IsSameProfile() checks in the
right place fixes it. Details in the CL description.

[tapted, 2014-10-20 05:37:19]

Patchset #6 (id:90001) has been deleted

[benwells, 2014-10-21 04:55:51]

lgtm. looks much nicer :)

[tapted, 2014-11-03 06:38:41]

The CQ bit was checked by tapted@chromium.org

[commit-bot: I haz the power, 2014-11-03 06:39:38]

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

[commit-bot: I haz the power, 2014-11-03 07:29:41]

Committed patchset #8 (id:280001)

[commit-bot: I haz the power, 2014-11-03 07:30:29]

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