Reverting this CL as it causes chrome frame net tests to crash at exit. The crash happens because the IPC channel proxy

code relies on the listener threads message loop being around. This change destroys the message loop when it goes out
of scope leading to the crash.

Revert 44323 - Don't call Thread::CleanUp() before the MessageLoop destruction observers have run.

This is consistent with the comment for Thread::CleanUp(), which says it runs after the message loop has "stopped".
Certain consumers depend on this ordering to avoid accessing variables which are deleted by Thread::CleanUp().

BUG=39723
TEST=ThreadTest.CleanUp
Review URL: http://codereview.chromium.org/1540002

TBR=eroman@chromium.org
Review URL: http://codereview.chromium.org/1528034

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@44357 0039d316-1c4b-4281-b951-d872f2087c98

3 files changed, 23 insertions, 136 deletions

diff --git a/base/thread.cc b/base/thread.cc
index e916f2c..aeaa0a1 100644
--- a/base/thread.cc
+++ b/base/thread.cc
@@ -35,7 +35,7 @@ struct Thread::StartupData {
         event(false, false) {}
 };
 
-Thread::Thread(const char* name)
+Thread::Thread(const char *name)
     : stopping_(false),
       startup_data_(NULL),
       thread_(0),
@@ -134,30 +134,25 @@ void Thread::Run(MessageLoop* message_loop) {
 }
 
 void Thread::ThreadMain() {
-  {
-    // The message loop for this thread.
-    MessageLoop message_loop(startup_data_->options.message_loop_type);
-
-    // Complete the initialization of our Thread object.
-    thread_id_ = PlatformThread::CurrentId();
-    PlatformThread::SetName(name_.c_str());
-    ANNOTATE_THREAD_NAME(name_.c_str());  // Tell the name to race detector.
-    message_loop.set_thread_name(name_);
-    message_loop_ = &message_loop;
-
-    // Let the thread do extra initialization.
-    // Let's do this before signaling we are started.
-    Init();
-
-    startup_data_->event.Signal();
-    // startup_data_ can't be touched anymore since the starting thread is now
-    // unlocked.
-
-    Run(message_loop_);
-    // Destroy |message_loop| upon leaving this scope.
-  }
+  // The message loop for this thread.
+  MessageLoop message_loop(startup_data_->options.message_loop_type);
 
-  message_loop_ = NULL;
+  // Complete the initialization of our Thread object.
+  thread_id_ = PlatformThread::CurrentId();
+  PlatformThread::SetName(name_.c_str());
+  ANNOTATE_THREAD_NAME(name_.c_str());  // Tell the name to race detector.
+  message_loop.set_thread_name(name_);
+  message_loop_ = &message_loop;
+
+  // Let the thread do extra initialization.
+  // Let's do this before signaling we are started.
+  Init();
+
+  startup_data_->event.Signal();
+  // startup_data_ can't be touched anymore since the starting thread is now
+  // unlocked.
+
+  Run(message_loop_);
 
   // Let the thread do extra cleanup.
   CleanUp();
@@ -166,6 +161,7 @@ void Thread::ThreadMain() {
   DCHECK(GetThreadWasQuitProperly());
 
   // We can't receive messages anymore.
+  message_loop_ = NULL;
   thread_id_ = 0;
 }
 
diff --git a/base/thread.h b/base/thread.h
index 75ea0b1..1f2690c 100644
--- a/base/thread.h
+++ b/base/thread.h
@@ -35,7 +35,7 @@ class Thread : PlatformThread::Delegate {
 
   // Constructor.
   // name is a display string to identify the thread.
-  explicit Thread(const char* name);
+  explicit Thread(const char *name);
 
   // Destroys the thread, stopping it if necessary.
   //
@@ -117,8 +117,7 @@ class Thread : PlatformThread::Delegate {
   // Called to start the message loop
   virtual void Run(MessageLoop* message_loop);
 
-  // Called just after the message loop ends. At this point, all of the
-  // MessageLoop::DestructionObservers have already been notified.
+  // Called just after the message loop ends
   virtual void CleanUp() {}
 
   static void SetThreadWasQuitProperly(bool flag);
diff --git a/base/thread_unittest.cc b/base/thread_unittest.cc
index 193f7c1..90aec51 100644
--- a/base/thread_unittest.cc
+++ b/base/thread_unittest.cc
@@ -2,14 +2,11 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#include "base/thread.h"
-
-#include <vector>
-
 #include "base/dynamic_annotations.h"
 #include "base/lock.h"
 #include "base/message_loop.h"
 #include "base/string_util.h"
+#include "base/thread.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "testing/platform_test.h"
 
@@ -57,81 +54,6 @@ class SleepInsideInitThread : public Thread {
   bool init_called_;
 };
 
-enum ThreadEvent {
-  // Thread::Init() was called.
-  THREAD_EVENT_INIT,
-
-  // The MessageLoop for the thread was deleted.
-  THREAD_EVENT_MESSAGE_LOOP_DESTROYED,
-
-  // Thread::CleanUp() was called.
-  THREAD_EVENT_CLEANUP,
-};
-
-typedef std::vector<ThreadEvent> EventList;
-
-class CaptureToEventList : public Thread {
- public:
-  // This Thread pushes events into the vector |event_list| to show
-  // the order they occured in. |event_list| must remain valid for the
-  // lifetime of this thread.
-  explicit CaptureToEventList(EventList* event_list)
-      : Thread("none"), event_list_(event_list) {
-  }
-
-  virtual ~CaptureToEventList() {
-    // Must call Stop() manually to have our CleanUp() function called.
-    Stop();
-  }
-
-  virtual void Init() {
-    event_list_->push_back(THREAD_EVENT_INIT);
-  }
-
-  virtual void CleanUp() {
-    event_list_->push_back(THREAD_EVENT_CLEANUP);
-  }
-
- private:
-  EventList* event_list_;
-};
-
-// Observer that writes a value into |event_list| when a message loop has been
-// destroyed.
-class CapturingDestructionObserver : public MessageLoop::DestructionObserver {
- public:
-  // |event_list| must remain valid throughout the observer's lifetime.
-  explicit CapturingDestructionObserver(EventList* event_list)
-      : event_list_(event_list) {
-  }
-
-  // DestructionObserver implementation:
-  virtual void WillDestroyCurrentMessageLoop() {
-    event_list_->push_back(THREAD_EVENT_MESSAGE_LOOP_DESTROYED);
-    event_list_ = NULL;
-  }
-
- private:
-  EventList* event_list_;
-};
-
-// Task that adds a destruction observer to the current message loop.
-class RegisterDestructionObserver : public Task {
- public:
-  explicit RegisterDestructionObserver(
-      MessageLoop::DestructionObserver* observer)
-      : observer_(observer) {
-  }
-
-  virtual void Run() {
-    MessageLoop::current()->AddDestructionObserver(observer_);
-    observer_ = NULL;
-  }
-
- private:
-  MessageLoop::DestructionObserver* observer_;
-};
-
 }  // namespace
 
 TEST_F(ThreadTest, Restart) {
@@ -219,33 +141,3 @@ TEST_F(ThreadTest, SleepInsideInit) {
   t.Start();
   EXPECT_TRUE(t.InitCalled());
 }
-
-// Make sure that MessageLoop::DestructionObserver is called before
-// Thread::Cleanup.
-TEST_F(ThreadTest, CleanUp) {
-  EventList captured_events;
-  CapturingDestructionObserver loop_destruction_observer(&captured_events);
-
-  {
-    // Start a thread which writes its event into |captured_events|.
-    CaptureToEventList t(&captured_events);
-    EXPECT_TRUE(t.Start());
-    EXPECT_TRUE(t.message_loop());
-    EXPECT_TRUE(t.IsRunning());
-
-    // Register an observer that writes into |captured_events| once the
-    // thread's message loop is destroyed.
-    t.message_loop()->PostTask(
-        FROM_HERE,
-        new RegisterDestructionObserver(&loop_destruction_observer));
-
-    // Upon leaving this scope, the thread is deleted.
-  }
-
-  // Check the order of events during shutdown. In particular, that the message
-  // loop destruction observer ran before Thread::CleanUp().
-  ASSERT_EQ(3u, captured_events.size());
-  EXPECT_EQ(THREAD_EVENT_INIT, captured_events[0]);
-  EXPECT_EQ(THREAD_EVENT_MESSAGE_LOOP_DESTROYED, captured_events[1]);
-  EXPECT_EQ(THREAD_EVENT_CLEANUP, captured_events[2]);
-}