Skip to content

Commit

Permalink
deps: cherry-pick 907d7bc from upstream V8
Browse files Browse the repository at this point in the history
Original commit message:

    [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}

Refs: v8/v8@907d7bc

Backport-PR-URL: #21668
PR-URL: #21838
Refs: nodejs/promises-debugging#8
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Yang Guo <yangguo@chromium.org>
Reviewed-By: Benedikt Meurer <benedikt.meurer@gmail.com>
  • Loading branch information
targos authored and MylesBorins committed Aug 10, 2018
1 parent 7299421 commit ada715f
Show file tree
Hide file tree
Showing 9 changed files with 149 additions and 45 deletions.
2 changes: 1 addition & 1 deletion common.gypi
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@

# Reset this number to 0 on major V8 upgrades.
# Increment by one for each non-official patch applied to deps/v8.
'v8_embedder_string': '-node.9',
'v8_embedder_string': '-node.10',

# Enable disassembler for `--print-code` v8 options
'v8_enable_disassembler': 1,
Expand Down
4 changes: 3 additions & 1 deletion deps/v8/include/v8.h
Original file line number Diff line number Diff line change
Expand Up @@ -6520,7 +6520,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 {
Expand Down
34 changes: 28 additions & 6 deletions deps/v8/src/builtins/builtins-promise-gen.cc
Original file line number Diff line number Diff line change
Expand Up @@ -246,6 +246,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;
}
Expand Down Expand Up @@ -736,17 +738,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.
ReturnIf(IsUndefined(promise), UndefinedConstant());
GotoIf(IsTrue(already_resolved), &if_already_resolved);

// 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
Expand All @@ -758,16 +770,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.
ReturnIf(IsUndefined(promise), UndefinedConstant());
GotoIf(IsTrue(already_resolved), &if_already_resolved);

// 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) {
Expand Down
7 changes: 4 additions & 3 deletions deps/v8/src/builtins/builtins-promise-gen.h
Original file line number Diff line number Diff line change
Expand Up @@ -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
// undefined, then we've already visited this callback and it
// should be a no-op.
// The promise which resolve/reject callbacks fulfill.
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,
Expand Down
4 changes: 4 additions & 0 deletions deps/v8/src/compiler/js-call-reducer.cc
Original file line number Diff line number Diff line change
Expand Up @@ -5406,6 +5406,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)),
Expand Down
3 changes: 1 addition & 2 deletions deps/v8/src/isolate.cc
Original file line number Diff line number Diff line change
Expand Up @@ -3872,10 +3872,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(
Expand Down
20 changes: 20 additions & 0 deletions deps/v8/src/runtime/runtime-promise.cc
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,26 @@ RUNTIME_FUNCTION(Runtime_PromiseRejectEventFromStack) {
return isolate->heap()->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 isolate->heap()->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 isolate->heap()->undefined_value();
}

RUNTIME_FUNCTION(Runtime_PromiseRevokeReject) {
DCHECK_EQ(1, args.length());
HandleScope scope(isolate);
Expand Down
4 changes: 3 additions & 1 deletion deps/v8/src/runtime/runtime.h
Original file line number Diff line number Diff line change
Expand Up @@ -432,7 +432,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) \
Expand Down
116 changes: 85 additions & 31 deletions deps/v8/test/cctest/test-api.cc
Original file line number Diff line number Diff line change
Expand Up @@ -17700,6 +17700,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;
Expand All @@ -17709,40 +17711,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) {
promise_reject_counter++;
global->Set(context, v8_str("rejected"), reject_message.GetPromise())
.FromJust();
global->Set(context, v8_str("value"), reject_message.GetValue()).FromJust();
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_column_number =
message->GetStartColumn(context).FromJust() + 1;

if (!stack_trace.IsEmpty()) {
promise_reject_frame_count = stack_trace->GetFrameCount();
if (promise_reject_frame_count > 0) {
CHECK(stack_trace->GetFrame(0)
->GetScriptName()
->Equals(context, v8_str("pro"))
.FromJust());
promise_reject_line_number = stack_trace->GetFrame(0)->GetLineNumber();
promise_reject_column_number = stack_trace->GetFrame(0)->GetColumn();
} else {
promise_reject_line_number = -1;
promise_reject_column_number = -1;
switch (reject_message.GetEvent()) {
case v8::kPromiseRejectWithNoHandler: {
promise_reject_counter++;
global->Set(context, v8_str("rejected"), reject_message.GetPromise())
.FromJust();
global->Set(context, v8_str("value"), reject_message.GetValue())
.FromJust();
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_column_number =
message->GetStartColumn(context).FromJust() + 1;

if (!stack_trace.IsEmpty()) {
promise_reject_frame_count = stack_trace->GetFrameCount();
if (promise_reject_frame_count > 0) {
CHECK(stack_trace->GetFrame(0)
->GetScriptName()
->Equals(context, v8_str("pro"))
.FromJust());
promise_reject_line_number =
stack_trace->GetFrame(0)->GetLineNumber();
promise_reject_column_number = stack_trace->GetFrame(0)->GetColumn();
} else {
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());
}
}

Expand All @@ -17765,6 +17783,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;
Expand Down Expand Up @@ -17990,6 +18010,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);

Expand Down

0 comments on commit ada715f

Please sign in to comment.