n-api: handle reference delete before finalize#24494

mhdawson · 2018-11-19T17:03:48Z

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

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

nodejs-github-bot · 2018-11-19T17:03:49Z

@mhdawson sadly an error occured when I tried to trigger a build :(

mhdawson · 2018-11-19T17:06:02Z

@hashseed would be good to get somebody from the V8 team to comment on the issue as well since it new gc behaviour seems to be the trigger for us finding the issue.

@gabrielschulhof I know you had run into some issues that resulted in the original code to handle the Reference being deleted in the callback. Would be good to get your review as well.

addaleax · 2018-11-19T17:07:26Z

This looks like it already needs a rebase? Plus, it might be nice to have a test for this or so…

mhdawson · 2018-11-19T17:22:13Z

Rebasing as we speak...

mhdawson · 2018-11-19T17:55:53Z

Rebased. Was planning to look at a test but I likely won't get to that later in the week.

sam-github · 2018-11-19T19:58:01Z

@mhdawson commit message has deletion misspelled as "deleteion"

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

mhdawson · 2018-11-19T21:47:19Z

@sam-github, thanks fixed.

gabrielschulhof · 2018-11-22T03:00:18Z

@mhdawson from testing locally your change also fixes #23999.

mhdawson · 2018-11-22T13:46:06Z

@gabrielschulhof that might help me with the test :)

mhdawson · 2018-11-22T21:54:06Z

Ok, test added. Validated that the test fails consistently without the fix and passes with it :) @addaleax should be good to go now.

mhdawson · 2018-11-22T21:55:14Z

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

mhdawson · 2018-11-22T21:56:26Z

and thanks to @toddwong for the basis of the test. I'm surprised it was straightforward to have a test that recreates relatively easily and that runs in a short time period.

mhdawson · 2018-11-22T23:34:54Z

CI run was good, will plan to land tomorrow unless there are any additional comments/objections.

refack · 2018-11-23T00:34:13Z

@mhdawson I'm assuming you didn't mean to close this?

toddwong · 2018-11-23T14:38:53Z

@mhdawson Ahh! Pleased to know that was helping 😄 And thanks for the fix!

codebytere · 2019-01-13T18:36:19Z

@mhdawson could you potentially backport this to v10.x? I've added a label, but feel free to remove it!

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 PR-URL: nodejs#24494 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>

mhdawson · 2019-01-17T22:59:15Z

@codebytere will add to my TODO list.

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#25572 PR-URL: nodejs#24494 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>

mhdawson · 2019-01-18T21:19:10Z

@codebytere backport PR as requested: #25574

mhdawson · 2019-01-18T21:19:58Z

@codebytere thanks for reminding me on this one, it is important to get back to 10.x and I should have thought of getting it done earlier.

mhdawson · 2019-01-22T15:15:32Z

@codebytere PR for backport as requested: #25574

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 <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>

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 <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>

nodejs#24494 fixed a crash but resulted in increased stress on gc finalization. A leak was reported in nodejs#26667 which we are still investigating. As part of this investigation I realized we can optimize to reduce amount of deferred finalization. Regardless of the root cause of the leak this should be a good optimization. It also resolves the leak for the case being reported in nodejs#26667. The OP in 26667 has confirmed that he can still recreate the original problem that 24494 fixed and that the fix still works with this optimization

#24494 fixed a crash but resulted in increased stress on gc finalization. A leak was reported in #26667 which we are still investigating. As part of this investigation I realized we can optimize to reduce amount of deferred finalization. Regardless of the root cause of the leak this should be a good optimization. It also resolves the leak for the case being reported in #26667. The OP in 26667 has confirmed that he can still recreate the original problem that 24494 fixed and that the fix still works with this optimization PR-URL: #27085 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]>

nodejs-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. labels Nov 19, 2018

mhdawson requested review from gabrielschulhof and hashseed November 19, 2018 17:04

addaleax approved these changes Nov 19, 2018
View reviewed changes

mhdawson force-pushed the finalizer-order2 branch from 2d127df to e4cb28eCompare November 19, 2018 17:54

mhdawson force-pushed the finalizer-order2 branch from e4cb28e to af87a67Compare November 19, 2018 21:46

fhinkel approved these changes Nov 20, 2018
View reviewed changes

refack approved these changes Nov 22, 2018
View reviewed changes

squash: add test
455b2db

mhdawson mentioned this pull request Nov 22, 2018
Signal SIGSEGV in v8::internal::GlobalHandles::Create(v8::internal::Object*) () nodejs/node-addon-api#393
Closed

mhdawson closed this Nov 22, 2018

refack reopened this Nov 23, 2018

addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 23, 2018

codebytere added the backport-requested-v10.x label Jan 13, 2019

mhdawson mentioned this pull request Jan 18, 2019
WIP still fixing up [v10.x backport] n-api: handle reference delete before finalize #25572
Closed
3 tasks

mhdawson mentioned this pull request Jan 18, 2019
[v10.x backport] n-api: handle reference delete before finalize #25574
Closed
3 tasks

BethGriggs mentioned this pull request Feb 12, 2019
v10.15.3 proposal #26063
Merged

anthony-tuininga mentioned this pull request Mar 14, 2019
Memory leak introduced in v10.15.3 #26667
Closed

vweevers mentioned this pull request Mar 16, 2019
End iterator on GC or db close Level/leveldown#601
Closed

mhdawson mentioned this pull request Apr 4, 2019
napi: reduce gc finalization stress #27085
Closed
2 tasks

This was referenced May 3, 2019
Segfault in node 12 node-ffi-napi/weak-napi#16
Closed
src: fix ObjectWrap destruction nodejs/node-addon-api#475
Closed

mhdawson deleted the finalizer-order2 branch September 30, 2019 13:13

Uh oh!

n-api: handle reference delete before finalize#24494

n-api: handle reference delete before finalize #24494

Uh oh!

Conversation

mhdawson commented Nov 19, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

nodejs-github-bot commented Nov 19, 2018

Uh oh!

mhdawson commented Nov 19, 2018

Uh oh!

addaleax commented Nov 19, 2018

Uh oh!

mhdawson commented Nov 19, 2018

Uh oh!

mhdawson commented Nov 19, 2018

Uh oh!

sam-github commented Nov 19, 2018

Uh oh!

mhdawson commented Nov 19, 2018

Uh oh!

gabrielschulhof commented Nov 22, 2018

Uh oh!

mhdawson commented Nov 22, 2018

Uh oh!

mhdawson commented Nov 22, 2018

Uh oh!

mhdawson commented Nov 22, 2018

Uh oh!

mhdawson commented Nov 22, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mhdawson commented Nov 22, 2018

Uh oh!

refack commented Nov 23, 2018

Uh oh!

toddwong commented Nov 23, 2018

Uh oh!

codebytere commented Jan 13, 2019

Uh oh!

mhdawson commented Jan 17, 2019

Uh oh!

mhdawson commented Jan 18, 2019

Uh oh!

mhdawson commented Jan 18, 2019

Uh oh!

mhdawson commented Jan 22, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

mhdawson commented Nov 19, 2018•
edited
Loading

mhdawson commented Nov 22, 2018•
edited
Loading