Remove the --allow-no-sandbox-job flag in favor of automatic recognition.

Deprecated the --allow-no-sandbox-job flag in favor of automatic recognition.

Instead of requiring the user to set the flag when running Chrome in TS
environment, try to recognize the environment and allow the sandbox to
run without this flag whenever needed.

Measure how often the automatic recognition for terminal services environment would have incorrectly decided that the job object should be applied when it shouldn't have been as dictated by the flag --allow-no-sanbox-job.

BUG=687147
TEST=Manual in TS env on Win Srv 2k8 R2.

Review-Url: https://codereview.chromium.org/2672003002
Cr-Commit-Position: refs/heads/master@{#454429}
Committed: https://chromium.googlesource.com/chromium/src/+/7091f0852e6a60119f3cdbcc7eb34c479bf93fc9

[pastarmovj, 2017-02-03 15:04:51]

pastarmovj@chromium.org changed reviewers:
+ wfh@chromium.org

[pastarmovj, 2017-02-03 15:04:52]

Hi Will, please take a look at this CL. This is one of the things we discussed
while I was in MTV.
Thanks,
Julian

[Will Harris, 2017-02-03 17:38:50]

I approve of the plan!

https://codereview.chromium.org/2672003002/diff/20001/content/common/sandbox_...
File content/common/sandbox_win.cc (right):

https://codereview.chromium.org/2672003002/diff/20001/content/common/sandbox_...
content/common/sandbox_win.cc:279: // According to MS this flag can be false for
a remote session only on Windows
can you link the source?

https://codereview.chromium.org/2672003002/diff/20001/content/common/sandbox_...
content/common/sandbox_win.cc:280: // Server 2012 and newer so if we do the
check last we should be on the safe
we can't just check for this version of OS instead?

How come we can't just detect the Job, as above? Why is that code not correctly
detecting the failure?

https://codereview.chromium.org/2672003002/diff/20001/content/common/sandbox_...
content/common/sandbox_win.cc:282: if (!::GetSystemMetrics(SM_REMOTESESSION))
how is this tested? is there a set of automated tests, or a test plan?

[pastarmovj, 2017-02-06 13:05:27]

https://codereview.chromium.org/2672003002/diff/20001/content/common/sandbox_...
File content/common/sandbox_win.cc (right):

https://codereview.chromium.org/2672003002/diff/20001/content/common/sandbox_...
content/common/sandbox_win.cc:279: // According to MS this flag can be false for
a remote session only on Windows
On 2017/02/03 17:38:50, Will Harris wrote:
> can you link the source?

Done.

https://codereview.chromium.org/2672003002/diff/20001/content/common/sandbox_...
content/common/sandbox_win.cc:280: // Server 2012 and newer so if we do the
check last we should be on the safe
On 2017/02/03 17:38:50, Will Harris wrote:
> we can't just check for this version of OS instead?
> 
> How come we can't just detect the Job, as above? Why is that code not
correctly
> detecting the failure?

Not sure what do you mean? What I am trying to say is that if we were to rely on
that on a newer Windows version we might end up assuming we should not allow the
Job escape hatch while we are actually running in a remote session. But at least
according to the documentation this function is always correct for Windows
Server 2008 . The check above on line 256 do check the OS version.

https://codereview.chromium.org/2672003002/diff/20001/content/common/sandbox_...
content/common/sandbox_win.cc:282: if (!::GetSystemMetrics(SM_REMOTESESSION))
On 2017/02/03 17:38:50, Will Harris wrote:
> how is this tested? is there a set of automated tests, or a test plan?

This is something I will start another thread about this week :)

Right now I tested it by building a fresh branded build and running it on my
Terminal Services installation on SkyTap.

I would love to have some automated testing and have started doing some tests
how to build that but I will love some more input on this however it is beyond
the scope of this CL I think.

[Will Harris, 2017-02-07 22:57:07]

okay thanks for the clarifications.

It would be good to have some testing here - at least verify that this was
tested, or perhaps a manual TEST line for TE to run to quality the build?

Do we still need to leave --allow-no-sandbox-job in for the edge case where our
detection does not correctly work, or there is some other configuration under
which --allow-no-sandbox-job is being used? I have a feeling this will go to
stable then these cases will pop out of the woodwork...?

