deps: V8: cherry-pick 9f0f2cb7f08d

Original commit message: [weakrefs] Call Isolate::ClearKeptObjects() as part of microtask checkpoint In the spec, WeakRefs that are dereferenced are kept alive until there's no JS on the stack, and then the host is expected to call ClearKeptObjects to clear those strong references [1]. HTML calls ClearKeptObjects at the end of a PerformMicrotaskCheckpoint [2]. In V8, leaving this up to the embedder is error prone in the same way the deprecated FinalizationGroup callback APIs were error prone: it depends on the embedder doing the right thing. This CL moves the call to ClearKeptObjects to be after running of microtasks within V8. However, the Isolate::ClearKeptObjects API should not be removed or deprecated in case an embedder uses an entirely custom MicrotaskQueue implementation and invokes MicrotaskQueue::PerformCheckpoint manually. [1] https://tc39.es/proposal-weakrefs/#sec-clear-kept-objects [2] whatwg/html#4571 Bug: v8:8179 Change-Id: Ie243804157b56241ca69ed8fad300e839a0c9f75 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2055967 Commit-Queue: Shu-yu Guo <syg@chromium.org> Reviewed-by: Ulan Degenbaev <ulan@chromium.org> Reviewed-by: Ross McIlroy <rmcilroy@chromium.org> Cr-Commit-Position: refs/heads/master@{#66327} Refs: v8/v8@9f0f2cb PR-URL: #32885 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Ujjwal Sharma <ryzokuken@disroot.org> Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de> Reviewed-By: Beth Griggs <Bethany.Griggs@uk.ibm.com>
nodejs · Apr 20, 2020 · da728c4 · da728c4
1 parent 2ee8b4a
commit da728c4
Show file tree

Hide file tree

Showing 11 changed files with 93 additions and 65 deletions.
diff --git a/common.gypi b/common.gypi
@@ -35,7 +35,7 @@
 
     # Reset this number to 0 on major V8 upgrades.
     # Increment by one for each non-official patch applied to deps/v8.
-    'v8_embedder_string': '-node.20',
+    'v8_embedder_string': '-node.21',
 
     ##### V8 defaults for Node.js #####
 

diff --git a/deps/v8/include/v8.h b/deps/v8/include/v8.h
@@ -7241,10 +7241,10 @@ typedef void (*MicrotasksCompletedCallback)(Isolate*);
 typedef void (*MicrotasksCompletedCallbackWithData)(Isolate*, void*);
 typedef void (*MicrotaskCallback)(void* data);
 
-
 /**
  * Policy for running microtasks:
- *   - explicit: microtasks are invoked with Isolate::RunMicrotasks() method;
+ *   - explicit: microtasks are invoked with the
+ *               Isolate::PerformMicrotaskCheckpoint() method;
  *   - scoped: microtasks invocation is controlled by MicrotasksScope objects;
  *   - auto: microtasks are invoked when the script call depth decrements
  *           to zero.
@@ -7335,7 +7335,7 @@ class V8_EXPORT MicrotaskQueue {
 };
 
 /**
- * This scope is used to control microtasks when kScopeMicrotasksInvocation
+ * This scope is used to control microtasks when MicrotasksPolicy::kScoped
  * is used on Isolate. In this mode every non-primitive call to V8 should be
  * done inside some MicrotasksScope.
  * Microtasks are executed when topmost MicrotasksScope marked as kRunMicrotasks
@@ -8416,10 +8416,13 @@ class V8_EXPORT Isolate {
    * objects are originally built when a WeakRef is created or
    * successfully dereferenced.
    *
-   * The embedder is expected to call this when a synchronous sequence
-   * of ECMAScript execution completes. It's the embedders
-   * responsiblity to make this call at a time which does not
-   * interrupt synchronous ECMAScript code execution.
+   * This is invoked automatically after microtasks are run. See
+   * MicrotasksPolicy for when microtasks are run.
+   *
+   * This needs to be manually invoked only if the embedder is manually running
+   * microtasks via a custom MicrotaskQueue class's PerformCheckpoint. In that
+   * case, it is the embedder's responsibility to make this call at a time which
+   * does not interrupt synchronous ECMAScript code execution.
    */
   void ClearKeptObjects();
 
