Base: Crash the process if we are not able to join threads. (issue 10398060)

0 views
Skip to first unread message

rva...@chromium.org

unread,
May 16, 2012, 4:06:27 PM5/16/12
to will...@chromium.org, chromium...@chromium.org, erikw...@chromium.org, bret...@chromium.org
Reviewers: willchan,

Description:
Base: Crash the process if we are not able to join threads.

BUG=127931
TEST=none

Please review this at https://chromiumcodereview.appspot.com/10398060/

SVN Base: svn://svn.chromium.org/chrome/trunk/src/

Affected files:
M base/threading/platform_thread_win.cc


Index: base/threading/platform_thread_win.cc
===================================================================
--- base/threading/platform_thread_win.cc (revision 137046)
+++ base/threading/platform_thread_win.cc (working copy)
@@ -181,7 +181,13 @@
// Wait for the thread to exit. It should already have terminated but
make
// sure this assumption is valid.
DWORD result = WaitForSingleObject(thread_handle, INFINITE);
- DCHECK_EQ(WAIT_OBJECT_0, result);
+ if (result != WAIT_OBJECT_0) {
+ // Debug info for bug 127931.
+ DWORD error = GetLastError();
+ debug::Alias(&error);
+ debug::Alias(&thread_handle);
+ CHECK(false) << "Failed to wait for thread";
+ }

CloseHandle(thread_handle);
}


will...@chromium.org

unread,
May 16, 2012, 4:15:50 PM5/16/12
to rva...@chromium.org, chromium...@chromium.org, erikw...@chromium.org, bret...@chromium.org
lgtm




https://chromiumcodereview.appspot.com/10398060/diff/1/base/threading/platform_thread_win.cc
File base/threading/platform_thread_win.cc (right):

https://chromiumcodereview.appspot.com/10398060/diff/1/base/threading/platform_thread_win.cc#newcode184
base/threading/platform_thread_win.cc:184: if (result != WAIT_OBJECT_0)
{
Should we alias &result too?

https://chromiumcodereview.appspot.com/10398060/diff/1/base/threading/platform_thread_win.cc#newcode189
base/threading/platform_thread_win.cc:189: CHECK(false) << "Failed to
wait for thread";
I'd be inclined to ditch the string since it bloats the release binary.
I think the line number is good enough. Up to you.

https://chromiumcodereview.appspot.com/10398060/

rva...@chromium.org

unread,
May 16, 2012, 4:37:15 PM5/16/12
to will...@chromium.org, chromium...@chromium.org, erikw...@chromium.org, bret...@chromium.org
Reply all
Reply to author
Forward
0 new messages