From 7558e1821f2f1287ac87fca7d8350324997f388c Mon Sep 17 00:00:00 2001 From: Francis McCabe Date: Sat, 29 Feb 2020 00:44:56 +0000 Subject: [PATCH] Revert "Use context of then function for PromiseResolveThenableJob" This reverts commit 93253978127f74ac776298fabc31091288d59897. Reason for revert: Causing blink layout failures. See https://ci.chromium.org/p/v8/builders/ci/V8%20Blink%20Linux%20Future/2684 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]: https://github.com/whatwg/html/pull/5212 > > Bug: v8:10200 > Change-Id: I2312788eeea0f9e870c13cf3cb5730a87d15609e > Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2071624 > Commit-Queue: Timothy Gu > Reviewed-by: Toon Verwaest > Reviewed-by: Shu-yu Guo > Cr-Commit-Position: refs/heads/master@{#66507} TBR=verwaest@chromium.org,timothygu@chromium.org,syg@chromium.org Change-Id: I81737750f8b369567ba586c5a2cfb489836b7e74 No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: v8:10200 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2081091 Reviewed-by: Francis McCabe Commit-Queue: Francis McCabe Cr-Commit-Position: refs/heads/master@{#66510} --- src/builtins/promise-abstract-operations.tq | 23 +--- src/builtins/promise-misc.tq | 4 +- src/builtins/promise-resolve.tq | 9 +- src/objects/objects.cc | 18 +--- .../execution/microtask-queue-unittest.cc | 100 +++++------------- 5 files changed, 35 insertions(+), 119 deletions(-) diff --git a/src/builtins/promise-abstract-operations.tq b/src/builtins/promise-abstract-operations.tq index d5e6ffab6e30..8828ab84d266 100644 --- a/src/builtins/promise-abstract-operations.tq +++ b/src/builtins/promise-abstract-operations.tq @@ -40,8 +40,7 @@ namespace promise { EnqueueMicrotask(Context, Microtask): Undefined; macro - ExtractHandlerContextInternal(implicit context: Context)(handler: Callable| - Undefined): + ExtractHandlerContext(implicit context: Context)(handler: Callable|Undefined): Context labels NotFound { let iter: JSAny = handler; while (true) { @@ -63,25 +62,16 @@ namespace promise { goto NotFound; } - macro - ExtractHandlerContext(implicit context: Context)(handler: Callable| - Undefined): Context { - try { - return ExtractHandlerContextInternal(handler) otherwise NotFound; - } - label NotFound deferred { - return context; - } - } - + // According to the HTML specification, we use the handler's context to + // EnqueueJob for Promise resolution. macro ExtractHandlerContext(implicit context: Context)( primary: Callable|Undefined, secondary: Callable|Undefined): Context { try { - return ExtractHandlerContextInternal(primary) otherwise NotFound; + return ExtractHandlerContext(primary) otherwise NotFound; } label NotFound deferred { - return ExtractHandlerContextInternal(secondary) otherwise Default; + return ExtractHandlerContext(secondary) otherwise Default; } label Default deferred { return context; @@ -102,9 +92,6 @@ 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 index 51aa53b7e55d..7996cc5b3d95 100644 --- 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, - thenContext: Context): PromiseResolveThenableJobTask { + thenableContext: Context): PromiseResolveThenableJobTask { return new PromiseResolveThenableJobTask{ map: PromiseResolveThenableJobTaskMapConstant(), - context: thenContext, + context: thenableContext, promise_to_resolve: promiseToResolve, then: then, thenable: thenable diff --git a/src/builtins/promise-resolve.tq b/src/builtins/promise-resolve.tq index 0fc98b556b22..af7dd7afa0ce 100644 --- a/src/builtins/promise-resolve.tq +++ b/src/builtins/promise-resolve.tq @@ -177,14 +177,7 @@ namespace promise { label Enqueue { // 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) - const thenContext: Context = - ExtractHandlerContext(UnsafeCast(then)); - const nativeContext = LoadNativeContext(thenContext); + const nativeContext = LoadNativeContext(context); const task = NewPromiseResolveThenableJobTask( promise, UnsafeCast(then), UnsafeCast(resolution), nativeContext); diff --git a/src/objects/objects.cc b/src/objects/objects.cc index db8d8db59fcb..0acf8b0ab873 100644 --- a/src/objects/objects.cc +++ b/src/objects/objects.cc @@ -6017,20 +6017,10 @@ MaybeHandle JSPromise::Resolve(Handle 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 then_context; - if (!JSReceiver::GetContextForMicrotask(Handle::cast(then_action)) - .ToHandle(&then_context)) { - then_context = isolate->native_context(); - } - Handle task = isolate->factory()->NewPromiseResolveThenableJobTask( promise, Handle::cast(then_action), - Handle::cast(resolution), then_context); + Handle::cast(resolution), isolate->native_context()); if (isolate->debug()->is_active() && resolution->IsJSPromise()) { // Mark the dependency of the new {promise} on the {resolution}. Object::SetProperty(isolate, resolution, @@ -6038,7 +6028,8 @@ MaybeHandle JSPromise::Resolve(Handle promise, promise) .Check(); } - MicrotaskQueue* microtask_queue = then_context->microtask_queue(); + MicrotaskQueue* microtask_queue = + isolate->native_context()->microtask_queue(); if (microtask_queue) microtask_queue->EnqueueMicrotask(*task); // 13. Return undefined. @@ -6074,9 +6065,6 @@ Handle JSPromise::TriggerPromiseReactions(Isolate* isolate, Handle reaction = Handle::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 handler_context; Handle primary_handler; diff --git a/test/unittests/execution/microtask-queue-unittest.cc b/test/unittests/execution/microtask-queue-unittest.cc index 01e26f1a630a..304891778186 100644 --- a/test/unittests/execution/microtask-queue-unittest.cc +++ b/test/unittests/execution/microtask-queue-unittest.cc @@ -68,15 +68,7 @@ using TestWithNativeContextAndFinalizationRegistry = // WithSharedIsolateMixin< // ::testing::Test>>>>>; -namespace { - -void DummyPromiseHook(PromiseHookType type, Local promise, - Local parent) {} - -} // namespace - -class MicrotaskQueueTest : public TestWithNativeContextAndFinalizationRegistry, - public ::testing::WithParamInterface { +class MicrotaskQueueTest : public TestWithNativeContextAndFinalizationRegistry { public: template Handle NewMicrotask(F&& f) { @@ -90,12 +82,6 @@ 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 { @@ -140,7 +126,7 @@ class RecordingVisitor : public RootVisitor { }; // Sanity check. Ensure a microtask is stored in a queue and run. -TEST_P(MicrotaskQueueTest, EnqueueAndRun) { +TEST_F(MicrotaskQueueTest, EnqueueAndRun) { bool ran = false; EXPECT_EQ(0, microtask_queue()->capacity()); EXPECT_EQ(0, microtask_queue()->size()); @@ -156,7 +142,7 @@ TEST_P(MicrotaskQueueTest, EnqueueAndRun) { } // Check for a buffer growth. -TEST_P(MicrotaskQueueTest, BufferGrowth) { +TEST_F(MicrotaskQueueTest, BufferGrowth) { int count = 0; // Enqueue and flush the queue first to have non-zero |start_|. @@ -190,7 +176,7 @@ TEST_P(MicrotaskQueueTest, BufferGrowth) { } // MicrotaskQueue instances form a doubly linked list. -TEST_P(MicrotaskQueueTest, InstanceChain) { +TEST_F(MicrotaskQueueTest, InstanceChain) { ClearTestMicrotaskQueue(); MicrotaskQueue* default_mtq = isolate()->default_microtask_queue(); @@ -221,7 +207,7 @@ TEST_P(MicrotaskQueueTest, InstanceChain) { // Pending Microtasks in MicrotaskQueues are strong roots. Ensure they are // visited exactly once. -TEST_P(MicrotaskQueueTest, VisitRoot) { +TEST_F(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([] {})); @@ -247,7 +233,7 @@ TEST_P(MicrotaskQueueTest, VisitRoot) { EXPECT_EQ(expected, actual); } -TEST_P(MicrotaskQueueTest, PromiseHandlerContext) { +TEST_F(MicrotaskQueueTest, PromiseHandlerContext) { Local v8_context2 = v8::Context::New(v8_isolate()); Local v8_context3 = v8::Context::New(v8_isolate()); Local v8_context4 = v8::Context::New(v8_isolate()); @@ -341,7 +327,7 @@ TEST_P(MicrotaskQueueTest, PromiseHandlerContext) { v8_context2->DetachGlobal(); } -TEST_P(MicrotaskQueueTest, DetachGlobal_Enqueue) { +TEST_F(MicrotaskQueueTest, DetachGlobal_Enqueue) { EXPECT_EQ(0, microtask_queue()->size()); // Detach MicrotaskQueue from the current context. @@ -353,7 +339,7 @@ TEST_P(MicrotaskQueueTest, DetachGlobal_Enqueue) { EXPECT_EQ(0, microtask_queue()->size()); } -TEST_P(MicrotaskQueueTest, DetachGlobal_Run) { +TEST_F(MicrotaskQueueTest, DetachGlobal_Run) { EXPECT_EQ(0, microtask_queue()->size()); // Enqueue microtasks to the current context. @@ -391,7 +377,18 @@ TEST_P(MicrotaskQueueTest, DetachGlobal_Run) { } } -TEST_P(MicrotaskQueueTest, DetachGlobal_PromiseResolveThenableJobTask) { +namespace { + +void DummyPromiseHook(PromiseHookType type, Local promise, + Local 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; });" @@ -413,50 +410,7 @@ TEST_P(MicrotaskQueueTest, DetachGlobal_PromiseResolveThenableJobTask) { EXPECT_EQ(0, microtask_queue()->size()); } -TEST_P(MicrotaskQueueTest, DetachGlobal_ResolveThenableForeignThen) { - Handle result = RunJS( - "let result = [false];" - "result"); - Handle then = RunJS("() => { result[0] = true; }"); - - Handle stale_promise; - - Local sub_context = v8::Context::New(v8_isolate()); - std::unique_ptr sub_microtask_queue = - MicrotaskQueue::New(isolate()); - Utils::OpenHandle(*sub_context) - ->native_context() - .set_microtask_queue(microtask_queue()); - { - v8::Context::Scope scope(sub_context); - CHECK(sub_context->Global() - ->Set(sub_context, NewString("then"), - Utils::ToLocal(Handle::cast(then))) - .FromJust()); - - EXPECT_EQ(0, microtask_queue()->size()); - EXPECT_EQ(0, sub_microtask_queue->size()); - EXPECT_TRUE( - Object::GetElement(isolate(), result, 0).ToHandleChecked()->IsFalse()); - - stale_promise = RunJS("Promise.resolve({ then })"); - - EXPECT_EQ(1, microtask_queue()->size()); - EXPECT_EQ(0, sub_microtask_queue->size()); - EXPECT_TRUE( - Object::GetElement(isolate(), result, 0).ToHandleChecked()->IsFalse()); - } - sub_context->DetachGlobal(); - sub_context.Clear(); - sub_microtask_queue.reset(); - - 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) { +TEST_F(MicrotaskQueueTest, DetachGlobal_HandlerContext) { // EnqueueMicrotask should use the context associated to the handler instead // of the current context. E.g. // // At Context A. @@ -535,7 +489,7 @@ TEST_P(MicrotaskQueueTest, DetachGlobal_HandlerContext) { .FromJust()); } -TEST_P(MicrotaskQueueTest, DetachGlobal_Chain) { +TEST_F(MicrotaskQueueTest, DetachGlobal_Chain) { Handle stale_rejected_promise; Local sub_context = v8::Context::New(v8_isolate()); @@ -562,7 +516,7 @@ TEST_P(MicrotaskQueueTest, DetachGlobal_Chain) { Object::GetElement(isolate(), result, 0).ToHandleChecked()->IsTrue()); } -TEST_P(MicrotaskQueueTest, DetachGlobal_InactiveHandler) { +TEST_F(MicrotaskQueueTest, DetachGlobal_InactiveHandler) { Local sub_context = v8::Context::New(v8_isolate()); Utils::OpenHandle(*sub_context) ->native_context() @@ -604,7 +558,7 @@ TEST_P(MicrotaskQueueTest, DetachGlobal_InactiveHandler) { Object::GetElement(isolate(), result, 1).ToHandleChecked()->IsFalse()); } -TEST_P(MicrotaskQueueTest, MicrotasksScope) { +TEST_F(MicrotaskQueueTest, MicrotasksScope) { ASSERT_NE(isolate()->default_microtask_queue(), microtask_queue()); microtask_queue()->set_microtasks_policy(MicrotasksPolicy::kScoped); @@ -620,11 +574,5 @@ TEST_P(MicrotaskQueueTest, MicrotasksScope) { EXPECT_TRUE(ran); } -INSTANTIATE_TEST_SUITE_P( - , MicrotaskQueueTest, ::testing::Values(false, true), - [](const ::testing::TestParamInfo& info) { - return info.param ? "runtime" : "builtin"; - }); - } // namespace internal } // namespace v8