Revert "bindings: Pass CppHeap on Isolate creation"

This reverts commit bd3925de0b.

Reason for revert: Suspecting for MSAN failures https://ci.chromium.org/ui/p/chromium/builders/ci/Linux%20MSan%20Tests/53660/overview

Original change's description:
> bindings: Pass CppHeap on Isolate creation
>
> This CL switches Blink to use the new APIs to pass the Oilpan (CppHeap)
> heap on construction. This allows establishing the invariant that an
> Oilpan heap always exists. V8 can thus rely on providing Oilpan-managed
> types on its API.
>
> Bug: 42203693
> Change-Id: I6f224299200239c688570eef5b6e99d617f05975
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6179405
> Commit-Queue: Andreas Haas <ahaas@chromium.org>
> Reviewed-by: Michael Lippautz <mlippautz@chromium.org>
> Cr-Commit-Position: refs/heads/main@{#1414023}

Bug: 42203693
Change-Id: Ia4e2d8685ad4a4772305f0e05d1148fb87eed462
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6220110
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Owners-Override: Kevin McNee <mcnee@google.com>
Auto-Submit: Kevin McNee <mcnee@chromium.org>
Commit-Queue: Kevin McNee <mcnee@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1414180}

gin/isolate_holder.cc

@@ -23,7 +23,6 @@
 #include "gin/v8_initializer.h"
 #include "gin/v8_isolate_memory_dump_provider.h"
 #include "gin/v8_shared_memory_dump_provider.h"
-#include "v8/include/v8-cppgc.h"
 #include "v8/include/v8-isolate.h"
 #include "v8/include/v8-locker.h"
 #include "v8/include/v8-snapshot.h"
@@ -40,16 +39,13 @@ std::unique_ptr<v8::Isolate::CreateParams> getModifiedIsolateParams(
     std::unique_ptr<v8::Isolate::CreateParams> params,
     IsolateHolder::AllowAtomicsWaitMode atomics_wait_mode,
     v8::CreateHistogramCallback create_histogram_callback,
-    v8::AddHistogramSampleCallback add_histogram_sample_callback,
-    std::unique_ptr<v8::CppHeap> cpp_heap) {
+    v8::AddHistogramSampleCallback add_histogram_sample_callback) {
   params->create_histogram_callback = create_histogram_callback;
   params->add_histogram_sample_callback = add_histogram_sample_callback;
   params->allow_atomics_wait =
       atomics_wait_mode ==
       IsolateHolder::AllowAtomicsWaitMode::kAllowAtomicsWait;
   params->array_buffer_allocator = g_array_buffer_allocator;
-  // V8 takes ownership of the CppHeap.
-  params->cpp_heap = cpp_heap.release();
   return params;
 }
 }  // namespace
@@ -80,16 +76,14 @@ IsolateHolder::IsolateHolder(
     v8::CreateHistogramCallback create_histogram_callback,
     v8::AddHistogramSampleCallback add_histogram_sample_callback,
     scoped_refptr<base::SingleThreadTaskRunner> user_visible_task_runner,
-    scoped_refptr<base::SingleThreadTaskRunner> best_effort_task_runner,
-    std::unique_ptr<v8::CppHeap> cpp_heap)
+    scoped_refptr<base::SingleThreadTaskRunner> best_effort_task_runner)
     : IsolateHolder(std::move(task_runner),
                     access_mode,
                     isolate_type,
                     getModifiedIsolateParams(getDefaultIsolateParams(),
                                              atomics_wait_mode,
                                              create_histogram_callback,
-                                             add_histogram_sample_callback,
-                                             std::move(cpp_heap)),
+                                             add_histogram_sample_callback),
                     isolate_creation_mode,
                     std::move(user_visible_task_runner),
                     std::move(best_effort_task_runner)) {}
