This adds some plumbing for propagating the reason for a renderer's death

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 TerminationStatus enum in process_util.h, so it can be used as the status returned by GetTerminationStatus.
- 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=http://crosbug.com/8505
TEST=ran new unit test. Test passes on try servers.

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

[Greg Spencer (Chromium), 2010-11-29 20:13:30]

Hi Adam,

In light of your recent change:
http://codereview.chromium.org/5377001/

Can you tell me if I should be doing something different to capture information
about dead child processes here?

BTW, this code isn't in the tree yet (even though you reviewed it before)
because the test is flaky on Macs, due to how long it can take to start
ReportCrash on the mac.  I'm discussing with mmentovai about how to do things
differently for the Mac, but maybe it's more a systemic problem of waitpid than
anything else.

[Mark Mentovai, 2010-11-29 21:19:30]

The context here is that Greg’s test highlights a fundamental limitation of
waiting for children with a timeout. His test is flaky on the Mac because the
details of crashing on the Mac mean that signal delivery is sometimes (often?)
delayed. I pointed out that while the details make this more likely to happen on
the Mac, you can’t generally know with any amount of assurance that a process
will crash (and be reported to its parent by waitpid) in any specific amount of
time.

This is sometimes slow (and flaky) on the Mac because ReportCrash needs to chew
on the process (and its symbols) for a while and generate a report before the
process actually dies in the process table sense and its parent can reap it.
While ReportCrash is an Apple special, it’s certainly possible for similar
constraints to introduce flake-inducing delays on other systems. There may be
delays writing a core file to a slow disk, or a system may be so heavily loaded
that the child hasn’t even reached main() by the time the parent expects it to
have crashed.

The Mac case illustrates the problem with choosing a good timeout. 10 seconds is
too long to wait for a child to exit in the normal case, but it’s not
necessarily long enough for ReportCrash to do its thing, especially on anemic
systems (like our Minis) with a huge process image (like our unit tests) and a
cold-ish buffer cache.

The one suggestion I had for Greg that didn’t involve rewriting a third of the
world (which would have meant testing non-production codepaths) was to get the
crash process into as tiny an executable as possible, essentially “int main() {
crash; }”, and to arrange for it to always be built without any debugging
symbols. This keeps the image size small (for core dumps and for ReportCrash),
ensures near-minimum time from exec to crash, and minimizes the amount of
unwinding and page faulting on the DWARF data that ReportCrash would need to
waste time on.

[agl, 2010-11-29 22:29:04]

On Mon, Nov 29, 2010 at 3:13 PM,  <gspencer@chromium.org> wrote:
> Hi Adam,

I think I've reviewed this code about twice before. It's too large to
constantly be re-reviewing I'm afraid.


AGL

[Greg Spencer, 2010-11-29 22:44:07]

On Mon, Nov 29, 2010 at 2:28 PM, Adam Langley <agl@chromium.org> wrote:

> On Mon, Nov 29, 2010 at 3:13 PM,  <gspencer@chromium.org> wrote:
> > Hi Adam,
>
> I think I've reviewed this code about twice before. It's too large to
> constantly be re-reviewing I'm afraid.
>

Sorry it keeps coming back, but I'm not asking you to re-review the whole
thing -- nothing has changed except for the mac portions of the process util
test.  Unfortunately, I don't want to check in any of it until it all works.

And I'm fine with spinning up someone new on the change if you don't have
time for that, but I'd still like to know what impact your recent change has
(and I noticed you backed it out, but I'm assuming it'll go in sometime).

I also figured that if someone else approved it, then you might be annoyed
if I checked it in while you're still working on your change, because they
definitely collide.

-Greg.

[agl, 2010-11-29 22:53:20]

On Mon, Nov 29, 2010 at 5:43 PM, Greg Spencer <gspencer@google.com> wrote:
> And I'm fine with spinning up someone new on the change if you don't have
> time for that, but I'd still like to know what impact your recent change has
> (and I noticed you backed it out, but I'm assuming it'll go in sometime).

Yea, I had a true/false around the wrong way. I was thinking of
landing it again tomorrow morning.

I don't think that they really collide all that much. I'm happy to
wait if you want to land first.


