I noticed one thing about the command line flags, which may be related.
For example, we have two flags, "foo", "bar". The standard way to add in the command like is: "--enable-features=foo,bar"

If we add flags like: "--enable-features=foo --enable-features=bar", then the first flag "foo" is ignored.

The standard way is correctly handled by "AddFeatureEnableOverride" [1]. But I see one place in chrome_setup.cc that to add the flag by attaching "AddArg("--enable-features") directly [2]. This will make earlier added feature flags got ignored.
[1] http://cs/chromeos_public/src/platform2/login_manager/chrome_setup.cc?l=410&rcl=f919dcf5394d81182d53d50ac14ae4c89486955e
[2] http://cs/chromeos_public/src/platform2/login_manager/chrome_setup.cc?l=360&rcl=f919dcf5394d81182d53d50ac14ae4c89486955e


So my guess of the "about://flags" is that, if any feature flags is turned on and attached by appending "--enable-features=baz" directly, it will make previous added feature flags got ignored. The correct way is parsing previous "--enable-features=foo,bar", and attach correctly like this "--enable-features=foo,bar,baz".

If my assumption is correct, then other about flags will not affect the feature-flags.

I had a quick test, when I turn on a feature flag from chrome://flags, there are two "--enable-features=", one is appended as before. The new flag is appended in the wrapper of --flag-switches-begin --enable-features=... --flag-switches-end.

If the following comments are true, then the original "--enable-features=flags" will be ignored.

// These two flags are added around the switches about:flags adds to the
// command line. This is useful to see which switches were added by about:flags
// on about:version. They don't have any effect.

Nice catch. Yes, that AddArg call manually adding --enable-features is broken. It's hidden behind a check for the cfm_enabled_device USE flag, though, so there should be a pretty limited scope for what it can break currently.

One solution is suggested by zelidrag@ in the other bug https://crbug.com/749145#c31, to copy original --enable-feature flags into the wrapper of --flag-switches-begin/end, and then append the new feature flags to the end of the feature flag list.

Just to summarize, there are two issues here, right?

a) session_manager can pass multiple --enable-features and --disable-features switches, and only the first (I think) of each is honored by Chrome.

b) chrome://flags (I think?) doesn't display the correct state of various features because --flag-switches-begin and --flag-switches-end aren't included around its switches in the command line.

I'm not sure if there would be pushback to this, but one option for a) may be to make Chrome parse all --enable-features and --disable-features switches passed to it. I suspect that the relevant code may be this from BrowserMainLoop::PreCreateThreads in content/browser/browser_main_loop.cc:

 833   const base::CommandLine* command_line =
 834       base::CommandLine::ForCurrentProcess();
 835   // Note that we do not initialize a new FeatureList when calling this for
 836   // the second time.
 837   base::FeatureList::InitializeInstance(
 838       command_line->GetSwitchValueASCII(switches::kEnableFeatures),
 839       command_line->GetSwitchValueASCII(switches::kDisableFeatures));

I think that this would be a departure from how Chrome handles most other flags, though, and I don't even know if there's any way to get duplicated switches from base::CommandLine without manually iterating through argv().

I'm okay with session_manager merging --enable-features and --disable-features args if it's getting them from both chrome://flags and its own code to build the command line, but as I said earlier, I'm not familiar with how flags that are added via chrome://flags make their way into the command line used by session_manager.

#6, It could be an easier solution to make Chrome parse all --enable-features and --disable-features switches passed to it.
There might be other cases (we are not familiar with all the types of switches) similar to enable/disable-features flags, which could have multiple entries. But at least we can fix this features flags here first?

BTW:
a) I think currently the last --enable-features is honored by Chrome.
b) is not a bug, as commented in the code, the wrapper is only added around the chrome://flags to indicate the flags are from chrome://flags.

The flags are stored by std::map [1], that is why Chrome honors the last --enable-features.
The command line appends switch at here [2] and has no knowledge about which kind of flags is. So it is not natural to do special handling in command line utils.
So the second solution in #6(a) may not work well unless we append whatever repeated keys (type of flag)?

I can investigate how to impl the idea in #5.

[1] https://cs.chromium.org/chromium/src/base/command_line.h?l=43&rcl=65ee9d219dd59396a24614cac6e589e4c2583874
[2] https://cs.chromium.org/chromium/src/base/command_line.cc?l=349&rcl=9fd90f3acee8e03fc303e11a86e431b484002eaf

