Make ProcessWatcher use kqueues on Mac.

Make ProcessWatcher use kqueues on Mac.

* Port ProcessWatcher::EnsureProcessTerminated() to kqueue() APIs on OS X.
* Make ProcessWatcher::EnsureProcessGetsReaped() Linux-only, since it's only used there.
* Add a unit test.

BUG=12731
TEST=Open Chrome/Mac, open and close a few tabs.  Processes shouldn't stay around.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=34547

[Avi (use Gerrit), 2009-12-13 17:11:34]

I'm not sure I understand this code change. The bug says we should use kqueues
to reap processes so that we don't have to spawn a thread. But we spawn no fewer
threads with this code change; the only difference is that rather than waiting
in a sleep loop (in the new thread) we wait in a kqueue call (in the new
thread).

http://codereview.chromium.org/496007/diff/2002/2003
File base/file_util.h (right):

http://codereview.chromium.org/496007/diff/2002/2003#newcode385
base/file_util.h:385: typedef scoped_ptr_malloc<int, ScopedFDClose> ScopedFD;
Not too fond of the "scoped_ptr_malloc" class name; it needs something
different, more generic. Doesn't have to be this CL.

http://codereview.chromium.org/496007/diff/2002/2004
File chrome/chrome.gyp (right):

http://codereview.chromium.org/496007/diff/2002/2004#newcode563
chrome/chrome.gyp:563: 'common/process_watcher_mac.cc',
Don't we have _posix and _mac files where _posix is only compiled on linux?
Usually, _linux is linux only.

http://codereview.chromium.org/496007/diff/2002/2007
File chrome/common/process_watcher_linux.cc (right):

http://codereview.chromium.org/496007/diff/2002/2007#newcode50
chrome/common/process_watcher_linux.cc:50: << " to terminate, we got the
following result: " << r;
indent

http://codereview.chromium.org/496007/diff/2002/2008
File chrome/common/process_watcher_mac.cc (right):

http://codereview.chromium.org/496007/diff/2002/2008#newcode25
chrome/common/process_watcher_mac.cc:25: NOTREACHED();
Why not log here like we do below in BlockingReap?

http://codereview.chromium.org/496007/diff/2002/2008#newcode44
chrome/common/process_watcher_mac.cc:44: // Converts a TimeDelta into a struct
timespec.
Can we put this into base/time? Feels out of place here.

[jeremy, 2009-12-14 15:12:51]

You're right, no need for a thread since
ChildProcessLauncher::Context::TerminateInternal already creates one.

http://codereview.chromium.org/496007/diff/2002/2003
File base/file_util.h (right):

http://codereview.chromium.org/496007/diff/2002/2003#newcode385
base/file_util.h:385: typedef scoped_ptr_malloc<int, ScopedFDClose> ScopedFD;
I tried to change this once already without luck :|

http://codereview.chromium.org/496007/diff/2002/2004
File chrome/chrome.gyp (right):

http://codereview.chromium.org/496007/diff/2002/2004#newcode563
chrome/chrome.gyp:563: 'common/process_watcher_mac.cc',
On 2009/12/13 17:11:34, Avi wrote:
> Don't we have _posix and _mac files where _posix is only compiled on linux?
> Usually, _linux is linux only.

Done.

http://codereview.chromium.org/496007/diff/2002/2007
File chrome/common/process_watcher_linux.cc (right):

http://codereview.chromium.org/496007/diff/2002/2007#newcode50
chrome/common/process_watcher_linux.cc:50: << " to terminate, we got the
following result: " << r;
On 2009/12/13 17:11:34, Avi wrote:
> indent

Done.

http://codereview.chromium.org/496007/diff/2002/2008
File chrome/common/process_watcher_mac.cc (right):

http://codereview.chromium.org/496007/diff/2002/2008#newcode25
chrome/common/process_watcher_mac.cc:25: NOTREACHED();
On 2009/12/13 17:11:34, Avi wrote:
> Why not log here like we do below in BlockingReap?

Done.

http://codereview.chromium.org/496007/diff/2002/2008#newcode44
chrome/common/process_watcher_mac.cc:44: // Converts a TimeDelta into a struct
timespec.
On 2009/12/13 17:11:34, Avi wrote:
> Can we put this into base/time? Feels out of place here.

Done.

[Avi (use Gerrit), 2009-12-14 15:20:07]

I'm afraid I still don't understand this change.

I thought the whole point was to make sure we weren't blocking, and wanted to
use a kqueue rather than spawning a thread. Why is it advantageous to use a
kqueue as you do here with no thread if it means we might end up blocking?

[jeremy, 2009-12-14 17:03:09]

The main issue here is cleanup.  I wanted to take a look at moving some of
the other calls in process_utils to use kqueues and thought this was a good
proof of concept.

While the same code is used for Linux & OS X, the use cases for the Linux
code are different.

This provides a clean separation and the following cleanup on the Mac side:
1. One less thread.
2. Process is reaped immediately, without an extra potential 500ms delay.

Also I can actually remove the first call to IsChildDead() since the kqueue
will return an error if the process no longer exists.

Best regards,
Jeremy

On Mon, Dec 14, 2009 at 5:20 PM, <avi@chromium.org> wrote:

> I'm afraid I still don't understand this change.
>
> I thought the whole point was to make sure we weren't blocking, and wanted
> to
> use a kqueue rather than spawning a thread. Why is it advantageous to use a
> kqueue as you do here with no thread if it means we might end up blocking?
>
>
> http://codereview.chromium.org/496007
>

[Avi (use Gerrit), 2009-12-14 17:36:37]

What of the potential 2s delay? What thread is this called from, and is such a
delay acceptable?

[jeremy, 2009-12-14 18:12:51]

Both before and after this CL the blocked thread is one that's not doing any
other work.

The Linux version of process_watcher uses the same code for
EnsureProcessGetsReaped()
[which apparently needs to be non-blocking which is why we use they need an
extra thread] and EnsureProcessTerminated().

On Mon, Dec 14, 2009 at 7:36 PM, <avi@chromium.org> wrote:

> What of the potential 2s delay? What thread is this called from, and is
> such a
> delay acceptable?
>
>
> http://codereview.chromium.org/496007
>

[Avi (use Gerrit), 2009-12-14 18:31:31]

LG