From 1aff25f11656e8c7ae4061c106bb7c7ddb54525e Mon Sep 17 00:00:00 2001
From: Gus Caplan <me@gus.host>
Date: Thu, 31 Oct 2019 09:13:47 -0700
Subject: [PATCH] src: refactor weakref cleanup

---
 src/api/callback.cc    |  2 +-
 src/api/environment.cc | 29 ++++++++++++++++++++++++++++-
 src/env-inl.h          |  5 -----
 src/env.cc             | 16 ----------------
 src/env.h              |  5 -----
 src/node_task_queue.cc |  2 +-
 6 files changed, 30 insertions(+), 29 deletions(-)

diff --git a/src/api/callback.cc b/src/api/callback.cc
index 6d4e28e1d9070f..cf103cbc56f4c7 100644
--- a/src/api/callback.cc
+++ b/src/api/callback.cc
@@ -101,7 +101,7 @@ void InternalCallbackScope::Close() {
 
   if (!env_->can_call_into_js()) return;
 
-  OnScopeLeave weakref_cleanup([&]() { env_->RunWeakRefCleanup(); });
+  OnScopeLeave weakref_cleanup([&]() { env_->isolate()->ClearKeptObjects(); });
 
   if (!tick_info->has_tick_scheduled()) {
     MicrotasksScope::PerformCheckpoint(env_->isolate());
diff --git a/src/api/environment.cc b/src/api/environment.cc
index e6a87d5a93a3b6..dbf5463283996f 100644
--- a/src/api/environment.cc
+++ b/src/api/environment.cc
@@ -14,6 +14,7 @@ using v8::Context;
 using v8::EscapableHandleScope;
 using v8::FinalizationGroup;
 using v8::Function;
+using v8::Global;
 using v8::HandleScope;
 using v8::Isolate;
 using v8::Local;
@@ -77,13 +78,39 @@ static MaybeLocal<Value> PrepareStackTraceCallback(Local<Context> context,
   return result;
 }
 
+class FinalizationGroupCleanupTask final {
+ public:
+  FinalizationGroupCleanupTask(Environment* env, Local<FinalizationGroup> group)
+      : env_(env), group_(env->isolate(), group) {}
+
+  inline Environment* env() { return env_; }
+  inline Local<FinalizationGroup> group() {
+    return group_.Get(env_->isolate());
+  }
+
+ private:
+  Environment* env_;
+  Global<FinalizationGroup> group_;
+};
+
+void HostCleanupFinalizationGroupMicrotask(void* data) {
+  FinalizationGroupCleanupTask* t =
+      reinterpret_cast<FinalizationGroupCleanupTask*>(data);
+  TryCatchScope try_catch(t->env());
+  if (!FinalizationGroup::Cleanup(t->group()).FromMaybe(false)) {
+    TriggerUncaughtException(t->env()->isolate(), try_catch);
+  }
+}
+
 static void HostCleanupFinalizationGroupCallback(
     Local<Context> context, Local<FinalizationGroup> group) {
   Environment* env = Environment::GetCurrent(context);
   if (env == nullptr) {
     return;
   }
-  env->RegisterFinalizationGroupForCleanup(group);
+  env->isolate()->EnqueueMicrotask(
+      HostCleanupFinalizationGroupMicrotask,
+      new FinalizationGroupCleanupTask(env, group));
 }
 
 void* NodeArrayBufferAllocator::Allocate(size_t size) {
diff --git a/src/env-inl.h b/src/env-inl.h
index d61ae8f8df1d17..a2ab98d66db10d 100644
--- a/src/env-inl.h
+++ b/src/env-inl.h
@@ -1115,11 +1115,6 @@ void Environment::RemoveCleanupHook(void (*fn)(void*), void* arg) {
   cleanup_hooks_.erase(search);
 }
 
-inline void Environment::RegisterFinalizationGroupForCleanup(
-    v8::Local<v8::FinalizationGroup> group) {
-  cleanup_finalization_groups_.emplace_back(isolate(), group);
-}
-
 size_t CleanupHookCallback::Hash::operator()(
     const CleanupHookCallback& cb) const {
   return std::hash<void*>()(cb.arg_);
diff --git a/src/env.cc b/src/env.cc
index ff1868e75ecd5a..2400785ea82fe4 100644
--- a/src/env.cc
+++ b/src/env.cc
@@ -29,7 +29,6 @@ using v8::ArrayBuffer;
 using v8::Boolean;
 using v8::Context;
 using v8::EmbedderGraph;
-using v8::FinalizationGroup;
 using v8::Function;
 using v8::FunctionTemplate;
 using v8::HandleScope;
@@ -1052,21 +1051,6 @@ void Environment::AddArrayBufferAllocatorToKeepAliveUntilIsolateDispose(
   keep_alive_allocators_->insert(allocator);
 }
 
-bool Environment::RunWeakRefCleanup() {
-  isolate()->ClearKeptObjects();
-
-  while (!cleanup_finalization_groups_.empty()) {
-    Local<FinalizationGroup> fg =
-        cleanup_finalization_groups_.front().Get(isolate());
-    cleanup_finalization_groups_.pop_front();
-    if (!FinalizationGroup::Cleanup(fg).FromMaybe(false)) {
-      return false;
-    }
-  }
-
-  return true;
-}
-
 void AsyncRequest::Install(Environment* env, void* data, uv_async_cb target) {
   CHECK_NULL(async_);
   env_ = env;
diff --git a/src/env.h b/src/env.h
index a649675c7cc902..25dcab79fcfc2d 100644
--- a/src/env.h
+++ b/src/env.h
@@ -1132,9 +1132,6 @@ class Environment : public MemoryRetainer {
   void AtExit(void (*cb)(void* arg), void* arg);
   void RunAtExitCallbacks();
 
-  void RegisterFinalizationGroupForCleanup(v8::Local<v8::FinalizationGroup> fg);
-  bool RunWeakRefCleanup();
-
   // Strings and private symbols are shared across shared contexts
   // The getters simply proxy to the per-isolate primitive.
 #define VP(PropertyName, StringValue) V(v8::Private, PropertyName)
@@ -1341,8 +1338,6 @@ class Environment : public MemoryRetainer {
   uint64_t thread_id_;
   std::unordered_set<worker::Worker*> sub_worker_contexts_;
 
-  std::deque<v8::Global<v8::FinalizationGroup>> cleanup_finalization_groups_;
-
   static void* const kNodeContextTagPtr;
   static int const kNodeContextTag;
 
diff --git a/src/node_task_queue.cc b/src/node_task_queue.cc
index ef1aff6cd4138d..a5e4b255a98a65 100644
--- a/src/node_task_queue.cc
+++ b/src/node_task_queue.cc
@@ -43,7 +43,7 @@ static void EnqueueMicrotask(const FunctionCallbackInfo<Value>& args) {
 
 // Should be in sync with runNextTicks in internal/process/task_queues.js
 bool RunNextTicksNative(Environment* env) {
-  OnScopeLeave weakref_cleanup([&]() { env->RunWeakRefCleanup(); });
+  OnScopeLeave weakref_cleanup([&]() { env->isolate()->ClearKeptObjects(); });
 
   TickInfo* tick_info = env->tick_info();
   if (!tick_info->has_tick_scheduled() && !tick_info->has_rejection_to_warn())