Skip to content

src: fix finalization crash #38250

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
Closed
wants to merge 1 commit into from
Closed

src: fix finalization crash #38250

wants to merge 1 commit into from

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented Apr 15, 2021 •
edited
Loading

Fixes: #38040

@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. labels Apr 15, 2021
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Apr 15, 2021 •
edited by jasnell
Loading

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

@jasnell jasnell mentioned this pull request Apr 15, 2021
Closed
@jasnell jasnell added fast-track PRs that do not need to wait for 48 hours to land. and removed needs-ci PRs that need a full CI run. labels Apr 15, 2021
@jasnell
Copy link
Member Author

jasnell commented Apr 15, 2021

If this does fix #38040 and CI is good, would be good to fast-track. Please 👍🏻

@jasnell jasnell changed the title src: maybe fix finalization crash src: fix finalization crash Apr 15, 2021
jasnell added a commit that referenced this pull request Apr 15, 2021
@jasnell
src: fix finalization crash
fc20e83 
PR-URL: #38250
Fixes: #38040
Reviewed-By: Beth Griggs <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
@jasnell
Copy link
Member Author

jasnell commented Apr 15, 2021

Landed in fc20e83

@jasnell jasnell closed this Apr 15, 2021
@BethGriggs BethGriggs mentioned this pull request Apr 16, 2021
Merged
@legendecas legendecas added the node-api Issues and PRs related to the Node-API. label Apr 16, 2021
@legendecas
Copy link
Member

@jasnell 👍 for quick fixing on the issue. Unfortunately, this patch doesn't ultimately fix the use-after-free issue on node-api finalization. #38000 is still required to fix the use-after-free on v8impl::Reference.

@legendecas legendecas mentioned this pull request Apr 21, 2021
Closed
targos pushed a commit that referenced this pull request May 1, 2021
@jasnell @targos
src: fix finalization crash
ec4be2d 
PR-URL: #38250
Fixes: #38040
Reviewed-By: Beth Griggs <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
@danielleadams danielleadams mentioned this pull request May 3, 2021
Merged
danielleadams pushed a commit that referenced this pull request May 8, 2021
@jasnell @danielleadams
src: fix finalization crash
27ebcb7 
PR-URL: #38250
Fixes: #38040
Reviewed-By: Beth Griggs <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
@legendecas legendecas mentioned this pull request Mar 29, 2022
Closed
legendecas pushed a commit to legendecas/node that referenced this pull request Mar 29, 2022
@jasnell @legendecas
src: fix finalization crash
2a8bdd1 
PR-URL: nodejs#38250
Fixes: nodejs#38040
Reviewed-By: Beth Griggs <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
richardlau pushed a commit that referenced this pull request Mar 30, 2022
@jasnell @richardlau
src: fix finalization crash
e707514 
PR-URL: #38250
Backport-PR-URL: #42512
Fixes: #38040
Reviewed-By: Beth Griggs <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
@richardlau richardlau mentioned this pull request Mar 30, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BethGriggs BethGriggs BethGriggs approved these changes

@aduh95 aduh95 aduh95 approved these changes

@targos targos Awaiting requested review from targos

@richardlau richardlau Awaiting requested review from richardlau

Assignees
No one assigned
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. fast-track PRs that do not need to wait for 48 hours to land. 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.

local fatal error for test/js-native-api/test_object/test.js
5 participants
@jasnell @nodejs-github-bot @legendecas @BethGriggs @aduh95