@@ -116,14 +110,13 @@ IsolateHolder::IsolateHolder(
   isolate_data_ = std::make_unique<PerIsolateData>(
       isolate_, allocator, access_mode_, task_runner,
       std::move(user_visible_task_runner), std::move(best_effort_task_runner));
+  //  TODO(crbug.com/40854483): Refactor such that caller need not
+  //  provide params when creating a snapshot.
   if (isolate_creation_mode == IsolateCreationMode::kCreateSnapshot) {
     // This branch is called when creating a V8 snapshot for Blink.
-    // SnapshotCreator initializes the Isolate using the CreateParams and calls
-    // isolate->Enter() in its construction. The Isolate is still owned by the
-    // IsolateHolder.
-    params->external_references = g_reference_table;
+    // Note SnapshotCreator calls isolate->Enter() in its construction.
     snapshot_creator_ =
-        std::make_unique<v8::SnapshotCreator>(isolate_, *params);
+        std::make_unique<v8::SnapshotCreator>(isolate_, g_reference_table);
     DCHECK_EQ(isolate_, snapshot_creator_->GetIsolate());
   } else {
     v8::Isolate::Initialize(isolate_, *params);

gin/public/isolate_holder.h

@@ -87,8 +87,7 @@ class GIN_EXPORT IsolateHolder {
       scoped_refptr<base::SingleThreadTaskRunner> user_visible_task_runner =
           nullptr,
       scoped_refptr<base::SingleThreadTaskRunner> best_effort_task_runner =
-          nullptr,
-      std::unique_ptr<v8::CppHeap> cpp_heap = {});
+          nullptr);
   IsolateHolder(
       scoped_refptr<base::SingleThreadTaskRunner> task_runner,
       AccessMode access_mode,

third_party/blink/renderer/bindings/core/v8/v8_initializer.cc

@@ -924,8 +924,7 @@ v8::Isolate* V8Initializer::InitializeMainThread() {
   v8::Isolate* isolate = V8PerIsolateData::Initialize(
       scheduler->V8TaskRunner(), scheduler->V8UserVisibleTaskRunner(),
       scheduler->V8BestEffortTaskRunner(), snapshot_mode,
-      create_histogram_callback, add_histogram_sample_callback,
-      ThreadState::Current()->ReleaseCppHeap());
+      create_histogram_callback, add_histogram_sample_callback);
   scheduler->SetV8Isolate(isolate);
 
   // ThreadState::isolate_ needs to be set before setting the EmbedderHeapTracer

third_party/blink/renderer/core/workers/worker_backing_thread.cc

@@ -18,7 +18,6 @@
 #include "third_party/blink/renderer/bindings/core/v8/v8_initializer.h"
 #include "third_party/blink/renderer/core/inspector/worker_thread_debugger.h"
 #include "third_party/blink/renderer/core/workers/worker_backing_thread_startup_data.h"
-#include "third_party/blink/renderer/platform/heap/thread_state.h"
 #include "third_party/blink/renderer/platform/runtime_enabled_features.h"
 #include "third_party/blink/renderer/platform/scheduler/public/main_thread.h"
 #include "third_party/blink/renderer/platform/scheduler/public/main_thread_scheduler.h"
@@ -127,7 +126,7 @@ void WorkerBackingThread::InitializeOnBackingThread(
       scheduler->V8TaskRunner(), scheduler->V8UserVisibleTaskRunner(),
       scheduler->V8BestEffortTaskRunner(),
       V8PerIsolateData::V8ContextSnapshotMode::kDontUseSnapshot, nullptr,
-      nullptr, ThreadState::Current()->ReleaseCppHeap());
+      nullptr);
   scheduler->SetV8Isolate(isolate_);
   AddWorkerIsolate(isolate_);
   V8Initializer::InitializeWorker(isolate_);
@@ -156,15 +155,12 @@ void WorkerBackingThread::ShutdownOnBackingThread() {
   Platform::Current()->WillStopWorkerThread();
 
   V8PerIsolateData::WillBeDestroyed(isolate_);
+  backing_thread_->ShutdownOnThread();
 
   RemoveForegroundedWorkerIsolate(isolate_);
   RemoveWorkerIsolate(isolate_);
   V8PerIsolateData::Destroy(isolate_);
   isolate_ = nullptr;
-
-  // Shutdown scheduler and GCSupport at the very end. This is necessary as
-  // Isolate shutdown invokes all Oilpan pre-finalizers and finalizers.
-  backing_thread_->ShutdownOnThread();
 }
 
 void WorkerBackingThread::SetForegrounded() {

third_party/blink/renderer/platform/bindings/v8_per_isolate_data.cc

@@ -129,8 +129,7 @@ V8PerIsolateData::V8PerIsolateData(
     scoped_refptr<base::SingleThreadTaskRunner> best_effort_task_runner,
     V8ContextSnapshotMode v8_context_snapshot_mode,
     v8::CreateHistogramCallback create_histogram_callback,
-    v8::AddHistogramSampleCallback add_histogram_sample_callback,
-    std::unique_ptr<v8::CppHeap> cpp_heap)
+    v8::AddHistogramSampleCallback add_histogram_sample_callback)
     : v8_context_snapshot_mode_(v8_context_snapshot_mode),
       isolate_holder_(
           std::move(task_runner),
@@ -147,8 +146,7 @@ V8PerIsolateData::V8PerIsolateData(
           create_histogram_callback,
           add_histogram_sample_callback,
           std::move(user_visible_task_runner),
-          std::move(best_effort_task_runner),
-          std::move(cpp_heap)),
+          std::move(best_effort_task_runner)),
       string_cache_(std::make_unique<StringCache>(GetIsolate())),
       private_property_(std::make_unique<V8PrivateProperty>()),
       constructor_mode_(ConstructorMode::kCreateNewObject),
@@ -184,15 +182,13 @@ v8::Isolate* V8PerIsolateData::Initialize(
     scoped_refptr<base::SingleThreadTaskRunner> best_effort_task_runner,
     V8ContextSnapshotMode context_mode,
     v8::CreateHistogramCallback create_histogram_callback,
-    v8::AddHistogramSampleCallback add_histogram_sample_callback,
-    std::unique_ptr<v8::CppHeap> cpp_heap) {
+    v8::AddHistogramSampleCallback add_histogram_sample_callback) {
   TRACE_EVENT1("v8", "V8PerIsolateData::Initialize", "V8ContextSnapshotMode",
                context_mode);
   V8PerIsolateData* data = new V8PerIsolateData(
       std::move(task_runner), std::move(user_visible_task_runner),
       std::move(best_effort_task_runner), context_mode,
-      create_histogram_callback, add_histogram_sample_callback,
-      std::move(cpp_heap));
+      create_histogram_callback, add_histogram_sample_callback);
   DCHECK(data);
 
   v8::Isolate* isolate = data->GetIsolate();

third_party/blink/renderer/platform/bindings/v8_per_isolate_data.h

@@ -112,8 +112,7 @@ class PLATFORM_EXPORT V8PerIsolateData final {
                                  scoped_refptr<base::SingleThreadTaskRunner>,
                                  V8ContextSnapshotMode,
                                  v8::CreateHistogramCallback,
-                                 v8::AddHistogramSampleCallback,
-                                 std::unique_ptr<v8::CppHeap>);
+                                 v8::AddHistogramSampleCallback);
 
   static V8PerIsolateData* From(v8::Isolate* isolate) {
     DCHECK(isolate);
@@ -266,8 +265,7 @@ class PLATFORM_EXPORT V8PerIsolateData final {
                    scoped_refptr<base::SingleThreadTaskRunner>,
                    V8ContextSnapshotMode,
                    v8::CreateHistogramCallback,
-                   v8::AddHistogramSampleCallback,
-                   std::unique_ptr<v8::CppHeap>);
+                   v8::AddHistogramSampleCallback);
   ~V8PerIsolateData();
 
   // A really simple hash function, which makes lookups faster. The set of

