node-api: force env shutdown deferring behavior by gabrielschulhof · Pull Request #37303 · nodejs/node · GitHub

The finalizer normally never gets called while a reference is strong.
However, during environment shutdown all finalizers must get called. In
order to unify the deferring behavior with that of a regular
finalization, we must force the reference to be weak when we call its
finalizer during environment shutdown.

Fixes: #37236
Co-authored-by: @legendecas

I think we should add this in the commit message too:

Fixes: https://github.com/nodejs/node/issues/36868

@RaisinTen I'm not 100% sure that the crash is the same. In test_worker_terminate_finalization the reference is weakened before it is deleted, whereas the problem fixed here is the self-deletion during environment teardown of a strong reference.

test_worker_terminate_finalization originally caused a leak which was fixed in c822ba7. Now, if it crashes intermittently, this PR may fix it, but I think we should keep #36868 open to remind ourselves to check on its flakiness.

@RaisinTen it sounds like @mhdawson tried to reproduce #36868 and was unable to do so. Therefore I think we should close #36868 unless the test in question remains flaky.

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

LGTM

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

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

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

CI: https://ci.nodejs.org/job/node-test-pull-request/36091/ ⚠️

@gabrielschulhof I think @legendecas may be away for a few days and I'm thinking it might still be good to wait for him to have a look as well. What do you think?

@mhdawson OK.

@gabrielschulhof PR wise looks good to me. However, I may have been mistaken on the test case of #37236 (comment). I've tried the PR on my internal case and it does crash regardlessly.

@legendecas if it still crashes, can you create a test case we can use here? You can even push it to my branch.

The finalizer normally never gets called while a reference is strong. However, during environment shutdown all finalizers must get called. In order to unify the deferring behavior with that of a regular finalization, we must force the reference to be weak when we call its finalizer during environment shutdown. Fixes: nodejs#37236 Co-authored-by: Chengzhong Wu <legendecas@gmail.com>

Rebased.

