Fix Launcher crash in ChromeOS guest mode.

Fix Launcher crash in ChromeOS guest mode.

The CHECK-fail was introduced in r315272 (64121b7), which switched the
app list to run with the "original profile" rather than the incognito
profile. Partially reverted that CL so that the app list runs with the
incognito profile, and fixed StartPageService to correctly use the
incognito profile (rather than being null when the app list profile is
incognito), which addresses the original issue
(http://crbug.com/440484).

Had to work around DCHECK on shutdown in StartPageService due to
crbug.com/463419.

Added a DCHECK to catch use of non-incognito profiles in guest mode
early (without having to select a search result to trigger the crash).
Added AppListControllerGuestModeBrowserTest to test opening the app list
in guest mode.

BUG=460437, 440484
Committed: https://crrev.com/3bbe0ce70d70451c7af6a29beea198d2bcd06305
Cr-Commit-Position: refs/heads/master@{#319036}

[Matt Giuca, 2015-03-03 08:35:02]

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

[Matt Giuca, 2015-03-04 02:34:23]

mgiuca@chromium.org changed reviewers:
+ tapted@chromium.org
- calamity@chromium.org

[Matt Giuca, 2015-03-04 02:34:24]

-calamity +tapted
(Please treat as urgent; this really should be fixed in beta ASAP.)

[tapted, 2015-03-04 05:01:11]

https://codereview.chromium.org/976553003/diff/20001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/app_list_controller_browsertest.cc (right):

https://codereview.chromium.org/976553003/diff/20001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_controller_browsertest.cc:235: };
nit: DISALLOW_COPY_AND_ASSIGN -- you'll need  default constructor

https://codereview.chromium.org/976553003/diff/20001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_controller_browsertest.cc:241: "user");
"user" -> TestingProfile::kTestUserProfileDir ?

https://codereview.chromium.org/976553003/diff/20001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_controller_browsertest.cc:243:
chromeos::GetOffTheRecordCommandLine(GetGoogleURL(), true, command_line_copy,
Can `GetOffTheRecordCommandLine` be cut out of the picture using the approach in
c/b/ui/webui/chromeos/guest_modE_options_ui_browsertest.cc?


otherwise....

can you just use a default-constructed `GURL()` ? DeriveCommandLine in
chrome_restart_request.cc seems to handle that. If not: comment for
GetGoogleURL() to say what it does and why it's google.com and not some other
URL like about:blank.    Using google.com in a test seems a bit sus -- I think
bots user some kind of fake DNS thingo that makes for flaky tests.

https://codereview.chromium.org/976553003/diff/20001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/app_list_view_delegate.cc (right):

https://codereview.chromium.org/976553003/diff/20001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_view_delegate.cc:291:
DCHECK(!profile_->IsGuestSession() || profile_->IsOffTheRecord())
perhaps worth noting in the comment that this is the same check as in
Browser::Browser()

https://codereview.chromium.org/976553003/diff/20001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/start_page_service.cc (right):

https://codereview.chromium.org/976553003/diff/20001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/start_page_service.cc:96:
content::Source<Profile>(service_->profile()->GetOriginalProfile()));
is that bug likely to get fixed? (could you just replace the registrar_.Add
below with the GetOriginalProfile() version?)

i.e. maybe there's just as much that assumes the order is as it currently
exists, and it's intentional that NOTIFICATION_PROFILE_DESTROYED goes out for
the original profile first.

https://codereview.chromium.org/976553003/diff/20001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/start_page_service.cc:111: DCHECK(source_profile ==
service_->profile() ||
up to you, but you could also do

DCHECK(service_->profile()->IsSameProfile(content::Source<Profile>(source).ptr())));

https://codereview.chromium.org/976553003/diff/20001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/start_page_service.cc:113: service_->Shutdown();
I doubt it really matters, but is it worth doing registrar_.RemoveAll() to avoid
a duplicate call to Shutdown()?

[Matt Giuca, 2015-03-04 06:07:06]

https://codereview.chromium.org/976553003/diff/20001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/app_list_controller_browsertest.cc (right):

https://codereview.chromium.org/976553003/diff/20001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_controller_browsertest.cc:235: };
On 2015/03/04 05:01:10, tapted wrote:
> nit: DISALLOW_COPY_AND_ASSIGN -- you'll need  default constructor

Done.

https://codereview.chromium.org/976553003/diff/20001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_controller_browsertest.cc:241: "user");
On 2015/03/04 05:01:10, tapted wrote:
> "user" -> TestingProfile::kTestUserProfileDir ?

Done.

https://codereview.chromium.org/976553003/diff/20001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_controller_browsertest.cc:243:
chromeos::GetOffTheRecordCommandLine(GetGoogleURL(), true, command_line_copy,
Yeah, I replaced this with the code from guest_mode_options_ui_browsertest.

I found a lot of different tests that each enter Guest Mode in their own special
way. I liked the GetOffTheRecordCommandLine since it seemed to be abstracting
away some of the details. But nah, not worth it as you say.

https://codereview.chromium.org/976553003/diff/20001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/app_list_view_delegate.cc (right):

https://codereview.chromium.org/976553003/diff/20001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_view_delegate.cc:291:
DCHECK(!profile_->IsGuestSession() || profile_->IsOffTheRecord())
On 2015/03/04 05:01:10, tapted wrote:
> perhaps worth noting in the comment that this is the same check as in
> Browser::Browser()

Done.

https://codereview.chromium.org/976553003/diff/20001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/start_page_service.cc (right):

https://codereview.chromium.org/976553003/diff/20001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/start_page_service.cc:96:
content::Source<Profile>(service_->profile()->GetOriginalProfile()));
I think it's definitely a bug either way, with one of two acceptable solutions:

- Swap the destruction order, or (if that doesn't work),
- Relax the DCHECK to allow services to exist on the OTR profile while the
Original profile is destroyed.

Note the asymmetry in the DCHECK at the moment: it is illegal to kill the
original profile while there are hosts on the OTR profile, but legal to kill the
OTR profile while there are hosts on the original profile. So killing the OTR
profile *first* should be fine, UNLESS there is code that explicitly assumes the
original profile is killed first (as you are asking me to do here).

We shouldn't write code that assumes the presence of the existing bug (because
what you are suggesting would break if the OTR profile was destroyed first). It
is an acceptable work-around to have the code work correctly regardless of
whether the bug exists.

https://codereview.chromium.org/976553003/diff/20001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/start_page_service.cc:111: DCHECK(source_profile ==
service_->profile() ||
On 2015/03/04 05:01:10, tapted wrote:
> up to you, but you could also do
> 
>
DCHECK(service_->profile()->IsSameProfile(content::Source<Profile>(source).ptr())));

