From 67469d910fd59c1a858c65de6347b48a7d273479 Mon Sep 17 00:00:00 2001
From: Jon Phillips <jphillips@cloudflare.com>
Date: Mon, 20 May 2024 13:51:29 +0100
Subject: [PATCH] Fix CppGc leak.

Despite claims in the documentation, V8 does not dispose of the
v8::CppHeap we pass to it on isolate construction, so we now do that
ourselves.
---
 src/workerd/jsg/setup.c++ | 24 ++++++++++++++++--------
 src/workerd/jsg/setup.h   |  1 +
 2 files changed, 17 insertions(+), 8 deletions(-)

diff --git a/src/workerd/jsg/setup.c++ b/src/workerd/jsg/setup.c++
index 71fe77ef89e..b143f7c7667 100644
--- a/src/workerd/jsg/setup.c++
+++ b/src/workerd/jsg/setup.c++
@@ -276,10 +276,8 @@ bool HeapTracer::TryResetRoot(const v8::TracedReference<v8::Value>& handle) {
 }
 
 namespace {
-  static v8::Isolate* newIsolate(
-      V8PlatformWrapper* system,
-      v8::Isolate::CreateParams&& params) {
-    return jsg::runInV8Stack([&](jsg::V8StackScope& stackScope) -> v8::Isolate* {
+  std::unique_ptr<v8::CppHeap> newCppHeap(V8PlatformWrapper* system) {
+    return jsg::runInV8Stack([&](jsg::V8StackScope& stackScope) {
       v8::CppHeapCreateParams heapParams {
         {},
         v8::WrapperDescriptor(
@@ -289,6 +287,13 @@ namespace {
       };
       heapParams.marking_support = cppgc::Heap::MarkingType::kAtomic;
       heapParams.sweeping_support = cppgc::Heap::SweepingType::kAtomic;
+      return v8::CppHeap::Create(system, heapParams);
+    });
+  }
+  static v8::Isolate* newIsolate(
+      v8::Isolate::CreateParams&& params,
+      v8::CppHeap* cppHeap) {
+    return jsg::runInV8Stack([&](jsg::V8StackScope& stackScope) -> v8::Isolate* {
       // We currently don't attempt to support incremental marking or sweeping. We probably could
       // support them, but it will take some careful investigation and testing. It's not clear if
       // this would be a win anyway, since Worker heaps are relatively small and therefore doing a
@@ -300,8 +305,10 @@ namespace {
       // fully utilized. This differs from browser environments, where a user is typically doing
       // only one thing at a time and thus likely has CPU cores to spare.
 
-      // v8 takes ownership of the v8::CppHeap when passed this way.
-      params.cpp_heap = v8::CppHeap::Create(system, heapParams).release();
+      // V8 *claims* to take ownership of the v8::CppHeap but actually releases ownership of it
+      // during v8::Isolate::Dispose.
+      // TODO(soon): submit a bug report/patch to v8.
+      params.cpp_heap = cppHeap;
 
       if (params.array_buffer_allocator == nullptr &&
           params.array_buffer_allocator_shared == nullptr) {
@@ -316,8 +323,8 @@ namespace {
 IsolateBase::IsolateBase(const V8System& system, v8::Isolate::CreateParams&& createParams,
                          kj::Own<IsolateObserver> observer)
     : system(system),
-      ptr(newIsolate(const_cast<V8PlatformWrapper*>(&system.platformWrapper),
-                     kj::mv(createParams))),
+      cppHeap(newCppHeap(const_cast<V8PlatformWrapper*>(&system.platformWrapper))),
+      ptr(newIsolate(kj::mv(createParams), cppHeap.get())),
       heapTracer(ptr),
       observer(kj::mv(observer)) {
   jsg::runInV8Stack([&](jsg::V8StackScope& stackScope) {
@@ -382,6 +389,7 @@ IsolateBase::IsolateBase(const V8System& system, v8::Isolate::CreateParams&& cre
 IsolateBase::~IsolateBase() noexcept(false) {
   jsg::runInV8Stack([&](jsg::V8StackScope& stackScope) {
     ptr->Dispose();
+    cppHeap.reset();;
   });
 }
 
diff --git a/src/workerd/jsg/setup.h b/src/workerd/jsg/setup.h
index 3cd91f5d021..9062bf7b763 100644
--- a/src/workerd/jsg/setup.h
+++ b/src/workerd/jsg/setup.h
@@ -187,6 +187,7 @@ class IsolateBase {
   using Item = kj::OneOf<v8::Global<v8::Data>, RefToDelete>;
 
   const V8System& system;
+  std::unique_ptr<class v8::CppHeap> cppHeap;
   v8::Isolate* ptr;
   kj::Maybe<kj::String> uuid;
   bool evalAllowed = false;