Skip to content

Commit

Permalink
fix: backport V8 promise context fix (#23186)
Browse files Browse the repository at this point in the history
  • Loading branch information
MarshallOfSound committed Apr 21, 2020
1 parent 4c09496 commit 890bd47
Show file tree
Hide file tree
Showing 2 changed files with 99 additions and 0 deletions.
1 change: 1 addition & 0 deletions patches/common/v8/.patches
Expand Up @@ -12,3 +12,4 @@ fix_bug_in_receiver_maps_inference.patch
make_createdynamicfunction_throw_if_disallowed.patch
merged_make_createdynamicfunction_switch_context_before_throwing.patch
intl_fix_intl_numberformat_constructor.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 ad70fb1dd1a49e8b879bc3296994292bdcb9db67..6e447fdf8a6e4b5e71a0edcc3cfae939de1ec999 100644
--- a/src/builtins/builtins-promise-gen.cc
+++ b/src/builtins/builtins-promise-gen.cc
@@ -2002,9 +2002,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 = Cast(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 3e685d901b4b52b8f45bbb12c6dfd71c3675746a..676a5799966d37ed71396614eb826515906c6143 100644
--- a/src/objects/objects.cc
+++ b/src/objects/objects.cc
@@ -6027,10 +6027,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,
@@ -6038,8 +6048,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.
@@ -6075,6 +6084,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;

0 comments on commit 890bd47

Please sign in to comment.