@@ -8944,10 +8947,18 @@ class V8_EXPORT Isolate {
   void SetPromiseRejectCallback(PromiseRejectCallback callback);
 
   /**
-   * Runs the default MicrotaskQueue until it gets empty.
-   * Any exceptions thrown by microtask callbacks are swallowed.
+   * An alias for PerformMicrotaskCheckpoint.
+   */
+  V8_DEPRECATE_SOON("Use PerformMicrotaskCheckpoint.")
+  void RunMicrotasks() { PerformMicrotaskCheckpoint(); }
+
+  /**
+   * Runs the default MicrotaskQueue until it gets empty and perform other
+   * microtask checkpoint steps, such as calling ClearKeptObjects. Asserts that
+   * the MicrotasksPolicy is not kScoped. Any exceptions thrown by microtask
+   * callbacks are swallowed.
    */
-  void RunMicrotasks();
+  void PerformMicrotaskCheckpoint();
 
   /**
    * Enqueues the callback to the default MicrotaskQueue

diff --git a/deps/v8/src/api/api.cc b/deps/v8/src/api/api.cc
@@ -309,6 +309,7 @@ class CallDepthScope {
 #ifdef V8_CHECK_MICROTASKS_SCOPES_CONSISTENCY
     if (do_callback) CheckMicrotasksScopesConsistency(microtask_queue);
 #endif
+    DCHECK(CheckKeptObjectsClearedAfterMicrotaskCheckpoint(microtask_queue));
     isolate_->set_next_v8_call_is_safe_for_termination(safe_for_termination_);
   }
 
@@ -323,6 +324,15 @@ class CallDepthScope {
   }
 
  private:
+  bool CheckKeptObjectsClearedAfterMicrotaskCheckpoint(
+      i::MicrotaskQueue* microtask_queue) {
+    bool did_perform_microtask_checkpoint =
+        do_callback && microtask_queue &&
+        microtask_queue->microtasks_policy() == MicrotasksPolicy::kAuto;
+    return !did_perform_microtask_checkpoint ||
+           isolate_->heap()->weak_refs_keep_during_job().IsUndefined(isolate_);
+  }
+
   i::Isolate* const isolate_;
   Local<Context> context_;
   bool escaped_;
@@ -8588,10 +8598,10 @@ void Isolate::SetPromiseRejectCallback(PromiseRejectCallback callback) {
   isolate->SetPromiseRejectCallback(callback);
 }
 
-void Isolate::RunMicrotasks() {
+void Isolate::PerformMicrotaskCheckpoint() {
   DCHECK_NE(MicrotasksPolicy::kScoped, GetMicrotasksPolicy());
   i::Isolate* isolate = reinterpret_cast<i::Isolate*>(this);
-  isolate->default_microtask_queue()->RunMicrotasks(isolate);
+  isolate->default_microtask_queue()->PerformCheckpoint(this);
 }
 
 void Isolate::EnqueueMicrotask(Local<Function> v8_function) {

diff --git a/deps/v8/src/d8/d8.cc b/deps/v8/src/d8/d8.cc
@@ -1066,7 +1066,7 @@ bool Shell::ExecuteModule(Isolate* isolate, const char* file_name) {
     // able to just busy loop waiting for execution to finish.
     Local<Promise> result_promise(Local<Promise>::Cast(result));
     while (result_promise->State() == Promise::kPending) {
-      isolate->RunMicrotasks();
+      isolate->PerformMicrotaskCheckpoint();
     }
 
     if (result_promise->State() == Promise::kRejected) {
@@ -3297,7 +3297,6 @@ bool ProcessMessages(
     while (v8::platform::PumpMessageLoop(g_default_platform, isolate,
                                          behavior())) {
       MicrotasksScope::PerformCheckpoint(isolate);
-      isolate->ClearKeptObjects();
     }
     if (g_default_platform->IdleTasksEnabled(isolate)) {
       v8::platform::RunIdleTasks(g_default_platform, isolate,

diff --git a/deps/v8/src/execution/microtask-queue.cc b/deps/v8/src/execution/microtask-queue.cc
@@ -115,6 +115,7 @@ void MicrotaskQueue::PerformCheckpoint(v8::Isolate* v8_isolate) {
       !HasMicrotasksSuppressions()) {
     Isolate* isolate = reinterpret_cast<Isolate*>(v8_isolate);
     RunMicrotasks(isolate);
+    isolate->ClearKeptObjects();
   }
 }
 

diff --git a/deps/v8/test/cctest/test-api.cc b/deps/v8/test/cctest/test-api.cc
@@ -19911,7 +19911,7 @@ TEST(RunMicrotasksIgnoresThrownExceptionsFromApi) {
     CHECK(!isolate->IsExecutionTerminating());
     isolate->EnqueueMicrotask(ThrowExceptionMicrotask);
     isolate->EnqueueMicrotask(IncrementCounterMicrotask);
-    isolate->RunMicrotasks();
+    isolate->PerformMicrotaskCheckpoint();
     CHECK_EQ(1, microtask_callback_count);
     CHECK(!try_catch.HasCaught());
   }
@@ -19953,7 +19953,7 @@ TEST(SetAutorunMicrotasks) {
   CHECK_EQ(0, CompileRun("ext2Calls")->Int32Value(env.local()).FromJust());
   CHECK_EQ(1u, microtasks_completed_callback_count);
 
-  env->GetIsolate()->RunMicrotasks();
+  env->GetIsolate()->PerformMicrotaskCheckpoint();
   CHECK_EQ(2, CompileRun("ext1Calls")->Int32Value(env.local()).FromJust());
   CHECK_EQ(1, CompileRun("ext2Calls")->Int32Value(env.local()).FromJust());
   CHECK_EQ(2u, microtasks_completed_callback_count);
@@ -19965,7 +19965,7 @@ TEST(SetAutorunMicrotasks) {
   CHECK_EQ(1, CompileRun("ext2Calls")->Int32Value(env.local()).FromJust());
   CHECK_EQ(2u, microtasks_completed_callback_count);
 
-  env->GetIsolate()->RunMicrotasks();
+  env->GetIsolate()->PerformMicrotaskCheckpoint();
   CHECK_EQ(2, CompileRun("ext1Calls")->Int32Value(env.local()).FromJust());
   CHECK_EQ(2, CompileRun("ext2Calls")->Int32Value(env.local()).FromJust());
   CHECK_EQ(3u, microtasks_completed_callback_count);
@@ -20015,7 +20015,7 @@ TEST(RunMicrotasksWithoutEnteringContext) {
     isolate->EnqueueMicrotask(
         Function::New(context, MicrotaskOne).ToLocalChecked());
   }
-  isolate->RunMicrotasks();
+  isolate->PerformMicrotaskCheckpoint();
   {
     Context::Scope context_scope(context);
     CHECK_EQ(1, CompileRun("ext1Calls")->Int32Value(context).FromJust());
@@ -20039,7 +20039,7 @@ static void Regress808911_CurrentContextWrapper(
   CHECK(isolate->GetCurrentContext() !=
         isolate->GetEnteredOrMicrotaskContext());
   isolate->EnqueueMicrotask(Regress808911_MicrotaskCallback, isolate);
-  isolate->RunMicrotasks();
+  isolate->PerformMicrotaskCheckpoint();
 }
 
 THREADED_TEST(Regress808911) {
@@ -22507,7 +22507,7 @@ TEST(PromiseThen) {
                   .ToLocalChecked()
                   ->Int32Value(context.local())
                   .FromJust());
-  isolate->RunMicrotasks();
+  isolate->PerformMicrotaskCheckpoint();
   CHECK_EQ(1, global->Get(context.local(), v8_str("x1"))
                   .ToLocalChecked()
                   ->Int32Value(context.local())
@@ -22533,7 +22533,7 @@ TEST(PromiseThen) {
                   .ToLocalChecked()
                   ->Int32Value(context.local())
                   .FromJust());
-  isolate->RunMicrotasks();
+  isolate->PerformMicrotaskCheckpoint();
   CHECK_EQ(0, global->Get(context.local(), v8_str("x1"))
                   .ToLocalChecked()
                   ->Int32Value(context.local())
@@ -22553,7 +22553,7 @@ TEST(PromiseThen) {
                   .ToLocalChecked()
                   ->Int32Value(context.local())
                   .FromJust());
-  isolate->RunMicrotasks();
+  isolate->PerformMicrotaskCheckpoint();
   CHECK_EQ(3, global->Get(context.local(), v8_str("x1"))
                   .ToLocalChecked()
                   ->Int32Value(context.local())
@@ -22602,7 +22602,7 @@ TEST(PromiseThen2) {
                   .ToLocalChecked()
                   ->Int32Value(context.local())
                   .FromJust());
-  isolate->RunMicrotasks();
+  isolate->PerformMicrotaskCheckpoint();
   CHECK_EQ(1, global->Get(context.local(), v8_str("x1"))
                   .ToLocalChecked()
                   ->Int32Value(context.local())
@@ -22613,7 +22613,7 @@ TEST(PromiseThen2) {
                   .FromJust());
 
   Local<v8::Promise> b = a->Then(context.local(), f3, f2).ToLocalChecked();
-  isolate->RunMicrotasks();
+  isolate->PerformMicrotaskCheckpoint();
   CHECK_EQ(1, global->Get(context.local(), v8_str("x1"))
                   .ToLocalChecked()
                   ->Int32Value(context.local())
@@ -22624,7 +22624,7 @@ TEST(PromiseThen2) {
                   .FromJust());
 
   Local<v8::Promise> c = b->Then(context.local(), f1, f2).ToLocalChecked();
-  isolate->RunMicrotasks();
+  isolate->PerformMicrotaskCheckpoint();
   CHECK_EQ(1, global->Get(context.local(), v8_str("x1"))
                   .ToLocalChecked()
                   ->Int32Value(context.local())
@@ -22635,7 +22635,7 @@ TEST(PromiseThen2) {
                     .FromJust());
 
   v8::Local<v8::Promise> d = c->Then(context.local(), f1, f2).ToLocalChecked();
-  isolate->RunMicrotasks();
+  isolate->PerformMicrotaskCheckpoint();
   CHECK_EQ(103, global->Get(context.local(), v8_str("x1"))
                     .ToLocalChecked()
                     ->Int32Value(context.local())
@@ -22646,7 +22646,7 @@ TEST(PromiseThen2) {
                     .FromJust());
 
   v8::Local<v8::Promise> e = d->Then(context.local(), f3, f2).ToLocalChecked();
-  isolate->RunMicrotasks();
+  isolate->PerformMicrotaskCheckpoint();
   CHECK_EQ(103, global->Get(context.local(), v8_str("x1"))
                     .ToLocalChecked()
                     ->Int32Value(context.local())
@@ -22657,7 +22657,7 @@ TEST(PromiseThen2) {
                     .FromJust());
 
   v8::Local<v8::Promise> f = e->Then(context.local(), f1, f3).ToLocalChecked();
-  isolate->RunMicrotasks();
+  isolate->PerformMicrotaskCheckpoint();
   CHECK_EQ(103, global->Get(context.local(), v8_str("x1"))
                     .ToLocalChecked()
                     ->Int32Value(context.local())
@@ -22668,7 +22668,7 @@ TEST(PromiseThen2) {
                     .FromJust());
 
   f->Then(context.local(), f1, f2).ToLocalChecked();
-  isolate->RunMicrotasks();
+  isolate->PerformMicrotaskCheckpoint();
   CHECK_EQ(103, global->Get(context.local(), v8_str("x1"))
                     .ToLocalChecked()
                     ->Int32Value(context.local())
@@ -25735,7 +25735,7 @@ TEST(DynamicImport) {
   i::MaybeHandle<i::JSPromise> maybe_promise =
       i_isolate->RunHostImportModuleDynamicallyCallback(referrer, specifier);
   i::Handle<i::JSPromise> promise = maybe_promise.ToHandleChecked();
-  isolate->RunMicrotasks();
+  isolate->PerformMicrotaskCheckpoint();
   CHECK(result->Equals(i::String::cast(promise->result())));
 }
 
@@ -26435,7 +26435,7 @@ TEST(MicrotaskContextShouldBeNativeContext) {
       "  await 42;"
       "})().then(callback);}");
 
-  isolate->RunMicrotasks();
+  isolate->PerformMicrotaskCheckpoint();
 }
 
 TEST(PreviewSetKeysIteratorEntriesWithDeleted) {

diff --git a/deps/v8/test/cctest/test-js-weak-refs.cc b/deps/v8/test/cctest/test-js-weak-refs.cc
@@ -158,6 +158,17 @@ void VerifyWeakCellKeyChain(Isolate* isolate, SimpleNumberDictionary key_map,
   va_end(args);
 }
 
+Handle<JSWeakRef> MakeWeakRefAndKeepDuringJob(Isolate* isolate) {
+  HandleScope inner_scope(isolate);
+
+  Handle<JSObject> js_object =
+      isolate->factory()->NewJSObject(isolate->object_function());
+  Handle<JSWeakRef> inner_weak_ref = ConstructJSWeakRef(js_object, isolate);
+  isolate->heap()->KeepDuringJob(js_object);
+
+  return inner_scope.CloseAndEscape(inner_weak_ref);
+}
+
 }  // namespace
 
 TEST(TestRegister) {
@@ -715,30 +726,35 @@ TEST(TestJSWeakRefKeepDuringJob) {
   LocalContext context;
 
   Isolate* isolate = CcTest::i_isolate();
-  Heap* heap = isolate->heap();
   HandleScope outer_scope(isolate);
-  Handle<JSWeakRef> weak_ref;
-  {
-    HandleScope inner_scope(isolate);
-
-    Handle<JSObject> js_object =
-        isolate->factory()->NewJSObject(isolate->object_function());
-    Handle<JSWeakRef> inner_weak_ref = ConstructJSWeakRef(js_object, isolate);
-    heap->KeepDuringJob(js_object);
+  Handle<JSWeakRef> weak_ref = MakeWeakRefAndKeepDuringJob(isolate);
+  CHECK(!weak_ref->target().IsUndefined(isolate));
+  CcTest::CollectAllGarbage();
+  CHECK(!weak_ref->target().IsUndefined(isolate));
 
-    weak_ref = inner_scope.CloseAndEscape(inner_weak_ref);
-  }
+  // Clears the KeepDuringJob set.
+  context->GetIsolate()->ClearKeptObjects();
+  CcTest::CollectAllGarbage();
+  CHECK(weak_ref->target().IsUndefined(isolate));
 
+  weak_ref = MakeWeakRefAndKeepDuringJob(isolate);
+  CHECK(!weak_ref->target().IsUndefined(isolate));
+  CcTest::CollectAllGarbage();
   CHECK(!weak_ref->target().IsUndefined(isolate));
 
+  // ClearKeptObjects should be called by PerformMicrotasksCheckpoint.
+  CcTest::isolate()->PerformMicrotaskCheckpoint();
   CcTest::CollectAllGarbage();
+  CHECK(weak_ref->target().IsUndefined(isolate));
 
+  weak_ref = MakeWeakRefAndKeepDuringJob(isolate);
   CHECK(!weak_ref->target().IsUndefined(isolate));
-
-  // Clears the KeepDuringJob set.
-  context->GetIsolate()->ClearKeptObjects();
   CcTest::CollectAllGarbage();
+  CHECK(!weak_ref->target().IsUndefined(isolate));
 
+  // ClearKeptObjects should be called by MicrotasksScope::PerformCheckpoint.
+  v8::MicrotasksScope::PerformCheckpoint(CcTest::isolate());
+  CcTest::CollectAllGarbage();
   CHECK(weak_ref->target().IsUndefined(isolate));
 }
 
@@ -754,17 +770,7 @@ TEST(TestJSWeakRefKeepDuringJobIncrementalMarking) {
   Isolate* isolate = CcTest::i_isolate();
   Heap* heap = isolate->heap();
   HandleScope outer_scope(isolate);
-  Handle<JSWeakRef> weak_ref;
-  {
-    HandleScope inner_scope(isolate);
-
-    Handle<JSObject> js_object =
-        isolate->factory()->NewJSObject(isolate->object_function());
-    Handle<JSWeakRef> inner_weak_ref = ConstructJSWeakRef(js_object, isolate);
-    heap->KeepDuringJob(js_object);
-
-    weak_ref = inner_scope.CloseAndEscape(inner_weak_ref);
-  }
+  Handle<JSWeakRef> weak_ref = MakeWeakRefAndKeepDuringJob(isolate);
 
   CHECK(!weak_ref->target().IsUndefined(isolate));
 

diff --git a/deps/v8/test/cctest/test-modules.cc b/deps/v8/test/cctest/test-modules.cc
@@ -830,7 +830,7 @@ TEST(ModuleEvaluationTopLevelAwaitDynamicImport) {
     CHECK_EQ(promise->State(), v8::Promise::kPending);
     CHECK(!try_catch.HasCaught());
 
-    isolate->RunMicrotasks();
+    isolate->PerformMicrotaskCheckpoint();
     CHECK_EQ(promise->State(), v8::Promise::kFulfilled);
   }
   i::FLAG_harmony_top_level_await = previous_top_level_await_flag_value;
@@ -874,7 +874,7 @@ TEST(ModuleEvaluationTopLevelAwaitDynamicImportError) {
     CHECK_EQ(promise->State(), v8::Promise::kPending);
     CHECK(!try_catch.HasCaught());
 
-    isolate->RunMicrotasks();
+    isolate->PerformMicrotaskCheckpoint();
     CHECK_EQ(Module::kErrored, module->GetStatus());
     CHECK_EQ(promise->State(), v8::Promise::kRejected);
     CHECK(promise->Result()->StrictEquals(v8_str("boom")));