node-api,test: fix test_reference_double_free crash

[vmoroz]

The issue

The js-native-api\test_reference_double_free\test_wrap.js test always crashes when I use Visual Studio 2022.
The cause of the crash is that the deleteImmediately function callback is returning an invalid napi_value.
It happens because the DeleteImmediately function that implements the deleteImmediately callback returns void instead of napi_value. It causes it to return a random value kept in RAX register - in my case it was 0x40.

The fix

The return type of the DeleteImmediately function is changed to napi_value.
The related changes:

• Changed use of macro NODE_API_CALL_RETURN_VOID to NODE_API_CALL because we changed the return type.
• Added NODE_API_ASSERT to verify argument type - previously we retrieved the type, but never used it.
• Return NULL from the function.

Notes

I did a quick search for void.*napi_callback_info in other tests to see if we have any other cases like this, but it found nothing.
It seems that it is the only place where we have this issue.

[legendecas]

👍

[nodejs-github-bot]

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

[legendecas]

@nodejs/node-api please take a look, thank you

[mhdawson]

LGTM

[nodejs-github-bot]

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

[nodejs-github-bot]

Commit Queue failed

- Loading data for nodejs/node/pull/44927
✔  Done loading data for nodejs/node/pull/44927
----------------------------------- PR info ------------------------------------
Title      node-api,test: fix test_reference_double_free crash (#44927)
   ⚠  Could not retrieve the email or name of the PR author's from user's GitHub profile!
Branch     vmoroz:PR/Fix_test_reference_double_free -> nodejs:main
Labels     test, node-api, needs-ci
Commits    2
 - node-api,test: fix test_reference_double_free crash
 - return NULL instead of undefined
Committers 1
 - Vladimir Morozov 
PR-URL: https://github.com/nodejs/node/pull/44927
Reviewed-By: Chengzhong Wu 
Reviewed-By: Michael Dawson 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/44927
Reviewed-By: Chengzhong Wu 
Reviewed-By: Michael Dawson 
--------------------------------------------------------------------------------
   ℹ  This PR was created on Sat, 08 Oct 2022 20:39:41 GMT
   ✔  Approvals: 2
   ✔  - Chengzhong Wu (@legendecas) (TSC): https://github.com/nodejs/node/pull/44927#pullrequestreview-1135308312
   ✔  - Michael Dawson (@mhdawson) (TSC): https://github.com/nodejs/node/pull/44927#pullrequestreview-1142647651
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2022-10-14T15:54:17Z: https://ci.nodejs.org/job/node-test-pull-request/47273/
- Querying data for job/node-test-pull-request/47273/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  No git cherry-pick in progress
   ✔  No git am in progress
   ✔  No git rebase in progress
--------------------------------------------------------------------------------
- Bringing origin/main up to date...
From https://github.com/nodejs/node
 * branch                  main       -> FETCH_HEAD
✔  origin/main is now up-to-date
- Downloading patch for 44927
From https://github.com/nodejs/node
 * branch                  refs/pull/44927/merge -> FETCH_HEAD
✔  Fetched commits as 1a8f8ba6c6ca..ba3a1e5b291f
--------------------------------------------------------------------------------
[main db8a6f99c7] node-api,test: fix test_reference_double_free crash
 Author: Vladimir Morozov 
 Date: Sat Oct 8 13:17:01 2022 -0700
 1 file changed, 12 insertions(+), 9 deletions(-)
[main 2ed8da8b5d] return NULL instead of undefined
 Author: Vladimir Morozov 
 Date: Sat Oct 8 21:37:30 2022 -0700
 1 file changed, 1 insertion(+), 3 deletions(-)
   ✔  Patches applied
There are 2 commits in the PR. Attempting autorebase.
Rebasing (2/4)
Executing: git node land --amend --yes

--------------------------------- New Message ----------------------------------

node-api,test: fix test_reference_double_free crash
PR-URL: #44927

Reviewed-By: Chengzhong Wu legendecas@gmail.com

Reviewed-By: Michael Dawson midawson@redhat.com
[detached HEAD b786d0193c] node-api,test: fix test_reference_double_free crash

Author: Vladimir Morozov vmoroz@users.noreply.github.com

Date: Sat Oct 8 13:17:01 2022 -0700

1 file changed, 12 insertions(+), 9 deletions(-)

Rebasing (3/4)

Rebasing (4/4)
Executing: git node land --amend --yes

--------------------------------- New Message ----------------------------------

return NULL instead of undefined
PR-URL: #44927

Reviewed-By: Chengzhong Wu legendecas@gmail.com

Reviewed-By: Michael Dawson midawson@redhat.com
[detached HEAD 50f50832bb] return NULL instead of undefined

Author: Vladimir Morozov vmoroz@users.noreply.github.com

Date: Sat Oct 8 21:37:30 2022 -0700

1 file changed, 1 insertion(+), 3 deletions(-)
Successfully rebased and updated refs/heads/main.
ℹ  Add commit-queue-squash label to land the PR as one commit, or commit-queue-rebase to land as separate commits.

https://github.com/nodejs/node/actions/runs/3272013058

[vmoroz]

Commit Queue failed

I had changed my profile to make the primary e-mail public. I hope it should address the issue.

[legendecas]

Landed in 5b316fe. Thank you for the work on this!

[vmoroz]

Landed in 5b316fe. Thank you for the work on this!

Thank you for landing it!