Skip to content

Commit ec0ff70

Browse files
targostargos
committed
deps: cherry-pick 907d7bc from upstream V8
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 <[email protected]> Reviewed-by: Benedikt Meurer <[email protected]> Reviewed-by: Yang Guo <[email protected]> Commit-Queue: Benedikt Meurer <[email protected]> Cr-Commit-Position: refs/heads/master@{#54309} Refs: v8/v8@907d7bc PR-URL: #21838 Refs: nodejs/promises-debugging#8 Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Yang Guo <[email protected]> Reviewed-By: Benedikt Meurer <[email protected]>
1 parent c23e8b5 commit ec0ff70

File tree

9 files changed

+149
-45
lines changed

9 files changed

+149
-45
lines changed

‎common.gypi

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,7 @@
2828

2929
# Reset this number to 0 on major V8 upgrades.
3030
# Increment by one for each non-official patch applied to deps/v8.
31-
'v8_embedder_string': '-node.16',
31+
'v8_embedder_string': '-node.17',
3232

3333
# Enable disassembler for `--print-code` v8 options
3434
'v8_enable_disassembler': 1,

‎deps/v8/include/v8.h

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6394,7 +6394,9 @@ typedef void (*PromiseHook)(PromiseHookType type, Local<Promise> promise,
63946394
// --- Promise Reject Callback ---
63956395
enum PromiseRejectEvent {
63966396
kPromiseRejectWithNoHandler = 0,
6397-
kPromiseHandlerAddedAfterReject = 1
6397+
kPromiseHandlerAddedAfterReject = 1,
6398+
kPromiseRejectAfterResolved = 2,
6399+
kPromiseResolveAfterResolved = 3,
63986400
};
63996401

64006402
class PromiseRejectMessage {

‎deps/v8/src/builtins/builtins-promise-gen.cc

Lines changed: 28 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -247,6 +247,8 @@ Node* PromiseBuiltinsAssembler::CreatePromiseResolvingFunctionsContext(
247247
Node* const context =
248248
CreatePromiseContext(native_context, kPromiseContextLength);
249249
StoreContextElementNoWriteBarrier(context, kPromiseSlot, promise);
250+
StoreContextElementNoWriteBarrier(context, kAlreadyResolvedSlot,
251+
FalseConstant());
250252
StoreContextElementNoWriteBarrier(context, kDebugEventSlot, debug_event);
251253
return context;
252254
}
@@ -733,17 +735,27 @@ TF_BUILTIN(PromiseCapabilityDefaultReject, PromiseBuiltinsAssembler) {
733735
Node* const promise = LoadContextElement(context, kPromiseSlot);
734736

735737
// 3. Let alreadyResolved be F.[[AlreadyResolved]].
738+
Label if_already_resolved(this, Label::kDeferred);
739+
Node* const already_resolved =
740+
LoadContextElement(context, kAlreadyResolvedSlot);
741+
736742
// 4. If alreadyResolved.[[Value]] is true, return undefined.
737-
// We use undefined as a marker for the [[AlreadyResolved]] state.
738-
ReturnIf(IsUndefined(promise), UndefinedConstant());
743+
GotoIf(IsTrue(already_resolved), &if_already_resolved);
739744

740745
// 5. Set alreadyResolved.[[Value]] to true.
741-
StoreContextElementNoWriteBarrier(context, kPromiseSlot, UndefinedConstant());
746+
StoreContextElementNoWriteBarrier(context, kAlreadyResolvedSlot,
747+
TrueConstant());
742748

743749
// 6. Return RejectPromise(promise, reason).
744750
Node* const debug_event = LoadContextElement(context, kDebugEventSlot);
745751
Return(CallBuiltin(Builtins::kRejectPromise, context, promise, reason,
746752
debug_event));
753+
754+
BIND(&if_already_resolved);
755+
{
756+
Return(CallRuntime(Runtime::kPromiseRejectAfterResolved, context, promise,
757+
reason));
758+
}
747759
}
748760

749761
// ES #sec-promise-resolve-functions
@@ -755,16 +767,26 @@ TF_BUILTIN(PromiseCapabilityDefaultResolve, PromiseBuiltinsAssembler) {
755767
Node* const promise = LoadContextElement(context, kPromiseSlot);
756768

757769
// 3. Let alreadyResolved be F.[[AlreadyResolved]].
770+
Label if_already_resolved(this, Label::kDeferred);
771+
Node* const already_resolved =
772+
LoadContextElement(context, kAlreadyResolvedSlot);
773+
758774
// 4. If alreadyResolved.[[Value]] is true, return undefined.
759-
// We use undefined as a marker for the [[AlreadyResolved]] state.
760-
ReturnIf(IsUndefined(promise), UndefinedConstant());
775+
GotoIf(IsTrue(already_resolved), &if_already_resolved);
761776

762777
// 5. Set alreadyResolved.[[Value]] to true.
763-
StoreContextElementNoWriteBarrier(context, kPromiseSlot, UndefinedConstant());
778+
StoreContextElementNoWriteBarrier(context, kAlreadyResolvedSlot,
779+
TrueConstant());
764780

765781
// The rest of the logic (and the catch prediction) is
766782
// encapsulated in the dedicated ResolvePromise builtin.
767783
Return(CallBuiltin(Builtins::kResolvePromise, context, promise, resolution));
784+
785+
BIND(&if_already_resolved);
786+
{
787+
Return(CallRuntime(Runtime::kPromiseResolveAfterResolved, context, promise,
788+
resolution));
789+
}
768790
}
769791

770792
TF_BUILTIN(PromiseConstructorLazyDeoptContinuation, PromiseBuiltinsAssembler) {

‎deps/v8/src/builtins/builtins-promise-gen.h

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -17,11 +17,12 @@ typedef compiler::CodeAssemblerState CodeAssemblerState;
1717
class PromiseBuiltinsAssembler : public CodeStubAssembler {
1818
public:
1919
enum PromiseResolvingFunctionContextSlot {
20-
// The promise which resolve/reject callbacks fulfill. If this is
21-
// undefined, then we've already visited this callback and it
22-
// should be a no-op.
20+
// The promise which resolve/reject callbacks fulfill.
2321
kPromiseSlot = Context::MIN_CONTEXT_SLOTS,
2422

23+
// Whether the callback was already invoked.
24+
kAlreadyResolvedSlot,
25+
2526
// Whether to trigger a debug event or not. Used in catch
2627
// prediction.
2728
kDebugEventSlot,

‎deps/v8/src/compiler/js-call-reducer.cc

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5345,6 +5345,10 @@ Reduction JSCallReducer::ReducePromiseConstructor(Node* node) {
53455345
graph()->NewNode(simplified()->StoreField(AccessBuilder::ForContextSlot(
53465346
PromiseBuiltinsAssembler::kPromiseSlot)),
53475347
promise_context, promise, effect, control);
5348+
effect = graph()->NewNode(
5349+
simplified()->StoreField(AccessBuilder::ForContextSlot(
5350+
PromiseBuiltinsAssembler::kAlreadyResolvedSlot)),
5351+
promise_context, jsgraph()->FalseConstant(), effect, control);
53485352
effect = graph()->NewNode(
53495353
simplified()->StoreField(AccessBuilder::ForContextSlot(
53505354
PromiseBuiltinsAssembler::kDebugEventSlot)),

‎deps/v8/src/isolate.cc

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3875,10 +3875,9 @@ void Isolate::SetPromiseRejectCallback(PromiseRejectCallback callback) {
38753875
void Isolate::ReportPromiseReject(Handle<JSPromise> promise,
38763876
Handle<Object> value,
38773877
v8::PromiseRejectEvent event) {
3878-
DCHECK_EQ(v8::Promise::kRejected, promise->status());
38793878
if (promise_reject_callback_ == nullptr) return;
38803879
Handle<FixedArray> stack_trace;
3881-
if (event == v8::kPromiseRejectWithNoHandler && value->IsJSObject()) {
3880+
if (event != v8::kPromiseHandlerAddedAfterReject && value->IsJSObject()) {
38823881
stack_trace = GetDetailedStackTrace(Handle<JSObject>::cast(value));
38833882
}
38843883
promise_reject_callback_(v8::PromiseRejectMessage(

‎deps/v8/src/runtime/runtime-promise.cc

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,26 @@ RUNTIME_FUNCTION(Runtime_PromiseRejectEventFromStack) {
3838
return isolate->heap()->undefined_value();
3939
}
4040

41+
RUNTIME_FUNCTION(Runtime_PromiseRejectAfterResolved) {
42+
DCHECK_EQ(2, args.length());
43+
HandleScope scope(isolate);
44+
CONVERT_ARG_HANDLE_CHECKED(JSPromise, promise, 0);
45+
CONVERT_ARG_HANDLE_CHECKED(Object, reason, 1);
46+
isolate->ReportPromiseReject(promise, reason,
47+
v8::kPromiseRejectAfterResolved);
48+
return isolate->heap()->undefined_value();
49+
}
50+
51+
RUNTIME_FUNCTION(Runtime_PromiseResolveAfterResolved) {
52+
DCHECK_EQ(2, args.length());
53+
HandleScope scope(isolate);
54+
CONVERT_ARG_HANDLE_CHECKED(JSPromise, promise, 0);
55+
CONVERT_ARG_HANDLE_CHECKED(Object, resolution, 1);
56+
isolate->ReportPromiseReject(promise, resolution,
57+
v8::kPromiseResolveAfterResolved);
58+
return isolate->heap()->undefined_value();
59+
}
60+
4161
RUNTIME_FUNCTION(Runtime_PromiseRevokeReject) {
4262
DCHECK_EQ(1, args.length());
4363
HandleScope scope(isolate);

‎deps/v8/src/runtime/runtime.h

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -433,7 +433,9 @@ namespace internal {
433433
F(PromiseRevokeReject, 1, 1) \
434434
F(PromiseStatus, 1, 1) \
435435
F(RejectPromise, 3, 1) \
436-
F(ResolvePromise, 2, 1)
436+
F(ResolvePromise, 2, 1) \
437+
F(PromiseRejectAfterResolved, 2, 1) \
438+
F(PromiseResolveAfterResolved, 2, 1)
437439

438440
#define FOR_EACH_INTRINSIC_PROXY(F) \
439441
F(CheckProxyGetSetTrapResult, 2, 1) \

‎deps/v8/test/cctest/test-api.cc

Lines changed: 85 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -17625,6 +17625,8 @@ TEST(RethrowBogusErrorStackTrace) {
1762517625
v8::PromiseRejectEvent reject_event = v8::kPromiseRejectWithNoHandler;
1762617626
int promise_reject_counter = 0;
1762717627
int promise_revoke_counter = 0;
17628+
int promise_reject_after_resolved_counter = 0;
17629+
int promise_resolve_after_resolved_counter = 0;
1762817630
int promise_reject_msg_line_number = -1;
1762917631
int promise_reject_msg_column_number = -1;
1763017632
int promise_reject_line_number = -1;
@@ -17634,40 +17636,56 @@ int promise_reject_frame_count = -1;
1763417636
void PromiseRejectCallback(v8::PromiseRejectMessage reject_message) {
1763517637
v8::Local<v8::Object> global = CcTest::global();
1763617638
v8::Local<v8::Context> context = CcTest::isolate()->GetCurrentContext();
17637-
CHECK_EQ(v8::Promise::PromiseState::kRejected,
17639+
CHECK_NE(v8::Promise::PromiseState::kPending,
1763817640
reject_message.GetPromise()->State());
17639-
if (reject_message.GetEvent() == v8::kPromiseRejectWithNoHandler) {
17640-
promise_reject_counter++;
17641-
global->Set(context, v8_str("rejected"), reject_message.GetPromise())
17642-
.FromJust();
17643-
global->Set(context, v8_str("value"), reject_message.GetValue()).FromJust();
17644-
v8::Local<v8::Message> message = v8::Exception::CreateMessage(
17645-
CcTest::isolate(), reject_message.GetValue());
17646-
v8::Local<v8::StackTrace> stack_trace = message->GetStackTrace();
17647-
17648-
promise_reject_msg_line_number = message->GetLineNumber(context).FromJust();
17649-
promise_reject_msg_column_number =
17650-
message->GetStartColumn(context).FromJust() + 1;
17651-
17652-
if (!stack_trace.IsEmpty()) {
17653-
promise_reject_frame_count = stack_trace->GetFrameCount();
17654-
if (promise_reject_frame_count > 0) {
17655-
CHECK(stack_trace->GetFrame(0)
17656-
->GetScriptName()
17657-
->Equals(context, v8_str("pro"))
17658-
.FromJust());
17659-
promise_reject_line_number = stack_trace->GetFrame(0)->GetLineNumber();
17660-
promise_reject_column_number = stack_trace->GetFrame(0)->GetColumn();
17661-
} else {
17662-
promise_reject_line_number = -1;
17663-
promise_reject_column_number = -1;
17641+
switch (reject_message.GetEvent()) {
17642+
case v8::kPromiseRejectWithNoHandler: {
17643+
promise_reject_counter++;
17644+
global->Set(context, v8_str("rejected"), reject_message.GetPromise())
17645+
.FromJust();
17646+
global->Set(context, v8_str("value"), reject_message.GetValue())
17647+
.FromJust();
17648+
v8::Local<v8::Message> message = v8::Exception::CreateMessage(
17649+
CcTest::isolate(), reject_message.GetValue());
17650+
v8::Local<v8::StackTrace> stack_trace = message->GetStackTrace();
17651+
17652+
promise_reject_msg_line_number =
17653+
message->GetLineNumber(context).FromJust();
17654+
promise_reject_msg_column_number =
17655+
message->GetStartColumn(context).FromJust() + 1;
17656+
17657+
if (!stack_trace.IsEmpty()) {
17658+
promise_reject_frame_count = stack_trace->GetFrameCount();
17659+
if (promise_reject_frame_count > 0) {
17660+
CHECK(stack_trace->GetFrame(0)
17661+
->GetScriptName()
17662+
->Equals(context, v8_str("pro"))
17663+
.FromJust());
17664+
promise_reject_line_number =
17665+
stack_trace->GetFrame(0)->GetLineNumber();
17666+
promise_reject_column_number = stack_trace->GetFrame(0)->GetColumn();
17667+
} else {
17668+
promise_reject_line_number = -1;
17669+
promise_reject_column_number = -1;
17670+
}
1766417671
}
17672+
break;
17673+
}
17674+
case v8::kPromiseHandlerAddedAfterReject: {
17675+
promise_revoke_counter++;
17676+
global->Set(context, v8_str("revoked"), reject_message.GetPromise())
17677+
.FromJust();
17678+
CHECK(reject_message.GetValue().IsEmpty());
17679+
break;
17680+
}
17681+
case v8::kPromiseRejectAfterResolved: {
17682+
promise_reject_after_resolved_counter++;
17683+
break;
17684+
}
17685+
case v8::kPromiseResolveAfterResolved: {
17686+
promise_resolve_after_resolved_counter++;
17687+
break;
1766517688
}
17666-
} else {
17667-
promise_revoke_counter++;
17668-
global->Set(context, v8_str("revoked"), reject_message.GetPromise())
17669-
.FromJust();
17670-
CHECK(reject_message.GetValue().IsEmpty());
1767117689
}
1767217690
}
1767317691

@@ -17690,6 +17708,8 @@ v8::Local<v8::Value> RejectValue() {
1769017708
void ResetPromiseStates() {
1769117709
promise_reject_counter = 0;
1769217710
promise_revoke_counter = 0;
17711+
promise_reject_after_resolved_counter = 0;
17712+
promise_resolve_after_resolved_counter = 0;
1769317713
promise_reject_msg_line_number = -1;
1769417714
promise_reject_msg_column_number = -1;
1769517715
promise_reject_line_number = -1;
@@ -17915,6 +17935,40 @@ TEST(PromiseRejectCallback) {
1791517935
CHECK_EQ(0, promise_revoke_counter);
1791617936
CHECK(RejectValue()->Equals(env.local(), v8_str("sss")).FromJust());
1791717937

17938+
ResetPromiseStates();
17939+
17940+
// Swallowed exceptions in the Promise constructor.
17941+
CompileRun(
17942+
"var v0 = new Promise(\n"
17943+
" function(res, rej) {\n"
17944+
" res(1);\n"
17945+
" throw new Error();\n"
17946+
" }\n"
17947+
");\n");
17948+
CHECK(!GetPromise("v0")->HasHandler());
17949+
CHECK_EQ(0, promise_reject_counter);
17950+
CHECK_EQ(0, promise_revoke_counter);
17951+
CHECK_EQ(1, promise_reject_after_resolved_counter);
17952+
CHECK_EQ(0, promise_resolve_after_resolved_counter);
17953+
17954+
ResetPromiseStates();
17955+
17956+
// Duplication resolve.
17957+
CompileRun(
17958+
"var r;\n"
17959+
"var y0 = new Promise(\n"
17960+
" function(res, rej) {\n"
17961+
" r = res;\n"
17962+
" throw new Error();\n"
17963+
" }\n"
17964+
");\n"
17965+
"r(1);\n");
17966+
CHECK(!GetPromise("y0")->HasHandler());
17967+
CHECK_EQ(1, promise_reject_counter);
17968+
CHECK_EQ(0, promise_revoke_counter);
17969+
CHECK_EQ(0, promise_reject_after_resolved_counter);
17970+
CHECK_EQ(1, promise_resolve_after_resolved_counter);
17971+
1791817972
// Test stack frames.
1791917973
env->GetIsolate()->SetCaptureStackTraceForUncaughtExceptions(true);
1792017974

0 commit comments

Comments
 (0)