GTTF: launch test processes using job objects on Windows.

GTTF: launch test processes using job objects on Windows.

This helps ensure proper termination of child processes when the launcher
gets killed.

BUG=236893
R=jar@chromium.org, sky@chromium.org

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

[Paweł Hajdan Jr., 2013-09-10 18:45:58]

Carlos, how does this look to you?

If you think it's good, I'll get the necessary OWNERS approvals.

[cpu_(ooo_6.6-7.5), 2013-09-10 21:08:56]

https://codereview.chromium.org/23480061/diff/1/base/test/test_launcher.cc
File base/test/test_launcher.cc (right):

https://codereview.chromium.org/23480061/diff/1/base/test/test_launcher.cc#ne...
base/test/test_launcher.cc:558: JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE |
but the job closes because its owned by the scopedhandle, so it will kill the
child, I guess it is because you always wait in line 572?

https://codereview.chromium.org/23480061/diff/1/base/test/test_launcher.cc#ne...
base/test/test_launcher.cc:560: LOG(ERROR) << "Could not
SetJobObjectLimitFlags.";
so if the sandbox breaks away from your job then you cannot kill anything
sandboxed, is that ok?

https://codereview.chromium.org/23480061/diff/1/base/test/test_launcher.cc#ne...
base/test/test_launcher.cc:579: base::KillProcess(process_handle, -1, true);
do we need 579?

[Paweł Hajdan Jr., 2013-09-10 21:49:53]

Thanks for review.

https://codereview.chromium.org/23480061/diff/1/base/test/test_launcher.cc
File base/test/test_launcher.cc (right):

https://codereview.chromium.org/23480061/diff/1/base/test/test_launcher.cc#ne...
base/test/test_launcher.cc:558: JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE |
On 2013/09/10 21:08:56, cpu wrote:
> but the job closes because its owned by the scopedhandle, so it will kill the
> child, I guess it is because you always wait in line 572?

Yes, this is intended: by the time this function returns the child process
should be terminated.

https://codereview.chromium.org/23480061/diff/1/base/test/test_launcher.cc#ne...
base/test/test_launcher.cc:560: LOG(ERROR) << "Could not
SetJobObjectLimitFlags.";
On 2013/09/10 21:08:56, cpu wrote:
> so if the sandbox breaks away from your job then you cannot kill anything
> sandboxed, is that ok?

This is not worse than the current state of things. I think it's fine, unless
you have a specific recommendation that would make it better.

https://codereview.chromium.org/23480061/diff/1/base/test/test_launcher.cc#ne...
base/test/test_launcher.cc:579: base::KillProcess(process_handle, -1, true);
On 2013/09/10 21:08:56, cpu wrote:
> do we need 579?

Yes. For example, POSIX (Linux, Mac...).

[cpu_(ooo_6.6-7.5), 2013-09-11 02:22:06]

looks ok then.

[Paweł Hajdan Jr., 2013-09-11 17:22:37]

Hey Scott, could you take a look?

Carlos says it makes sense, and I was thinking about getting a positive review
before asking OWNERS just for stamps.

[sky, 2013-09-11 19:29:06]

LGTM

https://codereview.chromium.org/23480061/diff/1/base/test/test_launcher.cc
File base/test/test_launcher.cc (right):

https://codereview.chromium.org/23480061/diff/1/base/test/test_launcher.cc#ne...
base/test/test_launcher.cc:561: return false;
-1?

[Paweł Hajdan Jr., 2013-09-11 20:43:08]

Jim, could you do base OWNERS review?

Note that this already has a positive review, so technically just a stamp is
needed.

https://codereview.chromium.org/23480061/diff/1/base/test/test_launcher.cc
File base/test/test_launcher.cc (right):

https://codereview.chromium.org/23480061/diff/1/base/test/test_launcher.cc#ne...
base/test/test_launcher.cc:561: return false;
On 2013/09/11 19:29:06, sky wrote:
> -1?

Done.

[jar (doing other things), 2013-09-13 19:23:56]

Patch set 2, base/* LGTM for a mostly rubber stamp (including Carlos'
commentary).

[Paweł Hajdan Jr., 2013-09-13 21:24:50]

Committed patchset #3 manually as r223125 (presubmit successful).