Perhaps we can measure this with UMA e.g. the number of users using
--allow-no-sandbox-job and also intersection of users who are using
--allow-no-sandbox-job where the detection is not correctly detecting them
(should be zero, if the code is correct and catching all cases).

[pastarmovj, 2017-02-08 12:21:28]

On 2017/02/07 22:57:07, Will Harris wrote:
> okay thanks for the clarifications.
> 
> It would be good to have some testing here - at least verify that this was
> tested, or perhaps a manual TEST line for TE to run to quality the build?
> 
> Do we still need to leave --allow-no-sandbox-job in for the edge case where
our
> detection does not correctly work, or there is some other configuration under
> which --allow-no-sandbox-job is being used? I have a feeling this will go to
> stable then these cases will pop out of the woodwork...?
> 
> Perhaps we can measure this with UMA e.g. the number of users using
> --allow-no-sandbox-job and also intersection of users who are using
> --allow-no-sandbox-job where the detection is not correctly detecting them
> (should be zero, if the code is correct and catching all cases).

Sounds reasonable although there was only one more restriction on top of what
used to be the case before (the GetSystemMetrics(SM_REMOTESESSION)). I reverted
the removal of the flag for now and added a metric about how many times we would
have guessed right and how many times the flag override was actually needed. 

We should keep in mind though that only a very small fraction of enterprises
turn UMA on so we should not expect large numbers out of that.

[pastarmovj, 2017-02-10 08:59:58]

On 2017/02/08 12:21:28, pastarmovj wrote:
> On 2017/02/07 22:57:07, Will Harris wrote:
> > okay thanks for the clarifications.
> > 
> > It would be good to have some testing here - at least verify that this was
> > tested, or perhaps a manual TEST line for TE to run to quality the build?
> > 
> > Do we still need to leave --allow-no-sandbox-job in for the edge case where
> our
> > detection does not correctly work, or there is some other configuration
under
> > which --allow-no-sandbox-job is being used? I have a feeling this will go to
> > stable then these cases will pop out of the woodwork...?
> > 
> > Perhaps we can measure this with UMA e.g. the number of users using
> > --allow-no-sandbox-job and also intersection of users who are using
> > --allow-no-sandbox-job where the detection is not correctly detecting them
> > (should be zero, if the code is correct and catching all cases).
> 
> Sounds reasonable although there was only one more restriction on top of what
> used to be the case before (the GetSystemMetrics(SM_REMOTESESSION)). I
reverted
> the removal of the flag for now and added a metric about how many times we
would
> have guessed right and how many times the flag override was actually needed. 
> 
> We should keep in mind though that only a very small fraction of enterprises
> turn UMA on so we should not expect large numbers out of that.

friendly ping? :)

[Will Harris, 2017-02-10 18:45:36]

sorry for delay I was at an offsite on wed/thur

https://codereview.chromium.org/2672003002/diff/80001/content/common/sandbox_...
File content/common/sandbox_win.cc (right):

https://codereview.chromium.org/2672003002/diff/80001/content/common/sandbox_...
content/common/sandbox_win.cc:272: return true;
are you sure that one of these earlier tests won't just pass and just return
true?

e.g. if Citrix is really running chrome browser processes inside an existing Job
object - does it set JOB_OBJECT_LIMIT_BREAKAWAY_OK?

https://codereview.chromium.org/2672003002/diff/80001/content/common/sandbox_...
content/common/sandbox_win.cc:285: // TODO(pastarmovj): Remove this check and
the flag altogher once we are
nit: typo

https://codereview.chromium.org/2672003002/diff/80001/content/common/sandbox_...
content/common/sandbox_win.cc:288:
UMA_HISTOGRAM_BOOLEAN("Process.Sandbox.JobAvoidedCorrectly", false);
I'd prefer to record this histogram all the time and have a variety of cases, or
only record it once - as data analysis is hard when it's only sometimes
recorded.

[pastarmovj, 2017-02-13 12:26:41]

https://codereview.chromium.org/2672003002/diff/80001/content/common/sandbox_...
File content/common/sandbox_win.cc (right):

