[promise] Implement Swallowed Rejection Hook.

This extends the current Promise Rejection Hook with two new events

  kPromiseRejectAfterResolved
  kPromiseResolveAfterResolved

which are used to detect (and signal) misuse of the Promise constructor.
Specifically the common bug like

  new Promise((res, rej) => {
    res(1);
    throw new Error("something")
  });

where the error is silently swallowed by the Promise constructor without
the user ever noticing can be caught via this hook.

Doc: https://goo.gl/2stLUY
Bug: v8:7919
Cq-Include-Trybots: luci.chromium.try:linux_chromium_rel_ng
Change-Id: I890a7e766cdd1be88db94844fb744f72823dba33
Reviewed-on: https://chromium-review.googlesource.com/1126099
Reviewed-by: Maya Lekova <mslekova@chromium.org>
Reviewed-by: Benedikt Meurer <bmeurer@chromium.org>
Reviewed-by: Yang Guo <yangguo@chromium.org>
Commit-Queue: Benedikt Meurer <bmeurer@chromium.org>
Cr-Commit-Position: refs/heads/master@{#54309}

include/v8.h

@@ -6570,7 +6570,9 @@ typedef void (*PromiseHook)(PromiseHookType type, Local<Promise> promise,
 // --- Promise Reject Callback ---
 enum PromiseRejectEvent {
   kPromiseRejectWithNoHandler = 0,
-  kPromiseHandlerAddedAfterReject = 1
+  kPromiseHandlerAddedAfterReject = 1,
+  kPromiseRejectAfterResolved = 2,
+  kPromiseResolveAfterResolved = 3,
 };
 
 class PromiseRejectMessage {

src/builtins/builtins-promise-gen.cc

@@ -247,6 +247,8 @@ Node* PromiseBuiltinsAssembler::CreatePromiseResolvingFunctionsContext(
   Node* const context =
       CreatePromiseContext(native_context, kPromiseContextLength);
   StoreContextElementNoWriteBarrier(context, kPromiseSlot, promise);
+  StoreContextElementNoWriteBarrier(context, kAlreadyResolvedSlot,
+                                    FalseConstant());
   StoreContextElementNoWriteBarrier(context, kDebugEventSlot, debug_event);
   return context;
 }
@@ -737,17 +739,27 @@ TF_BUILTIN(PromiseCapabilityDefaultReject, PromiseBuiltinsAssembler) {
   Node* const promise = LoadContextElement(context, kPromiseSlot);
 
   // 3. Let alreadyResolved be F.[[AlreadyResolved]].
+  Label if_already_resolved(this, Label::kDeferred);
+  Node* const already_resolved =
+      LoadContextElement(context, kAlreadyResolvedSlot);
+
   // 4. If alreadyResolved.[[Value]] is true, return undefined.
-  // We use undefined as a marker for the [[AlreadyResolved]] state.
+  GotoIf(IsTrue(already_resolved), &if_already_resolved);
-  ReturnIf(IsUndefined(promise), UndefinedConstant());
 
   // 5. Set alreadyResolved.[[Value]] to true.
-  StoreContextElementNoWriteBarrier(context, kPromiseSlot, UndefinedConstant());
+  StoreContextElementNoWriteBarrier(context, kAlreadyResolvedSlot,
+                                    TrueConstant());
 
   // 6. Return RejectPromise(promise, reason).
   Node* const debug_event = LoadContextElement(context, kDebugEventSlot);
   Return(CallBuiltin(Builtins::kRejectPromise, context, promise, reason,
                      debug_event));
+
+  BIND(&if_already_resolved);
+  {
+    Return(CallRuntime(Runtime::kPromiseRejectAfterResolved, context, promise,
+                       reason));
+  }
 }
 
 // ES #sec-promise-resolve-functions
@@ -759,16 +771,26 @@ TF_BUILTIN(PromiseCapabilityDefaultResolve, PromiseBuiltinsAssembler) {
   Node* const promise = LoadContextElement(context, kPromiseSlot);
 
   // 3. Let alreadyResolved be F.[[AlreadyResolved]].
+  Label if_already_resolved(this, Label::kDeferred);
+  Node* const already_resolved =
+      LoadContextElement(context, kAlreadyResolvedSlot);
+
   // 4. If alreadyResolved.[[Value]] is true, return undefined.
-  // We use undefined as a marker for the [[AlreadyResolved]] state.
+  GotoIf(IsTrue(already_resolved), &if_already_resolved);
-  ReturnIf(IsUndefined(promise), UndefinedConstant());
 
   // 5. Set alreadyResolved.[[Value]] to true.
-  StoreContextElementNoWriteBarrier(context, kPromiseSlot, UndefinedConstant());
+  StoreContextElementNoWriteBarrier(context, kAlreadyResolvedSlot,
+                                    TrueConstant());
 
   // The rest of the logic (and the catch prediction) is
   // encapsulated in the dedicated ResolvePromise builtin.
   Return(CallBuiltin(Builtins::kResolvePromise, context, promise, resolution));
+
+  BIND(&if_already_resolved);
+  {
+    Return(CallRuntime(Runtime::kPromiseResolveAfterResolved, context, promise,
+                       resolution));
+  }
 }
 
 TF_BUILTIN(PromiseConstructorLazyDeoptContinuation, PromiseBuiltinsAssembler) {

src/builtins/builtins-promise-gen.h

@@ -17,11 +17,12 @@ typedef compiler::CodeAssemblerState CodeAssemblerState;
 class PromiseBuiltinsAssembler : public CodeStubAssembler {
  public:
   enum PromiseResolvingFunctionContextSlot {
-    // The promise which resolve/reject callbacks fulfill. If this is
+    // The promise which resolve/reject callbacks fulfill.
-    // undefined, then we've already visited this callback and it
-    // should be a no-op.
     kPromiseSlot = Context::MIN_CONTEXT_SLOTS,
 
+    // Whether the callback was already invoked.
+    kAlreadyResolvedSlot,
+
     // Whether to trigger a debug event or not. Used in catch
     // prediction.
     kDebugEventSlot,

src/compiler/js-call-reducer.cc

@@ -5456,6 +5456,10 @@ Reduction JSCallReducer::ReducePromiseConstructor(Node* node) {
       graph()->NewNode(simplified()->StoreField(AccessBuilder::ForContextSlot(
                            PromiseBuiltinsAssembler::kPromiseSlot)),
                        promise_context, promise, effect, control);
+  effect = graph()->NewNode(
+      simplified()->StoreField(AccessBuilder::ForContextSlot(
+          PromiseBuiltinsAssembler::kAlreadyResolvedSlot)),
+      promise_context, jsgraph()->FalseConstant(), effect, control);
   effect = graph()->NewNode(
       simplified()->StoreField(AccessBuilder::ForContextSlot(
           PromiseBuiltinsAssembler::kDebugEventSlot)),

src/isolate.cc

@@ -3931,10 +3931,9 @@ void Isolate::SetPromiseRejectCallback(PromiseRejectCallback callback) {
 void Isolate::ReportPromiseReject(Handle<JSPromise> promise,
                                   Handle<Object> value,
                                   v8::PromiseRejectEvent event) {
-  DCHECK_EQ(v8::Promise::kRejected, promise->status());
   if (promise_reject_callback_ == nullptr) return;
   Handle<FixedArray> stack_trace;
-  if (event == v8::kPromiseRejectWithNoHandler && value->IsJSObject()) {
+  if (event != v8::kPromiseHandlerAddedAfterReject && value->IsJSObject()) {
     stack_trace = GetDetailedStackTrace(Handle<JSObject>::cast(value));
   }
   promise_reject_callback_(v8::PromiseRejectMessage(

src/runtime/runtime-promise.cc

@@ -39,6 +39,26 @@ RUNTIME_FUNCTION(Runtime_PromiseRejectEventFromStack) {
   return ReadOnlyRoots(isolate).undefined_value();
 }
 
+RUNTIME_FUNCTION(Runtime_PromiseRejectAfterResolved) {
+  DCHECK_EQ(2, args.length());
+  HandleScope scope(isolate);
+  CONVERT_ARG_HANDLE_CHECKED(JSPromise, promise, 0);
+  CONVERT_ARG_HANDLE_CHECKED(Object, reason, 1);
+  isolate->ReportPromiseReject(promise, reason,
+                               v8::kPromiseRejectAfterResolved);
+  return ReadOnlyRoots(isolate).undefined_value();
+}
+
+RUNTIME_FUNCTION(Runtime_PromiseResolveAfterResolved) {
+  DCHECK_EQ(2, args.length());
+  HandleScope scope(isolate);
+  CONVERT_ARG_HANDLE_CHECKED(JSPromise, promise, 0);
+  CONVERT_ARG_HANDLE_CHECKED(Object, resolution, 1);
+  isolate->ReportPromiseReject(promise, resolution,
+                               v8::kPromiseResolveAfterResolved);
+  return ReadOnlyRoots(isolate).undefined_value();
+}
+
 RUNTIME_FUNCTION(Runtime_PromiseRevokeReject) {
   DCHECK_EQ(1, args.length());
   HandleScope scope(isolate);

src/runtime/runtime.h

@@ -384,7 +384,9 @@ namespace internal {
   F(PromiseRevokeReject, 1, 1)         \
   F(PromiseStatus, 1, 1)               \
   F(RejectPromise, 3, 1)               \
-  F(ResolvePromise, 2, 1)
+  F(ResolvePromise, 2, 1)              \
+  F(PromiseRejectAfterResolved, 2, 1)  \
+  F(PromiseResolveAfterResolved, 2, 1)
 
 #define FOR_EACH_INTRINSIC_PROXY(F)   \
   F(CheckProxyGetSetTrapResult, 2, 1) \

test/cctest/test-api.cc

@@ -17718,6 +17718,8 @@ TEST(RethrowBogusErrorStackTrace) {
 v8::PromiseRejectEvent reject_event = v8::kPromiseRejectWithNoHandler;
 int promise_reject_counter = 0;
 int promise_revoke_counter = 0;
+int promise_reject_after_resolved_counter = 0;
+int promise_resolve_after_resolved_counter = 0;
 int promise_reject_msg_line_number = -1;
 int promise_reject_msg_column_number = -1;
 int promise_reject_line_number = -1;
@@ -17727,40 +17729,56 @@ int promise_reject_frame_count = -1;
 void PromiseRejectCallback(v8::PromiseRejectMessage reject_message) {
   v8::Local<v8::Object> global = CcTest::global();
   v8::Local<v8::Context> context = CcTest::isolate()->GetCurrentContext();
-  CHECK_EQ(v8::Promise::PromiseState::kRejected,
+  CHECK_NE(v8::Promise::PromiseState::kPending,
            reject_message.GetPromise()->State());
-  if (reject_message.GetEvent() == v8::kPromiseRejectWithNoHandler) {
+  switch (reject_message.GetEvent()) {
-    promise_reject_counter++;
+    case v8::kPromiseRejectWithNoHandler: {
-    global->Set(context, v8_str("rejected"), reject_message.GetPromise())
+      promise_reject_counter++;
-        .FromJust();
+      global->Set(context, v8_str("rejected"), reject_message.GetPromise())
-    global->Set(context, v8_str("value"), reject_message.GetValue()).FromJust();
+          .FromJust();
-    v8::Local<v8::Message> message = v8::Exception::CreateMessage(
+      global->Set(context, v8_str("value"), reject_message.GetValue())
-        CcTest::isolate(), reject_message.GetValue());
+          .FromJust();
-    v8::Local<v8::StackTrace> stack_trace = message->GetStackTrace();
+      v8::Local<v8::Message> message = v8::Exception::CreateMessage(
+          CcTest::isolate(), reject_message.GetValue());
+      v8::Local<v8::StackTrace> stack_trace = message->GetStackTrace();
 
-    promise_reject_msg_line_number = message->GetLineNumber(context).FromJust();
+      promise_reject_msg_line_number =
-    promise_reject_msg_column_number =
+          message->GetLineNumber(context).FromJust();
-        message->GetStartColumn(context).FromJust() + 1;
+      promise_reject_msg_column_number =
+          message->GetStartColumn(context).FromJust() + 1;
 
-    if (!stack_trace.IsEmpty()) {
+      if (!stack_trace.IsEmpty()) {
-      promise_reject_frame_count = stack_trace->GetFrameCount();
+        promise_reject_frame_count = stack_trace->GetFrameCount();
-      if (promise_reject_frame_count > 0) {
+        if (promise_reject_frame_count > 0) {
-        CHECK(stack_trace->GetFrame(0)
+          CHECK(stack_trace->GetFrame(0)
-                  ->GetScriptName()
+                    ->GetScriptName()
-                  ->Equals(context, v8_str("pro"))
+                    ->Equals(context, v8_str("pro"))
-                  .FromJust());
+                    .FromJust());
-        promise_reject_line_number = stack_trace->GetFrame(0)->GetLineNumber();
+          promise_reject_line_number =
-        promise_reject_column_number = stack_trace->GetFrame(0)->GetColumn();
+              stack_trace->GetFrame(0)->GetLineNumber();
-      } else {
+          promise_reject_column_number = stack_trace->GetFrame(0)->GetColumn();
-        promise_reject_line_number = -1;
+        } else {
-        promise_reject_column_number = -1;
+          promise_reject_line_number = -1;
+          promise_reject_column_number = -1;
+        }
       }
+      break;
+    }
+    case v8::kPromiseHandlerAddedAfterReject: {
+      promise_revoke_counter++;
+      global->Set(context, v8_str("revoked"), reject_message.GetPromise())
+          .FromJust();
+      CHECK(reject_message.GetValue().IsEmpty());
+      break;
+    }
+    case v8::kPromiseRejectAfterResolved: {
+      promise_reject_after_resolved_counter++;
+      break;
+    }
+    case v8::kPromiseResolveAfterResolved: {
+      promise_resolve_after_resolved_counter++;
+      break;
     }
-  } else {
-    promise_revoke_counter++;
-    global->Set(context, v8_str("revoked"), reject_message.GetPromise())
-        .FromJust();
-    CHECK(reject_message.GetValue().IsEmpty());
   }
 }
 
@@ -17783,6 +17801,8 @@ v8::Local<v8::Value> RejectValue() {
 void ResetPromiseStates() {
   promise_reject_counter = 0;
   promise_revoke_counter = 0;
+  promise_reject_after_resolved_counter = 0;
+  promise_resolve_after_resolved_counter = 0;
   promise_reject_msg_line_number = -1;
   promise_reject_msg_column_number = -1;
   promise_reject_line_number = -1;
@@ -18008,6 +18028,40 @@ TEST(PromiseRejectCallback) {
   CHECK_EQ(0, promise_revoke_counter);
   CHECK(RejectValue()->Equals(env.local(), v8_str("sss")).FromJust());
 
+  ResetPromiseStates();
+
+  // Swallowed exceptions in the Promise constructor.
+  CompileRun(
+      "var v0 = new Promise(\n"
+      "  function(res, rej) {\n"
+      "    res(1);\n"
+      "    throw new Error();\n"
+      "  }\n"
+      ");\n");
+  CHECK(!GetPromise("v0")->HasHandler());
+  CHECK_EQ(0, promise_reject_counter);
+  CHECK_EQ(0, promise_revoke_counter);
+  CHECK_EQ(1, promise_reject_after_resolved_counter);
+  CHECK_EQ(0, promise_resolve_after_resolved_counter);
+
+  ResetPromiseStates();
+
+  // Duplication resolve.
+  CompileRun(
+      "var r;\n"
+      "var y0 = new Promise(\n"
+      "  function(res, rej) {\n"
+      "    r = res;\n"
+      "    throw new Error();\n"
+      "  }\n"
+      ");\n"
+      "r(1);\n");
+  CHECK(!GetPromise("y0")->HasHandler());
+  CHECK_EQ(1, promise_reject_counter);
+  CHECK_EQ(0, promise_revoke_counter);
+  CHECK_EQ(0, promise_reject_after_resolved_counter);
+  CHECK_EQ(1, promise_resolve_after_resolved_counter);
+
   // Test stack frames.
   env->GetIsolate()->SetCaptureStackTraceForUncaughtExceptions(true);