This adds some plumbing for propagating the status and error code of a

This adds some plumbing for propagating the status and error code of a
renderer process that went away so that we can tell at the UI level
what happened to the tab: did it crash, or was it killed by the OOM
killer (or some other reason).  This is in preparation for
implementing a new UI for when a process is killed by the OOM on
ChromeOS which handles it differently from a crash.

Most of the changes are modifications of the argument list to include
a status and error code for the exited process, but in addition the
following was done:

- Changed the name of DidProcessCrash to GetTerminationStatus.
- Added some new enum values to TerminationStatus enum (and named it)
  in process_util.h, so it can be used as the status returned by
  WhatHappenedToProcess.
- Improved process_util_unittest to actually test for crashing and
  terminated processes on all platforms.
- Added a new notification for renderers that were killed.
- Added error code information to crash notification.
- Added status and error code information to renderer IPC message for
  RenderViewGone.
- Added a UMA histogram count for number of renderer kills.

BUG=none
TEST=ran new unit test.  Test passes on try servers.

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

[Greg Spencer (Chromium), 2010-09-21 16:54:10]

Adam, if you don't have time to review this right now, let me know and I'll find
someone else.  I picked you to send this to because you appear to have authored
DidProcessCrash originally.

Most of the files have pretty trivial changes (changes to an argument list, for
instance).

-Greg.

[agl, 2010-09-21 17:33:16]

LGTM

The flakyness of the tests needs to be fixed and I think some of the naming
needs to be adjusted also.

http://codereview.chromium.org/3386014/diff/7001/8001
File base/process_util.h (right):

http://codereview.chromium.org/3386014/diff/7001/8001#newcode91
base/process_util.h:91: PROCESS_END_NORMAL_TERMINATION = 0,  // zero exit status
I'm a little afraid of the amount of work that changing these names might be
now, so you can ignore this comment if you like.

Given the name of the enum, I would call these:
TERMINATION_STATUS_NORMAL,
TERMINATION_STATUS_KILLED,
TERMINATION_STATUS_HUNG,
etc

PROCESS_END_ABNORMAL_TERMINATION sounds very generic.
TERMINATION_STATUS_NON_ZERO_EXIT?

(Later: it looks like the PROCESS_END_... values are Windows based. Which means
that, if someone slips up in future they might (silently) get the Windows
constants, not our ones. So I think they should be renamed.)

http://codereview.chromium.org/3386014/diff/7001/8001#newcode96
base/process_util.h:96: PROCESS_END_STILL_RUNNING = 4  // child hasn't exited
yet
BUG: value 4 is duplicated.

http://codereview.chromium.org/3386014/diff/7001/8001#newcode301
base/process_util.h:301: TerminationStatus WhatHappenedToProcess(ProcessHandle
handle, int* exit_code);
s/WhatHappenedToProcess/GetTerminationStatus/?

(Just a suggestion, you can ignore if you like the current name)

http://codereview.chromium.org/3386014/diff/7001/8002
File base/process_util_posix.cc (right):

http://codereview.chromium.org/3386014/diff/7001/8002#newcode188
base/process_util_posix.cc:188: // If we're waiting and we haven't died by now,
force it with a
'we haven't died by now'? You mean if the /child/ hasn't died by now.

http://codereview.chromium.org/3386014/diff/7001/8002#newcode559
base/process_util_posix.cc:559: *exit_code = status;
BUG: |status| is uninitialised at this point. I'd just set it to zero.

http://codereview.chromium.org/3386014/diff/7001/8002#newcode579
base/process_util_posix.cc:579: case SIGKILL:
I'd include SIGINT in this list also.

http://codereview.chromium.org/3386014/diff/7001/8003
File base/process_util_unittest.cc (right):

http://codereview.chromium.org/3386014/diff/7001/8003#newcode50
base/process_util_unittest.cc:50: const char* const kSignalFileSlow =
"SlowChildProcess.die";
const char kName[] for each of these.

(saves a relocation record.)

http://codereview.chromium.org/3386014/diff/7001/8003#newcode79
base/process_util_unittest.cc:79: if (fp) fclose(fp);
style: fclose indented on a new line.

http://codereview.chromium.org/3386014/diff/7001/8003#newcode81
base/process_util_unittest.cc:81: // Do one last sleep to give the process a
chance to terminate.
Flaky warning! Flaky tests are worse than no tests. Remove this and see below...