https://codereview.chromium.org/2672003002/diff/80001/content/common/sandbox_...
content/common/sandbox_win.cc:272: return true;
On 2017/02/10 18:45:35, Will Harris wrote:
> are you sure that one of these earlier tests won't just pass and just return
> true?
> 
> e.g. if Citrix is really running chrome browser processes inside an existing
Job
> object - does it set JOB_OBJECT_LIMIT_BREAKAWAY_OK?

It is not setting the breakaway flag unfortunately. Also if any of those is true
there is no reason we should allow Chrome to run without a Job this is why those
return true regardless of the flag.

https://codereview.chromium.org/2672003002/diff/80001/content/common/sandbox_...
content/common/sandbox_win.cc:288:
UMA_HISTOGRAM_BOOLEAN("Process.Sandbox.JobAvoidedCorrectly", false);
On 2017/02/10 18:45:35, Will Harris wrote:
> I'd prefer to record this histogram all the time and have a variety of cases,
or
> only record it once - as data analysis is hard when it's only sometimes
> recorded.

As the histogram is worded now it is collected every time the decision has to be
made (compared to the old logic). I guess what you are asking is to make it not
comparing the old logic but in general track the "false" state if we ever return
true. Or alternatively have an enum tracking every reason why the job is to be
applied or not applied? Am I correct about that?

[Will Harris, 2017-02-13 18:28:56]

sorry this CL is warping my brain I can't seem to get my head round it.

https://codereview.chromium.org/2672003002/diff/80001/content/common/sandbox_...
File content/common/sandbox_win.cc (right):

https://codereview.chromium.org/2672003002/diff/80001/content/common/sandbox_...
content/common/sandbox_win.cc:253: bool ShouldSetJobLevel(const
base::CommandLine& cmd_line) {
so behavior before was that if --allow-no-sandbox-job was not specified then the
job object was always applied

behavior now is that that now tests are always run and if any of:

 - version >= win8
 - not in job
 - inside a job but it's JOB_OBJECT_LIMIT_BREAKAWAY_OK
 - not a remote session

then return true, otherwise return false.

I think what we want to UMA is the times that none of the above hit, but the
user has specified --allow-no-sandbox-job to override.

Right now the only time we record the histogram is for the final part.

So perhaps the solution here is to just move the code to the parent function and
record the whether histogram --allow-no-sandbox-job is specified or not, if
ShouldSetJobLevel returns true.

This will catch some false positives e.g. version > = win8 but I think we can
filter UMA by this. WDYT?

Or maybe I am overthinking this. I've spent an hour so far looking at this CL.

https://codereview.chromium.org/2672003002/diff/80001/content/common/sandbox_...
content/common/sandbox_win.cc:288:
UMA_HISTOGRAM_BOOLEAN("Process.Sandbox.JobAvoidedCorrectly", false);
On 2017/02/13 12:26:41, pastarmovj wrote:
> On 2017/02/10 18:45:35, Will Harris wrote:
> > I'd prefer to record this histogram all the time and have a variety of
cases,
> or
> > only record it once - as data analysis is hard when it's only sometimes
> > recorded.
> 
> As the histogram is worded now it is collected every time the decision has to
be
> made (compared to the old logic). I guess what you are asking is to make it
not
> comparing the old logic but in general track the "false" state if we ever
return
> true. Or alternatively have an enum tracking every reason why the job is to be
> applied or not applied? Am I correct about that?

normally best practice for histograms is to always record them but vary the
value being recorded. This makes analysis easier, and also reduces code size as
each histogram macro expands quite a bit in code.

[pastarmovj, 2017-02-14 12:10:33]

Moved the stats collecting above and fixed the typo.

https://codereview.chromium.org/2672003002/diff/80001/content/common/sandbox_...
File content/common/sandbox_win.cc (right):

https://codereview.chromium.org/2672003002/diff/80001/content/common/sandbox_...
content/common/sandbox_win.cc:285: // TODO(pastarmovj): Remove this check and
the flag altogher once we are
On 2017/02/10 18:45:35, Will Harris wrote:
> nit: typo

Done.

https://codereview.chromium.org/2672003002/diff/80001/content/common/sandbox_...
content/common/sandbox_win.cc:288:
UMA_HISTOGRAM_BOOLEAN("Process.Sandbox.JobAvoidedCorrectly", false);
On 2017/02/13 18:28:55, Will Harris wrote:
> On 2017/02/13 12:26:41, pastarmovj wrote:
> > On 2017/02/10 18:45:35, Will Harris wrote:
> > > I'd prefer to record this histogram all the time and have a variety of
> cases,
> > or
> > > only record it once - as data analysis is hard when it's only sometimes
> > > recorded.
> > 
> > As the histogram is worded now it is collected every time the decision has
to
> be
> > made (compared to the old logic). I guess what you are asking is to make it
> not
> > comparing the old logic but in general track the "false" state if we ever
> return
> > true. Or alternatively have an enum tracking every reason why the job is to
be
> > applied or not applied? Am I correct about that?
> 
> normally best practice for histograms is to always record them but vary the
> value being recorded. This makes analysis easier, and also reduces code size
as
> each histogram macro expands quite a bit in code.

Agree. Moved that to the calling function.

[Will Harris, 2017-02-16 23:48:17]

https://codereview.chromium.org/2672003002/diff/100001/content/common/sandbox...
File content/common/sandbox_win.cc (right):

https://codereview.chromium.org/2672003002/diff/100001/content/common/sandbox...
content/common/sandbox_win.cc:585:
UMA_HISTOGRAM_BOOLEAN("Process.Sandbox.ShouldSetJobNone", should_set_job);
I feel like I'm being a terrible reviewer here and making you go round in
circles but I think the point of having the UMA histogram is to detect the times
when

 - enterprise is using --allow-no-sandbox-job
 - had they not used this switch, then the sandbox would not have correctly
worked.

I think the code right now just reports when --allow-no-sandbox-job is being
used, but not when it would have worked had it not been used.

[pastarmovj, 2017-02-22 14:41:10]

On 2017/02/16 23:48:17, Will Harris wrote:
>
https://codereview.chromium.org/2672003002/diff/100001/content/common/sandbox...
> File content/common/sandbox_win.cc (right):
> 
>
https://codereview.chromium.org/2672003002/diff/100001/content/common/sandbox...
> content/common/sandbox_win.cc:585:
> UMA_HISTOGRAM_BOOLEAN("Process.Sandbox.ShouldSetJobNone", should_set_job);
> I feel like I'm being a terrible reviewer here and making you go round in
> circles but I think the point of having the UMA histogram is to detect the
times
> when
> 
>  - enterprise is using --allow-no-sandbox-job
>  - had they not used this switch, then the sandbox would not have correctly
> worked.
> 
> I think the code right now just reports when --allow-no-sandbox-job is being
> used, but not when it would have worked had it not been used.

Sorry now I had to pospone responding due to another CLs. :)

