Thread::Stop() must be called before any subclass's destructor comple…

…tes.

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

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(

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;

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();
   }
 

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();
 }
 

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) {

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() {

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;

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();
 }
 

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) {

chrome_frame/urlmon_url_request.cc

@@ -1413,6 +1413,7 @@ UrlmonUrlRequestManager::ResourceFetcherThread::ResourceFetcherThread(
 }
 
 UrlmonUrlRequestManager::ResourceFetcherThread::~ResourceFetcherThread() {
+  Stop();
 }
 
 void UrlmonUrlRequestManager::ResourceFetcherThread::Init() {

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));
   }

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

content/test/test_browser_thread.cc

@@ -15,6 +15,7 @@ TestBrowserThread::TestBrowserThread(ID identifier, MessageLoop* message_loop)
 }
 
 TestBrowserThread::~TestBrowserThread() {
+  Stop();
 }
 
 }  // namespace content

dbus/test_service.cc

@@ -30,6 +30,7 @@ TestService::TestService(const Options& options)
 }
 
 TestService::~TestService() {
+  Stop();
 }
 
 bool TestService::StartService() {

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();
 }
 

tools/valgrind/tsan/suppressions.txt

@@ -592,18 +592,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

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();
   }
 

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();
   }