Fix non-otr profile's file manager being launched in guest mode.

Fix non-otr profile's file manager being launched in guest mode.

There are two issues:
1. App laucher uses the active profile's original profile to determine
if we're in guest mode (and thus always assuming guest session is not
active).
2. Every file manager event router (each profile has one) opening
a file manager window when removable device is inserted. So the
original profile's event router will open a window too.

BUG=329658
TEST=
1. Login as guest, launch file manager from the app launcher, open a doc file
from the file manager using quick office, and verify the doc is opened in off
the record window (Ctrl-T should open guest tab).
2. Login as guest, insert an SD card, make sure only 1 file manager window is
opened. From the opened file manager window, open a doc file using quick
office and make sure the document is opened in off the record window.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=243474

[tbarzic, 2013-12-19 20:12:14]

derat: owner for c/b/u/ash
kinaba: owner for c/b/c/e/file_manager
dpolukhin,skuhne: fyi

[Dmitry Polukhin, 2013-12-19 20:14:24]

lgtm

[Daniel Erat, 2013-12-19 20:25:54]

https://codereview.chromium.org/115733007/diff/1/chrome/browser/ui/ash/launch...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc (right):

https://codereview.chromium.org/115733007/diff/1/chrome/browser/ui/ash/launch...
chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc:339:
active_profile->GetOriginalProfile();
what does GetOriginalProfile() return if we're in guest mode? it seems broken if
we have both incognito and original profiles while we're in guest mode.

[tbarzic, 2013-12-19 20:50:30]

https://codereview.chromium.org/115733007/diff/1/chrome/browser/ui/ash/launch...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc (right):

https://codereview.chromium.org/115733007/diff/1/chrome/browser/ui/ash/launch...
chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc:339:
active_profile->GetOriginalProfile();
On 2013/12/19 20:25:54, Daniel Erat wrote:
> what does GetOriginalProfile() return if we're in guest mode? it seems broken
if
> we have both incognito and original profiles while we're in guest mode.

it returns the original profile; and it does seem a bit broken.
I don't know if we can create an incognito profile without an original, though.

[Daniel Erat, 2013-12-19 20:59:58]

On 2013/12/19 20:50:30, tbarzic wrote:
>
https://codereview.chromium.org/115733007/diff/1/chrome/browser/ui/ash/launch...
> File chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc (right):
> 
>
https://codereview.chromium.org/115733007/diff/1/chrome/browser/ui/ash/launch...
> chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc:339:
> active_profile->GetOriginalProfile();
> On 2013/12/19 20:25:54, Daniel Erat wrote:
> > what does GetOriginalProfile() return if we're in guest mode? it seems
broken
> if
> > we have both incognito and original profiles while we're in guest mode.
> 
> it returns the original profile; and it does seem a bit broken.
> I don't know if we can create an incognito profile without an original,
though.

is there an issue tracking this? having a non-incognito profile in guest mode
that we're not supposed to use seems like it will be a source of frequent bugs.

[Dmitry Polukhin, 2013-12-19 21:04:55]

On 2013/12/19 20:59:58, Daniel Erat wrote:
> is there an issue tracking this? having a non-incognito profile in guest mode
> that we're not supposed to use seems like it will be a source of frequent
bugs.

What exactly you propose to track? By design incognito profile can't work
without non-incognito. There was issue that Guest mode shouldn't be implemented
as incognito crbug.com/262299

[Daniel Erat, 2013-12-19 21:10:12]

On 2013/12/19 21:04:55, Dmitry Polukhin wrote:
> On 2013/12/19 20:59:58, Daniel Erat wrote:
> > is there an issue tracking this? having a non-incognito profile in guest
mode
> > that we're not supposed to use seems like it will be a source of frequent
> bugs.
> 
> What exactly you propose to track? By design incognito profile can't work
> without non-incognito. There was issue that Guest mode shouldn't be
implemented
> as incognito crbug.com/262299

Thanks. Although that bug looks like it's going nowhere fast :-(, it's what I
was looking for.

[Mr4D (OOO till 08-26), 2013-12-19 21:11:47]

I agree with derat. I suggested earlier already that we might want to ignore the

GetOriginalProfile() call for that mode (or alternatively add a DCHECK that no
one tries to call it if he does not check before calling himself).

I wouldn't be surprised if more problems will be resolved that way for guest
mode (and if not now - in the future for sure).

[Daniel Erat, 2013-12-19 21:13:52]