So if I get this last comment right - the way we collected this metric in Patch
Set 5 was what you (and I) think is the more interesting metric?

[pastarmovj, 2017-02-27 14:08:03]

On 2017/02/22 14:41:10, pastarmovj wrote:
> On 2017/02/16 23:48:17, Will Harris wrote:
> >
>
https://codereview.chromium.org/2672003002/diff/100001/content/common/sandbox...
> > File content/common/sandbox_win.cc (right):
> > 
> >
>
https://codereview.chromium.org/2672003002/diff/100001/content/common/sandbox...
> > content/common/sandbox_win.cc:585:
> > UMA_HISTOGRAM_BOOLEAN("Process.Sandbox.ShouldSetJobNone", should_set_job);
> > I feel like I'm being a terrible reviewer here and making you go round in
> > circles but I think the point of having the UMA histogram is to detect the
> times
> > when
> > 
> >  - enterprise is using --allow-no-sandbox-job
> >  - had they not used this switch, then the sandbox would not have correctly
> > worked.
> > 
> > I think the code right now just reports when --allow-no-sandbox-job is being
> > used, but not when it would have worked had it not been used.
> 
> Sorry now I had to pospone responding due to another CLs. :)
> 
> So if I get this last comment right - the way we collected this metric in
Patch
> Set 5 was what you (and I) think is the more interesting metric?

Hi Will, can you take a look at my reply/question above?

[Will Harris, 2017-02-27 19:26:08]

