[heap] Ensure phantom callbacks are invoked before the next GC.

Currently we rely on tasks to invoke the second pass phantom callbacks.

This may accumulate phantom callbacks and make GCs ineffective if we
do not enter the message loop to run the tasks between the GCs.

Bug: v8:7912
Change-Id: I799c97ff99ed6967480bda24ea0bf1c6a7dd69be
Reviewed-on: https://chromium-review.googlesource.com/1122621
Commit-Queue: Ulan Degenbaev <ulan@chromium.org>
Reviewed-by: Clemens Hammacher <clemensh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#54144}

src/global-handles.cc

@@ -509,30 +509,16 @@ class GlobalHandles::NodeIterator {
 class GlobalHandles::PendingPhantomCallbacksSecondPassTask
     : public v8::internal::CancelableTask {
  public:
-  // Takes ownership of the contents of pending_phantom_callbacks, leaving it in
-  // the same state it would be after a call to Clear().
-  PendingPhantomCallbacksSecondPassTask(
-      std::vector<PendingPhantomCallback>* pending_phantom_callbacks,
-      Isolate* isolate)
-      : CancelableTask(isolate), isolate_(isolate) {
-    pending_phantom_callbacks_.swap(*pending_phantom_callbacks);
-  }
+  PendingPhantomCallbacksSecondPassTask(GlobalHandles* global_handles,
+                                        Isolate* isolate)
+      : CancelableTask(isolate), global_handles_(global_handles) {}
 
   void RunInternal() override {
-    TRACE_EVENT0("v8", "V8.GCPhantomHandleProcessingCallback");
-    isolate()->heap()->CallGCPrologueCallbacks(
-        GCType::kGCTypeProcessWeakCallbacks, kNoGCCallbackFlags);
-    InvokeSecondPassPhantomCallbacks(&pending_phantom_callbacks_, isolate());
-    isolate()->heap()->CallGCEpilogueCallbacks(
-        GCType::kGCTypeProcessWeakCallbacks, kNoGCCallbackFlags);
+    global_handles_->InvokeSecondPassPhantomCallbacksFromTask();
   }
 
-  Isolate* isolate() { return isolate_; }
-
  private:
-  Isolate* isolate_;
-  std::vector<PendingPhantomCallback> pending_phantom_callbacks_;
-
+  GlobalHandles* global_handles_;
   DISALLOW_COPY_AND_ASSIGN(PendingPhantomCallbacksSecondPassTask);
 };
 
@@ -765,18 +751,27 @@ void GlobalHandles::IterateNewSpaceWeakUnmodifiedRootsForPhantomHandles(
   }
 }
 
