From 88f33312efe703fdb349afdcf4dc6d3c9147c033 Mon Sep 17 00:00:00 2001 From: "phajdan.jr@chromium.org" Date: Mon, 14 Sep 2009 23:47:38 +0000 Subject: Fix a flaky crash in Thread by guarding access to message_loop_ more carefully. There was a window when message_loop_ could be non-NULL and invalid. The code assumed that when message_loop_ is non-NULL, it's valid. Added a stopping_ flag which indicates a state in which we shouldn't access the message loop because it is not safe. Also, reused StopSoon logic in Stop and fixed a comment which was inaccurate. TEST=Covered by base_unittests. BUG=15331 Review URL: http://codereview.chromium.org/201108 git-svn-id: svn://svn.chromium.org/chrome/trunk/src@26180 0039d316-1c4b-4281-b951-d872f2087c98 --- base/thread.cc | 29 +++++++++++------------------ 1 file changed, 11 insertions(+), 18 deletions(-) (limited to 'base/thread.cc') diff --git a/base/thread.cc b/base/thread.cc index dcda330..5fc8678 100644 --- a/base/thread.cc +++ b/base/thread.cc @@ -35,7 +35,8 @@ struct Thread::StartupData { }; Thread::Thread(const char *name) - : startup_data_(NULL), + : stopping_(false), + startup_data_(NULL), thread_(0), message_loop_(NULL), thread_id_(0), @@ -98,40 +99,32 @@ void Thread::Stop() { if (!thread_was_started()) return; - // We should only be called on the same thread that started us. - DCHECK_NE(thread_id_, PlatformThread::CurrentId()); - - // StopSoon may have already been called. - if (message_loop_) - message_loop_->PostTask(FROM_HERE, new ThreadQuitTask()); + StopSoon(); - // Wait for the thread to exit. It should already have terminated but make - // sure this assumption is valid. + // Wait for the thread to exit. // // TODO(darin): Unfortunately, we need to keep message_loop_ around until // the thread exits. Some consumers are abusing the API. Make them stop. // PlatformThread::Join(thread_); - // The thread can't receive messages anymore. - message_loop_ = NULL; + // The thread should NULL message_loop_ on exit. + DCHECK(!message_loop_); // The thread no longer needs to be joined. startup_data_ = NULL; + + stopping_ = false; } void Thread::StopSoon() { - if (!message_loop_) - return; - // We should only be called on the same thread that started us. DCHECK_NE(thread_id_, PlatformThread::CurrentId()); - // We had better have a message loop at this point! If we do not, then it - // most likely means that the thread terminated unexpectedly, probably due - // to someone calling Quit() on our message loop directly. - DCHECK(message_loop_); + if (!message_loop_ || stopping_) + return; + stopping_ = true; message_loop_->PostTask(FROM_HERE, new ThreadQuitTask()); } -- cgit v1.1