Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: backport V8 promise context fix #23183

Merged
merged 1 commit into from Apr 21, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions patches/v8/.patches
Expand Up @@ -12,3 +12,4 @@ fix_bug_in_receiver_maps_inference.patch
make_createdynamicfunction_throw_if_disallowed.patch
intl_fix_intl_numberformat_constructor.patch
merged_make_createdynamicfunction_switch_context_before_throwing.patch
use_context_of_then_function_for_promiseresolvethenablejob.patch
@@ -0,0 +1,98 @@
From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
From: Samuel Attard <samuel.r.attard@gmail.com>
Date: Mon, 20 Apr 2020 12:11:40 -0700
Subject: 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].

diff --git a/src/builtins/builtins-promise-gen.cc b/src/builtins/builtins-promise-gen.cc
index a1da55e0d931e32bd2eba5c1773efb2f3c2397e8..d761a394501b3d0edcc52ce8c03c51a8f4d783a6 100644
--- a/src/builtins/builtins-promise-gen.cc
+++ b/src/builtins/builtins-promise-gen.cc
@@ -1995,9 +1995,16 @@ TF_BUILTIN(ResolvePromise, PromiseBuiltinsAssembler) {
{
// 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)
+ VARIABLE(var_then_context, MachineRepresentation::kTagged, native_context);
+ ExtractHandlerContext(var_then.value(), &var_then_context);
+ const TNode<NativeContext> native_then_context = LoadNativeContext(var_then_context.value());
Node* const task = AllocatePromiseResolveThenableJobTask(
- promise, var_then.value(), resolution, native_context);
- TailCallBuiltin(Builtins::kEnqueueMicrotask, native_context, task);
+ promise, var_then.value(), resolution, native_then_context);
+ TailCallBuiltin(Builtins::kEnqueueMicrotask, native_then_context, task);
}

BIND(&if_fulfill);
diff --git a/src/objects/objects.cc b/src/objects/objects.cc
index 58cf79b84feb11f2d337c1d0f30c321ac33ddfea..4cdae30dba4dcfe05cb2edf32ef897fe2c1d8679 100644
--- a/src/objects/objects.cc
+++ b/src/objects/objects.cc
@@ -5974,10 +5974,20 @@ 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,
@@ -5985,8 +5995,7 @@ MaybeHandle<Object> JSPromise::Resolve(Handle<JSPromise> promise,
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.
@@ -6022,6 +6031,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;