Mac: ensure child processes are properly reaped -- eliminate Helper zombies.

Mac: ensure child processes are properly reaped -- eliminate Helper zombies.

BUG=32001
TEST=Surf a lot. Then close all Chrome/Chromium windows. Make sure there aren't any zombie Chrome/Chromium Helper processes (run "ps -Al" and look for things with status containing 'Z').

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

[viettrungluu, 2010-01-13 00:53:01]

Probably I should still add a test, but the change to the reaper itself should
be okay.

[Mark Mentovai, 2010-01-13 01:04:48]

I'm not so sure about this.  It might be right (and the code itself LGTM), but
I'm not convinced yet.

How do we get here normally?

Who is supposed to waitpid for this kid?  If it's this function, why is it doing
this kqueue stuff instead of waitpid?  Is this function called with pids other
than children of the running process?  Is it called with pids that something
elsewhere in the program will waitpid for sometimes?

[Mark Mentovai, 2010-01-13 01:15:52]

Jeremy, why did we move to kqueues in r34547, bug 12731,
http://codereview.chromium.org/496007?

Did it actually solve any problems?  I wasn't aware of that change
being made (and probably would have questioned it at the time if I did
see it).  This explains why the code we're using here looks different
from how I remember it.

If the kqueue model doesn't actually solve any problems, I think we
should go back to using waitpid like Linux uses, at least for the time
being.

Mark

[viettrungluu, 2010-01-13 01:31:02]

On 2010/01/13 01:04:48, Mark Mentovai wrote:
> I'm not so sure about this.  It might be right (and the code itself LGTM), but
> I'm not convinced yet.
> 
> How do we get here normally?
> 
> Who is supposed to waitpid for this kid?  If it's this function, why is it
doing
> this kqueue stuff instead of waitpid?  Is this function called with pids other
> than children of the running process?  Is it called with pids that something
> elsewhere in the program will waitpid for sometimes?

Those are fine questions, and I'm looking into it. FWIW, the (just-)added unit
test demonstrates that the old code failed if the child ends before
EnsureProcessTerminated() gets to it.

[viettrungluu, 2010-01-13 01:44:34]

A zombie is produced for me immediately upon visiting http://www.nytimes.com/.
Here's a stack, in case it's more meaningful to you than it is to me:

0   Chromium Framework                  0x00796cf8 _ZN10StackTraceC1Ev + 32
1   Chromium Framework                  0x001abcf5
_ZN20ChildProcessLauncher7Context9TerminateEv + 119
2   Chromium Framework                  0x001abeb3
_ZN20ChildProcessLauncher7ContextD1Ev + 17
3   Chromium Framework                  0x001abedd
_ZN4base20RefCountedThreadSafeIN20ChildProcessLauncher7ContextENS_33DefaultRefCountedThreadSafeTraitsIS2_EEE14DeleteInternalEPS2_
+ 29
4   Chromium Framework                  0x001abefb
_ZN4base33DefaultRefCountedThreadSafeTraitsIN20ChildProcessLauncher7ContextEE8DestructEPS2_
+ 17
5   Chromium Framework                  0x001abf1e
_ZN4base20RefCountedThreadSafeIN20ChildProcessLauncher7ContextENS_33DefaultRefCountedThreadSafeTraitsIS2_EEE7ReleaseEv
+ 32
6   Chromium Framework                  0x001abf3c
_ZN13scoped_refptrIN20ChildProcessLauncher7ContextEED1Ev + 28
7   Chromium Framework                  0x001ab36c _ZN20ChildProcessLauncherD1Ev
+ 36
8   Chromium Framework                  0x00038853
_ZN10scoped_ptrI20ChildProcessLauncherED1Ev + 31
9   Chromium Framework                  0x004d2c84
_ZN24BrowserRenderProcessHostD0Ev + 448
10  Chromium Framework                  0x004e5352
_ZN10DeleteTaskI17RenderProcessHostE3RunEv + 40
11  Chromium Framework                  0x007b0c9a
_ZN11MessageLoop7RunTaskEP4Task + 220
12  Chromium Framework                  0x007b1379
_ZN11MessageLoop21DeferOrRunPendingTaskERKNS_11PendingTaskE + 53
13  Chromium Framework                  0x007b1609 _ZN11MessageLoop6DoWorkEv +
253
14  Chromium Framework                  0x0077be4c
_ZN4base24MessagePumpCFRunLoopBase7RunWorkEv + 74
15  Chromium Framework                  0x0077be91
_ZN4base24MessagePumpCFRunLoopBase13RunWorkSourceEPv + 23
[...]

(The actual call to EnsureProcessTerminated() is done on another thread, but the
stack from that is definitely not interesting. It's
ChildProcessLauncher::Context::Terminate() which dispatches
EnsureProcessTerminated().)

[jeremy, 2010-01-13 08:22:48]

LGTM - Thanks for the fix!

mark: If you look in the original code review for the kqueue transition you'll
see a discussion with Avi about why I made this change.  tl;dr: the use case for
the Linux code is different than on the Mac and sharing the code here doesn't
necessarily make sense, IMHO.

The kqueue code has been live on trunk for a while and apart from this issue,
seems to work well.  If you feel strongly you're welcome to roll it out but
please read through the code review comments first.

[jeremy, 2010-01-13 08:25:23]

EnsureProcessTerminated() is called on it's own thread using a task, that's the
stack you're seeing here.

[Mark Mentovai, 2010-01-13 15:06:34]

LGTM as a fix now.  I'll read it more carefully later to review all of
the new code in here.