Done. (Yeah, that's better.)

(Although I had to spend 30 minutes digging around in the implementation of
IsSameProfile to convince myself it worked.)

https://codereview.chromium.org/976553003/diff/20001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/start_page_service.cc:113: service_->Shutdown();
On 2015/03/04 05:01:11, tapted wrote:
> I doubt it really matters, but is it worth doing registrar_.RemoveAll() to
avoid
> a duplicate call to Shutdown()?

Done. (Although I tested that Shutdown was idempotent; it seems to be, but this
is better.)

[tapted, 2015-03-04 07:45:10]

lgtm

https://codereview.chromium.org/976553003/diff/20001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/start_page_service.cc (right):

https://codereview.chromium.org/976553003/diff/20001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/start_page_service.cc:96:
content::Source<Profile>(service_->profile()->GetOriginalProfile()));
On 2015/03/04 06:07:05, Matt Giuca wrote:
> I think it's definitely a bug either way, with one of two acceptable
solutions:
> 
> - Swap the destruction order, or (if that doesn't work),
> - Relax the DCHECK to allow services to exist on the OTR profile while the
> Original profile is destroyed.
> 
> Note the asymmetry in the DCHECK at the moment: it is illegal to kill the
> original profile while there are hosts on the OTR profile, but legal to kill
the
> OTR profile while there are hosts on the original profile. So killing the OTR
> profile *first* should be fine, UNLESS there is code that explicitly assumes
the
> original profile is killed first (as you are asking me to do here).
> 
> We shouldn't write code that assumes the presence of the existing bug (because
> what you are suggesting would break if the OTR profile was destroyed first).
It
> is an acceptable work-around to have the code work correctly regardless of
> whether the bug exists.

Acknowledged.

[Matt Giuca, 2015-03-04 07:54:34]

The CQ bit was checked by mgiuca@chromium.org

[commit-bot: I haz the power, 2015-03-04 07:55:08]

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

[commit-bot: I haz the power, 2015-03-04 08:53:53]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-03-04 08:54:42]

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