[weakrefs] Make the dirty FinalizationGroup list weak

A FinalizationGroup that needs cleanup should not artificially prolong
its lifetime by being on the dirty list.

R=ulan@chromium.org

Bug: v8:8179
Change-Id: I19f102d154a9ac43b549b7d833d0c3ca7e61c6d0
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2051562
Reviewed-by: Ulan Degenbaev <ulan@chromium.org>
Reviewed-by: Toon Verwaest <verwaest@chromium.org>
Commit-Queue: Shu-yu Guo <syg@chromium.org>
Cr-Commit-Position: refs/heads/master@{#66251}

src/heap/heap-inl.h

@@ -641,7 +641,7 @@ void Heap::DecrementExternalBackingStoreBytes(ExternalBackingStoreType type,
 }
 
 bool Heap::HasDirtyJSFinalizationGroups() {
-  return !dirty_js_finalization_groups().IsUndefined(isolate());
+  return !dirty_js_finalization_groups_list().IsUndefined(isolate());
 }
 
 AlwaysAllocateScope::AlwaysAllocateScope(Heap* heap) : heap_(heap) {

src/heap/heap.cc

@@ -206,6 +206,7 @@ Heap::Heap()
 
   set_native_contexts_list(Smi::zero());
   set_allocation_sites_list(Smi::zero());
+  set_dirty_js_finalization_groups_list(Smi::zero());
   // Put a dummy entry in the remembered pages so we can find the list the
   // minidump even if there are no real unmapped pages.
   RememberUnmappedPage(kNullAddress, false);
@@ -2651,6 +2652,7 @@ void Heap::UpdateReferencesInExternalStringTable(
 void Heap::ProcessAllWeakReferences(WeakObjectRetainer* retainer) {
   ProcessNativeContexts(retainer);
   ProcessAllocationSites(retainer);
+  ProcessDirtyJSFinalizationGroups(retainer);
 }
 
 
@@ -2672,9 +2674,17 @@ void Heap::ProcessAllocationSites(WeakObjectRetainer* retainer) {
   set_allocation_sites_list(allocation_site_obj);
 }
 
+void Heap::ProcessDirtyJSFinalizationGroups(WeakObjectRetainer* retainer) {
+  Object head = VisitWeakList<JSFinalizationGroup>(
+      this, dirty_js_finalization_groups_list(), retainer);
+  set_dirty_js_finalization_groups_list(head);
+}
+
 void Heap::ProcessWeakListRoots(WeakObjectRetainer* retainer) {
   set_native_contexts_list(retainer->RetainAs(native_contexts_list()));
   set_allocation_sites_list(retainer->RetainAs(allocation_sites_list()));
+  set_dirty_js_finalization_groups_list(
+      retainer->RetainAs(dirty_js_finalization_groups_list()));
 }
 
 void Heap::ForeachAllocationSite(
@@ -6064,25 +6074,25 @@ void Heap::AddDirtyJSFinalizationGroup(
     std::function<void(HeapObject object, ObjectSlot slot, Object target)>
         gc_notify_updated_slot) {
   DCHECK(!HasDirtyJSFinalizationGroups() ||
-         dirty_js_finalization_groups().IsJSFinalizationGroup());
+         dirty_js_finalization_groups_list().IsJSFinalizationGroup());
   DCHECK(finalization_group.next().IsUndefined(isolate()));
   DCHECK(!finalization_group.scheduled_for_cleanup());
   finalization_group.set_scheduled_for_cleanup(true);
-  finalization_group.set_next(dirty_js_finalization_groups());
+  finalization_group.set_next(dirty_js_finalization_groups_list());
   gc_notify_updated_slot(
       finalization_group,
       finalization_group.RawField(JSFinalizationGroup::kNextOffset),
-      dirty_js_finalization_groups());
-  set_dirty_js_finalization_groups(finalization_group);
-  // Roots are rescanned after objects are moved, so no need to record a slot
-  // for the root pointing to the first JSFinalizationGroup.
+      dirty_js_finalization_groups_list());
+  set_dirty_js_finalization_groups_list(finalization_group);
+  // dirty_js_finalization_groups_list is rescanned by ProcessWeakListRoots.
 }
 
 MaybeHandle<JSFinalizationGroup> Heap::TakeOneDirtyJSFinalizationGroup() {
   if (HasDirtyJSFinalizationGroups()) {
     Handle<JSFinalizationGroup> finalization_group(
-        JSFinalizationGroup::cast(dirty_js_finalization_groups()), isolate());
-    set_dirty_js_finalization_groups(finalization_group->next());
+        JSFinalizationGroup::cast(dirty_js_finalization_groups_list()),
+        isolate());
+    set_dirty_js_finalization_groups_list(finalization_group->next());
     finalization_group->set_next(ReadOnlyRoots(isolate()).undefined_value());
     return finalization_group;
   }
@@ -6097,12 +6107,12 @@ void Heap::RemoveDirtyFinalizationGroupsOnContext(NativeContext context) {
 
   Isolate* isolate = this->isolate();
   Object prev = ReadOnlyRoots(isolate).undefined_value();
-  Object current = dirty_js_finalization_groups();
+  Object current = dirty_js_finalization_groups_list();
   while (!current.IsUndefined(isolate)) {
     JSFinalizationGroup finalization_group = JSFinalizationGroup::cast(current);
     if (finalization_group.native_context() == context) {
       if (prev.IsUndefined(isolate)) {
-        set_dirty_js_finalization_groups(finalization_group.next());
+        set_dirty_js_finalization_groups_list(finalization_group.next());
       } else {
         JSFinalizationGroup::cast(prev).set_next(finalization_group.next());
       }

src/heap/heap.h

@@ -502,6 +502,13 @@ class Heap {
   }
   Object allocation_sites_list() { return allocation_sites_list_; }
 
+  void set_dirty_js_finalization_groups_list(Object object) {
+    dirty_js_finalization_groups_list_ = object;
+  }
+  Object dirty_js_finalization_groups_list() {
+    return dirty_js_finalization_groups_list_;
+  }
+
   // Used in CreateAllocationSiteStub and the (de)serializer.
   Address allocation_sites_list_address() {
     return reinterpret_cast<Address>(&allocation_sites_list_);
@@ -1737,6 +1744,7 @@ class Heap {
   void ProcessYoungWeakReferences(WeakObjectRetainer* retainer);
   void ProcessNativeContexts(WeakObjectRetainer* retainer);
   void ProcessAllocationSites(WeakObjectRetainer* retainer);
+  void ProcessDirtyJSFinalizationGroups(WeakObjectRetainer* retainer);
   void ProcessWeakListRoots(WeakObjectRetainer* retainer);
 
   // ===========================================================================
@@ -2041,6 +2049,7 @@ class Heap {
   // List heads are initialized lazily and contain the undefined_value at start.
   Object native_contexts_list_;
   Object allocation_sites_list_;
+  Object dirty_js_finalization_groups_list_;
 
   std::vector<GCCallbackTuple> gc_epilogue_callbacks_;
   std::vector<GCCallbackTuple> gc_prologue_callbacks_;

src/heap/objects-visiting.cc

@@ -185,10 +185,31 @@ struct WeakListVisitor<AllocationSite> {
   static void VisitPhantomObject(Heap*, AllocationSite) {}
 };
 
+template <>
+struct WeakListVisitor<JSFinalizationGroup> {
+  static void SetWeakNext(JSFinalizationGroup obj, Object next) {
+    obj.set_next(next, UPDATE_WEAK_WRITE_BARRIER);
+  }
+
+  static Object WeakNext(JSFinalizationGroup obj) { return obj.next(); }
+
+  static HeapObject WeakNextHolder(JSFinalizationGroup obj) { return obj; }
+
+  static int WeakNextOffset() { return JSFinalizationGroup::kNextOffset; }
+
+  static void VisitLiveObject(Heap*, JSFinalizationGroup, WeakObjectRetainer*) {
+  }
+
+  static void VisitPhantomObject(Heap*, JSFinalizationGroup) {}
+};
+
 template Object VisitWeakList<Context>(Heap* heap, Object list,
                                        WeakObjectRetainer* retainer);
 
 template Object VisitWeakList<AllocationSite>(Heap* heap, Object list,
                                               WeakObjectRetainer* retainer);
+
+template Object VisitWeakList<JSFinalizationGroup>(
+    Heap* heap, Object list, WeakObjectRetainer* retainer);
 }  // namespace internal
 }  // namespace v8

src/heap/setup-heap-internal.cc

@@ -67,6 +67,7 @@ bool Heap::CreateHeapObjects() {
 
   set_native_contexts_list(ReadOnlyRoots(this).undefined_value());
   set_allocation_sites_list(ReadOnlyRoots(this).undefined_value());
+  set_dirty_js_finalization_groups_list(ReadOnlyRoots(this).undefined_value());
 
   return true;
 }
@@ -618,7 +619,6 @@ void Heap::CreateInitialObjects() {
   // There's no "current microtask" in the beginning.
   set_current_microtask(roots.undefined_value());
 
-  set_dirty_js_finalization_groups(roots.undefined_value());
   set_weak_refs_keep_during_job(roots.undefined_value());
 
   // Allocate cache for single character one byte strings.

src/objects/js-weak-refs.h

@@ -36,6 +36,8 @@ class JSFinalizationGroup : public JSObject {
 
   DECL_INT_ACCESSORS(flags)
 
+  class BodyDescriptor;
+
   inline static void Register(Handle<JSFinalizationGroup> finalization_group,
                               Handle<JSReceiver> target,
                               Handle<Object> holdings, Handle<Object> key,

src/objects/js-weak-refs.tq

@@ -8,6 +8,8 @@ extern class JSFinalizationGroup extends JSObject {
   active_cells: Undefined|WeakCell;
   cleared_cells: Undefined|WeakCell;
   key_map: Object;
+  // For the linked list of FinalizationGroups that need cleanup. This link
+  // is weak.
   next: Undefined|JSFinalizationGroup;
   flags: Smi;
 }

src/objects/objects-body-descriptors-inl.h

@@ -248,6 +248,27 @@ class JSWeakRef::BodyDescriptor final : public BodyDescriptorBase {
   }
 };
 
+class JSFinalizationGroup::BodyDescriptor final : public BodyDescriptorBase {
+ public:
+  static bool IsValidSlot(Map map, HeapObject obj, int offset) {
+    return IsValidJSObjectSlotImpl(map, obj, offset);
+  }
+
+  template <typename ObjectVisitor>
+  static inline void IterateBody(Map map, HeapObject obj, int object_size,
+                                 ObjectVisitor* v) {
+    IteratePointers(obj, JSObject::BodyDescriptor::kStartOffset, kNextOffset,
+                    v);
+    IterateCustomWeakPointer(obj, kNextOffset, v);
+    IterateJSObjectBodyImpl(map, obj, kNextOffset + kTaggedSize, object_size,
+                            v);
+  }
+
+  static inline int SizeOf(Map map, HeapObject object) {
+    return map.instance_size();
+  }
+};
+
 class SharedFunctionInfo::BodyDescriptor final : public BodyDescriptorBase {
  public:
   static bool IsValidSlot(Map map, HeapObject obj, int offset) {

src/roots/roots.h

@@ -251,8 +251,6 @@ class Symbol;
   V(TemplateList, message_listeners, MessageListeners)                     \
   /* Support for async stack traces */                                     \
   V(HeapObject, current_microtask, CurrentMicrotask)                       \
-  /* JSFinalizationGroup objects which need cleanup */                     \
-  V(Object, dirty_js_finalization_groups, DirtyJSFinalizationGroups)       \
   /* KeepDuringJob set for JS WeakRefs */                                  \
   V(HeapObject, weak_refs_keep_during_job, WeakRefsKeepDuringJob)          \
   V(HeapObject, interpreter_entry_trampoline_for_profiling,                \

src/snapshot/startup-deserializer.cc

@@ -54,6 +54,8 @@ void StartupDeserializer::DeserializeInto(Isolate* isolate) {
     isolate->heap()->set_allocation_sites_list(
         ReadOnlyRoots(isolate).undefined_value());
   }
+  isolate->heap()->set_dirty_js_finalization_groups_list(
+      ReadOnlyRoots(isolate).undefined_value());
 
   isolate->builtins()->MarkInitialized();
 

test/cctest/test-roots.cc

@@ -44,7 +44,6 @@ bool IsInitiallyMutable(Factory* factory, Address object_address) {
   V(builtins_constants_table)             \
   V(current_microtask)                    \
   V(detached_contexts)                    \
-  V(dirty_js_finalization_groups)         \
   V(feedback_vectors_for_profiling_tools) \
   V(shared_wasm_memories)                 \
   V(materialized_objects)                 \

test/mjsunit/harmony/weakrefs/finalizationgroup-independent-lifetime.js

@@ -0,0 +1,26 @@
+// Copyright 2020 the V8 project authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+// Flags: --harmony-weak-refs --expose-gc --noincremental-marking
+
+let cleanup_called = false;
+function cleanup(iter) {
+  [...iter];
+  cleanup_called = true;
+};
+(function() {
+  let fg = new FinalizationGroup(cleanup);
+  (function() {
+    let x = {};
+    fg.register(x, {});
+    x = null;
+  })();
+  // Schedule fg for cleanup.
+  gc();
+})();
+
+// Collect fg, which should result in cleanup not called.
+gc();
+
+setTimeout(function() { assertFalse(cleanup_called); }, 0);