Reland "Use context of then function for PromiseResolveThenableJob"

This is a reland of 9325397 Original change's description: > Use context of then function for PromiseResolveThenableJob > > When a microtask is executed, we need to use an appropriate, > non-detached Context for its execution. Currently with > PromiseResolveThenableJobs [1], the Context used is always drawn from > the realm of the Promise constructor being used. This may cause > non-intuitive behavior, such as in the following case: > > const DeadPromise = iframe.contentWindow.Promise; > const p = DeadPromise.resolve({ > then() { > return { success: true }; > } > }); > p.then(result => { console.log(result); }); > > // Some time later, but synchronously... > iframe.src = "http://example.com"; // navigate away. > // DeadPromise's Context is detached state now. > // p never gets resolved, and its reaction handler never gets called. > > To fix this behavior, when PromiseResolveThenableJob is being queued up, > the `then` method of the thenable should be used to determine the > context of the resultant microtask. Doing so aligns with Firefox, and > also with the latest HTML spec [2][3]. > > This change is analogous to CL 1465902, which uses the realm of the > reaction handlers to determine the Context PromiseReactionJobs run in. > > [1]: https://tc39.es/ecma262/#sec-promiseresolvethenablejob > [2]: https://html.spec.whatwg.org/C/#enqueuejob(queuename,-job,-arguments) > [3]: whatwg/html#5212 > > Bug: v8:10200 > Change-Id: I2312788eeea0f9e870c13cf3cb5730a87d15609e > Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2071624 > Commit-Queue: Timothy Gu <timothygu@chromium.org> > Reviewed-by: Toon Verwaest <verwaest@chromium.org> > Reviewed-by: Shu-yu Guo <syg@chromium.org> > Cr-Commit-Position: refs/heads/master@{#66507} Bug: v8:10200 Change-Id: I5af003a06c60b0c8cd19de47f847a947d40d046c Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2082109 Reviewed-by: Toon Verwaest <verwaest@chromium.org> Commit-Queue: Timothy Gu <timothygu@chromium.org> Cr-Commit-Position: refs/heads/master@{#66586}
p-g-krish · Mar 4, 2020 · 92a2f0c · 92a2f0c
1 parent 08c76f2
commit 92a2f0c
Show file tree

Hide file tree

