Replace ThreadLocal with ThreadLocalUniquePtr<T> (#8659)

Fixes https://github.com/flutter/flutter/issues/31292.

flow/export_node.cc

@@ -11,9 +11,8 @@ namespace {
 using ExportNodeBindings =
     std::unordered_map<zx_koid_t, std::unique_ptr<flutter::ExportNode>>;
 
-FML_THREAD_LOCAL fml::ThreadLocal tls_export_node_bindings([](intptr_t value) {
-  delete reinterpret_cast<ExportNodeBindings*>(value);
-});
+FML_THREAD_LOCAL fml::ThreadLocalUniquePtr<ExportNodeBindings>
+    tls_export_node_bindings;
 
 }  // namespace
 
@@ -27,13 +26,11 @@ ExportNode::ExportNode(zx::eventpair export_token)
 void ExportNode::Create(zx_koid_t id, zx::eventpair export_token) {
   // This GPU thread contains at least 1 ViewHolder.  Initialize the per-thread
   // bindings.
-  if (tls_export_node_bindings.Get() == 0) {
-    tls_export_node_bindings.Set(
-        reinterpret_cast<intptr_t>(new ExportNodeBindings()));
+  if (tls_export_node_bindings.get() == nullptr) {
+    tls_export_node_bindings.reset(new ExportNodeBindings());
   }
 
-  auto* bindings =
-      reinterpret_cast<ExportNodeBindings*>(tls_export_node_bindings.Get());
+  auto* bindings = tls_export_node_bindings.get();
   FML_DCHECK(bindings);
   FML_DCHECK(bindings->find(id) == bindings->end());
 
@@ -43,16 +40,14 @@ void ExportNode::Create(zx_koid_t id, zx::eventpair export_token) {
 }
 
 void ExportNode::Destroy(zx_koid_t id) {
-  auto* bindings =
-      reinterpret_cast<ExportNodeBindings*>(tls_export_node_bindings.Get());
+  auto* bindings = tls_export_node_bindings.get();
   FML_DCHECK(bindings);
 
   bindings->erase(id);
 }
 
 ExportNode* ExportNode::FromId(zx_koid_t id) {
-  auto* bindings =
-      reinterpret_cast<ExportNodeBindings*>(tls_export_node_bindings.Get());
+  auto* bindings = tls_export_node_bindings.get();
   if (!bindings) {
     return nullptr;
   }

flow/view_holder.cc

@@ -11,9 +11,8 @@ namespace {
 using ViewHolderBindings =
     std::unordered_map<zx_koid_t, std::unique_ptr<flutter::ViewHolder>>;
 
-FML_THREAD_LOCAL fml::ThreadLocal tls_view_holder_bindings([](intptr_t value) {
-  delete reinterpret_cast<ViewHolderBindings*>(value);
-});
+FML_THREAD_LOCAL fml::ThreadLocalUniquePtr<ViewHolderBindings>
+    tls_view_holder_bindings;
 
 fuchsia::ui::gfx::ViewProperties ToViewProperties(float width,
                                                   float height,
@@ -65,13 +64,11 @@ void ViewHolder::Create(zx_koid_t id,
                         BindCallback on_bind_callback) {
   // This GPU thread contains at least 1 ViewHolder.  Initialize the per-thread
   // bindings.
-  if (tls_view_holder_bindings.Get() == 0) {
-    tls_view_holder_bindings.Set(
-        reinterpret_cast<intptr_t>(new ViewHolderBindings()));
+  if (tls_view_holder_bindings.get() == nullptr) {
+    tls_view_holder_bindings.reset(new ViewHolderBindings());
   }
 
-  auto* bindings =
-      reinterpret_cast<ViewHolderBindings*>(tls_view_holder_bindings.Get());
+  auto* bindings = tls_view_holder_bindings.get();
   FML_DCHECK(bindings);
   FML_DCHECK(bindings->find(id) == bindings->end());
 
@@ -82,16 +79,14 @@ void ViewHolder::Create(zx_koid_t id,
 }
 
 void ViewHolder::Destroy(zx_koid_t id) {
-  auto* bindings =
-      reinterpret_cast<ViewHolderBindings*>(tls_view_holder_bindings.Get());
+  auto* bindings = tls_view_holder_bindings.get();
   FML_DCHECK(bindings);
 
   bindings->erase(id);
 }
 
 ViewHolder* ViewHolder::FromId(zx_koid_t id) {
-  auto* bindings =
-      reinterpret_cast<ViewHolderBindings*>(tls_view_holder_bindings.Get());
+  auto* bindings = tls_view_holder_bindings.get();
   if (!bindings) {
     return nullptr;
   }

fml/message_loop.cc

@@ -15,12 +15,10 @@
 
 namespace fml {
 
-FML_THREAD_LOCAL ThreadLocal tls_message_loop([](intptr_t value) {
-  delete reinterpret_cast<MessageLoop*>(value);
-});
+FML_THREAD_LOCAL ThreadLocalUniquePtr<MessageLoop> tls_message_loop;
 
 MessageLoop& MessageLoop::GetCurrent() {
-  auto* loop = reinterpret_cast<MessageLoop*>(tls_message_loop.Get());
+  auto* loop = tls_message_loop.get();
   FML_CHECK(loop != nullptr)
       << "MessageLoop::EnsureInitializedForCurrentThread was not called on "
          "this thread prior to message loop use.";
@@ -28,15 +26,15 @@ MessageLoop& MessageLoop::GetCurrent() {
 }
 
 void MessageLoop::EnsureInitializedForCurrentThread() {
-  if (tls_message_loop.Get() != 0) {
+  if (tls_message_loop.get() != nullptr) {
     // Already initialized.
     return;
   }
-  tls_message_loop.Set(reinterpret_cast<intptr_t>(new MessageLoop()));
+  tls_message_loop.reset(new MessageLoop());
 }
 
 bool MessageLoop::IsInitializedForCurrentThread() {
-  return tls_message_loop.Get() != 0;
+  return tls_message_loop.get() != nullptr;
 }
 
 MessageLoop::MessageLoop()

fml/thread_local.cc

@@ -4,66 +4,32 @@
 
 #include "flutter/fml/thread_local.h"
 
-namespace fml {
-
 #if FML_THREAD_LOCAL_PTHREADS
 
-ThreadLocal::ThreadLocal() : ThreadLocal(nullptr) {}
-
-ThreadLocal::ThreadLocal(ThreadLocalDestroyCallback destroy)
-    : destroy_(destroy) {
-  auto callback =
-      reinterpret_cast<void (*)(void*)>(&ThreadLocal::ThreadLocalDestroy);
-  FML_CHECK(pthread_key_create(&_key, callback) == 0);
-}
-
-ThreadLocal::~ThreadLocal() {
-  // This will NOT call the destroy callbacks on thread local values still
-  // active in other threads. Those must be cleared manually. The usage
-  // of this class should be similar to the thread_local keyword but with
-  // with a static storage specifier
+#include "flutter/fml/logging.h"
 
-  // Collect the container
-  delete reinterpret_cast<Box*>(pthread_getspecific(_key));
+namespace fml {
+namespace internal {
 
-  // Finally, collect the key
-  FML_CHECK(pthread_key_delete(_key) == 0);
+ThreadLocalPointer::ThreadLocalPointer(void (*destroy)(void*)) {
+  FML_CHECK(pthread_key_create(&key_, destroy) == 0);
 }
 
-ThreadLocal::Box::Box(ThreadLocalDestroyCallback destroy, intptr_t value)
-    : destroy_(destroy), value_(value) {}
-
-ThreadLocal::Box::~Box() = default;
-
-#else  // FML_THREAD_LOCAL_PTHREADS
-
-ThreadLocal::ThreadLocal() : ThreadLocal(nullptr) {}
-
-ThreadLocal::ThreadLocal(ThreadLocalDestroyCallback destroy)
-    : destroy_(destroy), value_(0) {}
-
-void ThreadLocal::Set(intptr_t value) {
-  if (value_ == value) {
-    return;
-  }
-
-  if (value_ != 0 && destroy_) {
-    destroy_(value_);
-  }
-
-  value_ = value;
+ThreadLocalPointer::~ThreadLocalPointer() {
+  FML_CHECK(pthread_key_delete(key_) == 0);
 }
 
-intptr_t ThreadLocal::Get() {
-  return value_;
+void* ThreadLocalPointer::get() const {
+  return pthread_getspecific(key_);
 }
 
-ThreadLocal::~ThreadLocal() {
-  if (value_ != 0 && destroy_) {
-    destroy_(value_);
-  }
+void* ThreadLocalPointer::swap(void* ptr) {
+  void* old_ptr = get();
+  FML_CHECK(pthread_setspecific(key_, ptr) == 0);
+  return old_ptr;
 }
 
-#endif  // FML_THREAD_LOCAL_PTHREADS
-
+}  // namespace internal
 }  // namespace fml
+
+#endif  // FML_THREAD_LOCAL_PTHREADS

fml/thread_local.h

@@ -2,13 +2,12 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#ifndef FLUTTER_FML_THREAD_LOCAL_H_
-#define FLUTTER_FML_THREAD_LOCAL_H_
+#ifndef FLUTTER_FML_THREAD_LOCAL_UNIQUE_PTR_H_
+#define FLUTTER_FML_THREAD_LOCAL_UNIQUE_PTR_H_
 
-#include <functional>
+#include <memory>
 
 #include "flutter/fml/build_config.h"
-#include "flutter/fml/logging.h"
 #include "flutter/fml/macros.h"
 
 #define FML_THREAD_LOCAL_PTHREADS OS_MACOSX || OS_LINUX || OS_ANDROID
@@ -19,103 +18,60 @@
 
 namespace fml {
 
-using ThreadLocalDestroyCallback = std::function<void(intptr_t)>;
-
 #if FML_THREAD_LOCAL_PTHREADS
 
-// thread_local is unavailable and we have to resort to pthreads.
-
 #define FML_THREAD_LOCAL static
 
-class ThreadLocal {
- private:
-  class Box {
-   public:
-    Box(ThreadLocalDestroyCallback destroy, intptr_t value);
-
-    ~Box();
+namespace internal {
 
-    intptr_t Value() const { return value_; }
-
-    void SetValue(intptr_t value) {
-      if (value == value_) {
-        return;
-      }
-
-      DestroyValue();
-      value_ = value;
-    }
+class ThreadLocalPointer {
+ public:
+  ThreadLocalPointer(void (*destroy)(void*));
+  ~ThreadLocalPointer();
 
-    void DestroyValue() {
-      if (destroy_) {
-        destroy_(value_);
-      }
-    }
+  void* get() const;
+  void* swap(void* ptr);
 
-   private:
-    ThreadLocalDestroyCallback destroy_;
-    intptr_t value_;
+ private:
+  pthread_key_t key_;
 
-    FML_DISALLOW_COPY_AND_ASSIGN(Box);
-  };
+  FML_DISALLOW_COPY_AND_ASSIGN(ThreadLocalPointer);
+};
 
-  static inline void ThreadLocalDestroy(void* value) {
-    FML_CHECK(value != nullptr);
-    auto* box = reinterpret_cast<Box*>(value);
-    box->DestroyValue();
-    delete box;
-  }
+}  // namespace internal
 
+template <typename T>
+class ThreadLocalUniquePtr {
  public:
-  ThreadLocal();
-
-  ThreadLocal(ThreadLocalDestroyCallback destroy);
-
-  void Set(intptr_t value) {
-    auto* box = reinterpret_cast<Box*>(pthread_getspecific(_key));
-    if (box == nullptr) {
-      box = new Box(destroy_, value);
-      FML_CHECK(pthread_setspecific(_key, box) == 0);
-    } else {
-      box->SetValue(value);
-    }
-  }
-
-  intptr_t Get() {
-    auto* box = reinterpret_cast<Box*>(pthread_getspecific(_key));
-    return box != nullptr ? box->Value() : 0;
-  }
+  ThreadLocalUniquePtr() : ptr_(destroy) {}
 
-  ~ThreadLocal();
+  T* get() const { return reinterpret_cast<T*>(ptr_.get()); }
+  void reset(T* ptr) { destroy(ptr_.swap(ptr)); }
 
  private:
-  pthread_key_t _key;
-  ThreadLocalDestroyCallback destroy_;
+  static void destroy(void* ptr) { delete reinterpret_cast<T*>(ptr); }
 
-  FML_DISALLOW_COPY_AND_ASSIGN(ThreadLocal);
+  internal::ThreadLocalPointer ptr_;
+
+  FML_DISALLOW_COPY_AND_ASSIGN(ThreadLocalUniquePtr);
 };
 
 #else  // FML_THREAD_LOCAL_PTHREADS
 
-#define FML_THREAD_LOCAL thread_local
+#define FML_THREAD_LOCAL static thread_local
 
-class ThreadLocal {
+template <typename T>
+class ThreadLocalUniquePtr {
  public:
-  ThreadLocal();
-
-  ThreadLocal(ThreadLocalDestroyCallback destroy);
-
-  void Set(intptr_t value);
-
-  intptr_t Get();
+  ThreadLocalUniquePtr() = default;
 
-  ~ThreadLocal();
+  T* get() const { return ptr_.get(); }
+  void reset(T* ptr) { ptr_.reset(ptr); }
 
  private:
-  ThreadLocalDestroyCallback destroy_;
-  intptr_t value_;
+  std::unique_ptr<T> ptr_;
 
-  FML_DISALLOW_COPY_AND_ASSIGN(ThreadLocal);
+  FML_DISALLOW_COPY_AND_ASSIGN(ThreadLocalUniquePtr);
 };
 
 #endif  // FML_THREAD_LOCAL_PTHREADS
@@ -128,4 +84,4 @@ class ThreadLocal {
 
 }  // namespace fml
 
-#endif  // FLUTTER_FML_THREAD_LOCAL_H_
+#endif  // FLUTTER_FML_THREAD_LOCAL_UNIQUE_PTR_H_

fml/thread_local_unittests.cc

@@ -2,108 +2,103 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include <atomic>
 #include <thread>
 
-#include "flutter/fml/logging.h"
+#include "flutter/fml/macros.h"
 #include "flutter/fml/thread_local.h"
 #include "gtest/gtest.h"
 
-// We are only going to test the pthreads based thread local boxes.
-#if FML_THREAD_LOCAL_PTHREADS
+namespace {
+
+class Box {
+ public:
+  Box(int value, std::atomic_int* destroys = nullptr)
+      : value_(value), destroys_(destroys) {}
+  ~Box() {
+    if (destroys_) {
+      ++*destroys_;
+    }
+  }
+
+  int value() const { return value_; }
+
+ private:
+  int value_;
+  std::atomic_int* destroys_;
+
+  FML_DISALLOW_COPY_AND_ASSIGN(Box);
+};
+
+FML_THREAD_LOCAL fml::ThreadLocalUniquePtr<Box> local;
+
+}  // namespace
 
 TEST(ThreadLocal, SimpleInitialization) {
   std::thread thread([&] {
-    fml::ThreadLocal local;
+    ASSERT_EQ(local.get(), nullptr);
     auto value = 100;
-    local.Set(value);
-    ASSERT_EQ(local.Get(), value);
+    local.reset(new Box(value));
+    ASSERT_EQ(local.get()->value(), value);
   });
   thread.join();
 }
 
 TEST(ThreadLocal, SimpleInitializationCheckInAnother) {
   std::thread thread([&] {
-    fml::ThreadLocal local;
+    ASSERT_EQ(local.get(), nullptr);
     auto value = 100;
-    local.Set(value);
-    ASSERT_EQ(local.Get(), value);
-    std::thread thread2([&]() { ASSERT_EQ(local.Get(), 0); });
+    local.reset(new Box(value));
+    ASSERT_EQ(local.get()->value(), value);
+    std::thread thread2([&]() { ASSERT_EQ(local.get(), nullptr); });
     thread2.join();
   });
   thread.join();
 }
 
 TEST(ThreadLocal, DestroyCallback) {
+  std::atomic_int destroys{0};
   std::thread thread([&] {
-    int destroys = 0;
-    fml::ThreadLocal local([&destroys](intptr_t) { destroys++; });
+    ASSERT_EQ(local.get(), nullptr);
     auto value = 100;
-    local.Set(value);
-    ASSERT_EQ(local.Get(), value);
-    ASSERT_EQ(destroys, 0);
+    local.reset(new Box(value, &destroys));
+    ASSERT_EQ(local.get()->value(), value);
+    ASSERT_EQ(destroys.load(), 0);
   });
   thread.join();
+  ASSERT_EQ(destroys.load(), 1);
 }
 
 TEST(ThreadLocal, DestroyCallback2) {
+  std::atomic_int destroys{0};
   std::thread thread([&] {
-    int destroys = 0;
-    fml::ThreadLocal local([&destroys](intptr_t) { destroys++; });
-
-    local.Set(100);
-    ASSERT_EQ(local.Get(), 100);
-    ASSERT_EQ(destroys, 0);
-    local.Set(200);
-    ASSERT_EQ(local.Get(), 200);
-    ASSERT_EQ(destroys, 1);
+    local.reset(new Box(100, &destroys));
+    ASSERT_EQ(local.get()->value(), 100);
+    ASSERT_EQ(destroys.load(), 0);
+    local.reset(new Box(200, &destroys));
+    ASSERT_EQ(local.get()->value(), 200);
+    ASSERT_EQ(destroys.load(), 1);
   });
   thread.join();
+  ASSERT_EQ(destroys.load(), 2);
 }
 
 TEST(ThreadLocal, DestroyThreadTimeline) {
+  std::atomic_int destroys{0};
   std::thread thread([&] {
-    int destroys = 0;
-    fml::ThreadLocal local([&destroys](intptr_t) { destroys++; });
-
-    std::thread thread([&]() {
-      local.Set(100);
-      ASSERT_EQ(local.Get(), 100);
-      ASSERT_EQ(destroys, 0);
-      local.Set(200);
-      ASSERT_EQ(local.Get(), 200);
-      ASSERT_EQ(destroys, 1);
+    std::thread thread2([&]() {
+      local.reset(new Box(100, &destroys));
+      ASSERT_EQ(local.get()->value(), 100);
+      ASSERT_EQ(destroys.load(), 0);
+      local.reset(new Box(200, &destroys));
+      ASSERT_EQ(local.get()->value(), 200);
+      ASSERT_EQ(destroys.load(), 1);
     });
-    ASSERT_EQ(local.Get(), 0);
-    thread.join();
-    ASSERT_EQ(local.Get(), 0);
-    ASSERT_EQ(destroys, 2);
-  });
-  thread.join();
-}
-
-TEST(ThreadLocal, SettingSameValue) {
-  std::thread thread([&] {
-    int destroys = 0;
-    {
-      fml::ThreadLocal local([&destroys](intptr_t) { destroys++; });
-
-      local.Set(100);
-      ASSERT_EQ(destroys, 0);
-      local.Set(100);
-      local.Set(100);
-      local.Set(100);
-      ASSERT_EQ(local.Get(), 100);
-      local.Set(100);
-      local.Set(100);
-      ASSERT_EQ(destroys, 0);
-      local.Set(200);
-      ASSERT_EQ(destroys, 1);
-      ASSERT_EQ(local.Get(), 200);
-    }
-
-    ASSERT_EQ(destroys, 1);
+    ASSERT_EQ(local.get(), nullptr);
+    thread2.join();
+    ASSERT_EQ(local.get(), nullptr);
+    ASSERT_EQ(destroys.load(), 2);
   });
   thread.join();
+  ASSERT_EQ(destroys.load(), 2);
 }
-
-#endif  // FML_THREAD_LOCAL_PTHREADS

lib/ui/compositing/scene_host.cc

@@ -20,9 +20,8 @@ namespace {
 
 using SceneHostBindings = std::unordered_map<zx_koid_t, flutter::SceneHost*>;
 
-FML_THREAD_LOCAL fml::ThreadLocal tls_scene_host_bindings([](intptr_t value) {
-  delete reinterpret_cast<SceneHostBindings*>(value);
-});
+FML_THREAD_LOCAL fml::ThreadLocalUniquePtr<SceneHostBindings>
+    tls_scene_host_bindings;
 
 void SceneHost_constructor(Dart_NativeArguments args) {
   tonic::DartCallConstructor(&flutter::SceneHost::Create, args);
@@ -31,17 +30,15 @@ void SceneHost_constructor(Dart_NativeArguments args) {
 void SceneHost_constructorViewHolderToken(Dart_NativeArguments args) {
   // This UI thread / Isolate contains at least 1 SceneHost.  Initialize the
   // per-Isolate bindings.
-  if (tls_scene_host_bindings.Get() == 0) {
-    tls_scene_host_bindings.Set(
-        reinterpret_cast<intptr_t>(new SceneHostBindings()));
+  if (tls_scene_host_bindings.get() == nullptr) {
+    tls_scene_host_bindings.reset(new SceneHostBindings());
   }
 
   tonic::DartCallConstructor(&flutter::SceneHost::CreateViewHolder, args);
 }
 
 flutter::SceneHost* GetSceneHost(scenic::ResourceId id) {
-  auto* bindings =
-      reinterpret_cast<SceneHostBindings*>(tls_scene_host_bindings.Get());
+  auto* bindings = tls_scene_host_bindings.get();
   FML_DCHECK(bindings);
 
   auto binding = bindings->find(id);
@@ -147,8 +144,7 @@ SceneHost::SceneHost(fml::RefPtr<zircon::dart::Handle> viewHolderTokenHandle,
   }
 
   auto bind_callback = [scene_host = this](scenic::ResourceId id) {
-    auto* bindings =
-        reinterpret_cast<SceneHostBindings*>(tls_scene_host_bindings.Get());
+    auto* bindings = tls_scene_host_bindings.get();
     FML_DCHECK(bindings);
     FML_DCHECK(bindings->find(id) == bindings->end());
 
@@ -171,8 +167,7 @@ SceneHost::SceneHost(fml::RefPtr<zircon::dart::Handle> viewHolderTokenHandle,
 
 SceneHost::~SceneHost() {
   if (use_view_holder_) {
-    auto* bindings =
-        reinterpret_cast<SceneHostBindings*>(tls_scene_host_bindings.Get());
+    auto* bindings = tls_scene_host_bindings.get();
     FML_DCHECK(bindings);
     bindings->erase(id_);