third_party/blink/renderer/platform/heap/thread_state.cc

@@ -103,16 +103,6 @@ ThreadState* ThreadState::AttachCurrentThreadForTesting(
   return thread_state;
 }
 
-namespace {
-void RecoverCppHeap(std::unique_ptr<v8::CppHeap> cpp_heap) {
-  ThreadState::Current()->SetCppHeap(std::move(cpp_heap));
-}
-}  // namespace
-
-void ThreadState::RecoverCppHeapAfterIsolateTearDown() {
-  isolate_->SetReleaseCppHeapCallbackForTesting(RecoverCppHeap);
-}
-
 // static
 void ThreadState::DetachCurrentThread() {
   auto* state = ThreadState::Current();
@@ -122,36 +112,30 @@ void ThreadState::DetachCurrentThread() {
 
 void ThreadState::AttachToIsolate(v8::Isolate* isolate,
                                   V8BuildEmbedderGraphCallback) {
-  CHECK(!owning_cpp_heap_);
-  CHECK_EQ(cpp_heap_, isolate->GetCppHeap());
+  isolate->AttachCppHeap(cpp_heap_.get());
+  CHECK_EQ(cpp_heap_.get(), isolate->GetCppHeap());
   isolate_ = isolate;
   embedder_roots_handler_ = std::make_unique<BlinkRootsHandler>(isolate);
   isolate_->SetEmbedderRootsHandler(embedder_roots_handler_.get());
 }
 
 void ThreadState::DetachFromIsolate() {
-  CHECK(!owning_cpp_heap_);
-  CHECK_EQ(cpp_heap_, isolate_->GetCppHeap());
+  CHECK_EQ(cpp_heap_.get(), isolate_->GetCppHeap());
+  isolate_->DetachCppHeap();
   isolate_->SetEmbedderRootsHandler(nullptr);
   isolate_ = nullptr;
-  cpp_heap_ = nullptr;
-}
-
-std::unique_ptr<v8::CppHeap> ThreadState::ReleaseCppHeap() {
-  return std::move(owning_cpp_heap_);
 }
 
 ThreadState::ThreadState(v8::Platform* platform)
-    : owning_cpp_heap_(v8::CppHeap::Create(
+    : cpp_heap_(v8::CppHeap::Create(
           platform,
           v8::CppHeapCreateParams(CustomSpaces::CreateCustomSpaces()))),
-      cpp_heap_(owning_cpp_heap_.get()),
       heap_handle_(cpp_heap_->GetHeapHandle()),
       thread_id_(CurrentThread()) {}
 
 ThreadState::~ThreadState() {
   DCHECK(IsCreationThread());
-  owning_cpp_heap_.reset();
+  cpp_heap_->Terminate();
   ThreadStateStorage::DetachNonMainThread(*ThreadStateStorage::Current());
 }
 
@@ -225,17 +209,6 @@ void ThreadState::EnableDetachedGarbageCollectionsForTesting() {
   cpp_heap().EnableDetachedGarbageCollectionsForTesting();
 }
 
-void ThreadState::SetCppHeap(std::unique_ptr<v8::CppHeap> cpp_heap) {
-  CHECK(!owning_cpp_heap_);
-  CHECK(!cpp_heap_);
-  // We want to keep the invariant that the ThreadState does not own a CppHeap
-  // while it is attached to an isolate. When it's attached to an isolate, the
-  // isolate owns the CppHeap.
-  CHECK(!isolate_);
-  owning_cpp_heap_ = std::move(cpp_heap);
-  cpp_heap_ = owning_cpp_heap_.get();
-}
-
 bool ThreadState::IsIncrementalMarking() {
   return cppgc::subtle::HeapState::IsMarking(
              ThreadState::Current()->heap_handle()) &&

third_party/blink/renderer/platform/heap/thread_state.h

@@ -57,8 +57,6 @@ class PLATFORM_EXPORT ThreadState final {
 
   void AttachToIsolate(v8::Isolate* isolate, V8BuildEmbedderGraphCallback);
   void DetachFromIsolate();
-  // Releases ownership of the CppHeap which is transferred to the v8::Isolate.
-  std::unique_ptr<v8::CppHeap> ReleaseCppHeap();
 
   ALWAYS_INLINE cppgc::HeapHandle& heap_handle() const { return heap_handle_; }
   ALWAYS_INLINE v8::CppHeap& cpp_heap() const { return *cpp_heap_; }
@@ -97,10 +95,6 @@ class PLATFORM_EXPORT ThreadState final {
   static ThreadState* AttachMainThreadForTesting(v8::Platform*);
   static ThreadState* AttachCurrentThreadForTesting(v8::Platform*);
 
-  void RecoverCppHeapAfterIsolateTearDown();
-
-  void SetCppHeap(std::unique_ptr<v8::CppHeap> cpp_heap);
-
   // Takes a heap snapshot that can be loaded into DevTools. Requires that
   // `ThreadState` is attached to a `v8::Isolate`.
   //
@@ -120,12 +114,7 @@ class PLATFORM_EXPORT ThreadState final {
   explicit ThreadState(v8::Platform*);
   ~ThreadState();
 
-  // During setup of a page ThreadState owns CppHeap. The ownership is
-  // transferred to the v8::Isolate on its creation.
-  std::unique_ptr<v8::CppHeap> owning_cpp_heap_;
-  // Even when not owning the CppHeap (as the heap is owned by a v8::Isolate),
-  // this pointer will keep a reference to the current CppHeap.
-  v8::CppHeap* cpp_heap_;
+  std::unique_ptr<v8::CppHeap> cpp_heap_;
   std::unique_ptr<v8::EmbedderRootsHandler> embedder_roots_handler_;
   cppgc::HeapHandle& heap_handle_;
   v8::Isolate* isolate_ = nullptr;

third_party/blink/renderer/platform/testing/main_thread_isolate.cc

@@ -15,7 +15,6 @@ namespace blink::test {
 
 MainThreadIsolate::MainThreadIsolate() {
   isolate_ = CreateMainThreadIsolate();
-  ThreadState::Current()->RecoverCppHeapAfterIsolateTearDown();
 }
 
 MainThreadIsolate::~MainThreadIsolate() {