[js weak refs] Fix cleanup task scheduling

If the user's cleanup function didn't iterate all available WeakCells, we need
to schedule the cleanup task again at some point. The previous condition
resulted it never being scheduled.

BUG=v8:8179

Change-Id: I8f5f4c01d1eb6a3cca8bd21bdc52c38663889882
Reviewed-on: https://chromium-review.googlesource.com/c/1286686
Commit-Queue: Marja Hölttä <marja@chromium.org>
Reviewed-by: Ulan Degenbaev <ulan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#56772}

src/builtins/builtins-weak-refs.cc

@@ -29,6 +29,8 @@ BUILTIN(WeakFactoryConstructor) {
   // function, if the spec said so.
   Handle<JSWeakFactory> weak_factory = Handle<JSWeakFactory>::cast(result);
   weak_factory->set_cleanup(*cleanup);
+  weak_factory->set_flags(
+      JSWeakFactory::ScheduledForCleanupField::encode(false));
   return *weak_factory;
 }
 

src/contexts-inl.h

@@ -224,6 +224,9 @@ void NativeContext::AddDirtyJSWeakFactory(
         gc_notify_updated_slot) {
   DCHECK(dirty_js_weak_factories()->IsUndefined(isolate) ||
          dirty_js_weak_factories()->IsJSWeakFactory());
+  DCHECK(weak_factory->next()->IsUndefined(isolate));
+  DCHECK(!weak_factory->scheduled_for_cleanup());
+  weak_factory->set_scheduled_for_cleanup(true);
   weak_factory->set_next(dirty_js_weak_factories());
   gc_notify_updated_slot(
       weak_factory,

src/heap/mark-compact.cc

@@ -2101,9 +2101,7 @@ void MarkCompactCollector::ClearJSWeakCells() {
     HeapObject* target = HeapObject::cast(weak_cell->target());
     JSWeakFactory* weak_factory = weak_cell->factory();
     if (!non_atomic_marking_state()->IsBlackOrGrey(target)) {
-      if (!weak_factory->NeedsCleanup()) {
-        // This is the first dirty JSWeakCell of that JSWeakFactory. Record
-        // the dirty JSWeakFactory in the native context.
+      if (!weak_factory->scheduled_for_cleanup()) {
         isolate()->native_context()->AddDirtyJSWeakFactory(
             weak_factory, isolate(),
             [](HeapObject* object, Object** slot, Object* target) {
@@ -2123,6 +2121,7 @@ void MarkCompactCollector::ClearJSWeakCells() {
                            }
                          });
       DCHECK(weak_factory->NeedsCleanup());
+      DCHECK(weak_factory->scheduled_for_cleanup());
     } else {
       // The value of the JSWeakCell is alive.
       Object** slot =

src/objects.cc

@@ -18965,6 +18965,7 @@ void JSWeakFactoryCleanupTask::Run() {
                isolate_);
     native_context->set_dirty_js_weak_factories(weak_factory->next());
     weak_factory->set_next(ReadOnlyRoots(isolate_).undefined_value());
+    weak_factory->set_scheduled_for_cleanup(false);
 
     // TODO(marja): After WeakCell.cleanup() is added, it's possible that it's
     // called for something already in cleared_cells list. In that case, we

src/objects/js-weak-refs-inl.h

@@ -19,6 +19,7 @@ namespace internal {
 ACCESSORS(JSWeakFactory, cleanup, Object, kCleanupOffset)
 ACCESSORS(JSWeakFactory, active_cells, Object, kActiveCellsOffset)
 ACCESSORS(JSWeakFactory, cleared_cells, Object, kClearedCellsOffset)
+SMI_ACCESSORS(JSWeakFactory, flags, kFlagsOffset)
 ACCESSORS(JSWeakFactory, next, Object, kNextOffset)
 CAST_ACCESSOR(JSWeakFactory)
 
@@ -45,6 +46,14 @@ bool JSWeakFactory::NeedsCleanup() const {
   return cleared_cells()->IsJSWeakCell();
 }
 
+bool JSWeakFactory::scheduled_for_cleanup() const {
+  return ScheduledForCleanupField::decode(flags());
+}
+
+void JSWeakFactory::set_scheduled_for_cleanup(bool scheduled_for_cleanup) {
+  set_flags(ScheduledForCleanupField::update(flags(), scheduled_for_cleanup));
+}
+
 JSWeakCell* JSWeakFactory::PopClearedCell(Isolate* isolate) {
   JSWeakCell* weak_cell = JSWeakCell::cast(cleared_cells());
   DCHECK(weak_cell->prev()->IsUndefined(isolate));

src/objects/js-weak-refs.h

@@ -30,12 +30,17 @@ class JSWeakFactory : public JSObject {
   // For storing a list of JSWeakFactory objects in NativeContext.
   DECL_ACCESSORS(next, Object)
 
+  DECL_INT_ACCESSORS(flags)
+
   // Adds a newly constructed JSWeakCell object into this JSWeakFactory.
   inline void AddWeakCell(JSWeakCell* weak_cell);
 
   // Returns true if the cleared_cells list is non-empty.
   inline bool NeedsCleanup() const;
 
+  inline bool scheduled_for_cleanup() const;
+  inline void set_scheduled_for_cleanup(bool scheduled_for_cleanup);
+
   // Get and remove the first cleared JSWeakCell from the cleared_cells
   // list. (Assumes there is one.)
   inline JSWeakCell* PopClearedCell(Isolate* isolate);
@@ -44,7 +49,11 @@ class JSWeakFactory : public JSObject {
   static const int kActiveCellsOffset = kCleanupOffset + kPointerSize;
   static const int kClearedCellsOffset = kActiveCellsOffset + kPointerSize;
   static const int kNextOffset = kClearedCellsOffset + kPointerSize;
-  static const int kSize = kNextOffset + kPointerSize;
+  static const int kFlagsOffset = kNextOffset + kPointerSize;
+  static const int kSize = kFlagsOffset + kPointerSize;
+
+  // Bitfields in flags.
+  class ScheduledForCleanupField : public BitField<bool, 0, 1> {};
 
  private:
   DISALLOW_IMPLICIT_CONSTRUCTORS(JSWeakFactory);

test/mjsunit/harmony/weakrefs/cleanup-doesnt-iterate-all-cells.js

+// Copyright 2018 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
+
+let cleanup_call_count = 0;
+let cleanup = function(iter) {
+  if (cleanup_call_count == 0) {
+    // First call: iterate 2 of the 3 cells
+    let cells = [];
+    for (wc of iter) {
+      cells.push(wc);
+      // Don't iterate the rest of the cells
+      if (cells.length == 2) {
+        break;
+      }
+    }
+    assertEquals(cells.length, 2);
+    assertTrue(cells[0].holdings < 3);
+    assertTrue(cells[1].holdings < 3);
+    // Update call count only after the asserts; this ensures that the test
+    // fails even if the exceptions inside the cleanup function are swallowed.
+    cleanup_call_count++;
+  } else {
+    // Second call: iterate one leftover cell and one new cell.
+    assertEquals(1, cleanup_call_count);
+    let cells = [];
+    for (wc of iter) {
+      cells.push(wc);
+    }
+    assertEquals(cells.length, 2);
+    assertTrue((cells[0].holdings < 3 && cells[1].holdings == 100) ||
+               (cells[1].holdings < 3 && cells[0].holdings == 100));
+    // Update call count only after the asserts; this ensures that the test
+    // fails even if the exceptions inside the cleanup function are swallowed.
+    cleanup_call_count++;
+  }
+}
+
+let wf = new WeakFactory(cleanup);
+// Create 3 objects and WeakCells pointing to them. The objects need to be
+// inside a closure so that we can reliably kill them!
+let weak_cells = [];
+
+(function() {
+  let objects = [];
+
+  for (let i = 0; i < 3; ++i) {
+    objects[i] = {a: i};
+    weak_cells[i] = wf.makeCell(objects[i], i);
+  }
+
+  gc();
+  assertEquals(0, cleanup_call_count);
+
+  // Drop the references to the objects.
+  objects = [];
+})();
+
+// This GC will discover dirty WeakCells.
+gc();
+assertEquals(0, cleanup_call_count);
+
+let timeout_func_1 = function() {
+  assertEquals(1, cleanup_call_count);
+
+  // Assert that the cleanup function won't be called unless new WeakCells appear.
+  setTimeout(timeout_func_2, 0);
+}
+
+setTimeout(timeout_func_1, 0);
+
+let timeout_func_2 = function() {
+  assertEquals(1, cleanup_call_count);
+
+  // Create a new WeakCells to be cleaned up.
+  let obj = {};
+  let wc = wf.makeCell(obj, 100);
+  obj = null;
+
+  gc();
+
+  setTimeout(timeout_func_3, 0);
+}
+
+let timeout_func_3 = function() {
+  assertEquals(2, cleanup_call_count);
+}

test/mjsunit/harmony/weakrefs/factory-scheduled-for-cleanup-multiple-times.js

+// Copyright 2018 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
+
+let cleanup0_call_count = 0;
+let cleanup0_weak_cell_count = 0;
+
+let cleanup1_call_count = 0;
+let cleanup1_weak_cell_count = 0;
+
+let cleanup0 = function(iter) {
+  for (wc of iter) {
+    ++cleanup0_weak_cell_count;
+  }
+  ++cleanup0_call_count;
+}
+
+let cleanup1 = function(iter) {
+  for (wc of iter) {
+    ++cleanup1_weak_cell_count;
+  }
+  ++cleanup1_call_count;
+}
+
+let wf0 = new WeakFactory(cleanup0);
+let wf1 = new WeakFactory(cleanup1);
+
+// Create 1 WeakCell for each WeakFactory and kill the objects they point to.
+(function() {
+  // The objects need to be inside a closure so that we can reliably kill them.
+  let objects = [];
+  objects[0] = {};
+  objects[1] = {};
+
+  wf0.makeCell(objects[0]);
+  wf1.makeCell(objects[1]);
+
+  // Drop the references to the objects.
+  objects = [];
+
+  // Will schedule both wf0 and wf1 for cleanup.
+  gc();
+})();
+
+// Before the cleanup task has a chance to run, do the same thing again, so both
+// factories are (again) scheduled for cleanup. This has to be a IIFE function
+// (so that we can reliably kill the objects) so we cannot use the same function
+// as before.
+(function() {
+  let objects = [];
+  objects[0] = {};
+  objects[1] = {};
+  wf0.makeCell(objects[0]);
+  wf1.makeCell(objects[1]);
+  objects = [];
+  gc();
+})();
+
+let timeout_func = function() {
+  assertEquals(1, cleanup0_call_count);
+  assertEquals(2, cleanup0_weak_cell_count);
+  assertEquals(1, cleanup1_call_count);
+  assertEquals(2, cleanup1_weak_cell_count);
+}
+
+// Give the cleanup task a chance to run. All WeakCells to cleanup will be
+// available during the same invocation of the cleanup function.
+setTimeout(timeout_func, 0);