Skip to content

Commit

Permalink
Revert "Use context of then function for PromiseResolveThenableJob"
Browse files Browse the repository at this point in the history 
This reverts commit 9325397.

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]: 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}

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 <fgm@chromium.org>
Commit-Queue: Francis McCabe <fgm@chromium.org>
Cr-Commit-Position: refs/heads/master@{#66510}
  • Loading branch information
@fgmccabe
fgmccabe authored and Commit Bot committed Feb 29, 2020
1 parent 2b4dd77 commit 7558e18
Show file tree
Hide file tree
Showing 5 changed files with 35 additions and 119 deletions.
23 changes: 5 additions & 18 deletions src/builtins/promise-abstract-operations.tq
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -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;
Expand All @@ -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);

Expand Down
4 changes: 2 additions & 2 deletions src/builtins/promise-misc.tq
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
9 changes: 1 addition & 8 deletions src/builtins/promise-resolve.tq
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<Callable>(then));
const nativeContext = LoadNativeContext(thenContext);
const nativeContext = LoadNativeContext(context);
const task = NewPromiseResolveThenableJobTask(
promise, UnsafeCast<JSReceiver>(then),
UnsafeCast<JSReceiver>(resolution), nativeContext);
Expand Down
18 changes: 3 additions & 15 deletions src/objects/objects.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6017,28 +6017,19 @@ 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), then_context);
Handle<JSReceiver>::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,
isolate->factory()->promise_handled_by_symbol(),
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.
Expand Down Expand Up @@ -6074,9 +6065,6 @@ 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;
Expand Down
100 changes: 24 additions & 76 deletions test/unittests/execution/microtask-queue-unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -68,15 +68,7 @@ using TestWithNativeContextAndFinalizationRegistry = //
WithSharedIsolateMixin< //
::testing::Test>>>>>;

namespace {

void DummyPromiseHook(PromiseHookType type, Local<Promise> promise,
Local<Value> parent) {}

} // namespace

class MicrotaskQueueTest : public TestWithNativeContextAndFinalizationRegistry,
public ::testing::WithParamInterface<bool> {
class MicrotaskQueueTest : public TestWithNativeContextAndFinalizationRegistry {
public:
template <typename F>
Handle<Microtask> NewMicrotask(F&& f) {
Expand All @@ -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 {
Expand Down Expand Up @@ -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());
Expand All @@ -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_|.
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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([] {}));
Expand All @@ -247,7 +233,7 @@ TEST_P(MicrotaskQueueTest, VisitRoot) {
EXPECT_EQ(expected, actual);
}

TEST_P(MicrotaskQueueTest, PromiseHandlerContext) {
TEST_F(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());
Expand Down Expand Up @@ -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.
Expand All @@ -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.
Expand Down Expand Up @@ -391,7 +377,18 @@ TEST_P(MicrotaskQueueTest, DetachGlobal_Run) {
}
}

TEST_P(MicrotaskQueueTest, DetachGlobal_PromiseResolveThenableJobTask) {
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; });"
Expand All @@ -413,50 +410,7 @@ TEST_P(MicrotaskQueueTest, DetachGlobal_PromiseResolveThenableJobTask) {
EXPECT_EQ(0, microtask_queue()->size());
}

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;

Local<v8::Context> sub_context = v8::Context::New(v8_isolate());
std::unique_ptr<MicrotaskQueue> 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<JSReceiver>::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<JSPromise>("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.
Expand Down Expand Up @@ -535,7 +489,7 @@ TEST_P(MicrotaskQueueTest, DetachGlobal_HandlerContext) {
.FromJust());
}

TEST_P(MicrotaskQueueTest, DetachGlobal_Chain) {
TEST_F(MicrotaskQueueTest, DetachGlobal_Chain) {
Handle<JSPromise> stale_rejected_promise;

Local<v8::Context> sub_context = v8::Context::New(v8_isolate());
Expand All @@ -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<v8::Context> sub_context = v8::Context::New(v8_isolate());
Utils::OpenHandle(*sub_context)
->native_context()
Expand Down Expand Up @@ -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);

Expand All @@ -620,11 +574,5 @@ TEST_P(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

0 comments on commit 7558e18

Please sign in to comment.