Skip to content

[v10.x backport] n-api: handle reference delete before finalize - #25574

Closed
mhdawson wants to merge 1 commit into
nodejs:v10.x-stagingfrom
mhdawson:backport-24494-to-v10.x
Closed

[v10.x backport] n-api: handle reference delete before finalize#25574
mhdawson wants to merge 1 commit into
nodejs:v10.x-stagingfrom
mhdawson:backport-24494-to-v10.x

Conversation

@mhdawson

@mhdawsonmhdawson commented Jan 18, 2019

Copy link
Copy Markdown
Member

Backport of #24494

Crashes were reported during finalization due to
the memory for a reference being deleted and the
finalizer running after the deletion.

This change ensures the deletion of the memory for
the reference only occurs after the finalizer has run.

Fixes: nodejs/node-addon-api#393

Backport-PR-URL: #25574
PR-URL: #24494
Reviewed-By: Anna Henningsen anna@addaleax.net
Reviewed-By: Franziska Hinkelmann franziska.hinkelmann@gmail.com
Reviewed-By: Refael Ackermann refack@gmail.com

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@nodejs-github-botnodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. node-api Issues and PRs related to the Node-API. v10.x labels Jan 18, 2019
@mhdawsonmhdawson changed the title n-api: handle reference delete before finalize[v10.x backport] n-api: handle reference delete before finalizeJan 18, 2019
@mhdawson
mhdawsonforce-pushed the backport-24494-to-v10.x branch from 79bd432 to ffb04fcCompareJanuary 18, 2019 21:15
@mhdawson

Copy link
Copy Markdown
MemberAuthor

@mhdawson

Copy link
Copy Markdown
MemberAuthor

Resume build as I doubt the windows failures are related to this PR: https://ci.nodejs.org/job/node-test-pull-request/20204/

@mhdawson

Copy link
Copy Markdown
MemberAuthor

Failure in earlier CI does look like this known issue: #23277

@mhdawson

Copy link
Copy Markdown
MemberAuthor

One more resume attempt, although the freebsd one is a compile failure in an unrelated file?

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

@mhdawson

Copy link
Copy Markdown
MemberAuthor

I'm guessing maybe something landed earlier in staging that is causing the failure? Trying a CI run against 10.x branch instead to see if the same failure happens there or not.

https://ci.nodejs.org/job/node-test-pull-request/20237/ (still to start at time of posting)

@mhdawson

Copy link
Copy Markdown
MemberAuthor

Run on FreeBSD of v10.x-staging - https://ci.nodejs.org/job/node-test-commit-freebsd/23523/. If it fails will confirm that its something already on the branch.

@mhdawson

Copy link
Copy Markdown
MemberAuthor

@codebytere@BethGriggs looks to me like v10.x-staging, freebsd10-64 fails without my PR ( I think that is also the case for v10.x as well based on the CI ran on this PR rebased onto v10.x). Is this a known issue?

I think this PR is good to go as that failure is pre-existing and there are no other failures in the CI run for the PR.

@mhdawson

Copy link
Copy Markdown
MemberAuthor

@codebytere, @BethGriggs seems like the problem is this commit: acf7e7d from the discusion here: nodejs/abi-stable-node#358. Sounds like this is being seen by others as well and blocking backports.

@codebytere
codebytereforce-pushed the v10.x-staging branch 3 times, most recently from 2d6e145 to 7840f71CompareJanuary 29, 2019 18:12
@mhdawson

Copy link
Copy Markdown
MemberAuthor

New CI as I think I saw another issue that might have been blocked by the same problem get a green CI:https://ci.nodejs.org/job/node-test-pull-request/20452/

@mhdawson

Copy link
Copy Markdown
MemberAuthor

See I need a rebase , doing that now.

Crashes were reported during finalization due to
the memory for a reference being deleted and the
finalizer running after the deletion.
This change ensures the deletion of the memory for
the reference only occurs after the finalizer has run.
Fixes: nodejs/node-addon-api#393
Backport-PR-URL: nodejs#25574
PR-URL: nodejs#24494
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
@mhdawson
mhdawsonforce-pushed the backport-24494-to-v10.x branch from ffb04fc to af63c6eCompareJanuary 30, 2019 20:17
@mhdawson

Copy link
Copy Markdown
MemberAuthor

@mhdawson

Copy link
Copy Markdown
MemberAuthor

@mhdawson

Copy link
Copy Markdown
MemberAuthor

@mhdawson

Copy link
Copy Markdown
MemberAuthor

Latest CI is good. @codebytere this is good to go.

BethGriggs pushed a commit that referenced this pull request Feb 5, 2019
Crashes were reported during finalization due to
the memory for a reference being deleted and the
finalizer running after the deletion.
This change ensures the deletion of the memory for
the reference only occurs after the finalizer has run.
Fixes: nodejs/node-addon-api#393
Backport-PR-URL: #25574
PR-URL: #24494
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
@BethGriggs

Copy link
Copy Markdown
Member

Landed on v10.x-staging in fe0e119

rvagg pushed a commit that referenced this pull request Feb 28, 2019
Crashes were reported during finalization due to
the memory for a reference being deleted and the
finalizer running after the deletion.
This change ensures the deletion of the memory for
the reference only occurs after the finalizer has run.
Fixes: nodejs/node-addon-api#393
Backport-PR-URL: #25574
PR-URL: #24494
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
@mhdawson
mhdawson deleted the backport-24494-to-v10.x branch September 30, 2019 13:13
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@mhdawson@nodejs-github-bot@BethGriggs