http://codereview.chromium.org/3386014/diff/7001/8003#newcode122
base/process_util_unittest.cc:122: TEST_F(ProcessUtilTest,
WhatHappenedToProcessExit) {
(note: if you rename WhatHappenedToProcess, remember to rename this test.)

http://codereview.chromium.org/3386014/diff/7001/8003#newcode130
base/process_util_unittest.cc:130: #if defined(OS_WIN)
This pattern appears several times. How about defining kExpectedStillRunningExit
code or something?

http://codereview.chromium.org/3386014/diff/7001/8003#newcode141
base/process_util_unittest.cc:141:
EXPECT_EQ(base::PROCESS_END_NORMAL_TERMINATION,
You should have a time limited, sleeping loop which ignores STILL_RUNNING.

http://codereview.chromium.org/3386014/diff/7001/8003#newcode181
base/process_util_unittest.cc:181: EXPECT_EQ(base::PROCESS_END_PROCESS_CRASHED,
ditto with the looping test

http://codereview.chromium.org/3386014/diff/7001/8003#newcode234
base/process_util_unittest.cc:234:
EXPECT_EQ(base::PROCESS_END_PROCESS_WAS_KILLED,
ditto with the looping test.

http://codereview.chromium.org/3386014/diff/7001/8046
File chrome/browser/zygote_host_linux.cc (right):

http://codereview.chromium.org/3386014/diff/7001/8046#newcode323
chrome/browser/zygote_host_linux.cc:323: 
Set exit_code to zero first to cover the early returns?

[Paweł Hajdan Jr., 2010-09-22 08:54:28]

Drive-by with test comments.

http://codereview.chromium.org/3386014/diff/7001/8003
File base/process_util_unittest.cc (right):

http://codereview.chromium.org/3386014/diff/7001/8003#newcode82
base/process_util_unittest.cc:82: PlatformThread::Sleep(kIntervalMs);
Yeah, can we wait without a polling loop? For example, can we use a pipe to the
child and wait for it to close or something like that?

http://codereview.chromium.org/3386014/diff/7001/8003#newcode141
base/process_util_unittest.cc:141:
EXPECT_EQ(base::PROCESS_END_NORMAL_TERMINATION,
On 2010/09/21 17:33:16, agl wrote:
> You should have a time limited, sleeping loop which ignores STILL_RUNNING.

Sleeping loops are bad. Can we avoid that?

[Greg Spencer (Chromium), 2010-09-22 19:12:13]

http://codereview.chromium.org/3386014/diff/7001/8001
File base/process_util.h (right):

http://codereview.chromium.org/3386014/diff/7001/8001#newcode91
base/process_util.h:91: PROCESS_END_NORMAL_TERMINATION = 0,  // zero exit status
On 2010/09/21 17:33:16, agl wrote:
> I'm a little afraid of the amount of work that changing these names might be
> now, so you can ignore this comment if you like.
> 
> Given the name of the enum, I would call these:
> TERMINATION_STATUS_NORMAL,
> TERMINATION_STATUS_KILLED,
> TERMINATION_STATUS_HUNG,
> etc
> 
> PROCESS_END_ABNORMAL_TERMINATION sounds very generic.
> TERMINATION_STATUS_NON_ZERO_EXIT?
> 
> (Later: it looks like the PROCESS_END_... values are Windows based. Which
means
> that, if someone slips up in future they might (silently) get the Windows
> constants, not our ones. So I think they should be renamed.)

Changing the names isn't any big deal.

Yeah, I don't really like the intertwining of the windows values and the values
of this enum, but it seems that the third party installers use those values to
indicate particular things.  I could separate those values into a different
enum, and use those in the appropriate place in process_util_win.cc.  That even
makes some sense -- basically I'm leaving the old enum around with its previous
use, and adding a new one for return values from WhatHappenedToProcess.

http://codereview.chromium.org/3386014/diff/7001/8001#newcode96
base/process_util.h:96: PROCESS_END_STILL_RUNNING = 4  // child hasn't exited
yet
On 2010/09/21 17:33:16, agl wrote:
> BUG: value 4 is duplicated.

Whoa.  Thanks for catching that...

http://codereview.chromium.org/3386014/diff/7001/8001#newcode301
base/process_util.h:301: TerminationStatus WhatHappenedToProcess(ProcessHandle
handle, int* exit_code);
On 2010/09/21 17:33:16, agl wrote:
> s/WhatHappenedToProcess/GetTerminationStatus/?
> 
> (Just a suggestion, you can ignore if you like the current name)

Hmm.  That's a good suggestion.  Done.

http://codereview.chromium.org/3386014/diff/7001/8002
File base/process_util_posix.cc (right):

http://codereview.chromium.org/3386014/diff/7001/8002#newcode188
base/process_util_posix.cc:188: // If we're waiting and we haven't died by now,
force it with a
On 2010/09/21 17:33:16, agl wrote:
> 'we haven't died by now'? You mean if the /child/ hasn't died by now.

Yes, the child. Fixed.

http://codereview.chromium.org/3386014/diff/7001/8002#newcode559
base/process_util_posix.cc:559: *exit_code = status;
On 2010/09/21 17:33:16, agl wrote:
> BUG: |status| is uninitialised at this point. I'd just set it to zero.

Good point. Fixed.

http://codereview.chromium.org/3386014/diff/7001/8002#newcode579
base/process_util_posix.cc:579: case SIGKILL:
On 2010/09/21 17:33:16, agl wrote:
> I'd include SIGINT in this list also.

Done.

http://codereview.chromium.org/3386014/diff/7001/8003
File base/process_util_unittest.cc (right):

http://codereview.chromium.org/3386014/diff/7001/8003#newcode50
base/process_util_unittest.cc:50: const char* const kSignalFileSlow =
"SlowChildProcess.die";
On 2010/09/21 17:33:16, agl wrote:
> const char kName[] for each of these.
> 
> (saves a relocation record.)

Done.

http://codereview.chromium.org/3386014/diff/7001/8003#newcode79
base/process_util_unittest.cc:79: if (fp) fclose(fp);
On 2010/09/21 17:33:16, agl wrote:
> style: fclose indented on a new line.

Done.

http://codereview.chromium.org/3386014/diff/7001/8003#newcode81
base/process_util_unittest.cc:81: // Do one last sleep to give the process a
chance to terminate.
On 2010/09/21 17:33:16, agl wrote:
> Flaky warning! Flaky tests are worse than no tests. Remove this and see
below...

So... I don't actually know that it even needs the extra sleep, I was just being
paranoid.

But I get your point.  I'll try the looping method.

http://codereview.chromium.org/3386014/diff/7001/8003#newcode82
base/process_util_unittest.cc:82: PlatformThread::Sleep(kIntervalMs);
On 2010/09/22 08:54:29, Paweł Hajdan Jr. wrote:
> Yeah, can we wait without a polling loop? For example, can we use a pipe to
the
> child and wait for it to close or something like that?

Well, the problem is that there is then platform specific code for doing pipes
on Windows vs POSIX, and the file method already implemented here is simple and
straightforward for a test.  I've actually implemented it for POSIX, and the
times are almost identical (both around 160ms per test), so I don't see a
compelling reason to make it more complicated.

Also, I can't eliminate a polling loop for the exit status -- normally I'd wait
for the process to exit with "wait" (or WaitForSingleProcess on Windows), but in
this case, that would slurp up the return code information that I'm trying to
test with GetTerminationStatus.

I've eliminated the wait loop in WaitForChildrenToHandleSignal that waits on the
file to go away, and now I just wait for the process to not be running.

http://codereview.chromium.org/3386014/diff/7001/8003#newcode122
base/process_util_unittest.cc:122: TEST_F(ProcessUtilTest,
WhatHappenedToProcessExit) {
On 2010/09/21 17:33:16, agl wrote:
> (note: if you rename WhatHappenedToProcess, remember to rename this test.)

Done.

http://codereview.chromium.org/3386014/diff/7001/8003#newcode130
base/process_util_unittest.cc:130: #if defined(OS_WIN)
On 2010/09/21 17:33:16, agl wrote:
> This pattern appears several times. How about defining
kExpectedStillRunningExit
> code or something?

Done.

http://codereview.chromium.org/3386014/diff/7001/8003#newcode141
base/process_util_unittest.cc:141:
EXPECT_EQ(base::PROCESS_END_NORMAL_TERMINATION,
On 2010/09/21 17:33:16, agl wrote:
> You should have a time limited, sleeping loop which ignores STILL_RUNNING.

Done.

http://codereview.chromium.org/3386014/diff/7001/8003#newcode181
base/process_util_unittest.cc:181: EXPECT_EQ(base::PROCESS_END_PROCESS_CRASHED,
On 2010/09/21 17:33:16, agl wrote:
> ditto with the looping test

Done.

http://codereview.chromium.org/3386014/diff/7001/8003#newcode234
base/process_util_unittest.cc:234:
EXPECT_EQ(base::PROCESS_END_PROCESS_WAS_KILLED,
On 2010/09/21 17:33:16, agl wrote:
> ditto with the looping test.

Done.

http://codereview.chromium.org/3386014/diff/7001/8046
File chrome/browser/zygote_host_linux.cc (right):

http://codereview.chromium.org/3386014/diff/7001/8046#newcode323
chrome/browser/zygote_host_linux.cc:323: 
On 2010/09/21 17:33:16, agl wrote:
> Set exit_code to zero first to cover the early returns?

Done.

[agl, 2010-09-22 19:19:50]

LGTM

[Paweł Hajdan Jr., 2010-09-23 10:08:30]

NACK. Just to be clear: please do not commit without my explicit "LGTM".

Waiting properly is not making things overcomplicated. You wouldn't use
wait-by-polling in production code. Then how is test code different?

http://codereview.chromium.org/3386014/diff/16001/17003
File base/process_util_unittest.cc (right):

http://codereview.chromium.org/3386014/diff/16001/17003#newcode76
base/process_util_unittest.cc:76: base::TerminationStatus
WaitForChildTermination(const char* filename,
nit: We no longer use |filename|

http://codereview.chromium.org/3386014/diff/16001/17003#newcode84
base/process_util_unittest.cc:84: do {
We should somehow call GetTerminationStatus when we know that it has terminated,
but we can't use wait() on it. Why would the pipe-to-the-child solution not
work? We can wait for the pipe to close.

[agl, 2010-09-23 13:45:00]

On Thu, Sep 23, 2010 at 6:08 AM,  <phajdan.jr@chromium.org> wrote:
> We should somehow call GetTerminationStatus when we know that it has
> terminated, but we can't use wait() on it. Why would the
> pipe-to-the-child solution not work? We can wait for the pipe to close.

There's no functional difference here, unless I've missed something.

The poll loop wastes a system call every 20ms and adds about 20ms to
the duration of the test. The tradeoff is much simpler code.

In terms of flaklyness, there's no difference. If the child doesn't
die then, by waiting on a lifeline pipe, you simply delay until the
test-runner times out the test for lack of output. It's the same with
the poll loop, modulo the duration of the timeout.

So I disagree with your objection here.


AGL

[Paweł Hajdan Jr., 2010-09-23 14:11:38]

After an offline discussion with agl I think the polling loop is fine here, but
we should add a comment saying that we considered using a pipe to the child to
wait for an event, but there were cases in the past that those pipes caused
problems (other libraries closing the fds, child deadlocking). This is a simple
case, so it's not necessarily worth the risk.

[Greg Spencer, 2010-09-23 15:14:04]

I'm sorry,  I'm gonna need an LGTM that's a leetle bit more explicit than
that...  :-)
On Sep 23, 2010 7:11 AM, <phajdan.jr@chromium.org> wrote:
> After an offline discussion with agl I think the polling loop is fine
here,
> but
> we should add a comment saying that we considered using a pipe to the
child
> to
> wait for an event, but there were cases in the past that those pipes
caused
> problems (other libraries closing the fds, child deadlocking). This is a
> simple
> case, so it's not necessarily worth the risk.
>
> http://codereview.chromium.org/3386014/show

[Paweł Hajdan Jr., 2010-09-23 15:22:02]

There are still some other issues from my earlier comments.

On Thu, Sep 23, 2010 at 17:07, Greg Spencer <gspencer@google.com> wrote:

> I'm sorry,  I'm gonna need an LGTM that's a leetle bit more explicit than
> that...  :-)
>  On Sep 23, 2010 7:11 AM, <phajdan.jr@chromium.org> wrote:
> > After an offline discussion with agl I think the polling loop is fine
> here,
> > but
> > we should add a comment saying that we considered using a pipe to the
> child
> > to
> > wait for an event, but there were cases in the past that those pipes
> caused
> > problems (other libraries closing the fds, child deadlocking). This is a
> > simple
> > case, so it's not necessarily worth the risk.
> >
> > http://codereview.chromium.org/3386014/show
>

[Paweł Hajdan Jr., 2010-09-23 15:28:28]

There are still some other issues from my earlier comments.

On Thu, Sep 23, 2010 at 17:07, Greg Spencer <gspencer@google.com> wrote:

> I'm sorry,  I'm gonna need an LGTM that's a leetle bit more explicit than
> that...  :-)
>  On Sep 23, 2010 7:11 AM, <phajdan.jr@chromium.org> wrote:
> > After an offline discussion with agl I think the polling loop is fine
> here,
> > but
> > we should add a comment saying that we considered using a pipe to the
> child
> > to
> > wait for an event, but there were cases in the past that those pipes
> caused
> > problems (other libraries closing the fds, child deadlocking). This is a
> > simple
> > case, so it's not necessarily worth the risk.
> >
> > http://codereview.chromium.org/3386014/show
>

[Greg Spencer (Chromium), 2010-09-23 16:46:24]

OK, I believe I've addressed your remaining comments, Paweł.

[Paweł Hajdan Jr., 2010-09-23 16:52:05]

http://codereview.chromium.org/3386014/diff/25001/26003
File base/process_util_unittest.cc (right):

http://codereview.chromium.org/3386014/diff/25001/26003#newcode61
base/process_util_unittest.cc:61: const int kMaxWaitTimeMs = 5000;
We should use chrome/test/test_timeouts. We can't depend on chrome, so that file
needs to be moved somewhere that base can depend on.

http://codereview.chromium.org/3386014/diff/25001/26003#newcode63
base/process_util_unittest.cc:63: // Sleeps until file filename is created. 
Using a pipe to the child
WaitToDie seems to be an old thing. Let's add the comment to
WaitForChildTermination, and also say that using such loops is discouraged.

http://codereview.chromium.org/3386014/diff/25001/26003#newcode87
base/process_util_unittest.cc:87: const int kIntervalMs = 20;
This should also be used based on test_timeouts file.

[Greg Spencer (Chromium), 2010-09-23 17:24:11]

http://codereview.chromium.org/3386014/diff/25001/26003
File base/process_util_unittest.cc (right):

http://codereview.chromium.org/3386014/diff/25001/26003#newcode61
base/process_util_unittest.cc:61: const int kMaxWaitTimeMs = 5000;
On 2010/09/23 16:52:06, Paweł Hajdan Jr. wrote:
> We should use chrome/test/test_timeouts. We can't depend on chrome, so that
file
> needs to be moved somewhere that base can depend on.

I don't really feel comfortable making that change in this CL -- it's unrelated,
and would impact many other tests in base that already do exactly what I have
here (actually, worse, they do what this test used to do, embed the numbers
directly in the tests).  This CL is big enough as it is.

http://codereview.chromium.org/3386014/diff/25001/26003#newcode63
base/process_util_unittest.cc:63: // Sleeps until file filename is created. 
Using a pipe to the child
On 2010/09/23 16:52:06, Paweł Hajdan Jr. wrote:
> WaitToDie seems to be an old thing. Let's add the comment to
> WaitForChildTermination, and also say that using such loops is discouraged.

Done.

[Paweł Hajdan Jr., 2010-09-23 17:27:36]

On Thu, Sep 23, 2010 at 19:24, <gspencer@chromium.org> wrote:

>
> http://codereview.chromium.org/3386014/diff/25001/26003
> File base/process_util_unittest.cc (right):
>
> http://codereview.chromium.org/3386014/diff/25001/26003#newcode61
> base/process_util_unittest.cc:61: const int kMaxWaitTimeMs = 5000;
> On 2010/09/23 16:52:06, Paweł Hajdan Jr. wrote:
>
>> We should use chrome/test/test_timeouts. We can't depend on chrome, so
>>
> that file
>
>> needs to be moved somewhere that base can depend on.
>>
>
> I don't really feel comfortable making that change in this CL -- it's
> unrelated, and would impact many other tests in base that already do
> exactly what I have here (actually, worse, they do what this test used
> to do, embed the numbers directly in the tests).  This CL is big enough
> as it is.


You can do the move in a separate CL. And hardcoding even more timeouts in
individual tests is a no-go.


>
> http://codereview.chromium.org/3386014/diff/25001/26003#newcode63
> base/process_util_unittest.cc:63: // Sleeps until file filename is
> created.  Using a pipe to the child
> On 2010/09/23 16:52:06, Paweł Hajdan Jr. wrote:
>
>> WaitToDie seems to be an old thing. Let's add the comment to
>> WaitForChildTermination, and also say that using such loops is
>>
> discouraged.
>
> Done.
>
>
> http://codereview.chromium.org/3386014/show
>

[Paweł Hajdan Jr., 2010-09-23 17:28:07]

Actually, I can do the move if you'd like me to do. Can do it tomorrow CEST.

On Thu, Sep 23, 2010 at 19:26, Paweł Hajdan, Jr. <phajdan.jr@chromium.org>wrote:

> On Thu, Sep 23, 2010 at 19:24, <gspencer@chromium.org> wrote:
>
>>
>> http://codereview.chromium.org/3386014/diff/25001/26003
>> File base/process_util_unittest.cc (right):
>>
>> http://codereview.chromium.org/3386014/diff/25001/26003#newcode61
>> base/process_util_unittest.cc:61: const int kMaxWaitTimeMs = 5000;
>> On 2010/09/23 16:52:06, Paweł Hajdan Jr. wrote:
>>
>>> We should use chrome/test/test_timeouts. We can't depend on chrome, so
>>>
>> that file
>>
>>> needs to be moved somewhere that base can depend on.
>>>
>>
>> I don't really feel comfortable making that change in this CL -- it's
>> unrelated, and would impact many other tests in base that already do
>> exactly what I have here (actually, worse, they do what this test used
>> to do, embed the numbers directly in the tests).  This CL is big enough
>> as it is.
>
>
> You can do the move in a separate CL. And hardcoding even more timeouts in
> individual tests is a no-go.
>
>
>>
>> http://codereview.chromium.org/3386014/diff/25001/26003#newcode63
>> base/process_util_unittest.cc:63: // Sleeps until file filename is
>> created.  Using a pipe to the child
>> On 2010/09/23 16:52:06, Paweł Hajdan Jr. wrote:
>>
>>> WaitToDie seems to be an old thing. Let's add the comment to
>>> WaitForChildTermination, and also say that using such loops is
>>>
>> discouraged.
>>
>> Done.
>>
>>
>> http://codereview.chromium.org/3386014/show
>>
>
>

[Greg Spencer, 2010-09-23 17:51:48]

That would be great.  I have one more issue that's cropped up on Mac
to solve first, however.

-Greg.

On Thu, Sep 23, 2010 at 10:27 AM, Paweł Hajdan, Jr.
<phajdan.jr@chromium.org> wrote:
> Actually, I can do the move if you'd like me to do. Can do it tomorrow CEST.
>
> On Thu, Sep 23, 2010 at 19:26, Paweł Hajdan, Jr. <phajdan.jr@chromium.org>
> wrote:
>>
>> On Thu, Sep 23, 2010 at 19:24, <gspencer@chromium.org> wrote:
>>>
>>> http://codereview.chromium.org/3386014/diff/25001/26003
>>> File base/process_util_unittest.cc (right):
>>>
>>> http://codereview.chromium.org/3386014/diff/25001/26003#newcode61
>>> base/process_util_unittest.cc:61: const int kMaxWaitTimeMs = 5000;
>>> On 2010/09/23 16:52:06, Paweł Hajdan Jr. wrote:
>>>>
>>>> We should use chrome/test/test_timeouts. We can't depend on chrome, so
>>>
>>> that file
>>>>
>>>> needs to be moved somewhere that base can depend on.
>>>
>>> I don't really feel comfortable making that change in this CL -- it's
>>> unrelated, and would impact many other tests in base that already do
>>> exactly what I have here (actually, worse, they do what this test used
>>> to do, embed the numbers directly in the tests).  This CL is big enough
>>> as it is.
>>
>> You can do the move in a separate CL. And hardcoding even more timeouts in
>> individual tests is a no-go.
>>
>>>
>>> http://codereview.chromium.org/3386014/diff/25001/26003#newcode63
>>> base/process_util_unittest.cc:63: // Sleeps until file filename is
>>> created.  Using a pipe to the child
>>> On 2010/09/23 16:52:06, Paweł Hajdan Jr. wrote:
>>>>
>>>> WaitToDie seems to be an old thing. Let's add the comment to
>>>> WaitForChildTermination, and also say that using such loops is
>>>
>>> discouraged.
>>>
>>> Done.
>>>
>>> http://codereview.chromium.org/3386014/show
>>
>
>

[Greg Spencer (Chromium), 2010-09-24 00:39:39]

OK, I've fixed my Mac issue, and I'm planning to check this in on Monday (PST)
if there are no new objections.
(I'm out tomorrow on vacation).

-Greg.


On 2010/09/23 17:51:48, Greg Spencer wrote:
> That would be great.  I have one more issue that's cropped up on Mac
> to solve first, however.
> 
> -Greg.
> 
> On Thu, Sep 23, 2010 at 10:27 AM, Paweł Hajdan, Jr.
> <mailto:phajdan.jr@chromium.org> wrote:
> > Actually, I can do the move if you'd like me to do. Can do it tomorrow CEST.