node-api: return napi_exception_pending on throwing proxy handlers#48607

legendecas · 2023-06-30T03:51:49Z

Accessing JS objects can be trapped with proxy handlers. These
handlers can throw when no node-api errors occur. In such cases,
napi_pending_exception should be returned.

Refs: #48440 (comment)

nodejs-github-bot · 2023-06-30T03:51:53Z

Review requested:

@nodejs/gyp
@nodejs/node-api

mhdawson · 2023-07-14T15:33:20Z

We discussed in the meeting today. We already have some similar callsites which are returning the exception correctly and therefore should already be handling the way the status code will be returned after the update.

@legendecas suggested that we run CITGM as well to gather some more info.

test/js-native-api/test_object/test_exceptions.c

gabrielschulhof

One minor suggestion.

github-actions · 2023-07-28T08:32:34Z

Failed to start CI

- Validating Jenkins credentials ✔ Jenkins credentials valid - Starting PR CI job ✘ Failed to start PR CI: 200 OK

https://github.com/nodejs/node/actions/runs/5689928114

nodejs-github-bot · 2023-07-28T09:58:47Z

CI: https://ci.nodejs.org/job/node-test-pull-request/52959/

nodejs-github-bot · 2023-07-31T02:26:25Z

CI: https://ci.nodejs.org/job/node-test-pull-request/53003/

mhdawson

LGTM

nodejs-github-bot · 2023-08-23T18:20:29Z

CI: https://ci.nodejs.org/job/node-test-pull-request/53527/

aduh95 · 2023-09-18T09:30:25Z

Seemingly related test failure:

not ok 3802 js-native-api/test_object/test_exceptions --- duration_ms: 718.38600 severity: crashed exitcode: -5 stack: |- # # Fatal error in ../deps/v8/src/ic/ic.cc, line 2690 # Debug check failed: !__isolate__->has_pending_exception(). # # # #FailureMessage Object: 0xffffc7888778 1: 0xaaaaea9d09a0 node::DumpBacktrace(_IO_FILE*) [out/Debug/node] 2: 0xaaaaeabb6b04 [out/Debug/node] 3: 0xaaaaeabb6b30 [out/Debug/node] 4: 0xaaaaec90a7b8 V8_Fatal(char const*, int, char const*, ...) [out/Debug/node] 5: 0xaaaaec90a7e0 V8_Dcheck(char const*, int, char const*) [out/Debug/node] 6: 0xaaaaeb3eb978 v8::internal::Runtime_LoadNoFeedbackIC_Miss(int, unsigned long*, v8::internal::Isolate*) [out/Debug/node] 7: 0xaaaaebf8c3e4 [out/Debug/node] ...

legendecas · 2023-09-18T15:58:06Z

Yes, I'm trying to reproduce it locally with an arm Linux setup.

legendecas · 2023-09-20T17:54:27Z

The debug test failure is caused by the fact that v8::Object::DefineProperty didn't clear the pending exception and v8::internal::Isolate::has_pending_exception() still returned true after the v8 API call returned. This can be fixed by https://chromium-review.googlesource.com/c/v8/v8/+/4594617.

The problem can happen whenever v8::Object::DefineProperty is called on a throwing JSProxy trap handler and invokes another V8 API (but only crashes on debug build).

Accessing JS objects can be trapped with proxy handlers. These handlers can throw when no node-api errors occur. In such cases, `napi_pending_exception` should be returned.

nodejs-github-bot · 2023-10-12T10:03:54Z

CI: https://ci.nodejs.org/job/node-test-pull-request/54742/

nodejs-github-bot · 2023-10-17T02:21:35Z

CI: https://ci.nodejs.org/job/node-test-pull-request/54866/

nodejs-github-bot · 2023-10-17T15:54:31Z

CI: https://ci.nodejs.org/job/node-test-pull-request/54888/

legendecas · 2023-10-18T17:15:41Z

@nodejs/node-api CI has passed. Would you mind taking a look at this again? Thank you!

legendecas · 2023-10-23T07:41:29Z

As discussed at node-api meeting Oct 20, the change is essentially a clean rebase so I'm going to land this.

This depends on 17a74dd which is included in v21.x.

nodejs-github-bot · 2023-10-23T07:45:30Z

Commit Queue failed