Showing 5 changed files with 140 additions and 35 deletions.
diff --git a/src/builtins/promise-abstract-operations.tq b/src/builtins/promise-abstract-operations.tq
@@ -40,7 +40,8 @@ namespace promise {
   EnqueueMicrotask(Context, Microtask): Undefined;
 
   macro
-  ExtractHandlerContext(implicit context: Context)(handler: Callable|Undefined):
+  ExtractHandlerContextInternal(implicit context: Context)(handler: Callable|
+                                                           Undefined):
       Context labels NotFound {
     let iter: JSAny = handler;
     while (true) {
@@ -62,16 +63,25 @@ namespace promise {
     goto NotFound;
   }
 
-  // According to the HTML specification, we use the handler's context to
-  // EnqueueJob for Promise resolution.
+  macro
+  ExtractHandlerContext(implicit context: Context)(handler: Callable|
+                                                   Undefined): Context {
+    try {
+      return ExtractHandlerContextInternal(handler) otherwise NotFound;
+    }
+    label NotFound deferred {
+      return context;
+    }
+  }
+
   macro
   ExtractHandlerContext(implicit context: Context)(
       primary: Callable|Undefined, secondary: Callable|Undefined): Context {
     try {
-      return ExtractHandlerContext(primary) otherwise NotFound;
+      return ExtractHandlerContextInternal(primary) otherwise NotFound;
     }
     label NotFound deferred {
-      return ExtractHandlerContext(secondary) otherwise Default;
+      return ExtractHandlerContextInternal(secondary) otherwise Default;
     }
     label Default deferred {
       return context;
@@ -92,6 +102,9 @@ namespace promise {
       secondaryHandler = promiseReaction.fulfill_handler;
     }
 
+    // According to HTML, we use the context of the appropriate handler as the
+    // context of the microtask. See step 3 of HTML's EnqueueJob:
+    // https://html.spec.whatwg.org/C/#enqueuejob(queuename,-job,-arguments)
     const handlerContext: Context =
         ExtractHandlerContext(primaryHandler, secondaryHandler);
 

diff --git a/src/builtins/promise-misc.tq b/src/builtins/promise-misc.tq
@@ -130,10 +130,10 @@ namespace promise {
 
   macro NewPromiseResolveThenableJobTask(implicit context: Context)(
       promiseToResolve: JSPromise, then: JSReceiver, thenable: JSReceiver,
-      thenableContext: Context): PromiseResolveThenableJobTask {
+      thenContext: Context): PromiseResolveThenableJobTask {
     return new PromiseResolveThenableJobTask{
       map: PromiseResolveThenableJobTaskMapConstant(),
-      context: thenableContext,
+      context: thenContext,
       promise_to_resolve: promiseToResolve,
       then: then,
       thenable: thenable

diff --git a/src/builtins/promise-resolve.tq b/src/builtins/promise-resolve.tq
@@ -177,7 +177,14 @@ namespace promise {
     label Enqueue {
       // 12. Perform EnqueueJob("PromiseJobs", PromiseResolveThenableJob,
       //                        «promise, resolution, thenAction»).
-      const nativeContext = LoadNativeContext(context);
+
+      // According to HTML, we use the context of the then function
+      // (|thenAction|) as the context of the microtask. See step 3 of HTML's
+      // EnqueueJob:
+      // https://html.spec.whatwg.org/C/#enqueuejob(queuename,-job,-arguments)
+      const thenContext: Context =
+          ExtractHandlerContext(UnsafeCast<Callable>(then));
+      const nativeContext = LoadNativeContext(thenContext);
       const task = NewPromiseResolveThenableJobTask(
           promise, UnsafeCast<JSReceiver>(then),
           UnsafeCast<JSReceiver>(resolution), nativeContext);

diff --git a/src/objects/objects.cc b/src/objects/objects.cc
@@ -6032,19 +6032,28 @@ MaybeHandle<Object> JSPromise::Resolve(Handle<JSPromise> promise,
 
   // 12. Perform EnqueueJob("PromiseJobs", PromiseResolveThenableJob,
   //                        «promise, resolution, thenAction»).
+
+  // According to HTML, we use the context of the then function (|thenAction|)
+  // as the context of the microtask. See step 3 of HTML's EnqueueJob:
+  // https://html.spec.whatwg.org/C/#enqueuejob(queuename,-job,-arguments)
+  Handle<NativeContext> then_context;
+  if (!JSReceiver::GetContextForMicrotask(Handle<JSReceiver>::cast(then_action))
+           .ToHandle(&then_context)) {
+    then_context = isolate->native_context();
+  }
+
   Handle<PromiseResolveThenableJobTask> task =
       isolate->factory()->NewPromiseResolveThenableJobTask(
           promise, Handle<JSReceiver>::cast(then_action),
-          Handle<JSReceiver>::cast(resolution), isolate->native_context());
+          Handle<JSReceiver>::cast(resolution), then_context);
   if (isolate->debug()->is_active() && resolution->IsJSPromise()) {
     // Mark the dependency of the new {promise} on the {resolution}.
     Object::SetProperty(isolate, resolution,
                         isolate->factory()->promise_handled_by_symbol(),
                         promise)
         .Check();
   }
-  MicrotaskQueue* microtask_queue =
-      isolate->native_context()->microtask_queue();
+  MicrotaskQueue* microtask_queue = then_context->microtask_queue();
   if (microtask_queue) microtask_queue->EnqueueMicrotask(*task);
 
   // 13. Return undefined.
@@ -6080,6 +6089,9 @@ Handle<Object> JSPromise::TriggerPromiseReactions(Isolate* isolate,
     Handle<PromiseReaction> reaction = Handle<PromiseReaction>::cast(task);
     reactions = handle(reaction->next(), isolate);
 
+    // According to HTML, we use the context of the appropriate handler as the
+    // context of the microtask. See step 3 of HTML's EnqueueJob:
+    // https://html.spec.whatwg.org/C/#enqueuejob(queuename,-job,-arguments)
     Handle<NativeContext> handler_context;
 
     Handle<HeapObject> primary_handler;

diff --git a/test/unittests/execution/microtask-queue-unittest.cc b/test/unittests/execution/microtask-queue-unittest.cc
@@ -68,7 +68,15 @@ using TestWithNativeContextAndFinalizationRegistry =  //
                     WithSharedIsolateMixin<           //
                         ::testing::Test>>>>>;
 
-class MicrotaskQueueTest : public TestWithNativeContextAndFinalizationRegistry {
+namespace {
+
+void DummyPromiseHook(PromiseHookType type, Local<Promise> promise,
+                      Local<Value> parent) {}
+
+}  // namespace
+
+class MicrotaskQueueTest : public TestWithNativeContextAndFinalizationRegistry,
+                           public ::testing::WithParamInterface<bool> {
  public:
   template <typename F>
   Handle<Microtask> NewMicrotask(F&& f) {
@@ -82,6 +90,12 @@ class MicrotaskQueueTest : public TestWithNativeContextAndFinalizationRegistry {
   void SetUp() override {
     microtask_queue_ = MicrotaskQueue::New(isolate());
     native_context()->set_microtask_queue(microtask_queue());
+
+    if (GetParam()) {
+      // Use a PromiseHook to switch the implementation to ResolvePromise
+      // runtime, instead of ResolvePromise builtin.
+      v8_isolate()->SetPromiseHook(&DummyPromiseHook);
+    }
   }
 
   void TearDown() override {
@@ -126,7 +140,7 @@ class RecordingVisitor : public RootVisitor {
 };
 
 // Sanity check. Ensure a microtask is stored in a queue and run.
-TEST_F(MicrotaskQueueTest, EnqueueAndRun) {
+TEST_P(MicrotaskQueueTest, EnqueueAndRun) {
   bool ran = false;
   EXPECT_EQ(0, microtask_queue()->capacity());
   EXPECT_EQ(0, microtask_queue()->size());
@@ -142,7 +156,7 @@ TEST_F(MicrotaskQueueTest, EnqueueAndRun) {
 }
 
 // Check for a buffer growth.
-TEST_F(MicrotaskQueueTest, BufferGrowth) {
+TEST_P(MicrotaskQueueTest, BufferGrowth) {
   int count = 0;
 
   // Enqueue and flush the queue first to have non-zero |start_|.
@@ -176,7 +190,7 @@ TEST_F(MicrotaskQueueTest, BufferGrowth) {
 }
 
 // MicrotaskQueue instances form a doubly linked list.
-TEST_F(MicrotaskQueueTest, InstanceChain) {
+TEST_P(MicrotaskQueueTest, InstanceChain) {
   ClearTestMicrotaskQueue();
 
   MicrotaskQueue* default_mtq = isolate()->default_microtask_queue();
@@ -207,7 +221,7 @@ TEST_F(MicrotaskQueueTest, InstanceChain) {
 
 // Pending Microtasks in MicrotaskQueues are strong roots. Ensure they are
 // visited exactly once.
-TEST_F(MicrotaskQueueTest, VisitRoot) {
+TEST_P(MicrotaskQueueTest, VisitRoot) {
   // Ensure that the ring buffer has separate in-use region.
   for (int i = 0; i < MicrotaskQueue::kMinimumCapacity / 2 + 1; ++i) {
     microtask_queue()->EnqueueMicrotask(*NewMicrotask([] {}));
@@ -233,7 +247,7 @@ TEST_F(MicrotaskQueueTest, VisitRoot) {
   EXPECT_EQ(expected, actual);
 }
 
-TEST_F(MicrotaskQueueTest, PromiseHandlerContext) {
+TEST_P(MicrotaskQueueTest, PromiseHandlerContext) {
   Local<v8::Context> v8_context2 = v8::Context::New(v8_isolate());
   Local<v8::Context> v8_context3 = v8::Context::New(v8_isolate());
   Local<v8::Context> v8_context4 = v8::Context::New(v8_isolate());
@@ -327,7 +341,7 @@ TEST_F(MicrotaskQueueTest, PromiseHandlerContext) {
   v8_context2->DetachGlobal();
 }
 
-TEST_F(MicrotaskQueueTest, DetachGlobal_Enqueue) {
+TEST_P(MicrotaskQueueTest, DetachGlobal_Enqueue) {
   EXPECT_EQ(0, microtask_queue()->size());
 
   // Detach MicrotaskQueue from the current context.
@@ -339,7 +353,7 @@ TEST_F(MicrotaskQueueTest, DetachGlobal_Enqueue) {
   EXPECT_EQ(0, microtask_queue()->size());
 }
 
-TEST_F(MicrotaskQueueTest, DetachGlobal_Run) {
+TEST_P(MicrotaskQueueTest, DetachGlobal_Run) {
   EXPECT_EQ(0, microtask_queue()->size());
 
   // Enqueue microtasks to the current context.
@@ -377,18 +391,7 @@ TEST_F(MicrotaskQueueTest, DetachGlobal_Run) {
   }
 }
 
-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);
-
+TEST_P(MicrotaskQueueTest, DetachGlobal_PromiseResolveThenableJobTask) {
   RunJS(
       "var resolve;"
       "var promise = new Promise(r => { resolve = r; });"
@@ -410,7 +413,71 @@ TEST_F(MicrotaskQueueTest, DetachGlobal_PromiseResolveThenableJobTask) {
   EXPECT_EQ(0, microtask_queue()->size());
 }
 
-TEST_F(MicrotaskQueueTest, DetachGlobal_HandlerContext) {
+TEST_P(MicrotaskQueueTest, DetachGlobal_ResolveThenableForeignThen) {
+  Handle<JSArray> result = RunJS<JSArray>(
+      "let result = [false];"
+      "result");
+  Handle<JSFunction> then = RunJS<JSFunction>("() => { result[0] = true; }");
+
+  Handle<JSPromise> stale_promise;
+
+  {
+    // Create a context with its own microtask queue.
+    std::unique_ptr<MicrotaskQueue> sub_microtask_queue =
+        MicrotaskQueue::New(isolate());
+    Local<v8::Context> sub_context = v8::Context::New(
+        v8_isolate(),
+        /* extensions= */ nullptr,
+        /* global_template= */ MaybeLocal<ObjectTemplate>(),
+        /* global_object= */ MaybeLocal<Value>(),
+        /* internal_fields_deserializer= */ DeserializeInternalFieldsCallback(),
+        sub_microtask_queue.get());
+
+    {
+      v8::Context::Scope scope(sub_context);
+      CHECK(sub_context->Global()
+                ->Set(sub_context, NewString("then"),
+                      Utils::ToLocal(Handle<JSReceiver>::cast(then)))
+                .FromJust());
+
+      ASSERT_EQ(0, microtask_queue()->size());
+      ASSERT_EQ(0, sub_microtask_queue->size());
+      ASSERT_TRUE(Object::GetElement(isolate(), result, 0)
+                      .ToHandleChecked()
+                      ->IsFalse());
+
+      // With a regular thenable, a microtask is queued on the sub-context.
+      RunJS<JSPromise>("Promise.resolve({ then: cb => cb(1) })");
+      EXPECT_EQ(0, microtask_queue()->size());
+      EXPECT_EQ(1, sub_microtask_queue->size());
+      EXPECT_TRUE(Object::GetElement(isolate(), result, 0)
+                      .ToHandleChecked()
+                      ->IsFalse());
+
+      // But when the `then` method comes from another context, a microtask is
+      // instead queued on the main context.
+      stale_promise = RunJS<JSPromise>("Promise.resolve({ then })");
+      EXPECT_EQ(1, microtask_queue()->size());
+      EXPECT_EQ(1, sub_microtask_queue->size());
+      EXPECT_TRUE(Object::GetElement(isolate(), result, 0)
+                      .ToHandleChecked()
+                      ->IsFalse());
+    }
+
+    sub_context->DetachGlobal();
+  }
+
+  EXPECT_EQ(1, microtask_queue()->size());
+  EXPECT_TRUE(
+      Object::GetElement(isolate(), result, 0).ToHandleChecked()->IsFalse());
+
+  EXPECT_EQ(1, microtask_queue()->RunMicrotasks(isolate()));
+  EXPECT_EQ(0, microtask_queue()->size());
+  EXPECT_TRUE(
+      Object::GetElement(isolate(), result, 0).ToHandleChecked()->IsTrue());
+}
+
+TEST_P(MicrotaskQueueTest, DetachGlobal_HandlerContext) {
   // EnqueueMicrotask should use the context associated to the handler instead
   // of the current context. E.g.
   //   // At Context A.
@@ -489,7 +556,7 @@ TEST_F(MicrotaskQueueTest, DetachGlobal_HandlerContext) {
           .FromJust());
 }
 
-TEST_F(MicrotaskQueueTest, DetachGlobal_Chain) {
+TEST_P(MicrotaskQueueTest, DetachGlobal_Chain) {
   Handle<JSPromise> stale_rejected_promise;
 
   Local<v8::Context> sub_context = v8::Context::New(v8_isolate());
@@ -516,7 +583,7 @@ TEST_F(MicrotaskQueueTest, DetachGlobal_Chain) {
       Object::GetElement(isolate(), result, 0).ToHandleChecked()->IsTrue());
 }
 
-TEST_F(MicrotaskQueueTest, DetachGlobal_InactiveHandler) {
+TEST_P(MicrotaskQueueTest, DetachGlobal_InactiveHandler) {
   Local<v8::Context> sub_context = v8::Context::New(v8_isolate());
   Utils::OpenHandle(*sub_context)
       ->native_context()
@@ -558,7 +625,7 @@ TEST_F(MicrotaskQueueTest, DetachGlobal_InactiveHandler) {
       Object::GetElement(isolate(), result, 1).ToHandleChecked()->IsFalse());
 }
 
-TEST_F(MicrotaskQueueTest, MicrotasksScope) {
+TEST_P(MicrotaskQueueTest, MicrotasksScope) {
   ASSERT_NE(isolate()->default_microtask_queue(), microtask_queue());
   microtask_queue()->set_microtasks_policy(MicrotasksPolicy::kScoped);
 
@@ -574,5 +641,11 @@ TEST_F(MicrotaskQueueTest, MicrotasksScope) {
   EXPECT_TRUE(ran);
 }
 
+INSTANTIATE_TEST_SUITE_P(
+    , MicrotaskQueueTest, ::testing::Values(false, true),
+    [](const ::testing::TestParamInfo<MicrotaskQueueTest::ParamType>& info) {
+      return info.param ? "runtime" : "builtin";
+    });
+
 }  // namespace internal
 }  // namespace v8