FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

deps: cherry-pick 907d7bc from upstream V8 · nodejs/node@ada715f · GitHub

/ node Public

Commit ada715f

Browse files
targos
authored and
MylesBorins
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 <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>
1 parent 7299421 commit ada715f

9 files changed

Lines changed: 149 additions & 45 deletions

File tree

‎common.gypi‎

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

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

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

‎deps/v8/include/v8.h‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6520,7 +6520,9 @@ typedef void (*PromiseHook)(PromiseHookType type, Local<Promise> promise,
65206520
// --- Promise Reject Callback ---
65216521
enum PromiseRejectEvent {
65226522
kPromiseRejectWithNoHandler = 0,
6523-
kPromiseHandlerAddedAfterReject = 1
6523+
kPromiseHandlerAddedAfterReject = 1,
6524+
kPromiseRejectAfterResolved = 2,
6525+
kPromiseResolveAfterResolved = 3,
65246526
};
65256527

65266528
class PromiseRejectMessage {

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

Lines changed: 28 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -246,6 +246,8 @@ Node* PromiseBuiltinsAssembler::CreatePromiseResolvingFunctionsContext(
246246
Node* const context =
247247
CreatePromiseContext(native_context, kPromiseContextLength);
248248
StoreContextElementNoWriteBarrier(context, kPromiseSlot, promise);
249+
StoreContextElementNoWriteBarrier(context, kAlreadyResolvedSlot,
250+
FalseConstant());
249251
StoreContextElementNoWriteBarrier(context, kDebugEventSlot, debug_event);
250252
return context;
251253
}
@@ -736,17 +738,27 @@ TF_BUILTIN(PromiseCapabilityDefaultReject, PromiseBuiltinsAssembler) {
736738
Node* const promise = LoadContextElement(context, kPromiseSlot);
737739

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

743748
// 5. Set alreadyResolved.[[Value]] to true.
744-
StoreContextElementNoWriteBarrier(context, kPromiseSlot, UndefinedConstant());
749+
StoreContextElementNoWriteBarrier(context, kAlreadyResolvedSlot,
750+
TrueConstant());
745751

746752
// 6. Return RejectPromise(promise, reason).
747753
Node* const debug_event = LoadContextElement(context, kDebugEventSlot);
748754
Return(CallBuiltin(Builtins::kRejectPromise, context, promise, reason,
749755
debug_event));
756+
757+
BIND(&if_already_resolved);
758+
{
759+
Return(CallRuntime(Runtime::kPromiseRejectAfterResolved, context, promise,
760+
reason));
761+
}
750762
}
751763

752764
// ES #sec-promise-resolve-functions
@@ -758,16 +770,26 @@ TF_BUILTIN(PromiseCapabilityDefaultResolve, PromiseBuiltinsAssembler) {
758770
Node* const promise = LoadContextElement(context, kPromiseSlot);
759771

760772
// 3. Let alreadyResolved be F.[[AlreadyResolved]].
773+
Label if_already_resolved(this, Label::kDeferred);
774+
Node* const already_resolved =
775+
LoadContextElement(context, kAlreadyResolvedSlot);
776+
761777
// 4. If alreadyResolved.[[Value]] is true, return undefined.
762-
// We use undefined as a marker for the [[AlreadyResolved]] state.
763-
ReturnIf(IsUndefined(promise), UndefinedConstant());
778+
GotoIf(IsTrue(already_resolved), &if_already_resolved);
764779

765780
// 5. Set alreadyResolved.[[Value]] to true.
766-
StoreContextElementNoWriteBarrier(context, kPromiseSlot, UndefinedConstant());
781+
StoreContextElementNoWriteBarrier(context, kAlreadyResolvedSlot,
782+
TrueConstant());
767783

768784
// The rest of the logic (and the catch prediction) is
769785
// encapsulated in the dedicated ResolvePromise builtin.
770786
Return(CallBuiltin(Builtins::kResolvePromise, context, promise, resolution));
787+
788+
BIND(&if_already_resolved);
789+
{
790+
Return(CallRuntime(Runtime::kPromiseResolveAfterResolved, context, promise,
791+
resolution));
792+
}
771793
}
772794

773795
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
@@ -5406,6 +5406,10 @@ Reduction JSCallReducer::ReducePromiseConstructor(Node* node) {
54065406
graph()->NewNode(simplified()->StoreField(AccessBuilder::ForContextSlot(
54075407
PromiseBuiltinsAssembler::kPromiseSlot)),
54085408
promise_context, promise, effect, control);
5409+
effect = graph()->NewNode(
5410+
simplified()->StoreField(AccessBuilder::ForContextSlot(
5411+
PromiseBuiltinsAssembler::kAlreadyResolvedSlot)),
5412+
promise_context, jsgraph()->FalseConstant(), effect, control);
54095413
effect = graph()->NewNode(
54105414
simplified()->StoreField(AccessBuilder::ForContextSlot(
54115415
PromiseBuiltinsAssembler::kDebugEventSlot)),

‎deps/v8/src/isolate.cc‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3872,10 +3872,9 @@ void Isolate::SetPromiseRejectCallback(PromiseRejectCallback callback) {
38723872
void Isolate::ReportPromiseReject(Handle<JSPromise> promise,
38733873
Handle<Object> value,
38743874
v8::PromiseRejectEvent event) {
3875-
DCHECK_EQ(v8::Promise::kRejected, promise->status());
38763875
if (promise_reject_callback_ == nullptr) return;
38773876
Handle<FixedArray> stack_trace;
3878-
if (event == v8::kPromiseRejectWithNoHandler && value->IsJSObject()) {
3877+
if (event != v8::kPromiseHandlerAddedAfterReject && value->IsJSObject()) {
38793878
stack_trace = GetDetailedStackTrace(Handle<JSObject>::cast(value));
38803879
}
38813880
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
@@ -432,7 +432,9 @@ namespace internal {
432432
F(PromiseRevokeReject, 1, 1) \
433433
F(PromiseStatus, 1, 1) \
434434
F(RejectPromise, 3, 1) \
435-
F(ResolvePromise, 2, 1)
435+
F(ResolvePromise, 2, 1) \
436+
F(PromiseRejectAfterResolved, 2, 1) \
437+
F(PromiseResolveAfterResolved, 2, 1)
436438

437439
#define FOR_EACH_INTRINSIC_PROXY(F) \
438440
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
@@ -17700,6 +17700,8 @@ TEST(RethrowBogusErrorStackTrace) {
1770017700
v8::PromiseRejectEvent reject_event = v8::kPromiseRejectWithNoHandler;
1770117701
int promise_reject_counter = 0;
1770217702
int promise_revoke_counter = 0;
17703+
int promise_reject_after_resolved_counter = 0;
17704+
int promise_resolve_after_resolved_counter = 0;
1770317705
int promise_reject_msg_line_number = -1;
1770417706
int promise_reject_msg_column_number = -1;
1770517707
int promise_reject_line_number = -1;
@@ -17709,40 +17711,56 @@ int promise_reject_frame_count = -1;
1770917711
void PromiseRejectCallback(v8::PromiseRejectMessage reject_message) {
1771017712
v8::Local<v8::Object> global = CcTest::global();
1771117713
v8::Local<v8::Context> context = CcTest::isolate()->GetCurrentContext();
17712-
CHECK_EQ(v8::Promise::PromiseState::kRejected,
17714+
CHECK_NE(v8::Promise::PromiseState::kPending,
1771317715
reject_message.GetPromise()->State());
17714-
if (reject_message.GetEvent() == v8::kPromiseRejectWithNoHandler) {
17715-
promise_reject_counter++;
17716-
global->Set(context, v8_str("rejected"), reject_message.GetPromise())
17717-
.FromJust();
17718-
global->Set(context, v8_str("value"), reject_message.GetValue()).FromJust();
17719-
v8::Local<v8::Message> message = v8::Exception::CreateMessage(
17720-
CcTest::isolate(), reject_message.GetValue());
17721-
v8::Local<v8::StackTrace> stack_trace = message->GetStackTrace();
17722-
17723-
promise_reject_msg_line_number = message->GetLineNumber(context).FromJust();
17724-
promise_reject_msg_column_number =
17725-
message->GetStartColumn(context).FromJust() + 1;
17726-
17727-
if (!stack_trace.IsEmpty()) {
17728-
promise_reject_frame_count = stack_trace->GetFrameCount();
17729-
if (promise_reject_frame_count > 0) {
17730-
CHECK(stack_trace->GetFrame(0)
17731-
->GetScriptName()
17732-
->Equals(context, v8_str("pro"))
17733-
.FromJust());
17734-
promise_reject_line_number = stack_trace->GetFrame(0)->GetLineNumber();
17735-
promise_reject_column_number = stack_trace->GetFrame(0)->GetColumn();
17736-
} else {
17737-
promise_reject_line_number = -1;
17738-
promise_reject_column_number = -1;
17716+
switch (reject_message.GetEvent()) {
17717+
case v8::kPromiseRejectWithNoHandler: {
17718+
promise_reject_counter++;
17719+
global->Set(context, v8_str("rejected"), reject_message.GetPromise())
17720+
.FromJust();
17721+
global->Set(context, v8_str("value"), reject_message.GetValue())
17722+
.FromJust();
17723+
v8::Local<v8::Message> message = v8::Exception::CreateMessage(
17724+
CcTest::isolate(), reject_message.GetValue());
17725+
v8::Local<v8::StackTrace> stack_trace = message->GetStackTrace();
17726+
17727+
promise_reject_msg_line_number =
17728+
message->GetLineNumber(context).FromJust();
17729+
promise_reject_msg_column_number =
17730+
message->GetStartColumn(context).FromJust() + 1;
17731+
17732+
if (!stack_trace.IsEmpty()) {
17733+
promise_reject_frame_count = stack_trace->GetFrameCount();
17734+
if (promise_reject_frame_count > 0) {
17735+
CHECK(stack_trace->GetFrame(0)
17736+
->GetScriptName()
17737+
->Equals(context, v8_str("pro"))
17738+
.FromJust());
17739+
promise_reject_line_number =
17740+
stack_trace->GetFrame(0)->GetLineNumber();
17741+
promise_reject_column_number = stack_trace->GetFrame(0)->GetColumn();
17742+
} else {
17743+
promise_reject_line_number = -1;
17744+
promise_reject_column_number = -1;
17745+
}
1773917746
}
17747+
break;
17748+
}
17749+
case v8::kPromiseHandlerAddedAfterReject: {
17750+
promise_revoke_counter++;
17751+
global->Set(context, v8_str("revoked"), reject_message.GetPromise())
17752+
.FromJust();
17753+
CHECK(reject_message.GetValue().IsEmpty());
17754+
break;
17755+
}
17756+
case v8::kPromiseRejectAfterResolved: {
17757+
promise_reject_after_resolved_counter++;
17758+
break;
17759+
}
17760+
case v8::kPromiseResolveAfterResolved: {
17761+
promise_resolve_after_resolved_counter++;
17762+
break;
1774017763
}
17741-
} else {
17742-
promise_revoke_counter++;
17743-
global->Set(context, v8_str("revoked"), reject_message.GetPromise())
17744-
.FromJust();
17745-
CHECK(reject_message.GetValue().IsEmpty());
1774617764
}
1774717765
}
1774817766

@@ -17765,6 +17783,8 @@ v8::Local<v8::Value> RejectValue() {
1776517783
void ResetPromiseStates() {
1776617784
promise_reject_counter = 0;
1776717785
promise_revoke_counter = 0;
17786+
promise_reject_after_resolved_counter = 0;
17787+
promise_resolve_after_resolved_counter = 0;
1776817788
promise_reject_msg_line_number = -1;
1776917789
promise_reject_msg_column_number = -1;
1777017790
promise_reject_line_number = -1;
@@ -17990,6 +18010,40 @@ TEST(PromiseRejectCallback) {
1799018010
CHECK_EQ(0, promise_revoke_counter);
1799118011
CHECK(RejectValue()->Equals(env.local(), v8_str("sss")).FromJust());
1799218012

18013+
ResetPromiseStates();
18014+
18015+
// Swallowed exceptions in the Promise constructor.
18016+
CompileRun(
18017+
"var v0 = new Promise(\n"
18018+
" function(res, rej) {\n"
18019+
" res(1);\n"
18020+
" throw new Error();\n"
18021+
" }\n"
18022+
");\n");
18023+
CHECK(!GetPromise("v0")->HasHandler());
18024+
CHECK_EQ(0, promise_reject_counter);
18025+
CHECK_EQ(0, promise_revoke_counter);
18026+
CHECK_EQ(1, promise_reject_after_resolved_counter);
18027+
CHECK_EQ(0, promise_resolve_after_resolved_counter);
18028+
18029+
ResetPromiseStates();
18030+
18031+
// Duplication resolve.
18032+
CompileRun(
18033+
"var r;\n"
18034+
"var y0 = new Promise(\n"
18035+
" function(res, rej) {\n"
18036+
" r = res;\n"
18037+
" throw new Error();\n"
18038+
" }\n"
18039+
");\n"
18040+
"r(1);\n");
18041+
CHECK(!GetPromise("y0")->HasHandler());
18042+
CHECK_EQ(1, promise_reject_counter);
18043+
CHECK_EQ(0, promise_revoke_counter);
18044+
CHECK_EQ(0, promise_reject_after_resolved_counter);
18045+
CHECK_EQ(1, promise_resolve_after_resolved_counter);
18046+
1799318047
// Test stack frames.
1799418048
env->GetIsolate()->SetCaptureStackTraceForUncaughtExceptions(true);
1799518049

0 commit comments

Comments
 (0)
Back | FazBrowse Home | New Git URL