On 2017/02/27 14:08:03, pastarmovj wrote:
> On 2017/02/22 14:41:10, pastarmovj wrote:
> > On 2017/02/16 23:48:17, Will Harris wrote:
> > >
> >
>
https://codereview.chromium.org/2672003002/diff/100001/content/common/sandbox...
> > > File content/common/sandbox_win.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2672003002/diff/100001/content/common/sandbox...
> > > content/common/sandbox_win.cc:585:
> > > UMA_HISTOGRAM_BOOLEAN("Process.Sandbox.ShouldSetJobNone", should_set_job);
> > > I feel like I'm being a terrible reviewer here and making you go round in
> > > circles but I think the point of having the UMA histogram is to detect the
> > times
> > > when
> > > 
> > >  - enterprise is using --allow-no-sandbox-job
> > >  - had they not used this switch, then the sandbox would not have
correctly
> > > worked.
> > > 
> > > I think the code right now just reports when --allow-no-sandbox-job is
being
> > > used, but not when it would have worked had it not been used.
> > 
> > Sorry now I had to pospone responding due to another CLs. :)
> > 
> > So if I get this last comment right - the way we collected this metric in
> Patch
> > Set 5 was what you (and I) think is the more interesting metric?
> 
> Hi Will, can you take a look at my reply/question above?

yes the data that was being reported by the histogram in ps5 was more what I was
thinking of, but just reporting it all the time (either false or true).

[pastarmovj, 2017-02-28 17:39:31]

On 2017/02/27 19:26:08, Will Harris wrote:
> On 2017/02/27 14:08:03, pastarmovj wrote:
> > On 2017/02/22 14:41:10, pastarmovj wrote:
> > > On 2017/02/16 23:48:17, Will Harris wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/2672003002/diff/100001/content/common/sandbox...
> > > > File content/common/sandbox_win.cc (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/2672003002/diff/100001/content/common/sandbox...
> > > > content/common/sandbox_win.cc:585:
> > > > UMA_HISTOGRAM_BOOLEAN("Process.Sandbox.ShouldSetJobNone",
should_set_job);
> > > > I feel like I'm being a terrible reviewer here and making you go round
in
> > > > circles but I think the point of having the UMA histogram is to detect
the
> > > times
> > > > when
> > > > 
> > > >  - enterprise is using --allow-no-sandbox-job
> > > >  - had they not used this switch, then the sandbox would not have
> correctly
> > > > worked.
> > > > 
> > > > I think the code right now just reports when --allow-no-sandbox-job is
> being
> > > > used, but not when it would have worked had it not been used.
> > > 
> > > Sorry now I had to pospone responding due to another CLs. :)
> > > 
> > > So if I get this last comment right - the way we collected this metric in
> > Patch
> > > Set 5 was what you (and I) think is the more interesting metric?
> > 
> > Hi Will, can you take a look at my reply/question above?
> 
> yes the data that was being reported by the histogram in ps5 was more what I
was
> thinking of, but just reporting it all the time (either false or true).

I think I got it :) ptal

[Will Harris, 2017-02-28 17:42:18]

lgtm. sorry for the long review timeline, and for being unclear!

[pastarmovj, 2017-02-28 17:48:17]

No worries and thanks for the review! 

I will test it on my actual Citrix instance tomorrow to make sure I didn't make
some stupid mistake somewhere and land if it passes.

[pastarmovj, 2017-03-01 16:22:54]

pastarmovj@chromium.org changed reviewers:
+ jwd@chromium.org

[pastarmovj, 2017-03-01 16:22:55]

Adding jwd for histograms.xml owner review.

[jwd, 2017-03-01 20:09:31]

https://codereview.chromium.org/2672003002/diff/120001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2672003002/diff/120001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:52682: +<histogram
name="Process.Sandbox.JobAvoidedCorrectly" enum="Boolean">
Can you add a more descriptive enum for this?

[pastarmovj, 2017-03-02 06:28:30]

https://codereview.chromium.org/2672003002/diff/120001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2672003002/diff/120001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:52682: +<histogram
name="Process.Sandbox.JobAvoidedCorrectly" enum="Boolean">
On 2017/03/01 20:09:31, jwd wrote:
> Can you add a more descriptive enum for this? 

What do you think would better fit here? I thought that provided the name of the
histogram a True/False enum fits perfectly.