I have a local impl of the idea in #5 and seems works.
Basically changing the FlagsState::MergeFeatureCommandLineSwitch function to merge both the feature flags inside/outside the wrapper of "--flag-switches-begin" and "--flag-switches-end".

[1] https://cs.chromium.org/chromium/src/components/flags_ui/flags_state.cc?l=683&rcl=504000e36e4dad517b14213b4e5e5d6799a3196c

When testing #10 at ChromeBooks, seems there is some problem after I merged the --enable-features flags inside/outside the wrapper.

At kevin, the device cannot successfully reboot after merging the --enable-features.

The original feature flags are: --enable-features=Pepper3DImageChromium,PointerEvent,QuickUnlockPin
The flags I turned on are "InstantTethering" and "WebAssemblyeStreaming"

After merging, the flags look like:
--enable-features=Pepper3DImageChromium,PointerEvent,QuickUnlockPin
...
--flag-switches-begin --enable-features=Pepper3DImageChromium,PointerEvent,QuickUnlockPin,InstantTethering,WebAssemblyeStreaming --flag-switches-end

The error msg in ui log is:
"Error executing...
/opt/google/chrome/chrome
[FATAL:child_exit_handler.cc(83)] Check failed: info.si_status != ChildJobInterface::kCantExec

zelidrag@ suggested that we also can fix it at ChromeOS side, to put any feature flags into the same flag wrapper as Chrome: --flag-switches-begin/end, then later Chrome code will automatically merge the feature flags.
http://cs/chromeos_public/src/platform2/libchromeos-ui/chromeos/ui/chromium_command_builder.cc?l=339&rcl=ddb5cedd1769b377acd5d3d9f4bb707ffdb178a9

We do not have "--disable-features" flags added from ChromeOS side, so it is not a problem for now.

We also need to change the expectation here:
https://cs.chromium.org/chromium/src/components/flags_ui/flags_ui_switches.cc?l=11&rcl=58f447053c73131de7008aa586380ed350010570
// These two flags are added around the switches about:flags adds to the
// command line. This is useful to see which switches were added by about:flags
// on about:version. They don't have any effect.
const char kFlagSwitchesBegin[] = "flag-switches-begin";
const char kFlagSwitchesEnd[] = "flag-switches-end";

I'm looking at making session_manager merge --enable-features and --disable-features in browser_job.cc. That feels a bit hacky, since ChromiumCommandBuilder already has its own code to build up these flags. session_manager already has code to merge --vmodule (for Telemetry, I guess -- see https://crrev.com/c/414660), though.

I might be misunderstanding your proposal, but I still don't understand exactly what to do with --flag-switches-begin and -end. I guess we can just put those around the merged --enable-features and --disable-features flags, but it may result in chrome://flags incorrectly believing that it's controlling some flags that are actually hardcoded by session_manager. That's probably not a big deal, though.

My understanding is that "--flag-switches-begin/end" are only used when we add flags from chrome://flags.
From the comments here, it will only add, modify, merge [2] flags in this wrapper, and compare to previous command line to decide should restart chrome or not [1].

[1] https://cs.chromium.org/chromium/src/components/flags_ui/flags_state.cc?l=48&rcl=f493473bcfa1d41a29a71e8d05a10903e6dede78

[2] https://cs.chromium.org/chromium/src/components/flags_ui/flags_state.cc?l=678&rcl=f493473bcfa1d41a29a71e8d05a10903e6dede78

#15, I think it will work if we do similar handling to --vmodule, merge all enable/disable-features and append at the end of the flags.
One minor concern is that in this way we do not know which flags is added by ChromeOS and chrome://flags. This could cause unnecessary restart because current logic to decide if we need to restart with flags changes. But this may not that important? [1,2]
[1] https://cs.chromium.org/chromium/src/components/flags_ui/flags_state.cc?l=585&rcl=3b09d5a1ec7d5ee8685e8b3fedc5d259d23bd634
[2] https://cs.chromium.org/chromium/src/components/flags_ui/flags_state.cc?l=45&rcl=3b09d5a1ec7d5ee8685e8b3fedc5d259d23bd634


