-
Notifications
You must be signed in to change notification settings - Fork 4.9k
fix(napi): make napi_create_error succeed when pending exception exists #29785
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 14 commits
60374b3
2a17bee
3914905
e06dff1
21d1bc2
20fbd29
ca51d6c
3c3b42b
c831a71
b80e724
adc989e
b90c9ff
67f33c1
1038bfe
e2f9db2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1070,8 +1070,6 @@ static napi_status createErrorWithNapiValues(napi_env env, napi_value code, napi | |
| { | ||
| auto* globalObject = toJS(env); | ||
| auto& vm = JSC::getVM(globalObject); | ||
| auto scope = DECLARE_THROW_SCOPE(vm); | ||
| RETURN_IF_EXCEPTION(scope, napi_pending_exception); | ||
|
|
||
| NAPI_CHECK_ARG(env, result); | ||
| NAPI_CHECK_ARG(env, message); | ||
|
|
@@ -1081,15 +1079,24 @@ static napi_status createErrorWithNapiValues(napi_env env, napi_value code, napi | |
| js_message.isString() && (js_code.isEmpty() || js_code.isString()), | ||
| napi_string_expected); | ||
|
|
||
| // napi_create_error-like functions should succeed even when a VM-level | ||
| // exception is already pending (matching Node.js behavior). We remember | ||
| // whether an exception was already present on entry so we only report | ||
| // NEW exceptions that may be raised by our own operations below. | ||
| bool hadPreexistingException = vm.hasExceptionsAfterHandlingTraps(); | ||
|
|
||
| auto wtf_code = js_code.isEmpty() ? WTF::String() : js_code.getString(globalObject); | ||
| RETURN_IF_EXCEPTION(scope, napi_set_last_error(env, napi_pending_exception)); | ||
| auto wtf_message = js_message.getString(globalObject); | ||
| RETURN_IF_EXCEPTION(scope, napi_set_last_error(env, napi_pending_exception)); | ||
|
|
||
| *result = toNapi( | ||
| createErrorWithCode(vm, globalObject, wtf_code, wtf_message, type), | ||
| globalObject); | ||
| RETURN_IF_EXCEPTION(scope, napi_set_last_error(env, napi_pending_exception)); | ||
|
|
||
| // Surface new exceptions originating from getString() / createErrorWithCode(), | ||
| // but ignore any exception that was already pending before we entered. | ||
| if (!hadPreexistingException && vm.hasExceptionsAfterHandlingTraps()) { | ||
| return napi_set_last_error(env, napi_pending_exception); | ||
| } | ||
|
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good point about the masking risk. The current approach is a conscious trade-off matching Node.js behavior: "don't fail and don't disturb the existing pending exception." The operations in question ( That said, the concern is valid in theory. If a future change makes
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Updated the approach: instead of snapshotting and restoring the pre-existing exception (which would require JSC APIs not safely available here — Key insight: Behavior matrix:
Comment on lines
+1082
to
+1098
|
||
| return napi_set_last_error(env, napi_ok); | ||
| } | ||
|
|
||
|
|
||
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Uh oh!
There was an error while loading. Please reload this page.