check for successful exit code in WaitForSingleProcess on Windows to

check for successful exit code in WaitForSingleProcess on Windows to
match other platforms.

consequentially, in browser tests, add PROCESS_QUERY_INFORMATION to
the rights requested when opening a child serviceprocess handle so the
exit code can be queried.

BUG=106234
TEST=Ran base_unittests.exe --gtest_filter="ProcessUtilTest.SpawnChild" with the child custom-tweaked to fail and verified that prior to my change (with HEAD) the failure is NOT detected and the test passes; with this change the failure is detected.


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

[Derek Bruening, 2011-12-15 22:03:48]

Partway through a trybot run.  Will also commit through CQ as extra precaution.

[Reid Kleckner, 2011-12-15 22:14:19]

http://codereview.chromium.org/8964004/diff/1/base/process_util_win.cc
File base/process_util_win.cc (right):

http://codereview.chromium.org/8964004/diff/1/base/process_util_win.cc#newcod...
base/process_util_win.cc:613: return exit_code == 0;
This has slightly different semantics from the POSIX version of
WaitForSingleProcess.  That one uses "return WIFEXITED(exit_code)", which will
return true even if the process exits non-zero.  This should return false only
if the child crashed.

[bruening, 2011-12-16 04:08:06]

http://codereview.chromium.org/8964004/diff/1/base/process_util_win.cc
File base/process_util_win.cc (right):

http://codereview.chromium.org/8964004/diff/1/base/process_util_win.cc#newcod...
base/process_util_win.cc:613: return exit_code == 0;
The problem is that on Windows exit codes set by the app can be any 32-bit value
so you're never really sure whether the app really crashed or it just used an
unusual exit code.  I suppose this could check for some standard values like
0xc0000005 or 0xc000001d but there are probably 20 or so different codes that
can constitute a crash and they aren't contiguous.  OTOH, no normal exit code is
likely to be in the STATUS_/EXCEPTION_ range.  The docs for this function talk
about a "clean exit": so checking for actual success seems reasonable.  It
depends on how this is meant to be used.  I would lean toward failing on
non-zero to catch more bugs unless we see existing usage that this breaks, which
so far has not been the case.

[cpu_(ooo_6.6-7.5), 2011-12-20 22:56:32]

lgtm

[commit-bot: I haz the power, 2011-12-20 23:38:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bruening@chromium.org/8964004/1

[commit-bot: I haz the power, 2011-12-20 23:38:55]

Presubmit check for 8964004-1 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for: base/process_util_win.cc

[Reid Kleckner, 2011-12-20 23:43:24]

LGTM

Just to follow up.

http://codereview.chromium.org/8964004/diff/1/base/process_util_win.cc
File base/process_util_win.cc (right):

http://codereview.chromium.org/8964004/diff/1/base/process_util_win.cc#newcod...
base/process_util_win.cc:613: return exit_code == 0;
On 2011/12/16 04:08:06, bruening wrote:
> The problem is that on Windows exit codes set by the app can be any 32-bit
value
> so you're never really sure whether the app really crashed or it just used an
> unusual exit code.  I suppose this could check for some standard values like
> 0xc0000005 or 0xc000001d but there are probably 20 or so different codes that
> can constitute a crash and they aren't contiguous.  OTOH, no normal exit code
is
> likely to be in the STATUS_/EXCEPTION_ range.  The docs for this function talk
> about a "clean exit": so checking for actual success seems reasonable.  It
> depends on how this is meant to be used.  I would lean toward failing on
> non-zero to catch more bugs unless we see existing usage that this breaks,
which
> so far has not been the case.

I was going to say that the same thing can happen on POSIX, but they do the
return code << 8 dance to make space for the signal bits.  Checking for != 0 is
most conservative, so this seems reasonable.

[brettw, 2011-12-20 23:44:40]

lgtm

[Derek Bruening, 2011-12-20 23:47:24]

On 2011/12/20 23:38:55, I haz the power (commit-bot) wrote:
> ** Presubmit ERRORS **
> Missing LGTM from an OWNER for: base/process_util_win.cc

Whoops, cpu volunteered to look at it and I assumed he was an OWNER.
Requesting a review from brettw.

[Derek Bruening, 2012-01-10 17:09:47]

On 2011/12/20 23:47:24, Derek Bruening wrote:
> On 2011/12/20 23:38:55, I haz the power (commit-bot) wrote:
> > ** Presubmit ERRORS **
> > Missing LGTM from an OWNER for: base/process_util_win.cc
> 
> Whoops, cpu volunteered to look at it and I assumed he was an OWNER.
> Requesting a review from brettw.

Trying willchan.

[Derek Bruening, 2012-01-19 19:05:56]

On 2012/01/10 17:09:47, Derek Bruening wrote:
> On 2011/12/20 23:47:24, Derek Bruening wrote:
> > On 2011/12/20 23:38:55, I haz the power (commit-bot) wrote:
> > > ** Presubmit ERRORS **
> > > Missing LGTM from an OWNER for: base/process_util_win.cc
> > 
> > Whoops, cpu volunteered to look at it and I assumed he was an OWNER.
> > Requesting a review from brettw.
> 
> Trying willchan.

Ping.

[commit-bot: I haz the power, 2012-01-23 15:43:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bruening@chromium.org/8964004/1

[commit-bot: I haz the power, 2012-01-23 17:35:16]

Change committed as 118695