[jwd, 2017-03-02 19:58:14]

https://codereview.chromium.org/2672003002/diff/120001/content/common/sandbox...
File content/common/sandbox_win.cc (right):

https://codereview.chromium.org/2672003002/diff/120001/content/common/sandbox...
content/common/sandbox_win.cc:252: // Checks if the sandbox should be let to run
without a job object assigned.
So, this comment implies to me that a return of true means that the sandbox
should be let to run without a job object assigned. The code, or at least the
use of the kAllowNoSandboxJob makes me think that it's the reverse. If it is the
reverse, can you update this comment to make that clearer?

https://codereview.chromium.org/2672003002/diff/120001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2672003002/diff/120001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:52682: +<histogram
name="Process.Sandbox.JobAvoidedCorrectly" enum="Boolean">
On 2017/03/02 06:28:30, pastarmovj wrote:
> On 2017/03/01 20:09:31, jwd wrote:
> > Can you add a more descriptive enum for this? 
> 
> What do you think would better fit here? I thought that provided the name of
the
> histogram a True/False enum fits perfectly.

I actually find the name, summary, and use of this histogram a bit confusing.
From the name alone I interpret the values as:
True: The job was avoided correctly.
False: The job was not avoided, or was avoided incorrectly. (it would be good to
clarify if this is the case)

From the code:
True: Whatever happened with avoiding the job, it was correct.
False: The job was avoided based on the flag, but it shouldn't have been because
the flag was used in a non-remote session.

From the summary:
One of the values, not sure which means: The job was avoided; for some reason it
would have been applied (not avoided?) but it wasn't.
The other value is not used.

[jwd, 2017-03-02 19:59:12]

The intent of this change appears to have changed, can you update the title and
description?

[pastarmovj, 2017-03-02 20:12:12]

Description was changed from

==========
Remove the --allow-no-sandbox-job flag in favor of automatic recognition.

Instead of requiring the user to set the flag when running crome in TS
environment, try to recognize the environment and allow the sandbox to
run without this flag whenever needed.

BUG=687147
TEST=Manual in TS env on Win Srv 2k8 R2.
==========

to

==========
Deprecated the --allow-no-sandbox-job flag in favor of automatic recognition.

Instead of requiring the user to set the flag when running Chrome in TS
environment, try to recognize the environment and allow the sandbox to
run without this flag whenever needed.

Measure how often the automatic recognition for terminal services environment
would have incorrectly decided that the job object should be applied when it
shouldn't have been as dictated by the flag --allow-no-sanbox-job.

BUG=687147
TEST=Manual in TS env on Win Srv 2k8 R2.
==========

[pastarmovj, 2017-03-02 20:21:21]

PTAL.

https://codereview.chromium.org/2672003002/diff/120001/content/common/sandbox...
File content/common/sandbox_win.cc (right):

https://codereview.chromium.org/2672003002/diff/120001/content/common/sandbox...
content/common/sandbox_win.cc:252: // Checks if the sandbox should be let to run
without a job object assigned.
On 2017/03/02 19:58:14, jwd wrote:
> So, this comment implies to me that a return of true means that the sandbox
> should be let to run without a job object assigned. The code, or at least the
> use of the kAllowNoSandboxJob makes me think that it's the reverse. If it is
the
> reverse, can you update this comment to make that clearer?

I clarified the return value of the function.

https://codereview.chromium.org/2672003002/diff/120001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2672003002/diff/120001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:52682: +<histogram
name="Process.Sandbox.JobAvoidedCorrectly" enum="Boolean">
On 2017/03/02 19:58:14, jwd wrote:
> On 2017/03/02 06:28:30, pastarmovj wrote:
> > On 2017/03/01 20:09:31, jwd wrote:
> > > Can you add a more descriptive enum for this? 
> > 
> > What do you think would better fit here? I thought that provided the name of
> the
> > histogram a True/False enum fits perfectly.
> 
> I actually find the name, summary, and use of this histogram a bit confusing.
> From the name alone I interpret the values as:
> True: The job was avoided correctly.
> False: The job was not avoided, or was avoided incorrectly. (it would be good
to
> clarify if this is the case)
> 
> From the code:
> True: Whatever happened with avoiding the job, it was correct.
> False: The job was avoided based on the flag, but it shouldn't have been
because
> the flag was used in a non-remote session.
> 
> From the summary:
> One of the values, not sure which means: The job was avoided; for some reason
it
> would have been applied (not avoided?) but it wasn't.
> The other value is not used.

