GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — node-api: fix crash in finalization by mhdawson · Pull Request #37876 · nodejs/node

Conversation

@mhdawson

@mhdawson mhdawson commented Mar 23, 2021

Copy link
Copy Markdown
Member

Refs: nodejs/node-addon-api#906
Refs: #37616

Fix crash introduced by #37616

Signed-off-by: Michael Dawson [email protected]

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@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 Mar 23, 2021
@mhdawson mhdawson mentioned this pull request Mar 23, 2021
Intermittent failure in ThreadSafe function nodejs/node-addon-api#906
Closed
@mhdawson

mhdawson commented Mar 23, 2021

Copy link
Copy Markdown
Member Author

@gabrielschulhof can you take a look to confirm you agree it makes sense.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
addaleax
addaleax approved these changes Mar 23, 2021
View reviewed changes

@addaleax addaleax left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Is it possible to add a regression test for this? This also seems like a V8 bug at first glance to me, maybe it's worth fixing this upstream (as well)?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@mhdawson

mhdawson commented Mar 23, 2021

Copy link
Copy Markdown
Member Author

@addaleax adding a regression test case would be good but it may take a bit of work to translate the node-addon-api one to an equivalent I can add to Node.js. Since this "unbreaks" the node-addon-api CI my preference would be to land this to get the CI green and then add the regression test through a follow-on PR. Sound reasonable?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@addaleax

addaleax commented Mar 23, 2021

Copy link
Copy Markdown
Member

@mhdawson Yeah, I don't consider that a blocker.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
cjihrig
cjihrig approved these changes Mar 23, 2021
View reviewed changes
@Trott Trott mentioned this pull request Mar 24, 2021
buffer: expose btoa and atob as globals #37786
Closed
Trott
Trott approved these changes Mar 24, 2021
View reviewed changes
jasnell
jasnell approved these changes Mar 24, 2021
View reviewed changes
@jasnell

jasnell commented Mar 24, 2021

Copy link
Copy Markdown
Member

If this ends up fixing the issue we've been having on FreeBSD in #37786, then I'd appreciate if we could Fast track this.
Trott reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nodejs-github-bot

nodejs-github-bot commented Mar 24, 2021

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/36952/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jasnell jasnell removed the needs-ci PRs that need a full CI run. label Mar 24, 2021
@Trott

Trott commented Mar 24, 2021

Copy link
Copy Markdown
Member

Adding a fast-track label as @jasnell proposed it and I endorse it. If anyone objects, please remove the label.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@Trott Trott added the fast-track PRs proposed for a shorter-than-standard waiting period before landing. label Mar 24, 2021
@nodejs-github-bot

nodejs-github-bot commented Mar 24, 2021

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/36953/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@mhdawson @Trott
node-api: fix crash in finalization
42dc4d1 
Refs: nodejs/node-addon-api#906
Refs: nodejs#37616

Fix crash introduced by nodejs#37616

Signed-off-by: Michael Dawson <[email protected]>

PR-URL: nodejs#37876
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@Trott
Trott force-pushed the node-api-finalization branch from c897654 to 42dc4d1 Compare March 24, 2021 23:35
@Trott
Trott merged commit 42dc4d1 into nodejs:master Mar 24, 2021
@Trott

Trott commented Mar 24, 2021

Copy link
Copy Markdown
Member

Landed in 42dc4d1
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@BethGriggs BethGriggs added the lts-watch-v14.x label Mar 25, 2021
@mhdawson mhdawson mentioned this pull request Mar 25, 2021
[v14.x] Backport environment teardown Node-API reference double free fixes #37802
Closed
ruyadorno pushed a commit that referenced this pull request Mar 29, 2021
@mhdawson @ruyadorno
node-api: fix crash in finalization
906eb53 
Refs: nodejs/node-addon-api#906
Refs: #37616

Fix crash introduced by #37616

Signed-off-by: Michael Dawson <[email protected]>

PR-URL: #37876
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
ruyadorno pushed a commit that referenced this pull request Mar 30, 2021
@mhdawson @ruyadorno
node-api: fix crash in finalization
3ab223d 
Refs: nodejs/node-addon-api#906
Refs: #37616

Fix crash introduced by #37616

Signed-off-by: Michael Dawson <[email protected]>

PR-URL: #37876
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@ruyadorno ruyadorno mentioned this pull request Mar 30, 2021
v15.13.0 proposal #37977
Merged
targos pushed a commit to gabrielschulhof/node that referenced this pull request Apr 24, 2021
@mhdawson @targos
node-api: fix crash in finalization
180c33d 
Refs: nodejs/node-addon-api#906
Refs: nodejs#37616

Fix crash introduced by nodejs#37616

Signed-off-by: Michael Dawson <[email protected]>

PR-URL: nodejs#37876
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@targos targos removed the lts-watch-v14.x label Apr 25, 2021
@targos targos added backport-open-v14.x and removed backport-open-v14.x fast-track PRs proposed for a shorter-than-standard waiting period before landing. labels Apr 25, 2021
targos pushed a commit that referenced this pull request Apr 26, 2021
@mhdawson @targos
node-api: fix crash in finalization
8413759 
Refs: nodejs/node-addon-api#906
Refs: #37616

Fix crash introduced by #37616

Signed-off-by: Michael Dawson <[email protected]>

PR-URL: #37876
Backport-PR-URL: #37802
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@danielleadams danielleadams mentioned this pull request May 3, 2021
v14.17.0 proposal #38507
Merged
@legendecas legendecas added the node-api Issues and PRs related to Node-API. label Mar 27, 2022
@legendecas legendecas mentioned this pull request Mar 27, 2022
Node.js 12.x need to cherry-pick https://github.com/nodejs/node/pull/37616 #42376
Closed
legendecas pushed a commit to legendecas/node that referenced this pull request Mar 29, 2022
@mhdawson @legendecas
node-api: fix crash in finalization
affb8b2 
Refs: nodejs/node-addon-api#906
Refs: nodejs#37616

Fix crash introduced by nodejs#37616

Signed-off-by: Michael Dawson <[email protected]>

PR-URL: nodejs#37876
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
richardlau pushed a commit that referenced this pull request Mar 30, 2022
@mhdawson @richardlau
node-api: fix crash in finalization
2aa9ca1 
Refs: nodejs/node-addon-api#906
Refs: #37616

Fix crash introduced by #37616

Signed-off-by: Michael Dawson <[email protected]>

PR-URL: #37876
Backport-PR-URL: #42512
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@richardlau richardlau mentioned this pull request Mar 30, 2022
v12.22.12 proposal #42531
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnell jasnell jasnell approved these changes
@Trott Trott Trott approved these changes
@addaleax addaleax addaleax approved these changes
@cjihrig cjihrig cjihrig approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. node-api Issues and PRs related to Node-API.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

9 participants

@mhdawson @addaleax @jasnell @nodejs-github-bot @Trott @cjihrig @targos @BethGriggs @legendecas
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.