[js weak refs] Make the cleanup task a microtask

BUG=v8:8179

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

src/heap/heap.cc

@@ -937,6 +937,23 @@ void Heap::GarbageCollectionEpilogue() {
     TRACE_GC(tracer(), GCTracer::Scope::HEAP_EPILOGUE_REDUCE_NEW_SPACE);
     ReduceNewSpaceSize();
   }
+
+  if (FLAG_harmony_weak_refs) {
+    // TODO(marja): (spec): The exact condition on when to schedule the cleanup
+    // task is unclear. This version schedules the cleanup task whenever there's
+    // a GC and we have dirty WeakCells (either because this GC discovered them
+    // or because an earlier invocation of the cleanup function didn't iterate
+    // through them). See https://github.com/tc39/proposal-weakrefs/issues/34
+    HandleScope handle_scope(isolate());
+    if (isolate()->context() != nullptr &&
+        isolate()->native_context()->IsNativeContext() &&
+        !isolate()->native_context()->dirty_js_weak_factories()->IsUndefined(
+            isolate())) {
+      v8::Isolate* v8_isolate = reinterpret_cast<v8::Isolate*>(isolate());
+      v8_isolate->EnqueueMicrotask(
+          JSWeakFactory::CleanupJSWeakFactoriesCallback, isolate());
+    }
+  }
 }
 
 class GCCallbacksScope {

src/heap/mark-compact.cc

@@ -2093,7 +2093,6 @@ void MarkCompactCollector::ClearJSWeakCells() {
     return;
   }
   JSWeakCell* weak_cell;
-  bool schedule_cleanup_task = false;
   HandleScope handle_scope(isolate());
   while (weak_objects_.js_weak_cells.Pop(kMainThread, &weak_cell)) {
     // We do not insert cleared weak cells into the list, so the value
@@ -2109,7 +2108,6 @@ void MarkCompactCollector::ClearJSWeakCells() {
                 RecordSlot(object, slot, HeapObject::cast(target));
               }
             });
-        schedule_cleanup_task = true;
       }
       // We're modifying the pointers in JSWeakCell and JSWeakFactory during GC;
       // thus we need to record the slots it writes. The normal write barrier is
@@ -2129,14 +2127,6 @@ void MarkCompactCollector::ClearJSWeakCells() {
       RecordSlot(weak_cell, slot, HeapObject::cast(*slot));
     }
   }
