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

content/common/sandbox_win.cc

Index: content/common/sandbox_win.cc
diff --git a/content/common/sandbox_win.cc b/content/common/sandbox_win.cc
index 6dee6e96a96299cd3aa01a46772f727c46ec6bf4..781a715465dffb43f811af329530ae0fddd2347e 100644
--- a/content/common/sandbox_win.cc
+++ b/content/common/sandbox_win.cc
@@ -251,9 +251,6 @@ base::string16 PrependWindowsSessionPath(const base::char16* object) {
 
 // Checks if the sandbox should be let to run without a job object assigned.
 bool ShouldSetJobLevel(const base::CommandLine& cmd_line) {

[Will Harris, 2017/02/13 18:28:55]

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.

-  if (!cmd_line.HasSwitch(switches::kAllowNoSandboxJob))
-    return true;
-
   // Windows 8 allows nested jobs so we don't need to check if we are in other
   // job.
   if (base::win::GetVersion() >= base::win::VERSION_WIN8)
@@ -277,6 +274,27 @@ bool ShouldSetJobLevel(const base::CommandLine& cmd_line) {
   if (job_info.BasicLimitInformation.LimitFlags & JOB_OBJECT_LIMIT_BREAKAWAY_OK)
     return true;
 
+  // Lastly in place of the flag which was supposed to be used only for running
+  // Chrome in remote sessions we do this check explicitly here.
+  // According to MS this flag can be false for a remote session only on Windows
+  // Server 2012 and newer so if we do the check last we should be on the safe
+  // side. See: https://msdn.microsoft.com/en-us/library/aa380798.aspx.
+  if (!::GetSystemMetrics(SM_REMOTESESSION)) {
+    // Measure how often we would have decided to apply the sandbox but the
+    // user actually wanted to avoid it.
+    // TODO(pastarmovj): Remove this check and the flag altogher once we are

[Will Harris, 2017/02/10 18:45:35]

nit: typo

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

Done.

+    // convinced that the automatic logic is good enough.
+    if (cmd_line.HasSwitch(switches::kAllowNoSandboxJob)) {
+      UMA_HISTOGRAM_BOOLEAN("Process.Sandbox.JobAvoidedCorrectly", false);

[Will Harris, 2017/02/10 18:45:35]

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]

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:55]

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]

Agree. Moved that to the calling function.

+      return false;
+    }
+    return true;
+  }
+
+  // Allow running without the sandbox in this case. This slightly reduces the
+  // ability of the sandbox to protect its children from spawning new processes
+  // or preventing them from shutting down Windows or accessing the clipboard.
+  UMA_HISTOGRAM_BOOLEAN("Process.Sandbox.JobAvoidedCorrectly", true);
   return false;
 }