-void GlobalHandles::InvokeSecondPassPhantomCallbacks(
-    std::vector<PendingPhantomCallback>* callbacks, Isolate* isolate) {
-  while (!callbacks->empty()) {
-    auto callback = callbacks->back();
-    callbacks->pop_back();
+void GlobalHandles::InvokeSecondPassPhantomCallbacksFromTask() {
+  DCHECK(second_pass_callbacks_task_posted_);
+  second_pass_callbacks_task_posted_ = false;
+  TRACE_EVENT0("v8", "V8.GCPhantomHandleProcessingCallback");
+  isolate()->heap()->CallGCPrologueCallbacks(
+      GCType::kGCTypeProcessWeakCallbacks, kNoGCCallbackFlags);
+  InvokeSecondPassPhantomCallbacks();
+  isolate()->heap()->CallGCEpilogueCallbacks(
+      GCType::kGCTypeProcessWeakCallbacks, kNoGCCallbackFlags);
+}
+
+void GlobalHandles::InvokeSecondPassPhantomCallbacks() {
+  while (!second_pass_callbacks_.empty()) {
+    auto callback = second_pass_callbacks_.back();
+    second_pass_callbacks_.pop_back();
     DCHECK_NULL(callback.node());
     // Fire second pass callback
-    callback.Invoke(isolate);
+    callback.Invoke(isolate());
   }
 }
 
-
 int GlobalHandles::PostScavengeProcessing(
     const int initial_post_gc_processing_count) {
   int freed_nodes = 0;
@@ -864,28 +859,29 @@ void GlobalHandles::UpdateListOfNewSpaceNodes() {
 int GlobalHandles::DispatchPendingPhantomCallbacks(
     bool synchronous_second_pass) {
   int freed_nodes = 0;
-  std::vector<PendingPhantomCallback> second_pass_callbacks;
+  // Protect against callback modifying pending_phantom_callbacks_.
+  std::vector<PendingPhantomCallback> pending_phantom_callbacks;
+  pending_phantom_callbacks.swap(pending_phantom_callbacks_);
   {
     // The initial pass callbacks must simply clear the nodes.
-    for (auto callback : pending_phantom_callbacks_) {
+    for (auto callback : pending_phantom_callbacks) {
       // Skip callbacks that have already been processed once.
       if (callback.node() == nullptr) continue;
       callback.Invoke(isolate());
-      if (callback.callback()) second_pass_callbacks.push_back(callback);
+      if (callback.callback()) second_pass_callbacks_.push_back(callback);
       freed_nodes++;
     }
   }
-  pending_phantom_callbacks_.clear();
-  if (!second_pass_callbacks.empty()) {
+  if (!second_pass_callbacks_.empty()) {
     if (FLAG_optimize_for_size || FLAG_predictable || synchronous_second_pass) {
       isolate()->heap()->CallGCPrologueCallbacks(
           GCType::kGCTypeProcessWeakCallbacks, kNoGCCallbackFlags);
-      InvokeSecondPassPhantomCallbacks(&second_pass_callbacks, isolate());
+      InvokeSecondPassPhantomCallbacks();
       isolate()->heap()->CallGCEpilogueCallbacks(
           GCType::kGCTypeProcessWeakCallbacks, kNoGCCallbackFlags);
-    } else {
-      auto task = new PendingPhantomCallbacksSecondPassTask(
-          &second_pass_callbacks, isolate());
+    } else if (!second_pass_callbacks_task_posted_) {
+      second_pass_callbacks_task_posted_ = true;
+      auto task = new PendingPhantomCallbacksSecondPassTask(this, isolate());
       V8::GetCurrentPlatform()->CallOnForegroundThread(
           reinterpret_cast<v8::Isolate*>(isolate()), task);
     }

src/global-handles.h

@@ -181,6 +181,8 @@ class GlobalHandles {
   void Print();
 #endif  // DEBUG
 
+  void InvokeSecondPassPhantomCallbacks();
+
  private:
   // Internal node structures.
   class Node;
@@ -191,9 +193,7 @@ class GlobalHandles {
 
   explicit GlobalHandles(Isolate* isolate);
 
-  // Helpers for PostGarbageCollectionProcessing.
-  static void InvokeSecondPassPhantomCallbacks(
-      std::vector<PendingPhantomCallback>* callbacks, Isolate* isolate);
+  void InvokeSecondPassPhantomCallbacksFromTask();
   int PostScavengeProcessing(int initial_post_gc_processing_count);
   int PostMarkSweepProcessing(int initial_post_gc_processing_count);
   int DispatchPendingPhantomCallbacks(bool synchronous_second_pass);
@@ -224,6 +224,8 @@ class GlobalHandles {
   size_t number_of_phantom_handle_resets_;
 
   std::vector<PendingPhantomCallback> pending_phantom_callbacks_;
+  std::vector<PendingPhantomCallback> second_pass_callbacks_;
+  bool second_pass_callbacks_task_posted_ = false;
 
   friend class Isolate;
 

src/heap/heap.cc

@@ -1362,6 +1362,9 @@ bool Heap::CollectGarbage(AllocationSpace space,
     InvokeNearHeapLimitCallback();
   }
 
+  // Ensure that all pending phantom callbacks are invoked.
+  isolate()->global_handles()->InvokeSecondPassPhantomCallbacks();
+
   // The VM is in the GC state until exiting this function.
   VMState<GC> state(isolate());
 

src/heap/heap.h

@@ -1674,6 +1674,7 @@ class Heap {
     return !allocation_trackers_.empty();
   }
 
+  // ===========================================================================
   // Retaining path tracking. ==================================================
   // ===========================================================================
 

test/cctest/test-global-handles.cc

@@ -483,5 +483,34 @@ TEST(GCFromWeakCallbacks) {
   }
 }
 
+namespace {
+
+void SecondPassCallback(const v8::WeakCallbackInfo<FlagAndPersistent>& data) {
+  data.GetParameter()->flag = true;
+}
+
+void FirstPassCallback(const v8::WeakCallbackInfo<FlagAndPersistent>& data) {
+  data.GetParameter()->handle.Reset();
+  data.SetSecondPassCallback(SecondPassCallback);
+}
+
+}  // namespace
+
+TEST(SecondPassPhantomCallbacks) {
+  v8::Isolate* isolate = CcTest::isolate();
+  v8::Locker locker(CcTest::isolate());
+  v8::HandleScope scope(isolate);
+  v8::Local<v8::Context> context = v8::Context::New(isolate);
+  v8::Context::Scope context_scope(context);
+  FlagAndPersistent fp;
+  ConstructJSApiObject(isolate, context, &fp);
+  fp.flag = false;
+  fp.handle.SetWeak(&fp, FirstPassCallback, v8::WeakCallbackType::kParameter);
+  CHECK(!fp.flag);
+  CcTest::CollectGarbage(i::OLD_SPACE);
+  CcTest::CollectGarbage(i::OLD_SPACE);
+  CHECK(fp.flag);
+}
+
 }  // namespace internal
 }  // namespace v8