[handles] Simplify GlobalHandles 2nd pass callback handling

- Rely on GCCallbacksScope to avoid nesting callbacks.
- Use a single entrypoint consistently for all callsites.

Change-Id: I6be1f749a2d6bfc9d5db4c84c753e9176472bce2
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3605821Reviewed-by: Dominik Inführ <dinfuehr@chromium.org>
Commit-Queue: Michael Lippautz <mlippautz@chromium.org>
Cr-Commit-Position: refs/heads/main@{#80268}

src/handles/global-handles.cc

@@ -1274,34 +1274,29 @@ void GlobalHandles::ProcessWeakYoungObjects(
   }
 }
 
-void GlobalHandles::InvokeSecondPassPhantomCallbacksFromTask() {
-  DCHECK(second_pass_callbacks_task_posted_);
-  second_pass_callbacks_task_posted_ = false;
-  Heap::DevToolsTraceEventScope devtools_trace_event_scope(
-      isolate()->heap(), "MajorGC", "invoke weak phantom callbacks");
-  TRACE_EVENT0("v8", "V8.GCPhantomHandleProcessingCallback");
-  isolate()->heap()->CallGCPrologueCallbacks(
-      GCType::kGCTypeProcessWeakCallbacks, kNoGCCallbackFlags);
-  InvokeSecondPassPhantomCallbacks();
-  isolate()->heap()->CallGCEpilogueCallbacks(
-      GCType::kGCTypeProcessWeakCallbacks, kNoGCCallbackFlags);
-}
-
 void GlobalHandles::InvokeSecondPassPhantomCallbacks() {
+  if (second_pass_callbacks_.empty()) return;
+
+  GCCallbacksScope scope(isolate()->heap());
   // The callbacks may execute JS, which in turn may lead to another GC run.
   // If we are already processing the callbacks, we do not want to start over
   // from within the inner GC. Newly added callbacks will always be run by the
   // outermost GC run only.
-  if (running_second_pass_callbacks_) return;
-  running_second_pass_callbacks_ = true;
-
-  AllowJavascriptExecution allow_js(isolate());
-  while (!second_pass_callbacks_.empty()) {
-    auto callback = second_pass_callbacks_.back();
-    second_pass_callbacks_.pop_back();
-    callback.Invoke(isolate(), PendingPhantomCallback::kSecondPass);
+  if (scope.CheckReenter()) {
+    TRACE_EVENT0("v8", "V8.GCPhantomHandleProcessingCallback");
+    isolate()->heap()->CallGCPrologueCallbacks(
+        GCType::kGCTypeProcessWeakCallbacks, kNoGCCallbackFlags);
+    {
+      AllowJavascriptExecution allow_js(isolate());
+      while (!second_pass_callbacks_.empty()) {
+        auto callback = second_pass_callbacks_.back();
+        second_pass_callbacks_.pop_back();
+        callback.Invoke(isolate(), PendingPhantomCallback::kSecondPass);
+      }
+    }
+    isolate()->heap()->CallGCEpilogueCallbacks(
+        GCType::kGCTypeProcessWeakCallbacks, kNoGCCallbackFlags);
   }
-  running_second_pass_callbacks_ = false;
 }
 
 namespace {
@@ -1387,27 +1382,6 @@ size_t GlobalHandles::InvokeFirstPassWeakCallbacks() {
          InvokeFirstPassWeakCallbacks(&traced_pending_phantom_callbacks_);
 }
 
-void GlobalHandles::InvokeOrScheduleSecondPassPhantomCallbacks(
-    bool synchronous_second_pass) {
-  if (!second_pass_callbacks_.empty()) {
-    if (FLAG_optimize_for_size || FLAG_predictable || synchronous_second_pass) {
-      Heap::DevToolsTraceEventScope devtools_trace_event_scope(
-          isolate()->heap(), "MajorGC", "invoke weak phantom callbacks");
-      isolate()->heap()->CallGCPrologueCallbacks(
-          GCType::kGCTypeProcessWeakCallbacks, kNoGCCallbackFlags);
-      InvokeSecondPassPhantomCallbacks();
-      isolate()->heap()->CallGCEpilogueCallbacks(
-          GCType::kGCTypeProcessWeakCallbacks, kNoGCCallbackFlags);
-    } else if (!second_pass_callbacks_task_posted_) {
-      second_pass_callbacks_task_posted_ = true;
-      auto taskrunner = V8::GetCurrentPlatform()->GetForegroundTaskRunner(
-          reinterpret_cast<v8::Isolate*>(isolate()));
-      taskrunner->PostTask(MakeCancelableTask(
-          isolate(), [this] { InvokeSecondPassPhantomCallbacksFromTask(); }));
-    }
-  }
-}
-
 void GlobalHandles::PendingPhantomCallback::Invoke(Isolate* isolate,
                                                    InvocationType type) {
   Data::Callback* callback_addr = nullptr;
@@ -1427,12 +1401,30 @@ void GlobalHandles::PostGarbageCollectionProcessing(
   // GC is completely done, because the callbacks may invoke arbitrary
   // API functions.
   DCHECK_EQ(Heap::NOT_IN_GC, isolate_->heap()->gc_state());
+
   const bool synchronous_second_pass =
+      FLAG_optimize_for_size || FLAG_predictable ||
       isolate_->heap()->IsTearingDown() ||
       (gc_callback_flags &
        (kGCCallbackFlagForced | kGCCallbackFlagCollectAllAvailableGarbage |
         kGCCallbackFlagSynchronousPhantomCallbackProcessing)) != 0;
-  InvokeOrScheduleSecondPassPhantomCallbacks(synchronous_second_pass);
+
+  if (synchronous_second_pass) {
+    InvokeSecondPassPhantomCallbacks();
+    return;
+  }
+
+  if (second_pass_callbacks_.empty() || second_pass_callbacks_task_posted_)
+    return;
+
+  second_pass_callbacks_task_posted_ = true;
+  V8::GetCurrentPlatform()
+      ->GetForegroundTaskRunner(reinterpret_cast<v8::Isolate*>(isolate()))
+      ->PostTask(MakeCancelableTask(isolate(), [this] {
+        DCHECK(second_pass_callbacks_task_posted_);
+        second_pass_callbacks_task_posted_ = false;
+        InvokeSecondPassPhantomCallbacks();
+      }));
 }
 
 void GlobalHandles::IterateStrongRoots(RootVisitor* v) {

src/handles/global-handles.h

@@ -199,9 +199,6 @@ class V8_EXPORT_PRIVATE GlobalHandles final {
 
   static GlobalHandles* From(const TracedNode*);
 
-  void InvokeSecondPassPhantomCallbacksFromTask();
-  void InvokeOrScheduleSecondPassPhantomCallbacks(bool synchronous_second_pass);
-
   template <typename T>
   size_t InvokeFirstPassWeakCallbacks(
       std::vector<std::pair<T*, PendingPhantomCallback>>* pending);
@@ -236,7 +233,6 @@ class V8_EXPORT_PRIVATE GlobalHandles final {
       traced_pending_phantom_callbacks_;
   std::vector<PendingPhantomCallback> second_pass_callbacks_;
   bool second_pass_callbacks_task_posted_ = false;
-  bool running_second_pass_callbacks_ = false;
 };
 
 class GlobalHandles::PendingPhantomCallback final {

src/heap/heap.cc

@@ -1500,18 +1500,15 @@ void Heap::GarbageCollectionEpilogue(GarbageCollector collector) {
   last_gc_time_ = MonotonicallyIncreasingTimeInMs();
 }
 
-class V8_NODISCARD GCCallbacksScope {
- public:
-  explicit GCCallbacksScope(Heap* heap) : heap_(heap) {
-    heap_->gc_callbacks_depth_++;
-  }
-  ~GCCallbacksScope() { heap_->gc_callbacks_depth_--; }
+GCCallbacksScope::GCCallbacksScope(Heap* heap) : heap_(heap) {
+  heap_->gc_callbacks_depth_++;
+}
 
-  bool CheckReenter() { return heap_->gc_callbacks_depth_ == 1; }
+GCCallbacksScope::~GCCallbacksScope() { heap_->gc_callbacks_depth_--; }
 
- private:
-  Heap* heap_;
-};
+bool GCCallbacksScope::CheckReenter() const {
+  return heap_->gc_callbacks_depth_ == 1;
+}
 
 void Heap::HandleGCRequest() {
   if (IsStressingScavenge() && stress_scavenge_observer_->HasRequestedGC()) {

src/heap/heap.h

@@ -2567,6 +2567,17 @@ class V8_NODISCARD AlwaysAllocateScope {
   Heap* heap_;
 };
 
+class V8_NODISCARD GCCallbacksScope final {
+ public:
+  explicit GCCallbacksScope(Heap* heap);
+  ~GCCallbacksScope();
+
+  bool CheckReenter() const;
+
+ private:
+  Heap* const heap_;
+};
+
 // Like AlwaysAllocateScope if the heap argument to the constructor is
 // non-null. No-op otherwise.
 //