Thread::Stop() must be called before any subclass's destructor completes.

Update base::Thread documentation, fix all subclasses I could find
that had a problem, and remove no-longer-necessary suppressions.

BUG=102134

Review URL: http://codereview.chromium.org/8427007

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

18 files changed, 42 insertions, 35 deletions

diff --git a/base/memory/weak_ptr_unittest.cc b/base/memory/weak_ptr_unittest.cc
index c244ec0..f9f8b3e 100644
--- a/base/memory/weak_ptr_unittest.cc
+++ b/base/memory/weak_ptr_unittest.cc
@@ -47,6 +47,10 @@ class BackgroundThread : public Thread {
       : Thread("owner_thread") {
   }
 
+  ~BackgroundThread() {
+    Stop();
+  }
+
   void CreateConsumerFromProducer(Consumer** consumer, Producer* producer) {
     WaitableEvent completion(true, false);
     message_loop()->PostTask(
diff --git a/base/threading/thread.h b/base/threading/thread.h
index d7451ec..2ee4fa2 100644
--- a/base/threading/thread.h
+++ b/base/threading/thread.h
@@ -21,6 +21,8 @@ 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.
+//
 // After the thread is stopped, the destruction sequence is:
 //
 //  (1) Thread::CleanUp()
@@ -48,9 +50,13 @@ class BASE_EXPORT Thread : PlatformThread::Delegate {
 
   // Destroys the thread, stopping it if necessary.
   //
-  // NOTE: If you are subclassing from Thread, and you wish for your CleanUp
-  // method to be called, then you need to call Stop() from your destructor.
-  //
+  // 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.
   virtual ~Thread();
 
   // Starts the thread.  Returns true if the thread was successfully started;
diff --git a/base/threading/thread_unittest.cc b/base/threading/thread_unittest.cc
index fe39627..68a24cf 100644
--- a/base/threading/thread_unittest.cc
+++ b/base/threading/thread_unittest.cc
@@ -1,4 +1,4 @@
-// Copyright (c) 2006-2008 The Chromium Authors. All rights reserved.
+// Copyright (c) 2011 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
@@ -48,7 +48,9 @@ class SleepInsideInitThread : public Thread {
     ANNOTATE_BENIGN_RACE(
         this, "Benign test-only data race on vptr - http://crbug.com/98219");
   }
-  virtual ~SleepInsideInitThread() { }
+  virtual ~SleepInsideInitThread() {
+    Stop();
+  }
 
   virtual void Init() {
     base::PlatformThread::Sleep(500);
@@ -85,7 +87,6 @@ class CaptureToEventList : public Thread {
   }
 
   virtual ~CaptureToEventList() {
-    // Must call Stop() manually to have our CleanUp() function called.
     Stop();
   }
 
diff --git a/chrome/browser/metrics/thread_watcher.cc b/chrome/browser/metrics/thread_watcher.cc
index 4252580..8d4fe59 100644
--- a/chrome/browser/metrics/thread_watcher.cc
+++ b/chrome/browser/metrics/thread_watcher.cc
@@ -630,8 +630,6 @@ WatchDogThread::WatchDogThread() : Thread("BrowserWatchdog") {
 }
 
 WatchDogThread::~WatchDogThread() {
-  // We cannot rely on our base class to stop the thread since we want our
-  // CleanUp function to run.
   Stop();
 }
 
diff --git a/chrome/browser/printing/print_job_worker.cc b/chrome/browser/printing/print_job_worker.cc
index 19e967b..1072fb5 100644
--- a/chrome/browser/printing/print_job_worker.cc
+++ b/chrome/browser/printing/print_job_worker.cc
@@ -68,6 +68,7 @@ PrintJobWorker::~PrintJobWorker() {
   // cancels printing or in the case of print preview, the worker is destroyed
   // on the I/O thread.
   DCHECK_EQ(owner_->message_loop(), MessageLoop::current());
+  Stop();
 }
 
 void PrintJobWorker::SetNewOwner(PrintJobWorkerOwner* new_owner) {
diff --git a/chrome/browser/ui/views/shell_dialogs_win.cc b/chrome/browser/ui/views/shell_dialogs_win.cc
index faa4cb9..38f5158 100644
--- a/chrome/browser/ui/views/shell_dialogs_win.cc
+++ b/chrome/browser/ui/views/shell_dialogs_win.cc
@@ -383,6 +383,9 @@ bool SaveFileAs(HWND owner,
 class ShellDialogThread : public base::Thread {
  public:
   ShellDialogThread() : base::Thread("Chrome_ShellDialogThread") { }
+  ~ShellDialogThread() {
+    Stop();
+  }
 
  protected:
   void Init() {
diff --git a/chrome/default_plugin/plugin_install_job_monitor.cc b/chrome/default_plugin/plugin_install_job_monitor.cc
index 4e19057..c969037 100644
--- a/chrome/default_plugin/plugin_install_job_monitor.cc
+++ b/chrome/default_plugin/plugin_install_job_monitor.cc
@@ -1,4 +1,4 @@
-// Copyright (c) 2010 The Chromium Authors. All rights reserved.
+// Copyright (c) 2011 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
@@ -16,6 +16,10 @@ PluginInstallationJobMonitorThread::PluginInstallationJobMonitorThread()
 }
 
 PluginInstallationJobMonitorThread::~PluginInstallationJobMonitorThread() {
+  // The way this class is used, Thread::Stop() has always been called
+  // by the time we reach this point, so we do not need to call it
+  // again.
+  DCHECK(!Thread::IsRunning());
   if (install_job_) {
     ::CloseHandle(install_job_);
     install_job_ = NULL;
diff --git a/chrome/service/service_process.cc b/chrome/service/service_process.cc
index 03c6296..5088334 100644
--- a/chrome/service/service_process.cc
+++ b/chrome/service/service_process.cc
@@ -62,8 +62,6 @@ class ServiceIOThread : public base::Thread {
 
 ServiceIOThread::ServiceIOThread(const char* name) : base::Thread(name) {}
 ServiceIOThread::~ServiceIOThread() {
-  // We cannot rely on our base class to stop the thread since we want our
-  // CleanUp function to run.
   Stop();
 }
 
diff --git a/chrome_frame/test/urlmon_moniker_integration_test.cc b/chrome_frame/test/urlmon_moniker_integration_test.cc
index 99c0357..0812b53 100644
--- a/chrome_frame/test/urlmon_moniker_integration_test.cc
+++ b/chrome_frame/test/urlmon_moniker_integration_test.cc
@@ -59,6 +59,10 @@ class RunTestServer : public base::Thread {
       ready_(::CreateEvent(NULL, TRUE, FALSE, NULL)) {
   }
 
+  ~RunTestServer() {
+    Stop();
+  }
+
   bool Start() {
     bool ret = StartWithOptions(Options(MessageLoop::TYPE_UI, 0));
     if (ret) {
diff --git a/chrome_frame/urlmon_url_request.cc b/chrome_frame/urlmon_url_request.cc
index ea75ccf..221831b 100644
--- a/chrome_frame/urlmon_url_request.cc
+++ b/chrome_frame/urlmon_url_request.cc
@@ -1413,6 +1413,7 @@ UrlmonUrlRequestManager::ResourceFetcherThread::ResourceFetcherThread(
 }
 
 UrlmonUrlRequestManager::ResourceFetcherThread::~ResourceFetcherThread() {
+  Stop();
 }
 
 void UrlmonUrlRequestManager::ResourceFetcherThread::Init() {
diff --git a/chrome_frame/utils.h b/chrome_frame/utils.h
index 95feac6..c76b2a4 100644
--- a/chrome_frame/utils.h
+++ b/chrome_frame/utils.h
@@ -387,6 +387,9 @@ STDMETHODIMP QueryInterfaceIfDelegateSupports(void* obj, REFIID iid,
 class STAThread : public base::Thread {
  public:
   explicit STAThread(const char *name) : Thread(name) {}
+  ~STAThread() {
+    Stop();
+  }
   bool Start() {
     return StartWithOptions(Options(MessageLoop::TYPE_UI, 0));
   }
diff --git a/content/browser/browser_thread_impl.cc b/content/browser/browser_thread_impl.cc
index ef5be1b..4b85b61 100644
--- a/content/browser/browser_thread_impl.cc
+++ b/content/browser/browser_thread_impl.cc
@@ -43,11 +43,6 @@ BrowserThreadImpl::BrowserThreadImpl(BrowserThread::ID identifier,
 }
 
 BrowserThreadImpl::~BrowserThreadImpl() {
-  // Subclasses of base::Thread() (or at least the most-derived
-  // subclass) must call Stop() in their destructor, otherwise the
-  // vtable for the object can change while the thread's message loop
-  // is still running, and it uses the object's vtable (it calls the
-  // virtual method Run).
   Stop();
 }
 
@@ -139,6 +134,7 @@ DeprecatedBrowserThread::DeprecatedBrowserThread(BrowserThread::ID identifier,
     : BrowserThread(identifier, message_loop) {
 }
 DeprecatedBrowserThread::~DeprecatedBrowserThread() {
+  Stop();
 }
 
 // An implementation of MessageLoopProxy to be used in conjunction
diff --git a/content/test/test_browser_thread.cc b/content/test/test_browser_thread.cc
index 002a93f..ca90930 100644
--- a/content/test/test_browser_thread.cc
+++ b/content/test/test_browser_thread.cc
@@ -15,6 +15,7 @@ TestBrowserThread::TestBrowserThread(ID identifier, MessageLoop* message_loop)
 }
 
 TestBrowserThread::~TestBrowserThread() {
+  Stop();
 }
 
 }  // namespace content
diff --git a/dbus/test_service.cc b/dbus/test_service.cc
index ca0eea3..b0207b0 100644
--- a/dbus/test_service.cc
+++ b/dbus/test_service.cc
@@ -30,6 +30,7 @@ TestService::TestService(const Options& options)
 }
 
 TestService::~TestService() {
+  Stop();
 }
 
 bool TestService::StartService() {
diff --git a/net/base/network_change_notifier_linux.cc b/net/base/network_change_notifier_linux.cc
index 77d8043..1e6db14 100644
--- a/net/base/network_change_notifier_linux.cc
+++ b/net/base/network_change_notifier_linux.cc
@@ -105,7 +105,9 @@ NetworkChangeNotifierLinux::Thread::Thread()
       ALLOW_THIS_IN_INITIALIZER_LIST(ptr_factory_(this)) {
 }
 
-NetworkChangeNotifierLinux::Thread::~Thread() {}
+NetworkChangeNotifierLinux::Thread::~Thread() {
+  DCHECK(!Thread::IsRunning());
+}
 
 void NetworkChangeNotifierLinux::Thread::Init() {
   resolv_file_watcher_.reset(new FilePathWatcher);
@@ -214,8 +216,8 @@ NetworkChangeNotifierLinux::NetworkChangeNotifierLinux()
 }
 
 NetworkChangeNotifierLinux::~NetworkChangeNotifierLinux() {
-  // We don't need to explicitly Stop(), but doing so allows us to sanity-
-  // check that the notifier thread shut down properly.
+  // Stopping from here allows us to sanity- check that the notifier
+  // thread shut down properly.
   notifier_thread_->Stop();
 }
 
diff --git a/tools/valgrind/tsan/suppressions.txt b/tools/valgrind/tsan/suppressions.txt
index a74fcec..ac140c1 100644
--- a/tools/valgrind/tsan/suppressions.txt
+++ b/tools/valgrind/tsan/suppressions.txt
@@ -593,18 +593,6 @@
   fun:HostContentSettingsMap::GetDefaultContentSetting
 }
 {
-  bug_102134a
-  ThreadSanitizer:Race
-  fun:content::BrowserThreadImpl::~BrowserThreadImpl
-  fun:content::TestBrowserThread::~TestBrowserThread
-}
-{
-  bug_102134b
-  ThreadSanitizer:Race
-  fun:content::BrowserThread::~BrowserThread
-  fun:content::DeprecatedBrowserThread::~DeprecatedBrowserThread
-}
-{
   bug_102327_a
   ThreadSanitizer:Race
   fun:tracked_objects::ThreadData::Initialize
diff --git a/webkit/appcache/appcache_update_job_unittest.cc b/webkit/appcache/appcache_update_job_unittest.cc
index 7fade2e..0739cda 100644
--- a/webkit/appcache/appcache_update_job_unittest.cc
+++ b/webkit/appcache/appcache_update_job_unittest.cc
@@ -514,8 +514,6 @@ class IOThread : public base::Thread {
   }
 
   ~IOThread() {
-    // We cannot rely on our base class to stop the thread since we want our
-    // CleanUp function to run.
     Stop();
   }
 
diff --git a/webkit/tools/test_shell/simple_resource_loader_bridge.cc b/webkit/tools/test_shell/simple_resource_loader_bridge.cc
index 0b355ac..a3e0f64 100644
--- a/webkit/tools/test_shell/simple_resource_loader_bridge.cc
+++ b/webkit/tools/test_shell/simple_resource_loader_bridge.cc
@@ -123,8 +123,6 @@ class IOThread : public base::Thread {
   }
 
   ~IOThread() {
-    // We cannot rely on our base class to stop the thread since we want our
-    // CleanUp function to run.
     Stop();
   }