AGL

[Paweł Hajdan Jr., 2010-11-30 09:39:38]

Drive-by with some test comments. Please do not commit without my explicit
"LGTM".

http://codereview.chromium.org/5172009/diff/1/base/process_util_unittest.cc
File base/process_util_unittest.cc (right):

http://codereview.chromium.org/5172009/diff/1/base/process_util_unittest.cc#n...
base/process_util_unittest.cc:62: const int kMaxWaitTimeMs = 5000;
Please use base/test/test_timeouts.

http://codereview.chromium.org/5172009/diff/1/base/process_util_unittest.cc#n...
base/process_util_unittest.cc:150: WaitForChildTermination(handle, &exit_code);
Why not WaitForSingleProcess?

http://codereview.chromium.org/5172009/diff/1/base/process_util_unittest.cc#n...
base/process_util_unittest.cc:194: WaitForChildTermination(handle, &exit_code);
Why not WaitForSingleProcess?

http://codereview.chromium.org/5172009/diff/1/base/process_util_unittest.cc#n...
base/process_util_unittest.cc:245: WaitForChildTermination(handle, &exit_code);
Why not WaitForSingleProcess?

[Greg Spencer (Chromium), 2010-11-30 19:42:29]

http://codereview.chromium.org/5172009/diff/1/base/process_util_unittest.cc
File base/process_util_unittest.cc (right):

http://codereview.chromium.org/5172009/diff/1/base/process_util_unittest.cc#n...
base/process_util_unittest.cc:62: const int kMaxWaitTimeMs = 5000;
On 2010/11/30 09:39:38, Paweł Hajdan Jr. wrote:
> Please use base/test/test_timeouts.

Done.

http://codereview.chromium.org/5172009/diff/1/base/process_util_unittest.cc#n...
base/process_util_unittest.cc:150: WaitForChildTermination(handle, &exit_code);
On 2010/11/30 09:39:38, Paweł Hajdan Jr. wrote:
> Why not WaitForSingleProcess?

Because on Linux, WaitForSingleProcess uses waitpid, and that is exactly what
we're testing here (GetTerminationStatus also uses waitpid), so it would
interfere with the test.

http://codereview.chromium.org/5172009/diff/1/base/process_util_unittest.cc#n...
base/process_util_unittest.cc:194: WaitForChildTermination(handle, &exit_code);
On 2010/11/30 09:39:38, Paweł Hajdan Jr. wrote:
> Why not WaitForSingleProcess?

See above.

http://codereview.chromium.org/5172009/diff/1/base/process_util_unittest.cc#n...
base/process_util_unittest.cc:245: WaitForChildTermination(handle, &exit_code);
On 2010/11/30 09:39:38, Paweł Hajdan Jr. wrote:
> Why not WaitForSingleProcess?

See above.

[Paweł Hajdan Jr., 2010-12-01 08:40:45]

http://codereview.chromium.org/5172009/diff/1/base/process_util_unittest.cc
File base/process_util_unittest.cc (right):

http://codereview.chromium.org/5172009/diff/1/base/process_util_unittest.cc#n...
base/process_util_unittest.cc:150: WaitForChildTermination(handle, &exit_code);
On 2010/11/30 19:42:29, Greg Spencer (Chromium) wrote:
> On 2010/11/30 09:39:38, Paweł Hajdan Jr. wrote:
> > Why not WaitForSingleProcess?
> 
> Because on Linux, WaitForSingleProcess uses waitpid, and that is exactly what
> we're testing here (GetTerminationStatus also uses waitpid), so it would
> interfere with the test.

I suspected something like that. However, the previous code _was_ using
WaitForSingleProcess, and I don't see any waitpid calls after
WaitForChildTermination. That suggests that WaitForChildTermination is a part of
the test.

Which is weird, because it's an extremely bad way to wait for child termination.

It looks like a regression in test to me, i.e. the test was good
(WaitForSingleProcess), and now it's less reliable.

If you really need it, could you instead of changing existing tests, copy them
and use WaitForChildTermination in the copy?

[Greg Spencer (Chromium), 2010-12-02 01:32:12]

