diff --git a/base/memory/weak_ptr_unittest.cc b/base/memory/weak_ptr_unittest.cc index c244ec05ab516f..f9f8b3e916a27b 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 d7451ec58ab923..2ee4fa27fdc57e 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 fe3962726cf5cd..68a24cf444a3ce 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 4252580ac987dc..8d4fe59b0885f2 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 19e967bea45a13..1072fb59786822 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 faa4cb94499077..38f51584b8876c 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 4e1905751e0362..c9690371317b1d 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 03c62963613d57..5088334e9245e6 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 99c0357f8d9184..0812b530335a4d 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 ea75ccf1c285e1..221831b7431a7c 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 95feac6637c76e..c76b2a483184dc 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 ef5be1b0c045ce..4b85b616cac113 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 002a93fd3e5429..ca909304c4b53a 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 ca0eea3fa12f45..b0207b0e7e9afb 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 77d80432366fe3..1e6db148bb55d5 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 a74fcec95d39a9..ac140c19415681 100644 --- a/tools/valgrind/tsan/suppressions.txt +++ b/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 diff --git a/webkit/appcache/appcache_update_job_unittest.cc b/webkit/appcache/appcache_update_job_unittest.cc index 7fade2e682ff25..0739cda7d52a92 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 0b355ac0b12aa3..a3e0f64aa7eec1 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(); }