- Loading data for nodejs/node/pull/48607 ✔ Done loading data for nodejs/node/pull/48607 ----------------------------------- PR info ------------------------------------ Title node-api: return napi_exception_pending on throwing proxy handlers (#48607) ⚠ Could not retrieve the email or name of the PR author's from user's GitHub profile! Branch legendecas:node-api/define-property -> nodejs:main Labels c++, node-api, needs-ci, dont-land-on-v18.x, dont-land-on-v20.x Commits 1 - node-api: return napi_exception_pending on proxy handlers Committers 1 - Chengzhong Wu PR-URL: https://github.com/nodejs/node/pull/48607 Reviewed-By: Gabriel Schulhof Reviewed-By: Michael Dawson ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/48607 Reviewed-By: Gabriel Schulhof Reviewed-By: Michael Dawson -------------------------------------------------------------------------------- ⚠ Commits were pushed since the last approving review: ⚠ - node-api: return napi_exception_pending on proxy handlers ℹ This PR was created on Fri, 30 Jun 2023 03:51:49 GMT ✔ Approvals: 2 ✔ - Gabriel Schulhof (@gabrielschulhof): https://github.com/nodejs/node/pull/48607#pullrequestreview-1584729738 ✔ - Michael Dawson (@mhdawson) (TSC): https://github.com/nodejs/node/pull/48607#pullrequestreview-1592156805 ✔ Last GitHub CI successful ℹ Last Full PR CI on 2023-10-17T15:54:31Z: https://ci.nodejs.org/job/node-test-pull-request/54888/ - Querying data for job/node-test-pull-request/54888/ ✔ Last Jenkins CI successful -------------------------------------------------------------------------------- ✔ Aborted `git node land` session in /home/runner/work/node/node/.ncu

https://github.com/nodejs/node/actions/runs/6610202932

mhdawson · 2023-10-23T22:18:19Z

Landed in 52fcf14

Accessing JS objects can be trapped with proxy handlers. These handlers can throw when no node-api errors occur. In such cases, `napi_pending_exception` should be returned. PR-URL: #48607 Reviewed-By: Gabriel Schulhof <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

Accessing JS objects can be trapped with proxy handlers. These handlers can throw when no node-api errors occur. In such cases, `napi_pending_exception` should be returned. PR-URL: nodejs#48607 Reviewed-By: Gabriel Schulhof <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

Accessing JS objects can be trapped with proxy handlers. These handlers can throw when no node-api errors occur. In such cases, `napi_pending_exception` should be returned. PR-URL: #48607 Reviewed-By: Gabriel Schulhof <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. node-api Issues and PRs related to the Node-API. labels Jun 30, 2023

legendecas force-pushed the node-api/define-property branch 2 times, most recently from e899363 to 3b3bc46Compare June 30, 2023 05:50

toyobayashi mentioned this pull request Jun 30, 2023
test: return napi_exception_pending on throwing proxy handlers toyobayashi/emnapi#75
Merged

gabrielschulhof reviewed Jul 14, 2023
View reviewed changes

test/js-native-api/test_object/test_exceptions.c Outdated Show resolvedHide resolved
test/js-native-api/test_object/test_exceptions.c Outdated Show resolvedHide resolved

gabrielschulhof reviewed Jul 14, 2023
View reviewed changes

legendecas added the request-ci Add this label to start a Jenkins CI on a PR. label Jul 28, 2023

github-actionsbot added request-ci-failed An error occurred while starting CI via request-ci label, and manual interventon is needed. and removed request-ci Add this label to start a Jenkins CI on a PR. labels Jul 28, 2023

This was referenced Jul 29, 2023
CI Reliability 2023-07-29 nodejs/reliability#627
Open
CI Reliability 2023-07-30 nodejs/reliability#628
Open
CI Reliability 2023-07-31 nodejs/reliability#629
Open

legendecas removed the request-ci-failed An error occurred while starting CI via request-ci label, and manual interventon is needed. label Jul 31, 2023

This was referenced Aug 1, 2023
CI Reliability 2023-08-01 nodejs/reliability#630
Open
CI Reliability 2023-08-02 nodejs/reliability#631
Open
CI Reliability 2023-08-03 nodejs/reliability#632
Open
CI Reliability 2023-08-04 nodejs/reliability#633
Open

gabrielschulhof approved these changes Aug 18, 2023
View reviewed changes

mhdawson approved these changes Aug 23, 2023
View reviewed changes

mhdawson added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 23, 2023

github-actionsbot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 23, 2023

github-actionsbot mentioned this pull request Aug 24, 2023
CI Reliability 2023-08-24 nodejs/reliability#644
Open
27 tasks

legendecas added request-ci Add this label to start a Jenkins CI on a PR. and removed request-ci Add this label to start a Jenkins CI on a PR. labels Sep 15, 2023

legendecas mentioned this pull request Sep 22, 2023
Tracking issues for V8 changes needed nodejs/abi-stable-node#457
Closed

node-api: return napi_exception_pending on proxy handlers
a4f2dbf
Accessing JS objects can be trapped with proxy handlers. These handlers can throw when no node-api errors occur. In such cases, `napi_pending_exception` should be returned.

legendecas force-pushed the node-api/define-property branch from 4b74fa5 to a4f2dbfCompare October 12, 2023 10:01

legendecas added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 12, 2023

github-actionsbot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 12, 2023

mhdawson added dont-land-on-v18.x dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. labels Oct 20, 2023

legendecas added the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 23, 2023

nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Oct 23, 2023

mhdawson closed this Oct 23, 2023

legendecas deleted the node-api/define-property branch October 24, 2023 06:55

targos mentioned this pull request Oct 24, 2023
v21.1.0 release proposal #50335
Merged

marco-ippolito mentioned this pull request Mar 6, 2025
v20.19.0 proposal #57349
Merged

Uh oh!

node-api: return napi_exception_pending on throwing proxy handlers#48607

node-api: return napi_exception_pending on throwing proxy handlers #48607

Uh oh!

Conversation

legendecas commented Jun 30, 2023

Uh oh!

nodejs-github-bot commented Jun 30, 2023

Uh oh!

mhdawson commented Jul 14, 2023

Uh oh!

Uh oh!

Uh oh!

gabrielschulhof left a comment

Choose a reason for hiding this comment

Uh oh!

github-actionsbot commented Jul 28, 2023

Uh oh!

nodejs-github-bot commented Jul 28, 2023

Uh oh!

nodejs-github-bot commented Jul 31, 2023

Uh oh!

mhdawson left a comment

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Aug 23, 2023

Uh oh!

aduh95 commented Sep 18, 2023

Uh oh!

legendecas commented Sep 18, 2023

Uh oh!

legendecas commented Sep 20, 2023

Uh oh!

nodejs-github-bot commented Oct 12, 2023

Uh oh!

nodejs-github-bot commented Oct 17, 2023

Uh oh!

nodejs-github-bot commented Oct 17, 2023

Uh oh!

legendecas commented Oct 18, 2023

Uh oh!

legendecas commented Oct 23, 2023

Uh oh!

nodejs-github-bot commented Oct 23, 2023

Uh oh!

mhdawson commented Oct 23, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants