Browse Source

fix: backport V8 promise context fix (#23186)

Samuel Attard 5 years ago
parent
commit
890bd47caf

+ 1 - 0
patches/common/v8/.patches

@@ -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

+ 98 - 0
patches/common/v8/use_context_of_then_function_for_promiseresolvethenablejob.patch

@@ -0,0 +1,98 @@
+From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
+From: Samuel Attard <[email protected]>
+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;