[PartitionAlloc] Don't use a "static local" for ThreadCacheRegistry.

ThreadCacheRegistry is a singleton, using the common static local
pattern. However this doesn't work for PartitionAlloc-Everywhere builds
on Windows, as static locals are not safe very early on, and this one is
called during CRT initialization.

To avoid using a static local here, use a regular global variable in the
.cc. This means that the object must be constexpr-constructible, so
this CL:
- Moves to PartitionLock vs base::Lock
- Makes PartitionLock's constructor constexpr

Bug: 998048
Change-Id: Ie16c642122696d34d9f07813a763ca986b25895c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2474857
Commit-Queue: Benoit L <lizeb@chromium.org>
Reviewed-by: Yuki Shiino <yukishiino@chromium.org>
Reviewed-by: Bartek Nowierski <bartekn@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#817661}

base/allocator/partition_allocator/partition_lock.h

@@ -57,7 +57,7 @@ class SCOPED_LOCKABLE ScopedUnlockGuard {
 // Spinlock. Do not use, to be removed. crbug.com/1061437.
 class BASE_EXPORT SpinLock {
  public:
-  SpinLock() = default;
+  constexpr SpinLock() = default;
   ~SpinLock() = default;
 
   ALWAYS_INLINE void Acquire() {
@@ -91,7 +91,7 @@ class BASE_EXPORT SpinLock {
 template <>
 class LOCKABLE MaybeSpinLock<true> {
  public:
-  MaybeSpinLock() : lock_() {}
+  constexpr MaybeSpinLock() : lock_() {}
   void Lock() EXCLUSIVE_LOCK_FUNCTION() {
 #if BUILDFLAG(USE_PARTITION_ALLOC_AS_MALLOC) && DCHECK_IS_ON()
     // When PartitionAlloc is malloc(), it can easily become reentrant. For
@@ -170,6 +170,9 @@ static_assert(
     sizeof(MaybeSpinLock<true>) == sizeof(MaybeSpinLock<false>),
     "Sizes should be equal to ensure identical layout of PartitionRoot");
 
+using PartitionLock = MaybeSpinLock<true>;
+using PartitionAutoLock = ScopedGuard<true>;
+
 }  // namespace internal
 }  // namespace base
 #endif  // BASE_ALLOCATOR_PARTITION_ALLOCATOR_PARTITION_LOCK_H_

base/allocator/partition_allocator/thread_cache.cc

@@ -14,6 +14,12 @@ namespace base {
 
 namespace internal {
 
+namespace {
+
+ThreadCacheRegistry g_instance;
+
+}
+
 BASE_EXPORT PartitionTlsKey g_thread_cache_key;
 
 namespace {
@@ -30,14 +36,11 @@ static std::atomic<bool> g_has_instance;
 
 // static
 ThreadCacheRegistry& ThreadCacheRegistry::Instance() {
-  static NoDestructor<ThreadCacheRegistry> instance;
-  return *instance.get();
+  return g_instance;
 }
 
-ThreadCacheRegistry::ThreadCacheRegistry() = default;
-
 void ThreadCacheRegistry::RegisterThreadCache(ThreadCache* cache) {
-  AutoLock scoped_locker(GetLock());
+  PartitionAutoLock scoped_locker(GetLock());
   cache->next_ = nullptr;
   cache->prev_ = nullptr;
 
@@ -49,7 +52,7 @@ void ThreadCacheRegistry::RegisterThreadCache(ThreadCache* cache) {
 }
 
 void ThreadCacheRegistry::UnregisterThreadCache(ThreadCache* cache) {
-  AutoLock scoped_locker(GetLock());
+  PartitionAutoLock scoped_locker(GetLock());
   if (cache->prev_)
     cache->prev_->next_ = cache->next_;
   if (cache->next_)
@@ -62,7 +65,7 @@ void ThreadCacheRegistry::DumpStats(bool my_thread_only,
                                     ThreadCacheStats* stats) {
   memset(reinterpret_cast<void*>(stats), 0, sizeof(ThreadCacheStats));
 
-  AutoLock scoped_locker(GetLock());
+  PartitionAutoLock scoped_locker(GetLock());
   if (my_thread_only) {
     auto* tcache = ThreadCache::Get();
     if (!tcache)
@@ -85,7 +88,7 @@ void ThreadCacheRegistry::PurgeAll() {
   auto* current_thread_tcache = ThreadCache::Get();
 
   {
-    AutoLock scoped_locker(GetLock());
+    PartitionAutoLock scoped_locker(GetLock());
     ThreadCache* tcache = list_head_;
     while (tcache) {
       // Cannot purge directly, need to ask the other thread to purge "at some

base/allocator/partition_allocator/thread_cache.h

@@ -12,6 +12,7 @@
 #include "base/allocator/partition_allocator/partition_alloc_forward.h"
 #include "base/allocator/partition_allocator/partition_cookie.h"
 #include "base/allocator/partition_allocator/partition_freelist_entry.h"
+#include "base/allocator/partition_allocator/partition_lock.h"
 #include "base/allocator/partition_allocator/partition_tls.h"
 #include "base/base_export.h"
 #include "base/gtest_prod_util.h"
@@ -63,7 +64,20 @@ struct ThreadCacheStats {
 class BASE_EXPORT ThreadCacheRegistry {
  public:
   static ThreadCacheRegistry& Instance();
-  ~ThreadCacheRegistry() = delete;
+  // Do not instantiate.
+  //
+  // Several things are surprising here:
+  // - The constructor is public even though this is intended to be a singleton:
+  //   we cannot use a "static local" variable in |Instance()| as this is
+  //   reached too early during CRT initialization on Windows, meaning that
+  //   static local variables don't work (as they call into the uninitialized
+  //   runtime). To sidestep that, we use a regular global variable in the .cc,
+  //   which is fine as this object's constructor is constexpr.
+  // - Marked inline so that the chromium style plugin doesn't complain that a
+  //   "complex constructor" has an inline body. This warning is disabled when
+  //   the constructor is explicitly marked "inline". Note that this is a false
+  //   positive of the plugin, since constexpr implies inline.
+  inline constexpr ThreadCacheRegistry();
 
   void RegisterThreadCache(ThreadCache* cache);
   void UnregisterThreadCache(ThreadCache* cache);
@@ -73,16 +87,17 @@ class BASE_EXPORT ThreadCacheRegistry {
   // a later point (during a deallocation).
   void PurgeAll();
 
-  static Lock& GetLock() { return Instance().lock_; }
+  static PartitionLock& GetLock() { return Instance().lock_; }
 
  private:
   friend class NoDestructor<ThreadCacheRegistry>;
-  ThreadCacheRegistry();
-
-  Lock lock_;
+  // Not using base::Lock as the object's constructor must be constexpr.
+  PartitionLock lock_;
   ThreadCache* list_head_ GUARDED_BY(GetLock()) = nullptr;
 };
 
+constexpr ThreadCacheRegistry::ThreadCacheRegistry() = default;
+
 // Optional statistics collection.
 #if DCHECK_IS_ON()
 #define PA_ENABLE_THREAD_CACHE_STATISTICS 1

base/allocator/partition_allocator/thread_cache_unittest.cc

@@ -260,7 +260,7 @@ TEST_F(ThreadCacheTest, ThreadCacheRegistry) {
     auto* tcache = g_root->thread_cache_for_testing();
     EXPECT_TRUE(tcache);
 
-    AutoLock lock(ThreadCacheRegistry::GetLock());
+    PartitionAutoLock lock(ThreadCacheRegistry::GetLock());
     EXPECT_EQ(tcache->prev_, nullptr);
     EXPECT_EQ(tcache->next_, parent_thread_tcache);
   })};
@@ -269,7 +269,7 @@ TEST_F(ThreadCacheTest, ThreadCacheRegistry) {
   PlatformThread::Create(0, &delegate, &thread_handle);
   PlatformThread::Join(thread_handle);
 
-  AutoLock lock(ThreadCacheRegistry::GetLock());
+  PartitionAutoLock lock(ThreadCacheRegistry::GetLock());
   EXPECT_EQ(parent_thread_tcache->prev_, nullptr);
   EXPECT_EQ(parent_thread_tcache->next_, nullptr);
 }