-  if (schedule_cleanup_task) {
-    // TODO(marja): Make this a microtask.
-    v8::Platform* platform = V8::GetCurrentPlatform();
-    v8::Isolate* v8_isolate = reinterpret_cast<v8::Isolate*>(isolate());
-    platform->GetForegroundTaskRunner(v8_isolate)
-        ->PostTask(
-            std::unique_ptr<v8::Task>(new JSWeakFactoryCleanupTask(isolate())));
-  }
 }
 
 void MarkCompactCollector::AbortWeakObjects() {

src/objects.cc

@@ -18958,23 +18958,21 @@ template void
 BaseNameDictionary<NameDictionary, NameDictionaryShape>::CollectKeysTo(
     Handle<NameDictionary> dictionary, KeyAccumulator* keys);
 
-void JSWeakFactoryCleanupTask::Run() {
+void JSWeakFactory::CleanupJSWeakFactoriesCallback(void* data) {
   DCHECK(FLAG_harmony_weak_refs);
-  HandleScope handle_scope(isolate_);
-  Handle<Context> native_context =
-      Handle<Context>::cast(Utils::OpenPersistent(native_context_));
-  v8::Local<v8::Context> context_local = Utils::ToLocal(native_context);
-  v8::Context::Scope context_scope(context_local);
+  Isolate* isolate = reinterpret_cast<Isolate*>(data);
+  HandleScope handle_scope(isolate);
+  Handle<Context> native_context = isolate->native_context();
 
   while (native_context->dirty_js_weak_factories()->IsJSWeakFactory()) {
     Handle<JSWeakFactory> weak_factory =
         handle(JSWeakFactory::cast(native_context->dirty_js_weak_factories()),
-               isolate_);
+               isolate);
     native_context->set_dirty_js_weak_factories(weak_factory->next());
-    weak_factory->set_next(ReadOnlyRoots(isolate_).undefined_value());
+    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
+    // TODO(marja): After WeakCell.clear() is added, it's possible that it's
     // called for something already in cleared_cells list. In that case, we
     // shouldn't call the user's cleanup function.
 
@@ -18982,24 +18980,24 @@ void JSWeakFactoryCleanupTask::Run() {
     Handle<JSWeakFactoryCleanupIterator> iterator;
     {
       Handle<Map> cleanup_iterator_map(
-          native_context->js_weak_factory_cleanup_iterator_map(), isolate_);
+          native_context->js_weak_factory_cleanup_iterator_map(), isolate);
       iterator = Handle<JSWeakFactoryCleanupIterator>::cast(
-          isolate_->factory()->NewJSObjectFromMap(
+          isolate->factory()->NewJSObjectFromMap(
               cleanup_iterator_map, NOT_TENURED,
               Handle<AllocationSite>::null()));
       iterator->set_factory(*weak_factory);
     }
-    Handle<Object> cleanup(weak_factory->cleanup(), isolate_);
+    Handle<Object> cleanup(weak_factory->cleanup(), isolate);
 
-    v8::TryCatch try_catch(reinterpret_cast<v8::Isolate*>(isolate_));
+    v8::TryCatch try_catch(reinterpret_cast<v8::Isolate*>(isolate));
     v8::Local<v8::Value> result;
     MaybeHandle<Object> exception;
     Handle<Object> args[] = {iterator};
     bool has_pending_exception = !ToLocal<Value>(
         Execution::TryCall(
-            isolate_, cleanup,
-            handle(ReadOnlyRoots(isolate_).undefined_value(), isolate_), 1,
-            args, Execution::MessageHandling::kReport, &exception,
+            isolate, cleanup,
+            handle(ReadOnlyRoots(isolate).undefined_value(), isolate), 1, args,
+            Execution::MessageHandling::kReport, &exception,
             Execution::Target::kCallable),
         &result);
     // TODO(marja): (spec): What if there's an exception?

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

@@ -122,15 +122,6 @@ void JSWeakCell::Nullify(
       this);
 }
 
-JSWeakFactoryCleanupTask::JSWeakFactoryCleanupTask(Isolate* isolate)
-    : isolate_(isolate) {
-  Handle<Context> native_context =
-      Handle<Context>::cast(isolate->native_context());
-  Local<v8::Context> native_context_local = Utils::ToLocal(native_context);
-  native_context_.Reset(reinterpret_cast<v8::Isolate*>(isolate),
-                        native_context_local);
-}
-
 }  // namespace internal
 }  // namespace v8
 

src/objects/js-weak-refs.h

@@ -55,6 +55,8 @@ class JSWeakFactory : public JSObject {
   // Bitfields in flags.
   class ScheduledForCleanupField : public BitField<bool, 0, 1> {};
 
+  static void CleanupJSWeakFactoriesCallback(void* data);
+
  private:
   DISALLOW_IMPLICIT_CONSTRUCTORS(JSWeakFactory);
 };
@@ -111,18 +113,6 @@ class JSWeakFactoryCleanupIterator : public JSObject {
   DISALLOW_IMPLICIT_CONSTRUCTORS(JSWeakFactoryCleanupIterator);
 };
 
-class JSWeakFactoryCleanupTask : public v8::Task {
- public:
-  inline explicit JSWeakFactoryCleanupTask(Isolate* isolate);
-  void Run() override;
-
- private:
-  v8::Persistent<v8::Context> native_context_;
-  Isolate* isolate_;
-
-  DISALLOW_IMPLICIT_CONSTRUCTORS(JSWeakFactoryCleanupTask);
-};
-
 }  // namespace internal
 }  // namespace v8
 

test/mjsunit/harmony/weakrefs/cleanup-is-a-microtask.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
+
+// This test asserts that the cleanup function call, scheduled by GC, is a
+// microtask and not a normal task.
+
+// Inside a microtask, cause GC (which should schedule the cleanup as
+// microtask).  lso schedule another microtask. Assert that the cleanup
+// function ran before the other microtask.
+
+function scheduleMicrotask(func) {
+  Promise.resolve().then(func);
+}
+
+let log = [];
+
+let cleanup = (iter) => {
+  log.push("cleanup");
+  for (wc of iter) { }
+}
+
+let wf = new WeakFactory(cleanup);
+let o = null;
+
+(function() {
+  // Use a closure here to avoid other references to o which might keep it alive
+  // (e.g., stack frames pointing to it).
+  o = {};
+  wf.makeCell(o);
+})();
+
+let microtask_after_cleanup = () => {
+  log.push("microtask_after_cleanup");
+}
+
+let first_microtask = function() {
+  log.push("first_microtask");
+
+  // This schedules the cleanup function as microtask.
+  o = null;
+  gc();
+
+  // Schedule a microtask which should run after the cleanup microtask.
+  scheduleMicrotask(microtask_after_cleanup);
+}
+
+scheduleMicrotask(first_microtask);
+
+setTimeout(() => {
+  // Assert that the functions got called in the right order.
+  let wanted_log = ["first_microtask", "cleanup", "microtask_after_cleanup"];
+  assertEquals(wanted_log, log);
+}, 0);