POSIX: don't spawn zombies.

POSIX: don't spawn zombies.

[Avi (use Gerrit), 2009-04-24 20:54:04]

On 2009/04/24 20:42:49, agl wrote:
> 

drive by: re chrome/common/process_watcher.cc, you're adding an empty file?

[Evan Martin, 2009-04-24 21:19:52]

LGTM, minor comments follow

http://codereview.chromium.org/93147/diff/1/3
File base/process_util_posix.cc (right):

http://codereview.chromium.org/93147/diff/1/3#newcode147
Line 147: NOTREACHED();
Typically we try to recover in release builds here as well.  Not sure what the
proper recovery for this case is, though.  Seems unlikely that falling through
to the remainder of the function is.

http://codereview.chromium.org/93147/diff/1/5
File chrome/browser/renderer_host/browser_render_process_host.cc (right):

http://codereview.chromium.org/93147/diff/1/5#newcode674
Line 674: // DidProcessCrash didn't crash, we'll need to reap the child in
"DidProcessCrash didn't crash" doesn't quite make sense.  I think you mean
"DidProcessCrash returned false"?

http://codereview.chromium.org/93147/diff/1/8
File chrome/common/process_watcher_posix.cc (right):

http://codereview.chromium.org/93147/diff/1/8#newcode16
Line 16: static bool IsChildDead(pid_t child) {
namespace {

} // anon namespace

in preference to "static"

http://codereview.chromium.org/93147/diff/1/8#newcode41
Line 41: void WaitForChildToDie() {
Could you add comments in here as to why it tries three times?

Would it make more sense to just put in a for loop with 0.5s waits that tries
four times?

[agl, 2009-04-24 21:28:40]

http://codereview.chromium.org/93147/diff/1/3
File base/process_util_posix.cc (right):

http://codereview.chromium.org/93147/diff/1/3#newcode147
Line 147: NOTREACHED();
On 2009/04/24 21:19:53, Evan Martin wrote:
> Typically we try to recover in release builds here as well.  Not sure what the
> proper recovery for this case is, though.  Seems unlikely that falling through
> to the remainder of the function is.

Ok. Downgraded to a LOG(ERROR).

http://codereview.chromium.org/93147/diff/1/5
File chrome/browser/renderer_host/browser_render_process_host.cc (right):

http://codereview.chromium.org/93147/diff/1/5#newcode674
Line 674: // DidProcessCrash didn't crash, we'll need to reap the child in
On 2009/04/24 21:19:53, Evan Martin wrote:
> "DidProcessCrash didn't crash" doesn't quite make sense.  I think you mean
> "DidProcessCrash returned false"?

Done.

http://codereview.chromium.org/93147/diff/1/8
File chrome/common/process_watcher_posix.cc (right):

http://codereview.chromium.org/93147/diff/1/8#newcode16
Line 16: static bool IsChildDead(pid_t child) {
On 2009/04/24 21:19:53, Evan Martin wrote:
> namespace {
> 
> } // anon namespace
> 
> in preference to "static"

Nope. From the style guide: "use an unnamed namespace or static linkage to limit
its scope". Anonymous namespaces are just hiding the fact that it's a static.

http://codereview.chromium.org/93147/diff/1/8#newcode41
Line 41: void WaitForChildToDie() {
On 2009/04/24 21:19:53, Evan Martin wrote:
> Would it make more sense to just put in a for loop with 0.5s waits that tries
> four times?

Hmm, probably yea. Done.

[agl, 2009-04-27 23:10:22]

rerev request

[Evan Martin, 2009-04-27 23:34:37]

ok

http://codereview.chromium.org/93147/diff/2001/2003
File base/process_util.h (right):

http://codereview.chromium.org/93147/diff/2001/2003#newcode180
Line 180: // a different manner on POSIX.
s/on POSIX//  (since it's already at the beginning of the sentence)

http://codereview.chromium.org/93147/diff/2001/2003#newcode181
Line 181: bool DidProcessCrash(bool* child_exited, ProcessHandle handle);
I repeat the earlier comment that this would look less strange if it took the
thing it was acting upon first, and the output param second.

http://codereview.chromium.org/93147/diff/2001/2006
File chrome/browser/browser_main.cc (right):

http://codereview.chromium.org/93147/diff/2001/2006#newcode185
Line 185: void SIGCHLDHandler(int signal) {
Is this any different than SIG_IGN?

Maybe put a comment in the body of this to the effect of "we ignore SIGCHLD and
[mumble] elsewhere".

http://codereview.chromium.org/93147/diff/2001/2006#newcode212
Line 212: memset(&action, 0, sizeof(action));
struct sigaction action = {0};  ?

http://codereview.chromium.org/93147/diff/2001/2007
File chrome/browser/renderer_host/browser_render_process_host.cc (right):

http://codereview.chromium.org/93147/diff/2001/2007#newcode678
Line 678: // If the process crashed, then the kernel closed the socket for it
and so
Maybe prefix this comment with "POSIX note:"

http://codereview.chromium.org/93147/diff/2001/2011
File chrome/common/process_watcher_posix.cc (right):

http://codereview.chromium.org/93147/diff/2001/2011#newcode46
Line 46: for (unsigned i = 0; i < 4; ++i) {
unsigned -> int *shrug*

[agl, 2009-04-28 00:44:14]

http://codereview.chromium.org/93147/diff/2001/2003
File base/process_util.h (right):

http://codereview.chromium.org/93147/diff/2001/2003#newcode180
Line 180: // a different manner on POSIX.
On 2009/04/27 23:34:37, Evan Martin wrote:
> s/on POSIX//  (since it's already at the beginning of the sentence)

Done.

http://codereview.chromium.org/93147/diff/2001/2006
File chrome/browser/browser_main.cc (right):

http://codereview.chromium.org/93147/diff/2001/2006#newcode185
Line 185: void SIGCHLDHandler(int signal) {
On 2009/04/27 23:34:37, Evan Martin wrote:
> Is this any different than SIG_IGN?
> 
> Maybe put a comment in the body of this to the effect of "we ignore SIGCHLD
and
> [mumble] elsewhere".

Done.

http://codereview.chromium.org/93147/diff/2001/2006#newcode212
Line 212: memset(&action, 0, sizeof(action));
On 2009/04/27 23:34:37, Evan Martin wrote:
> struct sigaction action = {0};  ?

Done.

http://codereview.chromium.org/93147/diff/2001/2007
File chrome/browser/renderer_host/browser_render_process_host.cc (right):

http://codereview.chromium.org/93147/diff/2001/2007#newcode678
Line 678: // If the process crashed, then the kernel closed the socket for it
and so
On 2009/04/27 23:34:37, Evan Martin wrote:
> Maybe prefix this comment with "POSIX note:"

Done.