I tried to document the histogram better. What do you think about this wording?

[jwd, 2017-03-02 20:55:24]

https://codereview.chromium.org/2672003002/diff/140001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2672003002/diff/140001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:52690: +    insignificant.
Ah, ok, so you care about the false case most. Based the name and summary, it
still seems strange that the true case also includes "the job object was
applied, but the flag agreed with that".

What about something like FlagOverrodeLocalSessionCheck, and reverse the values
recorded to the histogram? 

I also wonder why you need the histogram on 297? It seems to muddy the meaning
of what's being captured. If you do need it, I'd suggest switching this to a 3
value enum histogram with remote session, local session and flag agrees, local
session and flag disagrees or something.

[pastarmovj, 2017-03-02 21:11:10]

https://codereview.chromium.org/2672003002/diff/140001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2672003002/diff/140001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:52690: +    insignificant.
On 2017/03/02 20:55:24, jwd wrote:
> Ah, ok, so you care about the false case most. Based the name and summary, it
> still seems strange that the true case also includes "the job object was
> applied, but the flag agreed with that".
> 
> What about something like FlagOverrodeLocalSessionCheck, and reverse the
values
> recorded to the histogram? 

Agreed and renamed :)

> 
> I also wonder why you need the histogram on 297? It seems to muddy the meaning
> of what's being captured. If you do need it, I'd suggest switching this to a 3
> value enum histogram with remote session, local session and flag agrees, local
> session and flag disagrees or something.

This one was actually a mistake - it reappeared when I reverted Patchest 6 to
PS5 as part of the ongoing discussion. It is indeed only the binary state from
the first count.

[jwd, 2017-03-02 21:49:00]

lgtm

[pastarmovj, 2017-03-02 21:51:34]

The CQ bit was checked by pastarmovj@chromium.org

[pastarmovj, 2017-03-02 21:51:35]

The patchset sent to the CQ was uploaded after l-g-t-m from wfh@chromium.org
Link to the patchset: https://codereview.chromium.org/2672003002/#ps160001
(title: "Rename histogram.")

[commit-bot: I haz the power, 2017-03-02 21:52:02]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-02 23:24:21]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1488491494534120,
"parent_rev": "b21ac75e9523f23a71e27ed0455c93815e65f49b", "commit_rev":
"7091f0852e6a60119f3cdbcc7eb34c479bf93fc9"}

[commit-bot: I haz the power, 2017-03-02 23:25:01]

Description was changed from

==========
Deprecated the --allow-no-sandbox-job flag in favor of automatic recognition.

Instead of requiring the user to set the flag when running Chrome in TS
environment, try to recognize the environment and allow the sandbox to
run without this flag whenever needed.

Measure how often the automatic recognition for terminal services environment
would have incorrectly decided that the job object should be applied when it
shouldn't have been as dictated by the flag --allow-no-sanbox-job.

BUG=687147
TEST=Manual in TS env on Win Srv 2k8 R2.
==========

to

==========
Deprecated the --allow-no-sandbox-job flag in favor of automatic recognition.

Instead of requiring the user to set the flag when running Chrome in TS
environment, try to recognize the environment and allow the sandbox to
run without this flag whenever needed.

Measure how often the automatic recognition for terminal services environment
would have incorrectly decided that the job object should be applied when it
shouldn't have been as dictated by the flag --allow-no-sanbox-job.

BUG=687147
TEST=Manual in TS env on Win Srv 2k8 R2.

Review-Url: https://codereview.chromium.org/2672003002
Cr-Commit-Position: refs/heads/master@{#454429}
Committed:
https://chromium.googlesource.com/chromium/src/+/7091f0852e6a60119f3cdbcc7eb3...
==========

[commit-bot: I haz the power, 2017-03-02 23:25:03]

Committed patchset #9 (id:160001) as
https://chromium.googlesource.com/chromium/src/+/7091f0852e6a60119f3cdbcc7eb3...