My favorites | Sign in
Project Home Downloads Wiki Issues Code Search
New issue   Search
for
  Advanced search   Search tips   Subscriptions
Issue 106234: base::WaitForSingleProcess() should check exit code of child on Windows
1 person starred this issue and may be notified of changes. Back to list
 
Project Member Reported by bruen...@chromium.org, Dec 2, 2011
This was uncovered while investigating a Dr. Memory bug where child
processes crash very early on 32-bit XP:
https://code.google.com/p/drmemory/issues/detail?id=699

Yet the base_unittests.exe ProcessUtilTest.SpawnChild test succeeds.  On
investigation, its child dies right up front (this is after the system
loader is initialized, but before even kernel32.dll is loaded).  The parent
doesn't care and considers the test to have passed.

The POSIX version of base::WaitForSingleProcess() checks the status code
with WIFEXITED, while the Windows version just checks the return value of
WaitForSingleObject only and does not call GetExitCodeProcess() (via the
wrapper GetTerminationStatus()).

Since base:: is a central class the Dr. Memory team doesn't feel
comfortable messing with it so we're filing this bug.  Assigning
based on git blame: please triage.

Dec 5, 2011
#1 est...@chromium.org
so you're suggesting Windows be changed?
Status: Available
Owner: ---
Cc: brettw@chromium.org willchan@chromium.org cpu@chromium.org est...@chromium.org
Dec 5, 2011
#2 rnk@chromium.org
Um, no?  We're suggesting that base be changed to behave consistently on Windows and Linux.  Checking the return code of what is mostly a synchronization API is not the same as checking the process's exit status.  base::WaitForSingleProcess() should do one or the other consistently on all OSs.
Dec 5, 2011
#3 willchan@chromium.org
How important is this bug? Unfortunately there is no one who particular owns this code. I would encourage you guys not to be uncomfortable with messing around with base. Just go for it :) If you want advice, then one of the base/OWNERS (myself included) can offer some. And cpu@ should be able to comment on anything Windows specific.
Dec 12, 2011
#4 bruen...@chromium.org
this has now masked a second bug (issue 96010) and I'm afraid it might be masking more.
I'll take a stab at it.
Owner: bruen...@chromium.org
Dec 14, 2011
#5 c...@google.com
You can send me the review (cpu at chromium)
Jan 23, 2012
#6 bugdro...@chromium.org
The following revision refers to this bug:
    http://src.chromium.org/viewvc/chrome?view=rev&revision=118695

------------------------------------------------------------------------
r118695 | bruening@chromium.org | Mon Jan 23 09:35:14 PST 2012

Changed paths:
 M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/service/service_process_control_browsertest.cc?r1=118695&r2=118694&pathrev=118695
 M http://src.chromium.org/viewvc/chrome/trunk/src/base/process_util_win.cc?r1=118695&r2=118694&pathrev=118695

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.


Review URL: http://codereview.chromium.org/8964004
------------------------------------------------------------------------
Mar 10, 2013
#7 bugdro...@chromium.org
(No comment was entered for this change.)
Labels: -Area-Internals -Stability-DrMemory Cr-Internals Performance-Memory-DrMemory
Apr 1, 2013
#8 bugdro...@chromium.org
(No comment was entered for this change.)
Labels: -Performance-Memory-DrMemory Stability-Memory-DrMemory
Sign in to add a comment

Powered by Google Project Hosting