(cc-ing profiles owners to see if they have opinions about this)

https://codereview.chromium.org/115733007/diff/1/chrome/browser/ui/ash/launch...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc (right):

https://codereview.chromium.org/115733007/diff/1/chrome/browser/ui/ash/launch...
chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc:339:
active_profile->GetOriginalProfile();
On 2013/12/19 20:50:30, tbarzic wrote:
> On 2013/12/19 20:25:54, Daniel Erat wrote:
> > what does GetOriginalProfile() return if we're in guest mode? it seems
broken
> if
> > we have both incognito and original profiles while we're in guest mode.
> 
> it returns the original profile; and it does seem a bit broken.
> I don't know if we can create an incognito profile without an original,
though.

until http://crbug.com/262299 is fixed, would it be better to make
GetOriginalProfile() automatically return the guest profile when
IsGuestSession() is true? would that prevent us from ever accidentally using the
"original" profile while in guest mode? (i have no idea if it'd be problematic
for other reasons.)

[rpetterson, 2013-12-19 21:28:15]

https://codereview.chromium.org/115733007/diff/1/chrome/browser/ui/ash/launch...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc (right):

https://codereview.chromium.org/115733007/diff/1/chrome/browser/ui/ash/launch...
chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc:339:
active_profile->GetOriginalProfile();
To my knowledge, you cannot create an incognito profile without an original
profile.

My primary concern would be security. That is if someone were able to access an
original profile they had no business using. I'm unsure how the guest profile is
different from the original profile and/or the incognito profile.

On 2013/12/19 21:13:52, Daniel Erat wrote:
> On 2013/12/19 20:50:30, tbarzic wrote:
> > On 2013/12/19 20:25:54, Daniel Erat wrote:
> > > what does GetOriginalProfile() return if we're in guest mode? it seems
> broken
> > if
> > > we have both incognito and original profiles while we're in guest mode.
> > 
> > it returns the original profile; and it does seem a bit broken.
> > I don't know if we can create an incognito profile without an original,
> though.
> 
> until http://crbug.com/262299 is fixed, would it be better to make
> GetOriginalProfile() automatically return the guest profile when
> IsGuestSession() is true? would that prevent us from ever accidentally using
the
> "original" profile while in guest mode? (i have no idea if it'd be problematic
> for other reasons.)

[Daniel Erat, 2013-12-19 21:33:52]

after discussion with dpolukhin, skuhne, and abodenha: could you also add a
check somewhere in the browser-creation code to make sure that we never use the
"original" profile while in guest mode? something like:

  if (profile->IsGuestSession())
    CHECK(profile != profile->GetOriginalProfile());

