node-api: return napi_exception_pending on throwing proxy handlers by legendecas · Pull Request #48607 · nodejs/node · GitHub

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)

Review requested:

@nodejs/gyp
@nodejs/node-api

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.

One minor suggestion.

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

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

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

LGTM

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

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]
  ...

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

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.

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

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

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

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

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.

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

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 <gabrielschulhof@gmail.com> Reviewed-By: Michael Dawson <midawson@redhat.com>

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 3b3bc46 Compare 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

Comment thread test/js-native-api/test_object/test_exceptions.c Outdated

Comment thread test/js-native-api/test_object/test_exceptions.c Outdated

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-actions Bot 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-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 23, 2023

github-actions Bot 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 a4f2dbf Compare 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-actions Bot 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 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

node-api: return napi_exception_pending on throwing proxy handlers by legendecas · Pull Request #48607 · nodejs/node · GitHub

Uh oh!

Uh oh!

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

node-api: return napi_exception_pending on throwing proxy handlers#48607legendecas wants to merge 1 commit intonodejs:mainnodejs/node:mainfrom legendecas:node-api/define-propertylegendecas/node:node-api/define-propertyCopy head branch name to clipboard

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-actions Bot 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

node-api: return napi_exception_pending on throwing proxy handlers#48607
legendecas wants to merge 1 commit into
nodejs:mainfrom
legendecas:node-api/define-property