GTTF: kill spawned test processes when the launcher is killed

GTTF: kill spawned test processes when the launcher is killed

BUG=236893
R=sky@chromium.org

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

[Paweł Hajdan Jr., 2013-09-10 00:39:19]

Windows will follow (will use job objects there).

[sky, 2013-09-10 16:17:26]

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

https://codereview.chromium.org/23484026/diff/6001/base/test/test_launcher.cc...
base/test/test_launcher.cc:54: LazyInstance<std::set<ProcessHandle> >
g_live_process_handles;
These aren't globals. My take of the style guide is that these should not have a
g_ in front of them.

https://codereview.chromium.org/23484026/diff/6001/base/test/test_launcher.cc...
base/test/test_launcher.cc:658: if (exit_code != 0) {
How come you have this inside the lock? Doesn't seem like it needs to be part of
the lock.

[Paweł Hajdan Jr., 2013-09-10 17:17:08]

Thanks for review.

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

https://codereview.chromium.org/23484026/diff/6001/base/test/test_launcher.cc...
base/test/test_launcher.cc:54: LazyInstance<std::set<ProcessHandle> >
g_live_process_handles;
On 2013/09/10 16:17:27, sky wrote:
> These aren't globals. My take of the style guide is that these should not have
a
> g_ in front of them.

I'm fine to make changes but would like to understand first. Is it because of
anonymous namespace or LazyInstance, or something else?

Using git grep I've found plenty of examples of using g_ with LazyInstance.

I don't think anonymous namespace would change things either - they're certainly
not local variables...

https://codereview.chromium.org/23484026/diff/6001/base/test/test_launcher.cc...
base/test/test_launcher.cc:658: if (exit_code != 0) {
On 2013/09/10 16:17:27, sky wrote:
> How come you have this inside the lock? Doesn't seem like it needs to be part
of
> the lock.

This is tricky, and suggestions how to improve the comment above are welcome.

If this is not under lock, we'd possibly try to kill that process twice, and
base::KillProcessGroup spews ugly log messages in that case. Using the lock
avoids that.

Alternatively I think I could just use raw kill(...) here, but I still prefer
current solution.

[sky, 2013-09-10 19:14:45]

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

https://codereview.chromium.org/23484026/diff/6001/base/test/test_launcher.cc...
base/test/test_launcher.cc:54: LazyInstance<std::set<ProcessHandle> >
g_live_process_handles;
On 2013/09/10 17:17:08, Paweł Hajdan Jr. wrote:
> On 2013/09/10 16:17:27, sky wrote:
> > These aren't globals. My take of the style guide is that these should not
have
> a
> > g_ in front of them.
> 
> I'm fine to make changes but would like to understand first. Is it because of
> anonymous namespace or LazyInstance, or something else?
> 
> Using git grep I've found plenty of examples of using g_ with LazyInstance.
> 
> I don't think anonymous namespace would change things either - they're
certainly
> not local variables...

My reasoning is that it's not a global. g_browser_process is a global.

https://codereview.chromium.org/23484026/diff/6001/base/test/test_launcher.cc...
base/test/test_launcher.cc:658: if (exit_code != 0) {
On 2013/09/10 17:17:08, Paweł Hajdan Jr. wrote:
> On 2013/09/10 16:17:27, sky wrote:
> > How come you have this inside the lock? Doesn't seem like it needs to be
part
> of
> > the lock.
> 
> This is tricky, and suggestions how to improve the comment above are welcome.
> 
> If this is not under lock, we'd possibly try to kill that process twice, and
> base::KillProcessGroup spews ugly log messages in that case. Using the lock
> avoids that.
> 
> Alternatively I think I could just use raw kill(...) here, but I still prefer
> current solution.

Under what circumstances might you try to kill the same process twice?

[Paweł Hajdan Jr., 2013-09-10 19:56:45]

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

https://codereview.chromium.org/23484026/diff/6001/base/test/test_launcher.cc...
base/test/test_launcher.cc:54: LazyInstance<std::set<ProcessHandle> >
g_live_process_handles;
On 2013/09/10 19:14:46, sky wrote:
> My reasoning is that it's not a global. g_browser_process is a global.

Current version of the code is consistent with usage in base. See git grep
LazyInstance base. g_ is used even though the variable is not exposed via
headers.

https://codereview.chromium.org/23484026/diff/6001/base/test/test_launcher.cc...
base/test/test_launcher.cc:658: if (exit_code != 0) {
On 2013/09/10 19:14:46, sky wrote:
> Under what circumstances might you try to kill the same process twice?

Worker thread executing this code at the same time main thread executes
SignalFDWatcher::OnFileCanReadWithoutBlocking .

This has actually happened during my testing and prompted this change to the
code.

[sky, 2013-09-10 20:02:11]

Ok, LGTM

[Paweł Hajdan Jr., 2013-09-10 23:00:41]

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