Skip to content
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.

Sign up for GitHub

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

Jump to bottom

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

Closed
wants to merge 1 commit into from
Closed

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

wants to merge 1 commit into from

Conversation

legendecas
Copy link
Member

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
Copy link
Collaborator

Review requested:

  • @nodejs/gyp
  • @nodejs/node-api

@nodejs-github-bot 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 legendecas force-pushed the node-api/define-property branch 2 times, most recently from e899363 to 3b3bc46 Compare June 30, 2023 05:50
@toyobayashi toyobayashi mentioned this pull request Jun 30, 2023
Merged
@mhdawson
Copy link
Member

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.

gabrielschulhof
gabrielschulhof reviewed Jul 14, 2023
View reviewed changes
Copy link
Contributor

@gabrielschulhof gabrielschulhof left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One minor suggestion.

@legendecas legendecas added the request-ci Add this label to start a Jenkins CI on a PR. label Jul 28, 2023
@github-actions 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
@github-actions
Copy link
Contributor

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
Copy link
Collaborator

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

This was referenced Jul 29, 2023
Open
Open
Open
@legendecas 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
@nodejs-github-bot
Copy link
Collaborator

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

This was referenced Aug 1, 2023
Open
Open
Open
Open
mhdawson
mhdawson approved these changes Aug 23, 2023
View reviewed changes
Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mhdawson mhdawson added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 23, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 23, 2023
@nodejs-github-bot
Copy link
Collaborator

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

@github-actions github-actions bot mentioned this pull request Aug 24, 2023
Open
27 tasks
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 12, 2023
@nodejs-github-bot
Copy link
Collaborator

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

@legendecas 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
@aduh95
Copy link
Contributor

aduh95 commented Sep 18, 2023

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
Copy link
Member Author

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

@legendecas
Copy link
Member Author

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

@legendecas legendecas mentioned this pull request Sep 22, 2023
Closed
@legendecas
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 legendecas added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 12, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 12, 2023
@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@legendecas
Copy link
Member Author

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

@mhdawson mhdawson added dont-land-on-v18.x PRs that should not land on the v18.x-staging branch and should not be released in 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
Copy link
Member Author

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.

@legendecas legendecas added the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 23, 2023
@nodejs-github-bot 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
@nodejs-github-bot
Copy link
Collaborator

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
Copy link
Member

Landed in 52fcf14

mhdawson pushed a commit that referenced this pull request Oct 23, 2023
@legendecas @mhdawson
node-api: return napi_exception_pending on proxy handlers
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]>
@mhdawson mhdawson closed this Oct 23, 2023
@legendecas legendecas deleted the node-api/define-property branch October 24, 2023 06:55
targos pushed a commit that referenced this pull request Oct 24, 2023
@legendecas @targos
node-api: return napi_exception_pending on proxy handlers
91e373f 
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]>
@targos targos mentioned this pull request Oct 24, 2023
Merged
alexfernandez pushed a commit to alexfernandez/node that referenced this pull request Nov 1, 2023
@legendecas @alexfernandez
node-api: return napi_exception_pending on proxy handlers
8eb582b 
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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mhdawson mhdawson mhdawson approved these changes

@gabrielschulhof gabrielschulhof gabrielschulhof approved these changes

Assignees
No one assigned
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. commit-queue-failed An error occurred while landing this pull request using GitHub Actions. dont-land-on-v18.x PRs that should not land on the v18.x-staging branch and should not be released in 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. needs-ci PRs that need a full CI run. node-api Issues and PRs related to the Node-API.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@legendecas @nodejs-github-bot @mhdawson @richardlau @gabrielschulhof @aduh95