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

2 files changed, 15 insertions, 18 deletions

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());
 }
 
diff --git a/base/thread.h b/base/thread.h
index 36daa7f..1f2690c 100644
--- a/base/thread.h
+++ b/base/thread.h
@@ -135,6 +135,10 @@ class Thread : PlatformThread::Delegate {
   // started the thread.  This way we know that we need to call Join.
   bool thread_was_started() const { return startup_data_ != NULL; }
 
+  // If true, we're in the middle of stopping, and shouldn't access
+  // |message_loop_|. It may non-NULL and invalid.
+  bool stopping_;
+
   // Used to pass data to ThreadMain.
   struct StartupData;
   StartupData* startup_data_;