(perhaps there's a better way to check if a profile is incognito.)

[kinaba, 2013-12-20 01:37:40]

lgtm for file_manager/event_router

[tbarzic, 2013-12-20 20:08:36]

+stevenjb

I added some checks, and found another case where app is launched with wrong
profile: launching app list item from it's context menu.
Creating app_list_syncable_service with the off the record profile in guest mode
fixes the issue, but we should probably audit other BrowserContextKeyedServices
for which profile they should use in guest mode..

[Daniel Erat, 2013-12-20 20:42:38]

thanks! lgtm for ash

[Mr4D (OOO till 08-26), 2013-12-20 20:52:28]

lgtm (chrome_launcher_controller)

[tbarzic, 2013-12-20 21:19:12]

Adding some owners
ben: chrome/browser/ui/browser.cc
benwells: app/ and chrome/browser/ui/app_list/

[Ben Goodger (Google), 2013-12-20 21:27:19]

browser.cc lgtm

[stevenjb, 2013-12-26 22:34:16]

lgtm if comments are added / questions addressed.

https://codereview.chromium.org/115733007/diff/110001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/app_list_syncable_service.cc (right):

https://codereview.chromium.org/115733007/diff/110001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_syncable_service.cc:125: if
(!profile_->IsOffTheRecord() &&
Hmm, I guess this is OK for now. Eventually we will want to remove the switch
and combine the Initialize methods; we'll have to make certain they can deal
with OTR profiles at that point for Guest login. Could you add a comment here:
TODO(stevenjb): Correctly handle OTR profiles for Guest mode.

https://codereview.chromium.org/115733007/diff/110001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/app_list_syncable_service_factory.cc (right):

https://codereview.chromium.org/115733007/diff/110001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_syncable_service_factory.cc:64: }
Please add a comment for this. It would be even better if we had an OS_CHROMEOS
specific helper chrome::GetUserOrGuestProfile(context) which I think is what we
really want here. Or even just a non-chromeos specific
chrome::GetUserProfile(context) that Did The Right Thing for Guest mode.

https://codereview.chromium.org/115733007/diff/110001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc (right):

https://codereview.chromium.org/115733007/diff/110001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc:331: if (!profile_)
{
Do we need to check for the right profile for Guest mode here like we do for
AppList?

[tbarzic, 2013-12-27 22:35:37]

https://codereview.chromium.org/115733007/diff/110001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/app_list_syncable_service.cc (right):

https://codereview.chromium.org/115733007/diff/110001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_syncable_service.cc:125: if
(!profile_->IsOffTheRecord() &&
On 2013/12/26 22:34:17, stevenjb wrote:
> Hmm, I guess this is OK for now. Eventually we will want to remove the switch
> and combine the Initialize methods; we'll have to make certain they can deal
> with OTR profiles at that point for Guest login. Could you add a comment here:
> TODO(stevenjb): Correctly handle OTR profiles for Guest mode.
> 

Done.

https://codereview.chromium.org/115733007/diff/110001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/app_list_syncable_service_factory.cc (right):

https://codereview.chromium.org/115733007/diff/110001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_syncable_service_factory.cc:64: }
On 2013/12/26 22:34:17, stevenjb wrote:
> Please add a comment for this. It would be even better if we had an
OS_CHROMEOS
> specific helper chrome::GetUserOrGuestProfile(context) which I think is what
we
> really want here. Or even just a non-chromeos specific
> chrome::GetUserProfile(context) that Did The Right Thing for Guest mode.
> 

Yep, I agree a helper function for handling guest mode should be added (and used
in other places too), I planned to leave that part for after the holidays.

https://codereview.chromium.org/115733007/diff/110001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc (right):

https://codereview.chromium.org/115733007/diff/110001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc:331: if (!profile_)
{
On 2013/12/26 22:34:17, stevenjb wrote:
> Do we need to check for the right profile for Guest mode here like we do for
> AppList?

I don't think we do. If the profile is passed to the Controller, we should
probably assume that |profile_| is the profile that should be used at this point
(and profile_ seems to be set only in tests).

[Dmitry Polukhin, 2014-01-06 16:27:03]

Tony, please commit your changes ASAP. We have related regression on trunk, see
https://code.google.com/p/chromium/issues/detail?id=331208

[benwells, 2014-01-07 22:46:29]

lgtm with a maybe nit.

https://codereview.chromium.org/115733007/diff/360001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/app_list_syncable_service_factory.cc (right):

https://codereview.chromium.org/115733007/diff/360001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_syncable_service_factory.cc:66: Profile*
profile = static_cast<Profile*>(context);
Nit: this would be cleaner if you did the static cast first and then used
profile->IsGuestSession(), which I think would be equivalent. Can you do that
instead?

[tbarzic, 2014-01-07 23:24:28]

https://codereview.chromium.org/115733007/diff/360001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/app_list_syncable_service_factory.cc (right):

https://codereview.chromium.org/115733007/diff/360001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_syncable_service_factory.cc:66: Profile*
profile = static_cast<Profile*>(context);
On 2014/01/07 22:46:30, benwells wrote:
> Nit: this would be cleaner if you did the static cast first and then used
> profile->IsGuestSession(), which I think would be equivalent. Can you do that
> instead?

Done.

[benwells, 2014-01-07 23:25:17]

On 2014/01/07 23:24:28, tbarzic wrote:
>
https://codereview.chromium.org/115733007/diff/360001/chrome/browser/ui/app_l...
> File chrome/browser/ui/app_list/app_list_syncable_service_factory.cc (right):
> 
>
https://codereview.chromium.org/115733007/diff/360001/chrome/browser/ui/app_l...
> chrome/browser/ui/app_list/app_list_syncable_service_factory.cc:66: Profile*
> profile = static_cast<Profile*>(context);
> On 2014/01/07 22:46:30, benwells wrote:
> > Nit: this would be cleaner if you did the static cast first and then used
> > profile->IsGuestSession(), which I think would be equivalent. Can you do
that
> > instead?
> 
> Done.

slgtm

[commit-bot: I haz the power, 2014-01-07 23:28:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tbarzic@chromium.org/115733007/460001

[commit-bot: I haz the power, 2014-01-08 02:46:36]

Change committed as 243474