On 2010/12/01 08:40:45, Paweł Hajdan Jr. wrote:
> I suspected something like that. However, the previous code _was_ using
> WaitForSingleProcess, and I don't see any waitpid calls after
> WaitForChildTermination. That suggests that WaitForChildTermination is a part
of
> the test.

The previous code was using it because it was able to: it was never actually
testing to see if the process crashed (notice that it was always testing
EXPECT_FALSE for that), so it didn't matter that WaitForSingleProcess was eating
it's information.

> Which is weird, because it's an extremely bad way to wait for child
termination.

Why is it so bad?  It's using waitpid (through GetTerminationStatus, which is
what we're testing here) to reap the child process, and when it does, it
returns.  It does it without blocking so that the test won't ever hang, and we
can use a timeout.

> It looks like a regression in test to me, i.e. the test was good
> (WaitForSingleProcess), and now it's less reliable.

How exactly is it less reliable?  They both used waitpid to find when a process
exits.

> If you really need it, could you instead of changing existing tests, copy them
> and use WaitForChildTermination in the copy?

Well, the existing test was pretty worthless, so I removed it and replaced it
with ones that actually test for crashes and kills.  Copying the test wouldn't
help because DidProcessCrash doesn't exist anymore.

[Greg Spencer (Chromium), 2010-12-03 01:00:07]

http://codereview.chromium.org/5172009/diff/27001/chrome/browser/metrics/metr...
File chrome/browser/metrics/metrics_service.cc (right):

http://codereview.chromium.org/5172009/diff/27001/chrome/browser/metrics/metr...
chrome/browser/metrics/metrics_service.cc:615:
base::TERMINATION_STATUS_ABNORMAL_TERMINATION) {
I'm wondering if I should be adding logging here for renderer/extension "kills".
 We didn't report those previously because we didn't trap them.

http://codereview.chromium.org/5172009/diff/27001/chrome/browser/tab_contents...
File chrome/browser/tab_contents/tab_contents.h (right):

http://codereview.chromium.org/5172009/diff/27001/chrome/browser/tab_contents...
chrome/browser/tab_contents/tab_contents.h:314: crashed_status_ ==
base::TERMINATION_STATUS_PROCESS_WAS_KILLED);
Note: We didn't previously consider a renderer "crashed" if it was killed.

[brettw, 2010-12-03 01:01:54]

I realize moving the enum is a bit of extra work, but I did some poking around
and it looks like very few extra files will need touching. Probably the unit
test will be the only nontrivial thing.

http://codereview.chromium.org/5172009/diff/31001/base/process_util.h
File base/process_util.h (right):

http://codereview.chromium.org/5172009/diff/31001/base/process_util.h#newcode125
base/process_util.h:125: enum ExitCode {
I'm kind of sad to see this enumeration here (I realize you didn't add it). This
kind of thing doesn't belong in base isn't the right place to force meanings of
exit codes.

I did some poking and there are no dependencies in base outside of
base::DidProcessCrash, and this function is not used outside of Chrome and then
always in the context of ChildProcessLauncher. Can we just move DidProcessCrash
to a static function in ChildProcessLauncher and then just delete this enum
(using the big ResultCodes enum instead).

http://codereview.chromium.org/5172009/diff/31001/chrome/browser/browser_chil...
File chrome/browser/browser_child_process_host.cc (right):

