Revert "Use context of then function for PromiseResolveThenableJob"

This reverts commit 93253978.

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 <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/+/2081091Reviewed-by: Francis McCabe <fgm@chromium.org>
Commit-Queue: Francis McCabe <fgm@chromium.org>
Cr-Commit-Position: refs/heads/master@{#66510}

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);
 

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

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<Callable>(then));
-      const nativeContext = LoadNativeContext(thenContext);
+      const nativeContext = LoadNativeContext(context);
       const task = NewPromiseResolveThenableJobTask(
           promise, UnsafeCast<JSReceiver>(then),
           UnsafeCast<JSReceiver>(resolution), nativeContext);

src/objects/objects.cc

@@ -6017,20 +6017,10 @@ 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,
@@ -6038,7 +6028,8 @@ MaybeHandle<Object> JSPromise::Resolve(Handle<JSPromise> 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<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;

test/unittests/execution/microtask-queue-unittest.cc

@@ -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) {
@@ -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::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());
@@ -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> 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; });"
@@ -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.
@@ -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());
@@ -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()
@@ -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<MicrotaskQueueTest::ParamType>& info) {
-      return info.param ? "runtime" : "builtin";
-    });
-
 }  // namespace internal
 }  // namespace v8