From 82e8e2b5b879cd414e48e6418fb1a693093866e2 Mon Sep 17 00:00:00 2001 From: "willchan@chromium.org" Date: Fri, 12 Nov 2010 21:25:38 +0000 Subject: Disallow Singleton and LazyInstance on non-joinable threads. Fix all known instances or explicitly allow them. Usually the fix involves switching from Default traits to Lazy traits. BUG=61753 TEST=none Review URL: http://codereview.chromium.org/4635012 git-svn-id: svn://svn.chromium.org/chrome/trunk/src@65996 0039d316-1c4b-4281-b951-d872f2087c98 --- base/lazy_instance.h | 9 +++++++++ base/platform_thread_posix.cc | 33 ++++++++++++++++++++++++++------- base/platform_thread_win.cc | 42 +++++++++++++++++++++++++++++++----------- base/singleton.h | 11 +++++++++++ base/thread_restrictions.cc | 18 ++++++++++++++++++ base/thread_restrictions.h | 35 ++++++++++++++++++++++++++++++++--- base/tracked_objects.cc | 27 ++++++++++++++++++++++----- 7 files changed, 149 insertions(+), 26 deletions(-) (limited to 'base') diff --git a/base/lazy_instance.h b/base/lazy_instance.h index ebf1e73..27cb784 100644 --- a/base/lazy_instance.h +++ b/base/lazy_instance.h @@ -36,14 +36,18 @@ #define BASE_LAZY_INSTANCE_H_ #pragma once +#include // For placement new. #include "base/atomicops.h" #include "base/basictypes.h" #include "base/third_party/dynamic_annotations/dynamic_annotations.h" +#include "base/thread_restrictions.h" namespace base { template struct DefaultLazyInstanceTraits { + static const bool kAllowedToAccessOnNonjoinableThread = false; + static Type* New(void* instance) { // Use placement new to initialize our instance in our preallocated space. // The parenthesis is very important here to force POD type initialization. @@ -57,6 +61,8 @@ struct DefaultLazyInstanceTraits { template struct LeakyLazyInstanceTraits { + static const bool kAllowedToAccessOnNonjoinableThread = true; + static Type* New(void* instance) { return DefaultLazyInstanceTraits::New(instance); } @@ -112,6 +118,9 @@ class LazyInstance : public LazyInstanceHelper { } Type* Pointer() { + if (!Traits::kAllowedToAccessOnNonjoinableThread) + base::ThreadRestrictions::AssertSingletonAllowed(); + // We will hopefully have fast access when the instance is already created. if ((base::subtle::NoBarrier_Load(&state_) != STATE_CREATED) && NeedsInstance()) { diff --git a/base/platform_thread_posix.cc b/base/platform_thread_posix.cc index 66f3928..e442e9c 100644 --- a/base/platform_thread_posix.cc +++ b/base/platform_thread_posix.cc @@ -7,6 +7,11 @@ #include #include +#include "base/logging.h" +#include "base/safe_strerror_posix.h" +#include "base/scoped_ptr.h" +#include "base/thread_restrictions.h" + #if defined(OS_MACOSX) #include #include @@ -24,18 +29,27 @@ #include #endif -#include "base/logging.h" -#include "base/safe_strerror_posix.h" - #if defined(OS_MACOSX) namespace base { void InitThreading(); } // namespace base #endif -static void* ThreadFunc(void* closure) { - PlatformThread::Delegate* delegate = - static_cast(closure); +namespace { + +struct ThreadParams { + PlatformThread::Delegate* delegate; + bool joinable; +}; + +} // namespace + +static void* ThreadFunc(void* params) { + ThreadParams* thread_params = static_cast(params); + PlatformThread::Delegate* delegate = thread_params->delegate; + if (!thread_params->joinable) + base::ThreadRestrictions::SetSingletonAllowed(false); + delete thread_params; delegate->ThreadMain(); return NULL; } @@ -174,9 +188,14 @@ bool CreateThread(size_t stack_size, bool joinable, if (stack_size > 0) pthread_attr_setstacksize(&attributes, stack_size); - success = !pthread_create(thread_handle, &attributes, ThreadFunc, delegate); + ThreadParams* params = new ThreadParams; + params->delegate = delegate; + params->joinable = joinable; + success = !pthread_create(thread_handle, &attributes, ThreadFunc, params); pthread_attr_destroy(&attributes); + if (!success) + delete params; return success; } diff --git a/base/platform_thread_win.cc b/base/platform_thread_win.cc index a4a1b52..ce865da 100644 --- a/base/platform_thread_win.cc +++ b/base/platform_thread_win.cc @@ -5,6 +5,7 @@ #include "base/platform_thread.h" #include "base/logging.h" +#include "base/thread_restrictions.h" #include "base/win/windows_version.h" namespace { @@ -20,9 +21,17 @@ typedef struct tagTHREADNAME_INFO { DWORD dwFlags; // Reserved for future use, must be zero. } THREADNAME_INFO; -DWORD __stdcall ThreadFunc(void* closure) { - PlatformThread::Delegate* delegate = - static_cast(closure); +struct ThreadParams { + PlatformThread::Delegate* delegate; + bool joinable; +}; + +DWORD __stdcall ThreadFunc(void* params) { + ThreadParams* thread_params = static_cast(params); + PlatformThread::Delegate* delegate = thread_params->delegate; + if (!thread_params->joinable) + base::ThreadRestrictions::SetSingletonAllowed(false); + delete thread_params; delegate->ThreadMain(); return NULL; } @@ -66,7 +75,8 @@ void PlatformThread::SetName(const char* name) { // static bool PlatformThread::Create(size_t stack_size, Delegate* delegate, - PlatformThreadHandle* thread_handle) { + PlatformThreadHandle* out_thread_handle) { + PlatformThreadHandle thread_handle; unsigned int flags = 0; if (stack_size > 0 && base::win::GetVersion() >= base::win::VERSION_XP) { flags = STACK_SIZE_PARAM_IS_A_RESERVATION; @@ -74,22 +84,32 @@ bool PlatformThread::Create(size_t stack_size, Delegate* delegate, stack_size = 0; } + ThreadParams* params = new ThreadParams; + params->delegate = delegate; + params->joinable = out_thread_handle != NULL; + // Using CreateThread here vs _beginthreadex makes thread creation a bit // faster and doesn't require the loader lock to be available. Our code will // have to work running on CreateThread() threads anyway, since we run code // on the Windows thread pool, etc. For some background on the difference: // http://www.microsoft.com/msj/1099/win32/win321099.aspx - *thread_handle = CreateThread( - NULL, stack_size, ThreadFunc, delegate, flags, NULL); - return *thread_handle != NULL; + thread_handle = CreateThread( + NULL, stack_size, ThreadFunc, params, flags, NULL); + if (!thread_handle) { + delete params; + return false; + } + + if (out_thread_handle) + *out_thread_handle = thread_handle; + else + CloseHandle(thread_handle); + return true; } // static bool PlatformThread::CreateNonJoinable(size_t stack_size, Delegate* delegate) { - PlatformThreadHandle thread_handle; - bool result = Create(stack_size, delegate, &thread_handle); - CloseHandle(thread_handle); - return result; + return Create(stack_size, delegate, NULL); } // static diff --git a/base/singleton.h b/base/singleton.h index 564b686..7caa8ee 100644 --- a/base/singleton.h +++ b/base/singleton.h @@ -9,6 +9,7 @@ #include "base/at_exit.h" #include "base/atomicops.h" #include "base/third_party/dynamic_annotations/dynamic_annotations.h" +#include "base/thread_restrictions.h" // Default traits for Singleton. Calls operator new and operator delete on // the object. Registers automatic deletion at process exit. @@ -30,6 +31,11 @@ struct DefaultSingletonTraits { // Set to true to automatically register deletion of the object on process // exit. See below for the required call that makes this happen. static const bool kRegisterAtExit = true; + + // Set to false to disallow access on a non-joinable thread. This is + // different from kRegisterAtExit because StaticMemorySingletonTraits allows + // access on non-joinable threads, and gracefully handles this. + static const bool kAllowedToAccessOnNonjoinableThread = false; }; @@ -39,6 +45,7 @@ struct DefaultSingletonTraits { template struct LeakySingletonTraits : public DefaultSingletonTraits { static const bool kRegisterAtExit = false; + static const bool kAllowedToAccessOnNonjoinableThread = true; }; @@ -85,6 +92,7 @@ struct StaticMemorySingletonTraits { } static const bool kRegisterAtExit = true; + static const bool kAllowedToAccessOnNonjoinableThread = true; // Exposed for unittesting. static void Resurrect() { @@ -176,6 +184,9 @@ class Singleton { // Return a pointer to the one true instance of the class. static Type* get() { + if (!Traits::kAllowedToAccessOnNonjoinableThread) + base::ThreadRestrictions::AssertSingletonAllowed(); + // Our AtomicWord doubles as a spinlock, where a value of // kBeingCreatedMarker means the spinlock is being held for creation. static const base::subtle::AtomicWord kBeingCreatedMarker = 1; diff --git a/base/thread_restrictions.cc b/base/thread_restrictions.cc index 270d663..1f5794a 100644 --- a/base/thread_restrictions.cc +++ b/base/thread_restrictions.cc @@ -18,6 +18,9 @@ namespace { LazyInstance > g_io_disallowed(LINKER_INITIALIZED); +LazyInstance > + g_singleton_disallowed(LINKER_INITIALIZED); + } // anonymous namespace // static @@ -39,6 +42,21 @@ void ThreadRestrictions::AssertIOAllowed() { } } +bool ThreadRestrictions::SetSingletonAllowed(bool allowed) { + bool previous_allowed = g_singleton_disallowed.Get().Get(); + g_singleton_disallowed.Get().Set(!allowed); + return !previous_allowed; +} + +// static +void ThreadRestrictions::AssertSingletonAllowed() { + if (g_singleton_disallowed.Get().Get()) + LOG(FATAL) << "LazyInstance/Singleton is not allowed to be used on this " + << "thread. Most likely it's because this thread is not " + << "joinable, so AtExitManager may have deleted the object " + << "on shutdown, leading to a potential shutdown crash."; +} + } // namespace base #endif // NDEBUG diff --git a/base/thread_restrictions.h b/base/thread_restrictions.h index a10aaec..2d73927 100644 --- a/base/thread_restrictions.h +++ b/base/thread_restrictions.h @@ -9,8 +9,13 @@ namespace base { -// ThreadRestrictions helps protect threads that should not block from -// making blocking calls. It works like this: +// Certain behavior is disallowed on certain threads. ThreadRestrictions helps +// enforce these rules. Examples of such rules: +// +// * Do not do blocking IO (makes the thread janky) +// * Do not access Singleton/LazyInstance (may lead to shutdown crashes) +// +// Here's more about how the IO protection works: // // 1) If a thread should not be allowed to make IO calls, mark it: // base::ThreadRestrictions::SetIOAllowed(false); @@ -45,6 +50,20 @@ class ThreadRestrictions { DISALLOW_COPY_AND_ASSIGN(ScopedAllowIO); }; + // Constructing a ScopedAllowSingleton temporarily allows accessing for the + // current thread. Doing this is almost always incorrect. + class ScopedAllowSingleton { + public: + ScopedAllowSingleton() { previous_value_ = SetSingletonAllowed(true); } + ~ScopedAllowSingleton() { SetSingletonAllowed(previous_value_); } + private: + // Whether singleton use is allowed when the ScopedAllowSingleton was + // constructed. + bool previous_value_; + + DISALLOW_COPY_AND_ASSIGN(ScopedAllowSingleton); + }; + #ifndef NDEBUG // Set whether the current thread to make IO calls. // Threads start out in the *allowed* state. @@ -55,15 +74,25 @@ class ThreadRestrictions { // and DCHECK if not. See the block comment above the class for // a discussion of where to add these checks. static void AssertIOAllowed(); + + // Set whether the current thread can use singletons. Returns the previous + // value. + static bool SetSingletonAllowed(bool allowed); + + // Check whether the current thread is allowed to use singletons (Singleton / + // LazyInstance). DCHECKs if not. + static void AssertSingletonAllowed(); #else // In Release builds, inline the empty definitions of these functions so // that they can be compiled out. static bool SetIOAllowed(bool allowed) { return true; } static void AssertIOAllowed() {} + static bool SetSingletonAllowed(bool allowed) { return true; } + static void AssertSingletonAllowed() {} #endif private: - ThreadRestrictions(); // class for namespacing only + DISALLOW_IMPLICIT_CONSTRUCTORS(ThreadRestrictions); }; } // namespace base diff --git a/base/tracked_objects.cc b/base/tracked_objects.cc index 4631f34..9db25ff 100644 --- a/base/tracked_objects.cc +++ b/base/tracked_objects.cc @@ -10,6 +10,7 @@ #include "base/message_loop.h" #include "base/string_util.h" #include "base/stringprintf.h" +#include "base/thread_restrictions.h" using base::TimeDelta; @@ -89,7 +90,13 @@ Lock ThreadData::list_lock_; // static ThreadData::Status ThreadData::status_ = ThreadData::UNINITIALIZED; -ThreadData::ThreadData() : next_(NULL), message_loop_(MessageLoop::current()) {} +ThreadData::ThreadData() : next_(NULL) { + // This shouldn't use the MessageLoop::current() LazyInstance since this might + // be used on a non-joinable thread. + // http://crbug.com/62728 + base::ThreadRestrictions::ScopedAllowSingleton scoped_allow_singleton; + message_loop_ = MessageLoop::current(); +} ThreadData::~ThreadData() {} @@ -260,8 +267,14 @@ void ThreadData::WriteHTMLTotalAndSubtotals( } Births* ThreadData::TallyABirth(const Location& location) { - if (!message_loop_) // In case message loop wasn't yet around... - message_loop_ = MessageLoop::current(); // Find it now. + { + // This shouldn't use the MessageLoop::current() LazyInstance since this + // might be used on a non-joinable thread. + // http://crbug.com/62728 + base::ThreadRestrictions::ScopedAllowSingleton scoped_allow_singleton; + if (!message_loop_) // In case message loop wasn't yet around... + message_loop_ = MessageLoop::current(); // Find it now. + } BirthMap::iterator it = birth_map_.find(location); if (it != birth_map_.end()) { @@ -279,8 +292,12 @@ Births* ThreadData::TallyABirth(const Location& location) { void ThreadData::TallyADeath(const Births& lifetimes, const TimeDelta& duration) { - if (!message_loop_) // In case message loop wasn't yet around... - message_loop_ = MessageLoop::current(); // Find it now. + { + // http://crbug.com/62728 + base::ThreadRestrictions::ScopedAllowSingleton scoped_allow_singleton; + if (!message_loop_) // In case message loop wasn't yet around... + message_loop_ = MessageLoop::current(); // Find it now. + } DeathMap::iterator it = death_map_.find(&lifetimes); if (it != death_map_.end()) { -- cgit v1.1