Call Stop() from all destructors of base::Thread subclasses (except for a couple that ensure Stop() has already been called).

BUG=none
TEST=none
TBR=willchan,hans
Review URL: https://codereview.chromium.org/11026019

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

5 files changed, 21 insertions, 14 deletions

diff --git a/base/threading/thread.h b/base/threading/thread.h
index 7296854..1d24cf3 100644
--- a/base/threading/thread.h
+++ b/base/threading/thread.h
@@ -20,7 +20,7 @@ namespace base {
 // pending tasks queued on the thread's message loop will run to completion
 // before the thread is terminated.
 //
-// NOTE: Subclasses must call Stop() in their destructor. See ~Thread below.
+// WARNING! SUBCLASSES MUST CALL Stop() IN THEIR DESTRUCTORS!  See ~Thread().
 //
 // After the thread is stopped, the destruction sequence is:
 //
@@ -49,13 +49,12 @@ class BASE_EXPORT Thread : PlatformThread::Delegate {
 
   // Destroys the thread, stopping it if necessary.
   //
-  // NOTE: All subclasses of Thread must call Stop() in their
-  // destructor, or otherwise ensure Stop() is called before the
-  // subclass is destructed.  This is required to avoid a data race
-  // between the destructor modifying the vtable, and the thread's
-  // ThreadMain calling the virtual method Run.  It also ensures that
-  // the CleanUp() virtual method is called on the subclass before it
-  // is destructed.
+  // NOTE: ALL SUBCLASSES OF Thread MUST CALL Stop() IN THEIR DESTRUCTORS (or
+  // guarantee Stop() is explicitly called before the subclass is destroyed).
+  // This is required to avoid a data race between the destructor modifying the
+  // vtable, and the thread's ThreadMain calling the virtual method Run().  It
+  // also ensures that the CleanUp() virtual method is called on the subclass
+  // before it is destructed.
   virtual ~Thread();
 
   // Starts the thread.  Returns true if the thread was successfully started;
@@ -82,10 +81,9 @@ class BASE_EXPORT Thread : PlatformThread::Delegate {
   // Stop may be called multiple times and is simply ignored if the thread is
   // already stopped.
   //
-  // NOTE: This method is optional.  It is not strictly necessary to call this
-  // method as the Thread's destructor will take care of stopping the thread if
-  // necessary.
-  //
+  // NOTE: If you are a consumer of Thread, it is not necessary to call this
+  // before deleting your Thread objects, as the destructor will do it.
+  // IF YOU ARE A SUBCLASS OF Thread, YOU MUST CALL THIS IN YOUR DESTRUCTOR.
   void Stop();
 
   // Signals the thread to exit in the near future.
diff --git a/content/browser/device_orientation/provider_impl.cc b/content/browser/device_orientation/provider_impl.cc
index e436869..b566cb0 100644
--- a/content/browser/device_orientation/provider_impl.cc
+++ b/content/browser/device_orientation/provider_impl.cc
@@ -16,7 +16,6 @@
 namespace {
 
 void DeleteThread(base::Thread* thread) {
-  thread->Stop();
   delete thread;
 }
 
@@ -73,6 +72,7 @@ ProviderImpl::PollingThread::PollingThread(
 }
 
 ProviderImpl::PollingThread::~PollingThread() {
+  Stop();
 }
 
 void ProviderImpl::PollingThread::DoAddPollingDataType(DeviceData::Type type) {
diff --git a/content/common/gpu/client/gl_helper.cc b/content/common/gpu/client/gl_helper.cc
index f5c6ce3..775a651 100644
--- a/content/common/gpu/client/gl_helper.cc
+++ b/content/common/gpu/client/gl_helper.cc
@@ -33,8 +33,11 @@ class GLHelperThread : public base::Thread {
   GLHelperThread() : base::Thread(kGLHelperThreadName) {
     Start();
   }
-  virtual ~GLHelperThread() {}
+  virtual ~GLHelperThread() {
+    Stop();
+  }
 
+ private:
   DISALLOW_COPY_AND_ASSIGN(GLHelperThread);
 };
 
diff --git a/media/audio/audio_manager_base.cc b/media/audio/audio_manager_base.cc
index ccddbc8..9ae0166 100644
--- a/media/audio/audio_manager_base.cc
+++ b/media/audio/audio_manager_base.cc
@@ -43,6 +43,7 @@ AudioThread::AudioThread(const char* name) : base::Thread(name) {
 }
 
 AudioThread::~AudioThread() {
+  Stop();
 }
 
 void AudioThread::Init() {
diff --git a/remoting/host/setup/daemon_controller_win.cc b/remoting/host/setup/daemon_controller_win.cc
index 48f88f8..814fa40 100644
--- a/remoting/host/setup/daemon_controller_win.cc
+++ b/remoting/host/setup/daemon_controller_win.cc
@@ -67,6 +67,7 @@ const int kUnprivilegedTimeoutSec = 60;
 class ComThread : public base::Thread {
  public:
   explicit ComThread(const char* name);
+  virtual ~ComThread();
 
   bool Start();
 
@@ -171,6 +172,10 @@ class DaemonControllerWin : public remoting::DaemonController {
 ComThread::ComThread(const char* name) : base::Thread(name) {
 }
 
+ComThread::~ComThread() {
+  Stop();
+}
+
 bool ComThread::Start() {
   // N.B. The single threaded COM apartment must be run on a UI message loop.
   base::Thread::Options thread_options(MessageLoop::TYPE_UI, 0);