@legendecas P.S.: your original repro (https://github.com/legendecas/repro-napi-v8impl-refbase-double-free) no longer crashes with this PR.

@gabrielschulhof it's true. This PR does fix a crash case. I'll try to dig out the stable re-production on the one with weak references.

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

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

The finalizer normally never gets called while a reference is strong. However, during environment shutdown all finalizers must get called. In order to unify the deferring behavior with that of a regular finalization, we must force the reference to be weak when we call its finalizer during environment shutdown. Fixes: #37236 Co-authored-by: Chengzhong Wu <legendecas@gmail.com> PR-URL: #37303 Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Michael Dawson <midawson@redhat.com>

Landed in 03806a0.

The finalizer normally never gets called while a reference is strong. However, during environment shutdown all finalizers must get called. In order to unify the deferring behavior with that of a regular finalization, we must force the reference to be weak when we call its finalizer during environment shutdown. Fixes: #37236 Co-authored-by: Chengzhong Wu <legendecas@gmail.com> PR-URL: #37303 Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Michael Dawson <midawson@redhat.com>

The finalizer normally never gets called while a reference is strong. However, during environment shutdown all finalizers must get called. In order to unify the deferring behavior with that of a regular finalization, we must force the reference to be weak when we call its finalizer during environment shutdown. Fixes: nodejs#37236 Co-authored-by: Chengzhong Wu <legendecas@gmail.com> PR-URL: nodejs#37303 Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Michael Dawson <midawson@redhat.com>

The finalizer normally never gets called while a reference is strong. However, during environment shutdown all finalizers must get called. In order to unify the deferring behavior with that of a regular finalization, we must force the reference to be weak when we call its finalizer during environment shutdown. Fixes: #37236 Co-authored-by: Chengzhong Wu <legendecas@gmail.com> PR-URL: #37303 Backport-PR-URL: #37802 Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Michael Dawson <midawson@redhat.com>

The finalizer normally never gets called while a reference is strong. However, during environment shutdown all finalizers must get called. In order to unify the deferring behavior with that of a regular finalization, we must force the reference to be weak when we call its finalizer during environment shutdown. Fixes: nodejs#37236 Co-authored-by: Chengzhong Wu <legendecas@gmail.com> PR-URL: nodejs#37303 Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Michael Dawson <midawson@redhat.com>

The finalizer normally never gets called while a reference is strong. However, during environment shutdown all finalizers must get called. In order to unify the deferring behavior with that of a regular finalization, we must force the reference to be weak when we call its finalizer during environment shutdown. Fixes: #37236 Co-authored-by: Chengzhong Wu <legendecas@gmail.com> PR-URL: #37303 Backport-PR-URL: #42512 Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Michael Dawson <midawson@redhat.com>

gabrielschulhof added the node-api Issues and PRs related to the Node-API. label Feb 10, 2021

gabrielschulhof requested review from legendecas and mhdawson February 10, 2021 07:28

nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Feb 10, 2021

gabrielschulhof force-pushed the node-api-double-free-ref2 branch from 53f6c32 to cd3e2d1 Compare February 10, 2021 07:30

RaisinTen mentioned this pull request Feb 10, 2021

investigate flaky test/node-api/test_worker_terminate_finalization/test.js #36868

Closed

gabrielschulhof requested a review from RaisinTen February 10, 2021 16:32

mhdawson reviewed Feb 10, 2021

View reviewed changes

Comment thread src/js_native_api_v8.cc Outdated

gabrielschulhof commented Feb 11, 2021

View reviewed changes

Comment thread src/js_native_api_v8.cc Outdated

mhdawson approved these changes Feb 12, 2021

View reviewed changes

legendecas mentioned this pull request Feb 18, 2021

Crash on node api add-on finalization #37236

Closed

Gabriel Schulhof and others added 5 commits February 18, 2021 07:53

add test

167bb7f

heed the linter

9f723be

handle the case where the reference is not deleted in its destructor

df5463b

put the _finalize_ran = true back in the else body

21e5b1d

gabrielschulhof force-pushed the node-api-double-free-ref2 branch from 6415c99 to 21e5b1d Compare February 18, 2021 15:54

legendecas approved these changes Feb 18, 2021

View reviewed changes

gabrielschulhof closed this Feb 19, 2021

gabrielschulhof deleted the node-api-double-free-ref2 branch February 19, 2021 04:33

gabrielschulhof mentioned this pull request Feb 19, 2021

Handle the case where one cannot execute JavaScript #37446

Closed

targos mentioned this pull request Mar 2, 2021

v15.11.0 release proposal #37569

Merged

targos added backport-open-v14.x and removed backport-open-v14.x labels Apr 25, 2021

danielleadams mentioned this pull request May 3, 2021

v14.17.0 proposal #38507

Merged

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

richardlau mentioned this pull request Mar 30, 2022

v12.22.12 proposal #42531

Merged

node-api: force env shutdown deferring behavior by gabrielschulhof · Pull Request #37303 · nodejs/node · GitHub

Uh oh!

Uh oh!

node-api: force env shutdown deferring behavior - #37303

node-api: force env shutdown deferring behavior#37303gabrielschulhof wants to merge 5 commits intonodejs:masternodejs/node:masterfrom gabrielschulhof:node-api-double-free-ref2gabrielschulhof/node:node-api-double-free-ref2Copy head branch name to clipboard

Conversation

gabrielschulhof commented Feb 10, 2021

Uh oh!

RaisinTen commented Feb 10, 2021

Uh oh!

gabrielschulhof commented Feb 10, 2021

Uh oh!

gabrielschulhof commented Feb 10, 2021

Uh oh!

Uh oh!

Uh oh!

nodejs-github-bot commented Feb 11, 2021

Uh oh!

mhdawson left a comment

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Feb 12, 2021

Uh oh!

nodejs-github-bot commented Feb 12, 2021

Uh oh!

nodejs-github-bot commented Feb 13, 2021

Uh oh!

nodejs-github-bot commented Feb 13, 2021 • edited by gabrielschulhof Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mhdawson commented Feb 16, 2021

Uh oh!

gabrielschulhof commented Feb 17, 2021

Uh oh!

legendecas commented Feb 18, 2021

Uh oh!

gabrielschulhof commented Feb 18, 2021

Uh oh!

gabrielschulhof commented Feb 18, 2021

Uh oh!

gabrielschulhof commented Feb 18, 2021

Uh oh!

legendecas left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Feb 18, 2021

Uh oh!

nodejs-github-bot commented Feb 18, 2021

Uh oh!

gabrielschulhof commented Feb 19, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

node-api: force env shutdown deferring behavior#37303
gabrielschulhof wants to merge 5 commits into
nodejs:masterfrom
gabrielschulhof:node-api-double-free-ref2

nodejs-github-bot commented Feb 13, 2021 •
edited by gabrielschulhof
Loading

legendecas left a comment •
edited
Loading