Revert "[global-handles] Restructure GC post processing"

This reverts commit 2b77aaf4.

Reason for revert: https://ci.chromium.org/p/v8/builders/luci.v8.ci/V8%20Linux64%20-%20cfi/18836

Original change's description:
> [global-handles] Restructure GC post processing
> 
> Bug: chromium:923361
> Change-Id: I97a0314b6d5af543e0574f27892c73637739844d
> Reviewed-on: https://chromium-review.googlesource.com/c/1426124
> Commit-Queue: Michael Lippautz <mlippautz@chromium.org>
> Reviewed-by: Ulan Degenbaev <ulan@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#58984}

TBR=ulan@chromium.org,mlippautz@chromium.org

Change-Id: I8220c60573cf7f4a1ced99bcd18628933c45f41e
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: chromium:923361
Reviewed-on: https://chromium-review.googlesource.com/c/1425901Reviewed-by: Michael Lippautz <mlippautz@chromium.org>
Commit-Queue: Michael Lippautz <mlippautz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#58985}

src/global-handles.cc

@@ -373,12 +373,6 @@ class GlobalHandles::Node final {
     return state() == PENDING && IsPhantomResetHandle();
   }
 
-  bool IsPendingFinalizer() const {
-    return state() == PENDING && weakness_type() == FINALIZER_WEAK;
-  }
-
-  bool IsPending() const { return state() == PENDING; }
-
   bool IsRetainer() const {
     return state() != FREE &&
            !(state() == NEAR_DEATH && weakness_type() != FINALIZER_WEAK);
@@ -406,8 +400,6 @@ class GlobalHandles::Node final {
     return data_.parameter;
   }
 
-  bool has_callback() const { return weak_callback_ != nullptr; }
-
   // Accessors for next free node in the free list.
   Node* next_free() {
     DCHECK_EQ(FREE, state());
@@ -498,16 +490,25 @@ class GlobalHandles::Node final {
     NodeSpace<Node>::Release(this);
   }
 
-  void InvokeFinalizer(Isolate* isolate) {
-    CHECK(IsPendingFinalizer());
-    CHECK(!is_active());
+  bool PostGarbageCollectionProcessing(Isolate* isolate) {
+    // Handles only weak handles (not phantom) that are dying.
+    if (state() != Node::PENDING) return false;
+    if (weak_callback_ == nullptr) {
+      NodeSpace<Node>::Release(this);
+      return false;
+    }
     set_state(NEAR_DEATH);
+
     // Check that we are not passing a finalized external string to
     // the callback.
     DCHECK(!object()->IsExternalOneByteString() ||
            ExternalOneByteString::cast(object())->resource() != nullptr);
     DCHECK(!object()->IsExternalTwoByteString() ||
            ExternalTwoByteString::cast(object())->resource() != nullptr);
+    if (weakness_type() != FINALIZER_WEAK) {
+      return false;
+    }
+
     // Leaving V8.
     VMState<EXTERNAL> vmstate(isolate);
     HandleScope handle_scope(isolate);
@@ -516,9 +517,11 @@ class GlobalHandles::Node final {
     v8::WeakCallbackInfo<void> data(reinterpret_cast<v8::Isolate*>(isolate),
                                     parameter(), embedder_fields, nullptr);
     weak_callback_(data);
-    // For finalizers the handle must have either been reset or made strong.
-    // Both cases reset the state.
-    CHECK_NE(NEAR_DEATH, state());
+
+    // Absence of explicit cleanup or revival of weak handle
+    // in most of the cases would lead to memory leak.
+    CHECK(state() != NEAR_DEATH);
+    return true;
   }
 
   inline GlobalHandles* GetGlobalHandles();
@@ -816,44 +819,61 @@ void GlobalHandles::InvokeSecondPassPhantomCallbacks() {
   }
 }
 
-size_t GlobalHandles::PostScavengeProcessing(unsigned post_processing_count) {
-  size_t freed_nodes = 0;
+int GlobalHandles::PostScavengeProcessing(
+    unsigned initial_post_gc_processing_count) {
+  int freed_nodes = 0;
   for (Node* node : new_space_nodes_) {
-    // Filter free nodes.
-    if (!node->IsRetainer()) continue;
-
-    // Reset active state for all affected nodes.
+    DCHECK(node->is_in_new_space_list());
+    if (!node->IsRetainer()) {
+      // Free nodes do not have weak callbacks. Do not use them to compute
+      // the freed_nodes.
+      continue;
+    }
+    // Skip dependent or unmodified handles. Their weak callbacks might expect
+    // to be
+    // called between two global garbage collection callbacks which
+    // are not called for minor collections.
+    if (!node->is_independent() && (node->is_active())) {
+      node->set_active(false);
+      continue;
+    }
     node->set_active(false);
 
-    if (node->IsPending()) {
-      DCHECK(node->has_callback());
-      DCHECK(node->IsPendingFinalizer());
-      node->InvokeFinalizer(isolate_);
+    if (node->PostGarbageCollectionProcessing(isolate_)) {
+      if (initial_post_gc_processing_count != post_gc_processing_count_) {
+        // Weak callback triggered another GC and another round of
+        // PostGarbageCollection processing.  The current node might
+        // have been deleted in that round, so we need to bail out (or
+        // restart the processing).
+        return freed_nodes;
+      }
+    }
+    if (!node->IsRetainer()) {
+      freed_nodes++;
     }
-    if (InRecursiveGC(post_processing_count)) return freed_nodes;
-
-    if (!node->IsRetainer()) freed_nodes++;
   }
   return freed_nodes;
 }
 
-size_t GlobalHandles::PostMarkSweepProcessing(unsigned post_processing_count) {
-  size_t freed_nodes = 0;
+int GlobalHandles::PostMarkSweepProcessing(
+    unsigned initial_post_gc_processing_count) {
+  int freed_nodes = 0;
   for (Node* node : *regular_nodes_) {
-    // Filter free nodes.
-    if (!node->IsRetainer()) continue;
-
-    // Reset active state for all affected nodes.
+    if (!node->IsRetainer()) {
+      // Free nodes do not have weak callbacks. Do not use them to compute
+      // the freed_nodes.
+      continue;
+    }
     node->set_active(false);
-
-    if (node->IsPending()) {
-      DCHECK(node->has_callback());
-      DCHECK(node->IsPendingFinalizer());
-      node->InvokeFinalizer(isolate_);
+    if (node->PostGarbageCollectionProcessing(isolate_)) {
+      if (initial_post_gc_processing_count != post_gc_processing_count_) {
+        // See the comment above.
+        return freed_nodes;
+      }
+    }
+    if (!node->IsRetainer()) {
+      freed_nodes++;
     }
-    if (InRecursiveGC(post_processing_count)) return freed_nodes;
-
-    if (!node->IsRetainer()) freed_nodes++;
   }
   return freed_nodes;
 }
@@ -880,8 +900,8 @@ void GlobalHandles::UpdateListOfNewSpaceNodes() {
   new_space_nodes_.shrink_to_fit();
 }
 
-size_t GlobalHandles::InvokeFirstPassWeakCallbacks() {
-  size_t freed_nodes = 0;
+int GlobalHandles::InvokeFirstPassWeakCallbacks() {
+  int freed_nodes = 0;
   std::vector<PendingPhantomCallback> pending_phantom_callbacks;
   pending_phantom_callbacks.swap(pending_phantom_callbacks_);
   {
@@ -939,32 +959,38 @@ void GlobalHandles::PendingPhantomCallback::Invoke(Isolate* isolate) {
   }
 }
 
-bool GlobalHandles::InRecursiveGC(unsigned gc_processing_counter) {
-  return gc_processing_counter != post_gc_processing_count_;
-}
-
-size_t GlobalHandles::PostGarbageCollectionProcessing(
+int GlobalHandles::PostGarbageCollectionProcessing(
     GarbageCollector collector, const v8::GCCallbackFlags gc_callback_flags) {
   // Process weak global handle callbacks. This must be done after the
   // GC is completely done, because the callbacks may invoke arbitrary
   // API functions.
-  DCHECK_EQ(Heap::NOT_IN_GC, isolate_->heap()->gc_state());
-  const unsigned post_processing_count = ++post_gc_processing_count_;
-  size_t freed_nodes = 0;
+  DCHECK(isolate_->heap()->gc_state() == Heap::NOT_IN_GC);
+  const unsigned initial_post_gc_processing_count = ++post_gc_processing_count_;
+  int freed_nodes = 0;
   bool synchronous_second_pass =
       isolate_->heap()->IsTearingDown() ||
       (gc_callback_flags &
        (kGCCallbackFlagForced | kGCCallbackFlagCollectAllAvailableGarbage |
         kGCCallbackFlagSynchronousPhantomCallbackProcessing)) != 0;
   InvokeOrScheduleSecondPassPhantomCallbacks(synchronous_second_pass);
-  if (InRecursiveGC(post_processing_count)) return freed_nodes;
-
-  freed_nodes += Heap::IsYoungGenerationCollector(collector)
-                     ? PostScavengeProcessing(post_processing_count)
-                     : PostMarkSweepProcessing(post_processing_count);
-  if (InRecursiveGC(post_processing_count)) return freed_nodes;
-
-  UpdateListOfNewSpaceNodes();
+  if (initial_post_gc_processing_count != post_gc_processing_count_) {
+    // If the callbacks caused a nested GC, then return.  See comment in
+    // PostScavengeProcessing.
+    return freed_nodes;
+  }
+  if (Heap::IsYoungGenerationCollector(collector)) {
+    freed_nodes += PostScavengeProcessing(initial_post_gc_processing_count);
+  } else {
+    freed_nodes += PostMarkSweepProcessing(initial_post_gc_processing_count);
+  }
+  if (initial_post_gc_processing_count != post_gc_processing_count_) {
+    // If the callbacks caused a nested GC, then return.  See comment in
+    // PostScavengeProcessing.
+    return freed_nodes;
+  }
+  if (initial_post_gc_processing_count == post_gc_processing_count_) {
+    UpdateListOfNewSpaceNodes();
+  }
   return freed_nodes;
 }
 

src/global-handles.h

@@ -89,12 +89,12 @@ class GlobalHandles final {
 
   void RecordStats(HeapStats* stats);
 
-  size_t InvokeFirstPassWeakCallbacks();
+  int InvokeFirstPassWeakCallbacks();
   void InvokeSecondPassPhantomCallbacks();
 
   // Process pending weak handles.
   // Returns the number of freed nodes.
-  size_t PostGarbageCollectionProcessing(
+  int PostGarbageCollectionProcessing(
       GarbageCollector collector, const v8::GCCallbackFlags gc_callback_flags);
 
   void IterateStrongRoots(RootVisitor* v);
@@ -179,15 +179,11 @@ class GlobalHandles final {
   class NodeSpace;
   class PendingPhantomCallback;
 
-  bool InRecursiveGC(unsigned gc_processing_counter);
-
   void InvokeSecondPassPhantomCallbacksFromTask();
+  int PostScavengeProcessing(unsigned initial_post_gc_processing_count);
+  int PostMarkSweepProcessing(unsigned initial_post_gc_processing_count);
   void InvokeOrScheduleSecondPassPhantomCallbacks(bool synchronous_second_pass);
-  size_t PostScavengeProcessing(unsigned post_processing_count);
-  size_t PostMarkSweepProcessing(unsigned post_processing_count);
-
   void UpdateListOfNewSpaceNodes();
-
   void ApplyPersistentHandleVisitor(v8::PersistentHandleVisitor* visitor,
                                     Node* node);
 

src/heap/heap.cc

@@ -1644,7 +1644,7 @@ void Heap::UpdateSurvivalStatistics(int start_new_space_size) {
 
 bool Heap::PerformGarbageCollection(
     GarbageCollector collector, const v8::GCCallbackFlags gc_callback_flags) {
-  size_t freed_global_handles = 0;
+  int freed_global_handles = 0;
 
   if (!IsYoungGenerationCollector(collector)) {
     PROFILE(isolate_, CodeMovingGCEvent());