http://codereview.chromium.org/5172009/diff/31001/chrome/browser/browser_chil...
chrome/browser/browser_child_process_host.cc:151: switch (status) {
Random suggestion: I would probably write
  switch (GetChildTerminationStatus(&exit_code)) {
to save a line.

http://codereview.chromium.org/5172009/diff/31001/chrome/browser/child_proces...
File chrome/browser/child_process_launcher.h (right):

http://codereview.chromium.org/5172009/diff/31001/chrome/browser/child_proces...
chrome/browser/child_process_launcher.h:57: base::TerminationStatus
GetChildTerminationStatus(int* exit_code);
If we move the base function to a static function in here, this non-static
version should probably be named GetTerminationStatus. However, if you find we
don't even need the static version, it would be nice to only have one (I'm not
sure, I didn't check if there are external callers that don't use
ChildProcessLauncher).

[Paweł Hajdan Jr., 2010-12-03 18:07:23]

On Thu, Dec 2, 2010 at 02:32, <gspencer@chromium.org> wrote:

> On 2010/12/01 08:40:45, Paweł Hajdan Jr. wrote:
>
>> I suspected something like that. However, the previous code _was_ using
>> WaitForSingleProcess, and I don't see any waitpid calls after
>> WaitForChildTermination. That suggests that WaitForChildTermination is a
>> part
>>
> of
>
>> the test.
>>
>
> The previous code was using it because it was able to: it was never
> actually
> testing to see if the process crashed (notice that it was always testing
> EXPECT_FALSE for that), so it didn't matter that WaitForSingleProcess was
> eating
> it's information.


It seems at least some of the new tests should also be able to use
WaitForSingleProcess, for example GetTerminationStatusExit.

Furthermore, maybe that means we should create a variant or even replacement
of WaitForSingleProcess that would return termination status. That seems to
solve the loop-n-probe problem.

 Which is weird, because it's an extremely bad way to wait for child
>>
> termination.
>
> Why is it so bad?  It's using waitpid (through GetTerminationStatus, which
> is
> what we're testing here) to reap the child process, and when it does, it
> returns.  It does it without blocking so that the test won't ever hang, and
> we
> can use a timeout.


Looping and sleeping is a bad testing pattern. Even if it works in one case,
it encourages other people to use it in places where it won't work.

Also, if there is no other way to test a function, it further encourages the
spread of bad pattern.


>  It looks like a regression in test to me, i.e. the test was good
>> (WaitForSingleProcess), and now it's less reliable.
>>
>
> How exactly is it less reliable?  They both used waitpid to find when a
> process
> exits.


There is more potential for non-determinism when waitpid is called more than
once. Also, the point about other people copying the suspicious pattern
applies.

[Greg Spencer (Chromium), 2010-12-03 18:29:25]

http://codereview.chromium.org/5172009/diff/31001/base/process_util.h
File base/process_util.h (right):

http://codereview.chromium.org/5172009/diff/31001/base/process_util.h#newcode125
base/process_util.h:125: enum ExitCode {
On 2010/12/03 01:01:54, brettw wrote:
> I'm kind of sad to see this enumeration here (I realize you didn't add it).
This
> kind of thing doesn't belong in base isn't the right place to force meanings
of
> exit codes.
> 
> I did some poking and there are no dependencies in base outside of
> base::DidProcessCrash, and this function is not used outside of Chrome and
then
> always in the context of ChildProcessLauncher. Can we just move
DidProcessCrash
> to a static function in ChildProcessLauncher and then just delete this enum
> (using the big ResultCodes enum instead).

Yeah, I'm not a big fan of this enum either, but the comment was scary enough to
me that I didn't want to just eliminate it.  I'd be happy to get rid of it
though (I actually did that initially, and put it back to differentiate the
interpreted status from the exit codes).

So, just so I'm clear on what you're suggesting here:  You'd like me to move
GetTerminationStatus (the renamed DidProcessCrash) into ChildProcessLauncher,
eliminate the ExitCode enum, and replace usage of the EXIT_CODE_* values in the
code with the equivalent value in the TerminationStatus enum (which I presume
you'd want moved to ChildProcessLauncher as well) so that when KillProcess is
called (especially on Windows), it uses the same 0, 1, 2 values to keep the
third party installers from breaking.

It looks to me like zygote_main_linux.cc uses GetTerminationStatus outside of
the ChildProcessLauncher, so there will have to be a static.  So does
nacl_process_host.cc (when on wow64).

I actually think that this function belongs here, though.  (With the exception
of the exit code crap) it's not specific to Chrome, even if Chrome is the only
client right now.  How strongly do you feel about moving it? :)  It seems to fit
well with the other process utilities here.

[Greg Spencer (Chromium), 2010-12-03 18:48:08]

On 2010/12/03 18:07:23, PaweÅ Hajdan Jr. wrote:
> It seems at least some of the new tests should also be able to use
> WaitForSingleProcess, for example GetTerminationStatusExit.

No, because I need to call GetTerminationStatus in order to test it,
and if I used WaitForSingleProcess, then it would eat the termination
status and reap the child, and then when I called
GetTerminationStatus, it would not have information to return.

Note that WaitForChildTermination is calling the function I want to
test (GetTerminationStatus), it's not just a construct in the test to
wait for a child to terminate.

> Furthermore, maybe that means we should create a variant or even replacement
> of WaitForSingleProcess that would return termination status. That seems to
> solve the loop-n-probe problem.

OK, but then we're not testing what I need tested.  I need a function
which calls waitpid with E_NOHANG (i.e. without blocking) to reap
child processes and obtain their exit and signal information.  Any
implementation of WaitForSingleProcess would wait, and that's not what
I need tested.  If I could have used WaitForSingleProcess in the code
and not have GetTerminationStatus, I would.

> Looping and sleeping is a bad testing pattern. Even if it works in one case,
> it encourages other people to use it in places where it won't work.
>
> Also, if there is no other way to test a function, it further encourages the
> spread of bad pattern.

OK, so it sounds to me like your main objection here is that you're
worried that sleep loops will get used other places if you let one in.

This is why I put in the comment "Using wait loops is discouraged in
most instances." in there.  I can make it more explicit if that would
help.

[Greg Spencer (Chromium), 2010-12-03 18:49:53]

On 2010/12/03 18:48:08, Greg Spencer (Chromium) wrote:
> OK, but then we're not testing what I need tested.  I need a function
> which calls waitpid with E_NOHANG (i.e. without blocking) to reap
> child processes and obtain their exit and signal information.

Actually, I mean WNOHANG.

[Paweł Hajdan Jr., 2010-12-04 09:32:24]

On Fri, Dec 3, 2010 at 19:48, <gspencer@chromium.org> wrote:
>
> OK, but then we're not testing what I need tested.  I need a function
> which calls waitpid with E_NOHANG (i.e. without blocking) to reap
> child processes and obtain their exit and signal information.  Any
> implementation of WaitForSingleProcess would wait, and that's not what
> I need tested.  If I could have used WaitForSingleProcess in the code
> and not have GetTerminationStatus, I would.
>

How about creating a variant of WaitForSingleProcess that would also return
termination status?

You shouldn't need to test a scenario where GetTerminationStatus is used to
wait for process termination, because it should never be used for that
purpose.

Testing a scenario where the process is still running is already done in
this CL.

If it doesn't seem for you that tests I suggest would be less realistic, how
about testing at a bit higher level? For example, kill a renderer, simulate
a renderer crash, etc (in a browser test), and then somehow verify that we
recognize the termination status correctly.


> OK, so it sounds to me like your main objection here is that you're
> worried that sleep loops will get used other places if you let one in.
>

Right.


> This is why I put in the comment "Using wait loops is discouraged in
> most instances." in there.  I can make it more explicit if that would
> help.


I think that's not needed. I'd just like to make sure the comment is true
(that the loop is necessary).

I'm sorry my comments delay this review. How about chatting about it and
resolving remaining issues (if it turns out my suggestions make no sense,
that's fine for me)? I'm in GMT+1 TZ, and my Google Chat address is
phajdan.jr@gmail.com.

[Greg Spencer (Chromium), 2010-12-07 22:27:07]

On 2010/12/03 18:29:25, Greg Spencer (Chromium) wrote:
> http://codereview.chromium.org/5172009/diff/31001/base/process_util.h
> File base/process_util.h (right):
> 
>
http://codereview.chromium.org/5172009/diff/31001/base/process_util.h#newcode125
> base/process_util.h:125: enum ExitCode {
> On 2010/12/03 01:01:54, brettw wrote:
> > I'm kind of sad to see this enumeration here (I realize you didn't add it).
> This
> > kind of thing doesn't belong in base isn't the right place to force meanings
> of
> > exit codes.
> > 
> > I did some poking and there are no dependencies in base outside of
> > base::DidProcessCrash, and this function is not used outside of Chrome and
> then
> > always in the context of ChildProcessLauncher. Can we just move
> DidProcessCrash
> > to a static function in ChildProcessLauncher and then just delete this enum
> > (using the big ResultCodes enum instead).
> 
> Yeah, I'm not a big fan of this enum either, but the comment was scary enough
to
> me that I didn't want to just eliminate it.  I'd be happy to get rid of it
> though (I actually did that initially, and put it back to differentiate the
> interpreted status from the exit codes).
> 
> So, just so I'm clear on what you're suggesting here:  You'd like me to move
> GetTerminationStatus (the renamed DidProcessCrash) into ChildProcessLauncher,
> eliminate the ExitCode enum, and replace usage of the EXIT_CODE_* values in
the
> code with the equivalent value in the TerminationStatus enum (which I presume
> you'd want moved to ChildProcessLauncher as well) so that when KillProcess is
> called (especially on Windows), it uses the same 0, 1, 2 values to keep the
> third party installers from breaking.
> 
> It looks to me like zygote_main_linux.cc uses GetTerminationStatus outside of
> the ChildProcessLauncher, so there will have to be a static.  So does
> nacl_process_host.cc (when on wow64).
> 
> I actually think that this function belongs here, though.  (With the exception
> of the exit code crap) it's not specific to Chrome, even if Chrome is the only
> client right now.  How strongly do you feel about moving it? :)  It seems to
fit
> well with the other process utilities here.

Ping.

[Greg Spencer (Chromium), 2010-12-08 01:07:20]

On 2010/12/04 09:32:24, Paweł Hajdan Jr. wrote:
> How about creating a variant of WaitForSingleProcess that would also
> return termination status?

As long as it doesn't wait, that would be fine.  However, you'd wind
up with the same functionality as GetTerminationStatus, but with a
different name.

> You shouldn't need to test a scenario where GetTerminationStatus is
> used to wait for process termination, because it should never be
> used for that purpose.

In production code, it's used to determine if a renderer process has
crashed.  It does this periodically: It's being run whenever a child
process gets an IPC channel error to check for a crashed renderer.

The simulation of this in the test code is to create a separate
process, have it exit or crash in response to some message, and then
use GetTerminationStatus to check the status.

In real code, it won't generate the IPC error until the process is
already dead.  In the test code, it takes a moment for it to die,
hence the wait loop.  I could do it with a single wait-and-then-check,
and eliminate the loop, but that would produce a much more flaky
test.

> If it doesn't seem for you that tests I suggest would be less
> realistic, how about testing at a bit higher level? For example,
> kill a renderer, simulate a renderer crash, etc (in a browser test),
> and then somehow verify that we recognize the termination status
> correctly.

There are already browser tests that kill renderers and check that
they are properly noticed.  They don't test the termination status
directly, but I could add that.

You're not suggesting removing this unit test entirely though, are
you?: it seems to me to be a useful, non-flaky test to me.

Can I quote you from the last time you objected to this same code?:
"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."

> > OK, so it sounds to me like your main objection here is that you're
> > worried that sleep loops will get used other places if you let one in.
>
> Right.

So why can't I just make the comment loud and scary to keep that from
happening?  If people ignore that comment, then they were going to do
it anyhow.

> I think that's not needed. I'd just like to make sure the comment is true
> (that the loop is necessary).

The loop is necessary, unless you can think of a better way.

> I'm sorry my comments delay this review. How about chatting about it and
> resolving remaining issues (if it turns out my suggestions make no sense,
> that's fine for me)? I'm in GMT+1 TZ, and my Google Chat address is
> mailto:phajdan.jr@gmail.com.

It's OK: if it becomes necessary, I'll split the test into a separate
CL and we can continue trying to find resolution without blocking the
feature work.  I appreciate your effort.

[Greg Spencer (Chromium), 2010-12-08 01:10:23]

On 2010/12/08 01:07:20, Greg Spencer (Chromium) wrote:
> Can I quote you from the last time you objected to this same code?:

And by "same code", I mean exactly the same file, test, and code, not "this same
kind of code".  (I'm just trying to avoid wasting a round trip clarifying that
:)

It's from this previous review: http://codereview.chromium.org/3386014/

[Paweł Hajdan Jr., 2010-12-09 09:54:15]

Ah, it's the same code as with *that* review. Sorry, I was thinking we're
copy-pasting that polling loop further.

I have no more questions about this CL, my earlier quotes still apply. LGTM.

On Wed, Dec 8, 2010 at 02:10, <gspencer@chromium.org> wrote:

> On 2010/12/08 01:07:20, Greg Spencer (Chromium) wrote:
>
>> Can I quote you from the last time you objected to this same code?:
>>
>
> And by "same code", I mean exactly the same file, test, and code, not "this
> same
> kind of code".  (I'm just trying to avoid wasting a round trip clarifying
> that
> :)
>
> It's from this previous review: http://codereview.chromium.org/3386014/
>
>
>
>
> http://codereview.chromium.org/5172009/
>

[brettw, 2010-12-09 16:48:49]

http://codereview.chromium.org/5172009/diff/31001/base/process_util.h
File base/process_util.h (right):

http://codereview.chromium.org/5172009/diff/31001/base/process_util.h#newcode125
base/process_util.h:125: enum ExitCode {
I don't really like the exit codes here, personally. Lemme talk to some
different people today to see what the opinion is about this.

[brettw, 2010-12-09 17:22:13]

http://codereview.chromium.org/5172009/diff/31001/base/process_util.h
File base/process_util.h (right):

http://codereview.chromium.org/5172009/diff/31001/base/process_util.h#newcode125
base/process_util.h:125: enum ExitCode {
I'm more sure that I don't want this in base.

The main point is that no non-Chrome processes use these codes. This means that
having this in base is actively misleading. For example, none of the test
executables use this, they return 0 on success and 1 on failure. This
corresponds to WAS_KILLED. Another example is the stress_cache executable in
net, which returns 0 on success and -1 on failure.

The stuff in base should be always useful. But in any of these cases, if I call
GetTerminationStatus, the result will be incorrect, despite the function looking
like it will be checking something for me.

Previously, I think the use of these was a bit borderline. They were there, and
you could call WaitForExitCode to get the exit code. I would still have
preferred it to be in common for the reasons above. But with your change and the
addition of GetTerminationStatus, I think the code here is more actively
confusing about how exit codes are handled.

The situation we have now is that each process decides what its exit codes are.
For Chrome sub-processes, we have a scheme that we use, and the code and enums
that go with that scheme should be in Chrome.

[Greg Spencer (Chromium), 2010-12-09 17:58:42]

http://codereview.chromium.org/5172009/diff/31001/base/process_util.h
File base/process_util.h (right):

http://codereview.chromium.org/5172009/diff/31001/base/process_util.h#newcode125
base/process_util.h:125: enum ExitCode {
On 2010/12/09 17:22:13, brettw wrote:
> I'm more sure that I don't want this in base.

OK, as I said, I'm happy taking the exit code out and moving it to
ChildProcessLauncher (or wherever).

The thing I'd like to leave in base is GetTerminationStatus, since it's telling
us something generic about processes and signals, not relying on chrome-specific
exit codes.

[brettw, 2010-12-09 18:11:26]

> OK, as I said, I'm happy taking the exit code out and moving it to
> ChildProcessLauncher (or wherever).
> 
> The thing I'd like to leave in base is GetTerminationStatus, since it's
telling
> us something generic about processes and signals, not relying on
chrome-specific
> exit codes.

That's fine. Just be sure to factor it in such a way that it's clear that this
function tells you the generic status type (signals and those hard coded windows
values, which I agree are nice to re-use) and not the chrome-specific ones.

[Greg Spencer, 2010-12-09 18:21:24]

On Thu, Dec 9, 2010 at 10:11 AM, <brettw@chromium.org> wrote:

>
>  OK, as I said, I'm happy taking the exit code out and moving it to
>> ChildProcessLauncher (or wherever).
>>
>
>  The thing I'd like to leave in base is GetTerminationStatus, since it's
>>
> telling
>
>> us something generic about processes and signals, not relying on
>>
> chrome-specific
>
>> exit codes.
>>
>
> That's fine. Just be sure to factor it in such a way that it's clear that
> this
> function tells you the generic status type (signals and those hard coded
> windows
> values, which I agree are nice to re-use) and not the chrome-specific ones.


OK, will do.

-Greg.

[Greg Spencer (Chromium), 2010-12-10 18:49:25]

Ok, I've done one possible way of doing this, but there is a snag I'd like some
input on:

I've moved the error code values into result_codes.h, which seems to be a very
logical place for it: application specific error codes that are in 'common', it
already had entries for them, and it works well overall.

The snag is this:  On Windows there is ONE error code that is a problem: the
"HUNG" error code is really just 2, and is specific to the HungWindowDetector in
Chrome.  First, this is a horrible choice for a custom value, but that aside,
I'm trying to figure out if there is any way I can factor that out somehow (like
check for that return code in the places where we care about it, rather than
relying on GetTerminationStatus to return the "HUNG" status, but that seems
pretty impractical), but it may be that because of this one value I need to move
the entire mechanism into ChildProcessLauncher as you suggested.

Currently I just define the HUNG code in both process_util and result_codes,
which is a problem, I realize.

I'll bite the bullet and move it if I need to, but it's annoying to have to move
this functionality from where I would instinctively look for it (process_util)
into a place where I'd have to find it (and might miss it) just because of a
single enum value on a single platform.

(The "KILLED" value: "1" has a similar problem, but it seems to be in common use
in Windows programming, so I'm not really worried about that)

Any suggestions?

[Greg Spencer (Chromium), 2010-12-10 22:59:47]

On 2010/12/10 18:49:25, Greg Spencer (Chromium) wrote:
> Ok, I've done one possible way of doing this, but there is a snag
> I'd like some input on:

OK, I did some research, and it seems that nobody ever tests for that
TERMINATION_STATUS_PROCESS_WAS_HUNG value: the ResultCodes::HUNG value is used
in the Windows installer code, but that code never uses GetTerminationStatus to
find out the exit status, so there's no way it could depend on that
functionality.

So: I'm going to remove "HUNG" detection from GetTerminationStatus, which will
eliminate the need for the problematic value in "base", and life is good again.

[Greg Spencer (Chromium), 2010-12-10 23:44:33]

OK, PTAL.

The TERMINATION_STATUS_PROCESS_WAS_HUNG value has been removed, and the exit
code enum eliminated (replaced by values from result_codes.h).

[brettw, 2010-12-13 21:09:34]

LGTM, sorry this took so long! Just a couple things to check before you check
in:

http://codereview.chromium.org/5172009/diff/136001/base/process_util.h
File base/process_util.h (right):

http://codereview.chromium.org/5172009/diff/136001/base/process_util.h#newcod...
base/process_util.h:356: // caller is not interested in it.
On Linux, is my understanding correct that this function may only be called once
for a child process? If so, we should be sure to mention it in this comment.

http://codereview.chromium.org/5172009/diff/136001/chrome/browser/zygote_host...
File chrome/browser/zygote_host_linux.h (right):

http://codereview.chromium.org/5172009/diff/136001/chrome/browser/zygote_host...
chrome/browser/zygote_host_linux.h:50: kCmdFork = 0,             // Fork off a
new renderer.
Can you move these other comments over so they're aligned again?

[Greg Spencer (Chromium), 2010-12-13 22:36:16]

Thanks for the reviews!

http://codereview.chromium.org/5172009/diff/136001/base/process_util.h
File base/process_util.h (right):

http://codereview.chromium.org/5172009/diff/136001/base/process_util.h#newcod...
base/process_util.h:356: // caller is not interested in it.
On 2010/12/13 21:09:35, brettw wrote:
> On Linux, is my understanding correct that this function may only be called
once
> for a child process? If so, we should be sure to mention it in this comment.

Done.

http://codereview.chromium.org/5172009/diff/136001/chrome/browser/zygote_host...
File chrome/browser/zygote_host_linux.h (right):

http://codereview.chromium.org/5172009/diff/136001/chrome/browser/zygote_host...
chrome/browser/zygote_host_linux.h:50: kCmdFork = 0,             // Fork off a
new renderer.
On 2010/12/13 21:09:35, brettw wrote:
> Can you move these other comments over so they're aligned again?

Done.