I tested to put "--flag-switches-begin/end" around the enable/disable-features flag when chromium_command_builde add it, but later found out that Chrome side code will add this wrapper anyway without check if the ChromeOS already added it [3].
[3] https://cs.chromium.org/chromium/src/chrome/browser/chromeos/login/session/user_session_manager.cc?l=272&rcl=3b09d5a1ec7d5ee8685e8b3fedc5d259d23bd634

I've written flag-merging and -reordering code for session_manager, but I think this is more complicated than previously understood.

I believe we use --policy-switches-begin/end sentinels instead of --flag-switches-begin/end when chrome://flags is used by the owner (or flags are set by policy -- I don't know if there's even a way for session_manager to know which flags were set by policy vs. by the device owner).

If session_manager is reordering flags, what should we do if both --policy-switches-begin and --flag-switches-begin are present in the original command line?

To be more explicit: It's not clear to me that there's any way to fix this in session_manager. Right now we have multiple sentinel flags (--policy-switches-begin/end, --flag-switches-begin/end). --enable-features and --disable-features can each only appear once, and there's no way to preserve their placement within the sentinels while merging their values.

I think that the only workable approach is to update Chrome to honor multiple occurrences of --enable-features and --disable-features. I'm not familiar with this code, but I suspect that many places need to be updated, including at least the following:

base/metrics/field_trial.h: CopyFieldTrialStateToFlags
chrome/browser/about_flags.cc: ConvertFlagsToSwitches
components/variations/service/variations_service.h: SetupFieldTrials
components/variations/service/variations_field_trial_creator.cc: SetupFieldTrials
components/flags_ui/flags_state.h: ConvertFlagsToSwitches
content/browser/browser_main_loop.cc: PreCreateThreads

This will likely be fragile, as any new code that uses these flags is likely to just call base::CommandLine::GetSwitchValueASCII() without merging the values.

I don't see any way that this can happen for M61. If there's a simpler approach that I'm missing, please mention it here.

Rahul, do you know more about --policy-switches-begin/end? They're only used by KioskAppManager. If it weren't for them, it'd probably be easier to implement a somewhat-broken attempt that always moves --enable/disable-features between --flags-switches-begin/end.

derat@, can we do some thing like this:

1. Keep all wrappers --policy-switches-begin/end and --flag-switches-begin/end as it, so that it will not break any current code.

2. In browser_job.cc, we merge all the enable-features/disable-features, and append them at the end of command line in another new wrapper, e.g. --merged-feature-switches-begin/end. In this way, although we might keep multiple enable-features in the command line, but we only use the last list due to how command line parse args now.

I'm a bit nervous about depending on Chrome using the final occurrences of switches, but it looks like base::CommandLine has at least one unit test that checks this behavior, so it's hopefully stable.

I've uploaded https://chromium-review.googlesource.com/#/c/chromiumos/platform2/+/685496 to give this a try, although I haven't had a chance to test it on a live device yet.

I'm not in favor of merging this change, as there's a lot of Chrome code that does things with the command line, and it seems unwise to assert that this won't break any of it.

re #21: I am fine with letting this properly bake and ship with M-63 given that most of our users don't mess with about:flags.

Assigning back to Dan since he is already working on it.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform2/+/2ce91672b0a2a0c9a2ec7eb1cb2bd100beb2b560

commit 2ce91672b0a2a0c9a2ec7eb1cb2bd100beb2b560
Author: Daniel Erat <derat@chromium.org>
Date: Thu Sep 28 09:04:06 2017

login: Merge --enable-features and --disable-features.

Make session_manager's BrowserJob class append
--enable-features and --disable-features args to Chrome's
command line containing merged copies of the values from all
earlier occurrences of the args.

The earlier args are still preserved, as Chrome assigns
significance to their position relative to
--policy-switches-begin/end and --flag-switches-begin/end
args.

BUG= chromium:767266 
TEST=added tests

Change-Id: Ic5bb900c766950494aa693a48529522a0bc475b5
Reviewed-on: https://chromium-review.googlesource.com/685496
Commit-Ready: Dan Erat <derat@chromium.org>
Tested-by: Dan Erat <derat@chromium.org>
Reviewed-by: Dan Erat <derat@chromium.org>

[modify] https://crrev.com/2ce91672b0a2a0c9a2ec7eb1cb2bd100beb2b560/login_manager/browser_job_unittest.cc
[modify] https://crrev.com/2ce91672b0a2a0c9a2ec7eb1cb2bd100beb2b560/login_manager/browser_job.cc