diff options
author | willchan@chromium.org <willchan@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2010-11-19 20:34:18 +0000 |
---|---|---|
committer | willchan@chromium.org <willchan@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2010-11-19 20:34:18 +0000 |
commit | 359d2bf31d2edc43a9ca8b08ee07707038efce09 (patch) | |
tree | 10bf00aa3964c73a37bae063a349bd496010ed4b | |
parent | 1da05ebbe6301142de46ef7cb100b6cc5aaa38c2 (diff) | |
download | chromium_src-359d2bf31d2edc43a9ca8b08ee07707038efce09.zip chromium_src-359d2bf31d2edc43a9ca8b08ee07707038efce09.tar.gz chromium_src-359d2bf31d2edc43a9ca8b08ee07707038efce09.tar.bz2 |
Reland 66791 (change was innocent)
Revert 66719 - Reland r65996. Disallows Singletons on non-joinable thread.
Test breakages caused by this change have been fixed here or in other changelists.
BUG=61753
TEST=none
Review URL: http://codereview.chromium.org/5024003
TBR=willchan@chromium.org
Review URL: http://codereview.chromium.org/5206005
TBR=willchan@chromium.org
Review URL: http://codereview.chromium.org/5242002
git-svn-id: svn://svn.chromium.org/chrome/trunk/src@66808 0039d316-1c4b-4281-b951-d872f2087c98
-rw-r--r-- | base/lazy_instance.h | 11 | ||||
-rw-r--r-- | base/message_loop_proxy_impl.cc | 7 | ||||
-rw-r--r-- | base/platform_thread_posix.cc | 33 | ||||
-rw-r--r-- | base/platform_thread_win.cc | 74 | ||||
-rw-r--r-- | base/singleton.h | 11 | ||||
-rw-r--r-- | base/thread_restrictions.cc | 23 | ||||
-rw-r--r-- | base/thread_restrictions.h | 35 | ||||
-rw-r--r-- | base/tracked_objects.cc | 27 | ||||
-rw-r--r-- | ceee/ie/broker/executors_manager.h | 6 | ||||
-rw-r--r-- | chrome/browser/browser_thread.cc | 11 | ||||
-rw-r--r-- | net/base/dns_reload_timer.cc | 16 | ||||
-rw-r--r-- | net/base/ev_root_ca_metadata.cc | 8 | ||||
-rw-r--r-- | net/base/ev_root_ca_metadata.h | 6 | ||||
-rw-r--r-- | net/base/keygen_handler_unittest.cc | 4 | ||||
-rw-r--r-- | net/base/x509_certificate.cc | 54 | ||||
-rw-r--r-- | net/base/x509_certificate.h | 2 | ||||
-rw-r--r-- | net/base/x509_certificate_mac.cc | 19 | ||||
-rw-r--r-- | net/ocsp/nss_ocsp.cc | 11 |
18 files changed, 258 insertions, 100 deletions
diff --git a/base/lazy_instance.h b/base/lazy_instance.h index ebf1e73..f8d5987 100644 --- a/base/lazy_instance.h +++ b/base/lazy_instance.h @@ -36,14 +36,19 @@ #define BASE_LAZY_INSTANCE_H_ #pragma once +#include <new> // 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 <typename Type> 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 +62,8 @@ struct DefaultLazyInstanceTraits { template <typename Type> struct LeakyLazyInstanceTraits { + static const bool kAllowedToAccessOnNonjoinableThread = true; + static Type* New(void* instance) { return DefaultLazyInstanceTraits<Type>::New(instance); } @@ -112,6 +119,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()) { @@ -126,7 +136,6 @@ class LazyInstance : public LazyInstanceHelper { // and CompleteInstance(...) happens before "return instance_" below. // See the corresponding HAPPENS_BEFORE in CompleteInstance(...). ANNOTATE_HAPPENS_AFTER(&state_); - return instance_; } diff --git a/base/message_loop_proxy_impl.cc b/base/message_loop_proxy_impl.cc index 2e0f809..7998a94 100644 --- a/base/message_loop_proxy_impl.cc +++ b/base/message_loop_proxy_impl.cc @@ -3,6 +3,7 @@ // found in the LICENSE file. #include "base/message_loop_proxy_impl.h" +#include "base/thread_restrictions.h" namespace base { @@ -45,6 +46,11 @@ bool MessageLoopProxyImpl::PostNonNestableDelayedTask( } bool MessageLoopProxyImpl::BelongsToCurrentThread() { + // We shouldn't use MessageLoop::current() since it uses LazyInstance which + // may be deleted by ~AtExitManager when a WorkerPool thread calls this + // function. + // http://crbug.com/63678 + base::ThreadRestrictions::ScopedAllowSingleton allow_singleton; AutoLock lock(message_loop_lock_); return (target_message_loop_ && (MessageLoop::current() == target_message_loop_)); @@ -98,4 +104,3 @@ MessageLoopProxy::CreateForCurrentThread() { } } // namespace base - 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 <errno.h> #include <sched.h> +#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 <mach/mach.h> #include <sys/resource.h> @@ -24,18 +29,27 @@ #include <sys/nacl_syscalls.h> #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<PlatformThread::Delegate*>(closure); +namespace { + +struct ThreadParams { + PlatformThread::Delegate* delegate; + bool joinable; +}; + +} // namespace + +static void* ThreadFunc(void* params) { + ThreadParams* thread_params = static_cast<ThreadParams*>(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..e5afc52 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,13 +21,58 @@ 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<PlatformThread::Delegate*>(closure); +struct ThreadParams { + PlatformThread::Delegate* delegate; + bool joinable; +}; + +DWORD __stdcall ThreadFunc(void* params) { + ThreadParams* thread_params = static_cast<ThreadParams*>(params); + PlatformThread::Delegate* delegate = thread_params->delegate; + if (!thread_params->joinable) + base::ThreadRestrictions::SetSingletonAllowed(false); + delete thread_params; delegate->ThreadMain(); return NULL; } +// CreateThreadInternal() matches PlatformThread::Create(), except that +// |out_thread_handle| may be NULL, in which case a non-joinable thread is +// created. +bool CreateThreadInternal(size_t stack_size, + PlatformThread::Delegate* delegate, + 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; + } else { + 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, 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; +} + } // namespace // static @@ -67,29 +113,13 @@ void PlatformThread::SetName(const char* name) { // static bool PlatformThread::Create(size_t stack_size, Delegate* delegate, 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; - } else { - stack_size = 0; - } - - // 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; + DCHECK(thread_handle); + return CreateThreadInternal(stack_size, delegate, thread_handle); } // 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 CreateThreadInternal(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<Type>. 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<typename Type> struct LeakySingletonTraits : public DefaultSingletonTraits<Type> { 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..6767d80 100644 --- a/base/thread_restrictions.cc +++ b/base/thread_restrictions.cc @@ -18,13 +18,16 @@ namespace { LazyInstance<ThreadLocalBoolean, LeakyLazyInstanceTraits<ThreadLocalBoolean> > g_io_disallowed(LINKER_INITIALIZED); +LazyInstance<ThreadLocalBoolean, LeakyLazyInstanceTraits<ThreadLocalBoolean> > + g_singleton_disallowed(LINKER_INITIALIZED); + } // anonymous namespace // static bool ThreadRestrictions::SetIOAllowed(bool allowed) { - bool previous_allowed = g_io_disallowed.Get().Get(); + bool previous_disallowed = g_io_disallowed.Get().Get(); g_io_disallowed.Get().Set(!allowed); - return !previous_allowed; + return !previous_disallowed; } // static @@ -39,6 +42,22 @@ void ThreadRestrictions::AssertIOAllowed() { } } +bool ThreadRestrictions::SetSingletonAllowed(bool allowed) { + bool previous_disallowed = g_singleton_disallowed.Get().Get(); + g_singleton_disallowed.Get().Set(!allowed); + return !previous_disallowed; +} + +// 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..51e5a15 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 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()) { diff --git a/ceee/ie/broker/executors_manager.h b/ceee/ie/broker/executors_manager.h index 3b9ac3b..cba7b3f 100644 --- a/ceee/ie/broker/executors_manager.h +++ b/ceee/ie/broker/executors_manager.h @@ -90,14 +90,10 @@ class ExecutorsManager { // Traits for Singleton<ExecutorsManager> so that we can pass an argument // to the constructor. - struct SingletonTraits { + struct SingletonTraits : public DefaultSingletonTraits<ExecutorsManager> { static ExecutorsManager* New() { return new ExecutorsManager(false); // By default, we want a thread. } - static void Delete(ExecutorsManager* x) { - delete x; - } - static const bool kRegisterAtExit = true; }; protected: diff --git a/chrome/browser/browser_thread.cc b/chrome/browser/browser_thread.cc index 7ba53e0..e6edfd0 100644 --- a/chrome/browser/browser_thread.cc +++ b/chrome/browser/browser_thread.cc @@ -6,6 +6,7 @@ #include "base/message_loop.h" #include "base/message_loop_proxy.h" +#include "base/thread_restrictions.h" // Friendly names for the well-known threads. static const char* browser_thread_names[BrowserThread::ID_COUNT] = { @@ -112,6 +113,11 @@ bool BrowserThread::IsWellKnownThread(ID identifier) { // static bool BrowserThread::CurrentlyOn(ID identifier) { + // We shouldn't use MessageLoop::current() since it uses LazyInstance which + // may be deleted by ~AtExitManager when a WorkerPool thread calls this + // function. + // http://crbug.com/63678 + base::ThreadRestrictions::ScopedAllowSingleton allow_singleton; AutoLock lock(lock_); DCHECK(identifier >= 0 && identifier < ID_COUNT); return browser_threads_[identifier] && @@ -160,6 +166,11 @@ bool BrowserThread::PostNonNestableDelayedTask( // static bool BrowserThread::GetCurrentThreadIdentifier(ID* identifier) { + // We shouldn't use MessageLoop::current() since it uses LazyInstance which + // may be deleted by ~AtExitManager when a WorkerPool thread calls this + // function. + // http://crbug.com/63678 + base::ThreadRestrictions::ScopedAllowSingleton allow_singleton; MessageLoop* cur_message_loop = MessageLoop::current(); for (int i = 0; i < ID_COUNT; ++i) { if (browser_threads_[i] && diff --git a/net/base/dns_reload_timer.cc b/net/base/dns_reload_timer.cc index 5931c5b..1bfe535 100644 --- a/net/base/dns_reload_timer.cc +++ b/net/base/dns_reload_timer.cc @@ -5,11 +5,11 @@ #include "net/base/dns_reload_timer.h" #if defined(OS_POSIX) && !defined(OS_MACOSX) && !defined(OS_OPENBSD) -#include "base/singleton.h" +#include "base/lazy_instance.h" #include "base/thread_local_storage.h" #include "base/time.h" -namespace net { +namespace { // On Linux/BSD, changes to /etc/resolv.conf can go unnoticed thus resulting // in DNS queries failing either because nameservers are unknown on startup @@ -58,7 +58,7 @@ class DnsReloadTimer { } private: - friend struct DefaultSingletonTraits<DnsReloadTimer>; + friend struct base::DefaultLazyInstanceTraits<DnsReloadTimer>; DnsReloadTimer() { // During testing the DnsReloadTimer Singleton may be created and destroyed @@ -81,8 +81,16 @@ class DnsReloadTimer { // static ThreadLocalStorage::Slot DnsReloadTimer::tls_index_(base::LINKER_INITIALIZED); +base::LazyInstance<DnsReloadTimer, + base::LeakyLazyInstanceTraits<DnsReloadTimer> > + g_dns_reload_timer(base::LINKER_INITIALIZED); + +} // namespace + +namespace net { + bool DnsReloadTimerHasExpired() { - DnsReloadTimer* dns_timer = Singleton<DnsReloadTimer>::get(); + DnsReloadTimer* dns_timer = g_dns_reload_timer.Pointer(); return dns_timer->Expired(); } diff --git a/net/base/ev_root_ca_metadata.cc b/net/base/ev_root_ca_metadata.cc index 661b652..a721357 100644 --- a/net/base/ev_root_ca_metadata.cc +++ b/net/base/ev_root_ca_metadata.cc @@ -13,8 +13,8 @@ #include <stdlib.h> #endif +#include "base/lazy_instance.h" #include "base/logging.h" -#include "base/singleton.h" namespace net { @@ -283,9 +283,13 @@ const EVRootCAMetadata::PolicyOID EVRootCAMetadata::policy_oids_[] = { }; #endif +static base::LazyInstance<EVRootCAMetadata, + base::LeakyLazyInstanceTraits<EVRootCAMetadata> > + g_ev_root_ca_metadata(base::LINKER_INITIALIZED); + // static EVRootCAMetadata* EVRootCAMetadata::GetInstance() { - return Singleton<EVRootCAMetadata>::get(); + return g_ev_root_ca_metadata.Pointer(); } bool EVRootCAMetadata::GetPolicyOID( diff --git a/net/base/ev_root_ca_metadata.h b/net/base/ev_root_ca_metadata.h index e0961f3..832ebe2 100644 --- a/net/base/ev_root_ca_metadata.h +++ b/net/base/ev_root_ca_metadata.h @@ -17,8 +17,10 @@ #include "net/base/x509_certificate.h" +namespace base { template <typename T> -struct DefaultSingletonTraits; +struct DefaultLazyInstanceTraits; +} // namespace base namespace net { @@ -55,7 +57,7 @@ class EVRootCAMetadata { PolicyOID policy_oid) const; private: - friend struct DefaultSingletonTraits<EVRootCAMetadata>; + friend struct base::DefaultLazyInstanceTraits<EVRootCAMetadata>; typedef std::map<SHA1Fingerprint, PolicyOID, SHA1FingerprintLessThan> PolicyOidMap; diff --git a/net/base/keygen_handler_unittest.cc b/net/base/keygen_handler_unittest.cc index 62c5191..d3bf4f5 100644 --- a/net/base/keygen_handler_unittest.cc +++ b/net/base/keygen_handler_unittest.cc @@ -16,6 +16,7 @@ #include "base/logging.h" #include "base/nss_util.h" #include "base/task.h" +#include "base/thread_restrictions.h" #include "base/waitable_event.h" #include "base/worker_pool.h" #include "testing/gtest/include/gtest/gtest.h" @@ -90,6 +91,9 @@ class ConcurrencyTestTask : public Task { } virtual void Run() { + // We allow Singleton use on the worker thread here since we use a + // WaitableEvent to synchronize, so it's safe. + base::ThreadRestrictions::ScopedAllowSingleton scoped_allow_singleton; KeygenHandler handler(768, "some challenge", GURL("http://www.example.com")); handler.set_stores_key(false); // Don't leave the key-pair behind. diff --git a/net/base/x509_certificate.cc b/net/base/x509_certificate.cc index d93d270..310defb 100644 --- a/net/base/x509_certificate.cc +++ b/net/base/x509_certificate.cc @@ -6,9 +6,9 @@ #include <map> +#include "base/lazy_instance.h" #include "base/logging.h" #include "base/metrics/histogram.h" -#include "base/singleton.h" #include "base/string_piece.h" #include "base/time.h" #include "net/base/pem_tokenizer.h" @@ -39,17 +39,6 @@ const char kCertificateHeader[] = "CERTIFICATE"; // The PEM block header used for PKCS#7 data const char kPKCS7Header[] = "PKCS7"; -} // namespace - -bool X509Certificate::LessThan::operator()(X509Certificate* lhs, - X509Certificate* rhs) const { - if (lhs == rhs) - return false; - - SHA1FingerprintLessThan fingerprint_functor; - return fingerprint_functor(lhs->fingerprint_, rhs->fingerprint_); -} - // A thread-safe cache for X509Certificate objects. // // The cache does not hold a reference to the certificate objects. The objects @@ -57,9 +46,8 @@ bool X509Certificate::LessThan::operator()(X509Certificate* lhs, // will be holding dead pointers to the objects). // TODO(rsleevi): There exists a chance of a use-after-free, due to a race // between Find() and Remove(). See http://crbug.com/49377 -class X509Certificate::Cache { +class X509CertificateCache { public: - static Cache* GetInstance(); void Insert(X509Certificate* cert); void Remove(X509Certificate* cert); X509Certificate* Find(const SHA1Fingerprint& fingerprint); @@ -68,9 +56,10 @@ class X509Certificate::Cache { typedef std::map<SHA1Fingerprint, X509Certificate*, SHA1FingerprintLessThan> CertMap; - // Obtain an instance of X509Certificate::Cache via GetInstance(). - Cache() {} - friend struct DefaultSingletonTraits<Cache>; + // Obtain an instance of X509CertificateCache via a LazyInstance. + X509CertificateCache() {} + ~X509CertificateCache() {} + friend struct base::DefaultLazyInstanceTraits<X509CertificateCache>; // You must acquire this lock before using any private data of this object. // You must not block while holding this lock. @@ -79,18 +68,16 @@ class X509Certificate::Cache { // The certificate cache. You must acquire |lock_| before using |cache_|. CertMap cache_; - DISALLOW_COPY_AND_ASSIGN(Cache); + DISALLOW_COPY_AND_ASSIGN(X509CertificateCache); }; -// Get the singleton object for the cache. -// static -X509Certificate::Cache* X509Certificate::Cache::GetInstance() { - return Singleton<X509Certificate::Cache>::get(); -} +base::LazyInstance<X509CertificateCache, + base::LeakyLazyInstanceTraits<X509CertificateCache> > + g_x509_certificate_cache(base::LINKER_INITIALIZED); // Insert |cert| into the cache. The cache does NOT AddRef |cert|. // Any existing certificate with the same fingerprint will be replaced. -void X509Certificate::Cache::Insert(X509Certificate* cert) { +void X509CertificateCache::Insert(X509Certificate* cert) { AutoLock lock(lock_); DCHECK(!IsNullFingerprint(cert->fingerprint())) << @@ -100,7 +87,7 @@ void X509Certificate::Cache::Insert(X509Certificate* cert) { // Remove |cert| from the cache. The cache does not assume that |cert| is // already in the cache. -void X509Certificate::Cache::Remove(X509Certificate* cert) { +void X509CertificateCache::Remove(X509Certificate* cert) { AutoLock lock(lock_); CertMap::iterator pos(cache_.find(cert->fingerprint())); @@ -111,7 +98,7 @@ void X509Certificate::Cache::Remove(X509Certificate* cert) { // Find a certificate in the cache with the given fingerprint. If one does // not exist, this method returns NULL. -X509Certificate* X509Certificate::Cache::Find( +X509Certificate* X509CertificateCache::Find( const SHA1Fingerprint& fingerprint) { AutoLock lock(lock_); @@ -122,6 +109,17 @@ X509Certificate* X509Certificate::Cache::Find( return pos->second; }; +} // namespace + +bool X509Certificate::LessThan::operator()(X509Certificate* lhs, + X509Certificate* rhs) const { + if (lhs == rhs) + return false; + + SHA1FingerprintLessThan fingerprint_functor; + return fingerprint_functor(lhs->fingerprint_, rhs->fingerprint_); +} + // static X509Certificate* X509Certificate::CreateFromHandle( OSCertHandle cert_handle, @@ -131,7 +129,7 @@ X509Certificate* X509Certificate::CreateFromHandle( DCHECK(source != SOURCE_UNUSED); // Check if we already have this certificate in memory. - X509Certificate::Cache* cache = X509Certificate::Cache::GetInstance(); + X509CertificateCache* cache = g_x509_certificate_cache.Pointer(); X509Certificate* cached_cert = cache->Find(CalculateFingerprint(cert_handle)); if (cached_cert) { @@ -311,7 +309,7 @@ X509Certificate::X509Certificate(const std::string& subject, X509Certificate::~X509Certificate() { // We might not be in the cache, but it is safe to remove ourselves anyway. - X509Certificate::Cache::GetInstance()->Remove(this); + g_x509_certificate_cache.Get().Remove(this); if (cert_handle_) FreeOSCertHandle(cert_handle_); for (size_t i = 0; i < intermediate_ca_certs_.size(); ++i) diff --git a/net/base/x509_certificate.h b/net/base/x509_certificate.h index 1866a17..9f44952 100644 --- a/net/base/x509_certificate.h +++ b/net/base/x509_certificate.h @@ -284,8 +284,6 @@ class X509Certificate : public base::RefCountedThreadSafe<X509Certificate> { FRIEND_TEST_ALL_PREFIXES(X509CertificateTest, Cache); FRIEND_TEST_ALL_PREFIXES(X509CertificateTest, IntermediateCertificates); - class Cache; - // Construct an X509Certificate from a handle to the certificate object // in the underlying crypto library. X509Certificate(OSCertHandle cert_handle, Source source, diff --git a/net/base/x509_certificate_mac.cc b/net/base/x509_certificate_mac.cc index a2a0eea..2a604ee 100644 --- a/net/base/x509_certificate_mac.cc +++ b/net/base/x509_certificate_mac.cc @@ -8,6 +8,7 @@ #include <Security/Security.h> #include <time.h> +#include "base/lazy_instance.h" #include "base/logging.h" #include "base/pickle.h" #include "base/mac/scoped_cftyperef.h" @@ -21,6 +22,8 @@ using base::Time; namespace net { +namespace { + class MacTrustedCertificates { public: // Sets the trusted root certificate used by tests. Call with |cert| set @@ -57,7 +60,7 @@ class MacTrustedCertificates { return merged_array; } private: - friend struct DefaultSingletonTraits<MacTrustedCertificates>; + friend struct base::DefaultLazyInstanceTraits<MacTrustedCertificates>; // Obtain an instance of MacTrustedCertificates via the singleton // interface. @@ -73,11 +76,9 @@ class MacTrustedCertificates { DISALLOW_COPY_AND_ASSIGN(MacTrustedCertificates); }; -void SetMacTestCertificate(X509Certificate* cert) { - Singleton<MacTrustedCertificates>::get()->SetTestCertificate(cert); -} - -namespace { +base::LazyInstance<MacTrustedCertificates, + base::LeakyLazyInstanceTraits<MacTrustedCertificates> > + g_mac_trusted_certificates(base::LINKER_INITIALIZED); typedef OSStatus (*SecTrustCopyExtendedResultFuncPtr)(SecTrustRef, CFDictionaryRef*); @@ -443,6 +444,10 @@ void AddCertificatesFromBytes(const char* data, size_t length, } // namespace +void SetMacTestCertificate(X509Certificate* cert) { + g_mac_trusted_certificates.Get().SetTestCertificate(cert); +} + void X509Certificate::Initialize() { const CSSM_X509_NAME* name; OSStatus status = SecCertificateGetSubject(cert_handle_, &name); @@ -545,7 +550,7 @@ int X509Certificate::Verify(const std::string& hostname, int flags, // Set the trusted anchor certificates for the SecTrustRef by merging the // system trust anchors and the test root certificate. CFArrayRef anchor_array = - Singleton<MacTrustedCertificates>::get()->CopyTrustedCertificateArray(); + g_mac_trusted_certificates.Get().CopyTrustedCertificateArray(); ScopedCFTypeRef<CFArrayRef> scoped_anchor_array(anchor_array); if (anchor_array) { status = SecTrustSetAnchorCertificates(trust_ref, anchor_array); diff --git a/net/ocsp/nss_ocsp.cc b/net/ocsp/nss_ocsp.cc index fafaa68..7618f9e 100644 --- a/net/ocsp/nss_ocsp.cc +++ b/net/ocsp/nss_ocsp.cc @@ -85,7 +85,8 @@ class OCSPIOLoop { DISALLOW_COPY_AND_ASSIGN(OCSPIOLoop); }; -base::LazyInstance<OCSPIOLoop> g_ocsp_io_loop(base::LINKER_INITIALIZED); +base::LazyInstance<OCSPIOLoop, base::LeakyLazyInstanceTraits<OCSPIOLoop> > + g_ocsp_io_loop(base::LINKER_INITIALIZED); const int kRecvBufferSize = 4096; @@ -560,7 +561,6 @@ OCSPNSSInitialization::~OCSPNSSInitialization() {} SECStatus OCSPCreateSession(const char* host, PRUint16 portnum, SEC_HTTP_SERVER_SESSION* pSession) { VLOG(1) << "OCSP create session: host=" << host << " port=" << portnum; - DCHECK(!MessageLoop::current()); pthread_mutex_lock(&g_request_context_lock); URLRequestContext* request_context = g_request_context; pthread_mutex_unlock(&g_request_context_lock); @@ -579,7 +579,6 @@ SECStatus OCSPCreateSession(const char* host, PRUint16 portnum, SECStatus OCSPKeepAliveSession(SEC_HTTP_SERVER_SESSION session, PRPollDesc **pPollDesc) { VLOG(1) << "OCSP keep alive"; - DCHECK(!MessageLoop::current()); if (pPollDesc) *pPollDesc = NULL; return SECSuccess; @@ -587,7 +586,6 @@ SECStatus OCSPKeepAliveSession(SEC_HTTP_SERVER_SESSION session, SECStatus OCSPFreeSession(SEC_HTTP_SERVER_SESSION session) { VLOG(1) << "OCSP free session"; - DCHECK(!MessageLoop::current()); delete reinterpret_cast<OCSPServerSession*>(session); return SECSuccess; } @@ -602,7 +600,6 @@ SECStatus OCSPCreate(SEC_HTTP_SERVER_SESSION session, << " path_and_query=" << path_and_query_string << " http_request_method=" << http_request_method << " timeout=" << timeout; - DCHECK(!MessageLoop::current()); OCSPServerSession* ocsp_session = reinterpret_cast<OCSPServerSession*>(session); @@ -624,7 +621,6 @@ SECStatus OCSPSetPostData(SEC_HTTP_REQUEST_SESSION request, const PRUint32 http_data_len, const char* http_content_type) { VLOG(1) << "OCSP set post data len=" << http_data_len; - DCHECK(!MessageLoop::current()); OCSPRequestSession* req = reinterpret_cast<OCSPRequestSession*>(request); req->SetPostData(http_data, http_data_len, http_content_type); @@ -636,7 +632,6 @@ SECStatus OCSPAddHeader(SEC_HTTP_REQUEST_SESSION request, const char* http_header_value) { VLOG(1) << "OCSP add header name=" << http_header_name << " value=" << http_header_value; - DCHECK(!MessageLoop::current()); OCSPRequestSession* req = reinterpret_cast<OCSPRequestSession*>(request); req->AddHeader(http_header_name, http_header_value); @@ -696,7 +691,6 @@ SECStatus OCSPTrySendAndReceive(SEC_HTTP_REQUEST_SESSION request, } VLOG(1) << "OCSP try send and receive"; - DCHECK(!MessageLoop::current()); OCSPRequestSession* req = reinterpret_cast<OCSPRequestSession*>(request); // We support blocking mode only. if (pPollDesc) @@ -774,7 +768,6 @@ SECStatus OCSPTrySendAndReceive(SEC_HTTP_REQUEST_SESSION request, SECStatus OCSPFree(SEC_HTTP_REQUEST_SESSION request) { VLOG(1) << "OCSP free"; - DCHECK(!MessageLoop::current()); OCSPRequestSession* req = reinterpret_cast<OCSPRequestSession*>(request); req->Cancel(); req->Release(); |