Reland "Do not enqueue or run a microtask on detached contexts"

This is a reland of 734a6575

Original change's description:
> Do not enqueue or run a microtask on detached contexts
>
> This CL disables EnqueueMicrotask and RunMicrotasks on detached
> contexts. That is, if an embedder call DetachGlobal() on a v8::Context,
> EnqueueMicrotask on that context will not take effect, and all Microtask
> that is enqueued before DetachGlobal will be cancelled.
>
> On Blink, this implies that a frame will no longer run a microtask after
> it's navigated away. OTOH, detached frames in Blink are not affected.
>
> Bug: v8:8124
> Change-Id: I5b00ceef5ea2afb87cf067a65eb95c29bf91176d
> Reviewed-on: https://chromium-review.googlesource.com/c/1416071
> Reviewed-by: Toon Verwaest <verwaest@chromium.org>
> Reviewed-by: Yang Guo <yangguo@chromium.org>
> Reviewed-by: Benedikt Meurer <bmeurer@chromium.org>
> Reviewed-by: Adam Klein <adamk@chromium.org>
> Commit-Queue: Taiju Tsuiki <tzik@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#59445}

Tbr: adamk@chromium.org, yangguo@chromium.org, verwaest@chromium.org
Bug: v8:8124
Change-Id: I959a18ae214f1385d5f453b3ed94772e60f71e0f
Reviewed-on: https://chromium-review.googlesource.com/c/1469544
Commit-Queue: Taiju Tsuiki <tzik@chromium.org>
Reviewed-by: Benedikt Meurer <bmeurer@chromium.org>
Cr-Commit-Position: refs/heads/master@{#59884}

src/bootstrapper.cc

@@ -353,6 +353,8 @@ void Bootstrapper::DetachGlobal(Handle<Context> env) {
   if (FLAG_track_detached_contexts) {
     isolate_->AddDetachedContext(env);
   }
+
+  env->native_context()->set_microtask_queue(nullptr);
 }
 
 namespace {

src/builtins/builtins-microtask-queue-gen.cc

@@ -35,7 +35,7 @@ class MicrotaskQueueBuiltinsAssembler : public CodeStubAssembler {
                                            TNode<IntPtrT> start,
                                            TNode<IntPtrT> index);
 
-  void PrepareForContext(TNode<Context> microtask_context);
+  void PrepareForContext(TNode<Context> microtask_context, Label* bailout);
   void RunSingleMicrotask(TNode<Context> current_context,
                           TNode<Microtask> microtask);
   void IncrementFinishedMicrotaskCount(TNode<RawPtrT> microtask_queue);
@@ -105,8 +105,13 @@ TNode<IntPtrT> MicrotaskQueueBuiltinsAssembler::CalculateRingBufferOffset(
 }
 
 void MicrotaskQueueBuiltinsAssembler::PrepareForContext(
-    TNode<Context> native_context) {
+    TNode<Context> native_context, Label* bailout) {
   CSA_ASSERT(this, IsNativeContext(native_context));
+
+  // Skip the microtask execution if the associated context is shutdown.
+  GotoIf(WordEqual(GetMicrotaskQueue(native_context), IntPtrConstant(0)),
+         bailout);
+
   EnterMicrotaskContext(native_context);
   SetCurrentContext(native_context);
 }
@@ -151,7 +156,7 @@ void MicrotaskQueueBuiltinsAssembler::RunSingleMicrotask(
     TNode<Context> microtask_context =
         LoadObjectField<Context>(microtask, CallableTask::kContextOffset);
     TNode<Context> native_context = LoadNativeContext(microtask_context);
-    PrepareForContext(native_context);
+    PrepareForContext(native_context, &done);
 
     TNode<JSReceiver> callable =
         LoadObjectField<JSReceiver>(microtask, CallableTask::kCallableOffset);
@@ -194,7 +199,7 @@ void MicrotaskQueueBuiltinsAssembler::RunSingleMicrotask(
     TNode<Context> microtask_context = LoadObjectField<Context>(
         microtask, PromiseResolveThenableJobTask::kContextOffset);
     TNode<Context> native_context = LoadNativeContext(microtask_context);
-    PrepareForContext(native_context);
+    PrepareForContext(native_context, &done);
 
     Node* const promise_to_resolve = LoadObjectField(
         microtask, PromiseResolveThenableJobTask::kPromiseToResolveOffset);
@@ -218,7 +223,7 @@ void MicrotaskQueueBuiltinsAssembler::RunSingleMicrotask(
     TNode<Context> microtask_context = LoadObjectField<Context>(
         microtask, PromiseReactionJobTask::kContextOffset);
     TNode<Context> native_context = LoadNativeContext(microtask_context);
-    PrepareForContext(native_context);
+    PrepareForContext(native_context, &done);
 
     Node* const argument =
         LoadObjectField(microtask, PromiseReactionJobTask::kArgumentOffset);
@@ -251,7 +256,7 @@ void MicrotaskQueueBuiltinsAssembler::RunSingleMicrotask(
     TNode<Context> microtask_context = LoadObjectField<Context>(
         microtask, PromiseReactionJobTask::kContextOffset);
     TNode<Context> native_context = LoadNativeContext(microtask_context);
-    PrepareForContext(native_context);
+    PrepareForContext(native_context, &done);
 
     Node* const argument =
         LoadObjectField(microtask, PromiseReactionJobTask::kArgumentOffset);
@@ -287,7 +292,7 @@ void MicrotaskQueueBuiltinsAssembler::RunSingleMicrotask(
             FinalizationGroupCleanupJobTask::kFinalizationGroupOffset);
     TNode<Context> native_context = LoadObjectField<Context>(
         finalization_group, JSFinalizationGroup::kNativeContextOffset);
-    PrepareForContext(native_context);
+    PrepareForContext(native_context, &done);
 
     Node* const result = CallRuntime(Runtime::kFinalizationGroupCleanupJob,
                                      native_context, finalization_group);
@@ -472,6 +477,11 @@ TF_BUILTIN(EnqueueMicrotask, MicrotaskQueueBuiltinsAssembler) {
   TNode<Context> native_context = LoadNativeContext(context);
   TNode<RawPtrT> microtask_queue = GetMicrotaskQueue(native_context);
 
+  // Do not store the microtask if MicrotaskQueue is not available, that may
+  // happen when the context shutdown.
+  Label if_shutdown(this, Label::kDeferred);
+  GotoIf(WordEqual(microtask_queue, IntPtrConstant(0)), &if_shutdown);
+
   TNode<RawPtrT> ring_buffer = GetMicrotaskRingBuffer(microtask_queue);
   TNode<IntPtrT> capacity = GetMicrotaskQueueCapacity(microtask_queue);
   TNode<IntPtrT> size = GetMicrotaskQueueSize(microtask_queue);
@@ -504,6 +514,9 @@ TF_BUILTIN(EnqueueMicrotask, MicrotaskQueueBuiltinsAssembler) {
                    isolate_constant, microtask_queue, microtask);
     Return(UndefinedConstant());
   }
+
+  Bind(&if_shutdown);
+  Return(UndefinedConstant());
 }
 
 TF_BUILTIN(RunMicrotasks, MicrotaskQueueBuiltinsAssembler) {

src/objects/js-objects.h

@@ -100,7 +100,8 @@ class JSReceiver : public HeapObject {
       bool use_set = true);
 
   // Implementation of [[HasProperty]], ECMA-262 5th edition, section 8.12.6.
-  V8_WARN_UNUSED_RESULT static Maybe<bool> HasProperty(LookupIterator* it);
+  V8_EXPORT_PRIVATE V8_WARN_UNUSED_RESULT static Maybe<bool> HasProperty(
+      LookupIterator* it);
   V8_WARN_UNUSED_RESULT static inline Maybe<bool> HasProperty(
       Handle<JSReceiver> object, Handle<Name> name);
   V8_WARN_UNUSED_RESULT static inline Maybe<bool> HasElement(

test/unittests/microtask-queue-unittest.cc

@@ -10,7 +10,10 @@
 #include <vector>
 
 #include "src/heap/factory.h"
+#include "src/objects-inl.h"
 #include "src/objects/foreign.h"
+#include "src/objects/js-array-inl.h"
+#include "src/objects/js-objects-inl.h"
 #include "src/objects/promise-inl.h"
 #include "src/visitors.h"
 #include "test/unittests/test-utils.h"
@@ -26,7 +29,29 @@ void RunStdFunction(void* data) {
   (*f)();
 }
 
-class MicrotaskQueueTest : public TestWithNativeContext {
+template <typename TMixin>
+class WithFinalizationGroupMixin : public TMixin {
+ public:
+  WithFinalizationGroupMixin() {
+    FLAG_harmony_weak_refs = true;
+    FLAG_expose_gc = true;
+  }
+
+ private:
+  SaveFlags save_flags_;
+
+  DISALLOW_COPY_AND_ASSIGN(WithFinalizationGroupMixin);
+};
+
+using TestWithNativeContextAndFinalizationGroup =  //
+    WithInternalIsolateMixin<                      //
+        WithContextMixin<                          //
+            WithFinalizationGroupMixin<            //
+                WithIsolateScopeMixin<             //
+                    WithSharedIsolateMixin<        //
+                        ::testing::Test>>>>>;
+
+class MicrotaskQueueTest : public TestWithNativeContextAndFinalizationGroup {
  public:
   template <typename F>
   Handle<Microtask> NewMicrotask(F&& f) {
@@ -56,6 +81,11 @@ class MicrotaskQueueTest : public TestWithNativeContext {
     microtask_queue_ = nullptr;
   }
 
+  template <size_t N>
+  Handle<Name> NameFromChars(const char (&chars)[N]) {
+    return isolate()->factory()->NewStringFromStaticChars(chars);
+  }
+
  private:
   std::unique_ptr<MicrotaskQueue> microtask_queue_;
 };
@@ -280,5 +310,193 @@ TEST_F(MicrotaskQueueTest, PromiseHandlerContext) {
   v8_context2->DetachGlobal();
 }
 
+TEST_F(MicrotaskQueueTest, DetachGlobal_Enqueue) {
+  EXPECT_EQ(0, microtask_queue()->size());
+
+  // Detach MicrotaskQueue from the current context.
+  context()->DetachGlobal();
+
+  // No microtask should be enqueued after DetachGlobal call.
+  EXPECT_EQ(0, microtask_queue()->size());
+  RunJS("Promise.resolve().then(()=>{})");
+  EXPECT_EQ(0, microtask_queue()->size());
+}
+
+TEST_F(MicrotaskQueueTest, DetachGlobal_Run) {
+  EXPECT_EQ(0, microtask_queue()->size());
+
+  // Enqueue microtasks to the current context.
+  Handle<JSArray> ran = RunJS<JSArray>(
+      "var ran = [false, false, false, false];"
+      "Promise.resolve().then(() => { ran[0] = true; });"
+      "Promise.reject().catch(() => { ran[1] = true; });"
+      "ran");
+
+  Handle<JSFunction> function =
+      RunJS<JSFunction>("(function() { ran[2] = true; })");
+  Handle<CallableTask> callable =
+      factory()->NewCallableTask(function, Utils::OpenHandle(*context()));
+  microtask_queue()->EnqueueMicrotask(*callable);
+
+  // The handler should not run at this point.
+  const int kNumExpectedTasks = 3;
+  for (int i = 0; i < kNumExpectedTasks; ++i) {
+    EXPECT_TRUE(
+        Object::GetElement(isolate(), ran, i).ToHandleChecked()->IsFalse());
+  }
+  EXPECT_EQ(kNumExpectedTasks, microtask_queue()->size());
+
+  // Detach MicrotaskQueue from the current context.
+  context()->DetachGlobal();
+
+  // RunMicrotasks processes pending Microtasks, but Microtasks that are
+  // associated to a detached context should be cancelled and should not take
+  // effect.
+  microtask_queue()->RunMicrotasks(isolate());
+  EXPECT_EQ(0, microtask_queue()->size());
+  for (int i = 0; i < kNumExpectedTasks; ++i) {
+    EXPECT_TRUE(
+        Object::GetElement(isolate(), ran, i).ToHandleChecked()->IsFalse());
+  }
+}
+
+TEST_F(MicrotaskQueueTest, DetachGlobal_FinalizationGroup) {
+  // Enqueue an FinalizationGroupCleanupTask.
+  Handle<JSArray> ran = RunJS<JSArray>(
+      "var ran = [false];"
+      "var wf = new FinalizationGroup(() => { ran[0] = true; });"
+      "(function() { wf.register({}, {}); })();"
+      "gc();"
+      "ran");
+
+  EXPECT_TRUE(
+      Object::GetElement(isolate(), ran, 0).ToHandleChecked()->IsFalse());
+  EXPECT_EQ(1, microtask_queue()->size());
+
+  // Detach MicrotaskQueue from the current context.
+  context()->DetachGlobal();
+
+  microtask_queue()->RunMicrotasks(isolate());
+
+  // RunMicrotasks processes the pending Microtask, but Microtasks that are
+  // associated to a detached context should be cancelled and should not take
+  // effect.
+  EXPECT_EQ(0, microtask_queue()->size());
+  EXPECT_TRUE(
+      Object::GetElement(isolate(), ran, 0).ToHandleChecked()->IsFalse());
+}
+
+namespace {
+
+void DummyPromiseHook(PromiseHookType type, Local<Promise> promise,
+                      Local<Value> parent) {}
+
+}  // namespace
+
+TEST_F(MicrotaskQueueTest, DetachGlobal_PromiseResolveThenableJobTask) {
+  // Use a PromiseHook to switch the implementation to ResolvePromise runtime,
+  // instead of ResolvePromise builtin.
+  v8_isolate()->SetPromiseHook(&DummyPromiseHook);
+
+  RunJS(
+      "var resolve;"
+      "var promise = new Promise(r => { resolve = r; });"
+      "promise.then(() => {});"
+      "resolve({});");
+
+  // A PromiseResolveThenableJobTask is pending in the MicrotaskQueue.
+  EXPECT_EQ(1, microtask_queue()->size());
+
+  // Detach MicrotaskQueue from the current context.
+  context()->DetachGlobal();
+
+  // RunMicrotasks processes the pending Microtask, but Microtasks that are
+  // associated to a detached context should be cancelled and should not take
+  // effect.
+  // As PromiseResolveThenableJobTask queues another task for resolution,
+  // the return value is 2 if it ran.
+  EXPECT_EQ(1, microtask_queue()->RunMicrotasks(isolate()));
+  EXPECT_EQ(0, microtask_queue()->size());
+}
+
+TEST_F(MicrotaskQueueTest, DetachGlobal_HandlerContext) {
+  // EnqueueMicrotask should use the context associated to the handler instead
+  // of the current context. E.g.
+  //   // At Context A.
+  //   let resolved = Promise.resolve();
+  //   // Call DetachGlobal on A, so that microtasks associated to A is
+  //   // cancelled.
+  //
+  //   // At Context B.
+  //   let handler = () => {
+  //     console.log("here");
+  //   };
+  //   // The microtask to run |handler| should be associated to B instead of A,
+  //   // so that handler runs even |resolved| is on the detached context A.
+  //   resolved.then(handler);
+
+  Handle<JSReceiver> results = isolate()->factory()->NewJSObjectWithNullProto();
+
+  // These belong to a stale Context.
+  Handle<JSPromise> stale_resolved_promise;
+  Handle<JSPromise> stale_rejected_promise;
+  Handle<JSReceiver> stale_handler;
+
+  Local<v8::Context> sub_context = v8::Context::New(v8_isolate());
+  {
+    v8::Context::Scope scope(sub_context);
+    stale_resolved_promise = RunJS<JSPromise>("Promise.resolve()");
+    stale_rejected_promise = RunJS<JSPromise>("Promise.reject()");
+    stale_handler = RunJS<JSReceiver>(
+        "(results, label) => {"
+        "  results[label] = true;"
+        "}");
+  }
+  // DetachGlobal() cancells all microtasks associated to the context.
+  sub_context->DetachGlobal();
+  sub_context.Clear();
+
+  SetGlobalProperty("results", Utils::ToLocal(results));
+  SetGlobalProperty(
+      "stale_resolved_promise",
+      Utils::ToLocal(Handle<JSReceiver>::cast(stale_resolved_promise)));
+  SetGlobalProperty(
+      "stale_rejected_promise",
+      Utils::ToLocal(Handle<JSReceiver>::cast(stale_rejected_promise)));
+  SetGlobalProperty("stale_handler", Utils::ToLocal(stale_handler));
+
+  // Set valid handlers to stale promises.
+  RunJS(
+      "stale_resolved_promise.then(() => {"
+      "  results['stale_resolved_promise'] = true;"
+      "})");
+  RunJS(
+      "stale_rejected_promise.catch(() => {"
+      "  results['stale_rejected_promise'] = true;"
+      "})");
+  microtask_queue()->RunMicrotasks(isolate());
+  EXPECT_TRUE(
+      JSReceiver::HasProperty(results, NameFromChars("stale_resolved_promise"))
+          .FromJust());
+  EXPECT_TRUE(
+      JSReceiver::HasProperty(results, NameFromChars("stale_rejected_promise"))
+          .FromJust());
+
+  // Set stale handlers to valid promises.
+  RunJS(
+      "Promise.resolve("
+      "    stale_handler.bind(null, results, 'stale_handler_resolve'))");
+  RunJS(
+      "Promise.reject("
+      "    stale_handler.bind(null, results, 'stale_handler_reject'))");
+  microtask_queue()->RunMicrotasks(isolate());
+  EXPECT_FALSE(
+      JSReceiver::HasProperty(results, NameFromChars("stale_handler_resolve"))
+          .FromJust());
+  EXPECT_FALSE(
+      JSReceiver::HasProperty(results, NameFromChars("stale_handler_reject"))
+          .FromJust());
+}
+
 }  // namespace internal
 }  // namespace v8