Restart Chrome if per session flags have been specified on ChromeOS.

Restart Chrome if per session flags have been specified on ChromeOS.

After the profile is loaded we verify if the user has specfied any flags
or if there were flags specified per policy that differ from the user
specfied flags and in either case restart Chrome with the desired flags.

This allows non-owners to use the about:flags page and prevents policy
set flags from leaking inside user sessions.

BUG=221352
TEST=unit_tests & Manually by specifying flags in non-owner session and observing them respected.

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

[pastarmovj, 2013-06-11 15:07:18]

Hi guys please review this CL part of the effort to introduce about:flags for
non-owners on ChromeOS.

@Mattias: Please do a general sanity check.
@Nico: Please review the changes to about_flags.*
@Dave: Please do owner review for chromeos/dbus/* and
chrome/browser/chromeos/login/*

Thanks!

P.S. More try bots to come when I commit the DEPS roll with the new DBus call
constant in third_party/cros_system_api.

[Mattias Nissler (ping if slow), 2013-06-12 14:53:03]

https://codereview.chromium.org/16770002/diff/9001/chrome/browser/about_flags.cc
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/16770002/diff/9001/chrome/browser/about_flags...
chrome/browser/about_flags.cc:1767: bool
CompareSwitchesToCurrentCommandLine(const CommandLine& new_cmdline,
Not using this here? Maybe move to where the function gets used?

https://codereview.chromium.org/16770002/diff/9001/chrome/browser/about_flags...
chrome/browser/about_flags.cc:1771: // Scoop the flags from the new command
line.
Seems like you could make a helper function for doing that for a given command
line object?

https://codereview.chromium.org/16770002/diff/9001/chrome/browser/about_flags...
chrome/browser/about_flags.cc:1791: // Then add those between
--policy-switches-begin and --policy-switches-end.
Oh, and bonus points for the helper to handle this case as well :)

https://codereview.chromium.org/16770002/diff/9001/chrome/browser/about_flags...
chrome/browser/about_flags.cc:1801: // Don't even bother sorting and comparing
if the sizes differ.
If you were to insert into a std::set directly, the comparison would become
trivial

https://codereview.chromium.org/16770002/diff/9001/chrome/browser/about_flags.h
File chrome/browser/about_flags.h (right):

https://codereview.chromium.org/16770002/diff/9001/chrome/browser/about_flags...
chrome/browser/about_flags.h:111: // in the current process command line and
returns true if they are the same
nit: you're actually passing both CommandLine references in.

https://codereview.chromium.org/16770002/diff/9001/chrome/browser/chromeos/lo...
File chrome/browser/chromeos/login/login_utils.cc (right):

https://codereview.chromium.org/16770002/diff/9001/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/login_utils.cc:293:
UserManager::Get()->GetActiveUser()->email(), flags);
Is this now a synchronous call? Not sure that's allowed?

https://codereview.chromium.org/16770002/diff/9001/chromeos/dbus/session_mana...
File chromeos/dbus/session_manager_client.cc (right):

https://codereview.chromium.org/16770002/diff/9001/chromeos/dbus/session_mana...
chromeos/dbus/session_manager_client.cc:276:
CallSetFlagsByUsername(login_manager::kSessionManagerSetFlagsForUser,
You can just inline CallSetFlagsByUsername, the function above calls the helper
just for code sharing.

https://codereview.chromium.org/16770002/diff/9001/chromeos/dbus/session_mana...
File chromeos/dbus/session_manager_client.h (right):

https://codereview.chromium.org/16770002/diff/9001/chromeos/dbus/session_mana...
chromeos/dbus/session_manager_client.h:163: // The result of the operation is
reported through |callback|.
no callback?

[pastarmovj, 2013-06-12 16:19:32]

Thanks for the review Mattias! PTAL.

https://codereview.chromium.org/16770002/diff/9001/chrome/browser/about_flags.cc
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/16770002/diff/9001/chrome/browser/about_flags...
chrome/browser/about_flags.cc:1767: bool
CompareSwitchesToCurrentCommandLine(const CommandLine& new_cmdline,
On 2013/06/12 14:53:03, Mattias Nissler wrote:
> Not using this here? Maybe move to where the function gets used?

Yet this function is used specifically for flags therefore I kept it in the
about_flags scope. Or do you mean something else by not used here?

https://codereview.chromium.org/16770002/diff/9001/chrome/browser/about_flags...
chrome/browser/about_flags.cc:1771: // Scoop the flags from the new command
line.
On 2013/06/12 14:53:03, Mattias Nissler wrote:
> Seems like you could make a helper function for doing that for a given command
> line object?

Done.

https://codereview.chromium.org/16770002/diff/9001/chrome/browser/about_flags...
chrome/browser/about_flags.cc:1791: // Then add those between
--policy-switches-begin and --policy-switches-end.
On 2013/06/12 14:53:03, Mattias Nissler wrote:
> Oh, and bonus points for the helper to handle this case as well :)

Well I can do this for both CommandLine's therefore simply put it in that
function of ours too.

https://codereview.chromium.org/16770002/diff/9001/chrome/browser/about_flags...
chrome/browser/about_flags.cc:1801: // Don't even bother sorting and comparing
if the sizes differ.
On 2013/06/12 14:53:03, Mattias Nissler wrote:
> If you were to insert into a std::set directly, the comparison would become
> trivial

Done.

https://codereview.chromium.org/16770002/diff/9001/chrome/browser/about_flags.h
File chrome/browser/about_flags.h (right):

https://codereview.chromium.org/16770002/diff/9001/chrome/browser/about_flags...
chrome/browser/about_flags.h:111: // in the current process command line and
returns true if they are the same
On 2013/06/12 14:53:03, Mattias Nissler wrote:
> nit: you're actually passing both CommandLine references in.

This comment was outdated. I changed this for better testability. Fixed.

https://codereview.chromium.org/16770002/diff/9001/chrome/browser/chromeos/lo...
File chrome/browser/chromeos/login/login_utils.cc (right):

https://codereview.chromium.org/16770002/diff/9001/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/login_utils.cc:293:
UserManager::Get()->GetActiveUser()->email(), flags);
On 2013/06/12 14:53:03, Mattias Nissler wrote:
> Is this now a synchronous call? Not sure that's allowed?

It is. I checked with out calls around the session manager and it seems for this
kind of call (that does no IO or other heavy lifting) it is fine to do it
synchronously. What would you wait for to finish copying the array into memory -
you'd have to copy it to some local buffer anyhow and this is actually all that
we do.

https://codereview.chromium.org/16770002/diff/9001/chromeos/dbus/session_mana...
File chromeos/dbus/session_manager_client.cc (right):

https://codereview.chromium.org/16770002/diff/9001/chromeos/dbus/session_mana...
chromeos/dbus/session_manager_client.cc:276:
CallSetFlagsByUsername(login_manager::kSessionManagerSetFlagsForUser,
On 2013/06/12 14:53:03, Mattias Nissler wrote:
> You can just inline CallSetFlagsByUsername, the function above calls the
helper
> just for code sharing.

Done.

https://codereview.chromium.org/16770002/diff/9001/chromeos/dbus/session_mana...
File chromeos/dbus/session_manager_client.h (right):

https://codereview.chromium.org/16770002/diff/9001/chromeos/dbus/session_mana...
chromeos/dbus/session_manager_client.h:163: // The result of the operation is
reported through |callback|.
On 2013/06/12 14:53:03, Mattias Nissler wrote:
> no callback?

Sorry stale comment.

[Mattias Nissler (ping if slow), 2013-06-13 16:55:57]

code-wise LGTM with nits

https://codereview.chromium.org/16770002/diff/9001/chrome/browser/about_flags.cc
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/16770002/diff/9001/chrome/browser/about_flags...
chrome/browser/about_flags.cc:1767: bool
CompareSwitchesToCurrentCommandLine(const CommandLine& new_cmdline,
On 2013/06/12 16:19:33, pastarmovj wrote:
> On 2013/06/12 14:53:03, Mattias Nissler wrote:
> > Not using this here? Maybe move to where the function gets used?
> 
> Yet this function is used specifically for flags therefore I kept it in the
> about_flags scope. Or do you mean something else by not used here?

You are right, it depends on flags knowledge (i.e. delimiters), so let's keep it
here.

https://codereview.chromium.org/16770002/diff/9001/chrome/browser/chromeos/lo...
File chrome/browser/chromeos/login/login_utils.cc (right):

https://codereview.chromium.org/16770002/diff/9001/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/login_utils.cc:293:
UserManager::Get()->GetActiveUser()->email(), flags);
On 2013/06/12 16:19:33, pastarmovj wrote:
> On 2013/06/12 14:53:03, Mattias Nissler wrote:
> > Is this now a synchronous call? Not sure that's allowed?
> 
> It is. I checked with out calls around the session manager and it seems for
this
> kind of call (that does no IO or other heavy lifting) it is fine to do it
> synchronously. What would you wait for to finish copying the array into memory
-
> you'd have to copy it to some local buffer anyhow and this is actually all
that
> we do.

I think there's actually an effort going on to convert all the blocking calls
into asynchronous variants. You might want to check with satorux.

https://codereview.chromium.org/16770002/diff/26001/chrome/browser/about_flag...
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/16770002/diff/26001/chrome/browser/about_flag...
chrome/browser/about_flags.cc:113: flags.insert(first + 1, last);
if you pass the delimiters as parameters, you can make a separate call for
OS_CHROMEOS. That'd further reduce the size of this function and bring it down
to doing a single well-defined thing. Might be worthwhile, but I don't insist -
your call.

https://codereview.chromium.org/16770002/diff/26001/chrome/browser/about_flag...
chrome/browser/about_flags.cc:1806: // Don't even bother comparing if the sizes
differ.
can't you just do this?

std::equal(new_flags.first(), new_flags.last(),
           active_flags.first(), active_flags.last())

https://codereview.chromium.org/16770002/diff/26001/chrome/browser/about_flag...
File chrome/browser/about_flags_unittest.cc (right):

https://codereview.chromium.org/16770002/diff/26001/chrome/browser/about_flag...
chrome/browser/about_flags_unittest.cc:239: command_line));
nit: indentation.

[pastarmovj, 2013-06-13 17:29:36]

Mattias, thanks for the review.

Nico, Dave: a gentle ping :)

https://codereview.chromium.org/16770002/diff/9001/chrome/browser/chromeos/lo...
File chrome/browser/chromeos/login/login_utils.cc (right):

https://codereview.chromium.org/16770002/diff/9001/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/login_utils.cc:293:
UserManager::Get()->GetActiveUser()->email(), flags);
On 2013/06/13 16:55:57, Mattias Nissler wrote:
> On 2013/06/12 16:19:33, pastarmovj wrote:
> > On 2013/06/12 14:53:03, Mattias Nissler wrote:
> > > Is this now a synchronous call? Not sure that's allowed?
> > 
> > It is. I checked with out calls around the session manager and it seems for
> this
> > kind of call (that does no IO or other heavy lifting) it is fine to do it
> > synchronously. What would you wait for to finish copying the array into
memory
> -
> > you'd have to copy it to some local buffer anyhow and this is actually all
> that
> > we do.
> 
> I think there's actually an effort going on to convert all the blocking calls
> into asynchronous variants. You might want to check with satorux.
> 

Sure I will check with him but from my understanding of it - we do so for any
call that might actually do something with the data more than simply stash it in
a var. If we needed to say sort that list and save it to file etc. But in this
case we would have to make a copy in the DBus entry function post a request to
the setter and then reply with the answer which would be even slower than simply
to copy the data directly in its destination and be good :)

https://codereview.chromium.org/16770002/diff/26001/chrome/browser/about_flag...
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/16770002/diff/26001/chrome/browser/about_flag...
chrome/browser/about_flags.cc:113: flags.insert(first + 1, last);
On 2013/06/13 16:55:57, Mattias Nissler wrote:
> if you pass the delimiters as parameters, you can make a separate call for
> OS_CHROMEOS. That'd further reduce the size of this function and bring it down
> to doing a single well-defined thing. Might be worthwhile, but I don't insist
-
> your call.

I decided that it was not worth it, because it will make each call location more
verbose and in the end it will be harder to understand the call locations than
to grasp why this function has two seemingly similar and easy to follow
constructs.

https://codereview.chromium.org/16770002/diff/26001/chrome/browser/about_flag...
chrome/browser/about_flags.cc:1806: // Don't even bother comparing if the sizes
differ.
On 2013/06/13 16:55:57, Mattias Nissler wrote:
> can't you just do this?
> 
> std::equal(new_flags.first(), new_flags.last(),
>            active_flags.first(), active_flags.last())
>                            

True, dunno why I got the heavier version here :) Anyway I couldn't get rid of
the size check because the std::equals doesn't check the second list for being
empty if the first is not it will happily deref the *iter2 even if it is end()
:-(

https://codereview.chromium.org/16770002/diff/26001/chrome/browser/about_flag...
File chrome/browser/about_flags_unittest.cc (right):

https://codereview.chromium.org/16770002/diff/26001/chrome/browser/about_flag...
chrome/browser/about_flags_unittest.cc:239: command_line));
On 2013/06/13 16:55:57, Mattias Nissler wrote:
> nit: indentation.

Done.

[Nico, 2013-06-13 17:59:57]

Two minor comments. Also, s/Chorme/Chrome/ in the CL description.

https://codereview.chromium.org/16770002/diff/32001/chrome/browser/about_flag...
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/16770002/diff/32001/chrome/browser/about_flag...
chrome/browser/about_flags.cc:98: return result;
Hm, can you do this by building a temporary CommandLine object, appending flag,
and getting argv()? Then you don't need to add knowledge to CommandLine
internals here

https://codereview.chromium.org/16770002/diff/32001/chrome/browser/about_flags.h
File chrome/browser/about_flags.h (right):

https://codereview.chromium.org/16770002/diff/32001/chrome/browser/about_flag...
chrome/browser/about_flags.h:112: bool CompareSwitchesToCurrentCommandLine(const
CommandLine& new_cmdline,
AreSwitchesIdenticalToCurrentCommandLine() makes it more obvious what this
returns

[pastarmovj, 2013-06-14 11:40:37]

Thanks! PTAL.

https://codereview.chromium.org/16770002/diff/32001/chrome/browser/about_flag...
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/16770002/diff/32001/chrome/browser/about_flag...
chrome/browser/about_flags.cc:98: return result;
On 2013/06/13 17:59:57, Nico wrote:
> Hm, can you do this by building a temporary CommandLine object, appending
flag,
> and getting argv()? Then you don't need to add knowledge to CommandLine
> internals here

Done.

https://codereview.chromium.org/16770002/diff/32001/chrome/browser/about_flags.h
File chrome/browser/about_flags.h (right):

https://codereview.chromium.org/16770002/diff/32001/chrome/browser/about_flag...
chrome/browser/about_flags.h:112: bool CompareSwitchesToCurrentCommandLine(const
CommandLine& new_cmdline,
On 2013/06/13 17:59:57, Nico wrote:
> AreSwitchesIdenticalToCurrentCommandLine() makes it more obvious what this
> returns

Done.

[Nico, 2013-06-14 17:36:54]

lgtm

(It's easier for reviewers if you rebase and address comments in separate patch
sets on rietveld.)

[DaveMoore, 2013-06-17 16:55:59]

lgtm

[commit-bot: I haz the power, 2013-06-17 19:32:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pastarmovj@chromium.org/16770002/39001

[commit-bot: I haz the power, 2013-06-17 23:35:52]

Change committed as 206832

[Dmitry Polukhin, 2013-06-18 13:12:40]

I would revert this change.

https://chromiumcodereview.appspot.com/16770002/diff/39001/chrome/browser/chr...
File chrome/browser/chromeos/login/login_utils.cc (right):

https://chromiumcodereview.appspot.com/16770002/diff/39001/chrome/browser/chr...
chrome/browser/chromeos/login/login_utils.cc:294:
DBusThreadManager::Get()->GetSessionManagerClient()->SetFlagsForUser(
This changes broke locally managed user creation flow. In general this hack
looks ugly because black screen will appear when Chrome restarts. We need to
find better approach.

[Dmitry Polukhin, 2013-06-18 13:57:39]

I found easy solution how to fix LMU creation flow, CL uis under review
https://codereview.chromium.org/17346003

[Mattias Nissler (ping if slow), 2013-06-18 14:56:23]

On Tue, Jun 18, 2013 at 3:12 PM, <dpolukhin@chromium.org> wrote:

> I would revert this change.
>
>
> https://chromiumcodereview.**appspot.com/16770002/diff/**
>
39001/chrome/browser/chromeos/**login/login_utils.cc<https://chromiumcodereview.appspot.com/16770002/diff/39001/chrome/browser/chromeos/login/login_utils.cc>
> File chrome/browser/chromeos/login/**login_utils.cc (right):
>
> https://chromiumcodereview.**appspot.com/16770002/diff/**
>
39001/chrome/browser/chromeos/**login/login_utils.cc#**newcode294<https://chromiumcodereview.appspot.com/16770002/diff/39001/chrome/browser/chromeos/login/login_utils.cc#newcode294>
> chrome/browser/chromeos/login/**login_utils.cc:294:
> DBusThreadManager::Get()->**GetSessionManagerClient()->**SetFlagsForUser(
> This changes broke locally managed user creation flow. In general this
> hack looks ugly because black screen will appear when Chrome restarts.
> We need to find better approach.
>

FWIW, this has been discussed at length and there is agreement between the
various stakeholders that we'll accept the black screen for people who mess
with flags:
https://docs.google.com/a/google.com/document/d/1z6C5u8PhGZZH0TAa9BmJn3LK9Jgo...


>
>
https://chromiumcodereview.**appspot.com/16770002/<https://chromiumcodereview...
>

[Dmitry Polukhin, 2013-06-18 15:04:59]

I always see black splash and long login time. Filed issue
https://code.google.com/p/chromium/issues/detail?id=251261