n-api: back up env before finalize

Heed the comment to not use fields of a Reference after calling its
finalize callback, because such a call may destroy the Reference.

Fixes: https://github.com/nodejs/node/issues/19673
PR-URL: https://github.com/nodejs/node/pull/19718
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

3 files changed, 31 insertions, 5 deletions

diff --git a/test/addons-napi/8_passing_wrapped/binding.cc b/test/addons-napi/8_passing_wrapped/binding.cc
index c284c85f9b4..48e94f10ec4 100644
--- a/test/addons-napi/8_passing_wrapped/binding.cc
+++ b/test/addons-napi/8_passing_wrapped/binding.cc
@@ -1,7 +1,9 @@
 #include "myobject.h"
 #include "../common.h"
 
-napi_value CreateObject(napi_env env, napi_callback_info info) {
+extern size_t finalize_count;
+
+static napi_value CreateObject(napi_env env, napi_callback_info info) {
   size_t argc = 1;
   napi_value args[1];
   NAPI_CALL(env, napi_get_cb_info(env, info, &argc, args, nullptr, nullptr));
@@ -12,7 +14,7 @@ napi_value CreateObject(napi_env env, napi_callback_info info) {
   return instance;
 }
 
-napi_value Add(napi_env env, napi_callback_info info) {
+static napi_value Add(napi_env env, napi_callback_info info) {
   size_t argc = 2;
   napi_value args[2];
   NAPI_CALL(env, napi_get_cb_info(env, info, &argc, args, nullptr, nullptr));
@@ -29,12 +31,19 @@ napi_value Add(napi_env env, napi_callback_info info) {
   return sum;
 }
 
-napi_value Init(napi_env env, napi_value exports) {
+static napi_value FinalizeCount(napi_env env, napi_callback_info info) {
+  napi_value return_value;
+  NAPI_CALL(env, napi_create_uint32(env, finalize_count, &return_value));
+  return return_value;
+}
+
+static napi_value Init(napi_env env, napi_value exports) {
   MyObject::Init(env);
 
   napi_property_descriptor desc[] = {
     DECLARE_NAPI_PROPERTY("createObject", CreateObject),
     DECLARE_NAPI_PROPERTY("add", Add),
+    DECLARE_NAPI_PROPERTY("finalizeCount", FinalizeCount),
   };
 
   NAPI_CALL(env,
diff --git a/test/addons-napi/8_passing_wrapped/myobject.cc b/test/addons-napi/8_passing_wrapped/myobject.cc
index 19cc7dd2a29..0c9ca90f52f 100644
--- a/test/addons-napi/8_passing_wrapped/myobject.cc
+++ b/test/addons-napi/8_passing_wrapped/myobject.cc
@@ -1,9 +1,14 @@
 #include "myobject.h"
 #include "../common.h"
 
+size_t finalize_count = 0;
+
 MyObject::MyObject() : env_(nullptr), wrapper_(nullptr) {}
 
-MyObject::~MyObject() { napi_delete_reference(env_, wrapper_); }
+MyObject::~MyObject() {
+  finalize_count++;
+  napi_delete_reference(env_, wrapper_);
+}
 
 void MyObject::Destructor(
   napi_env env, void* nativeObject, void* /*finalize_hint*/) {
@@ -45,6 +50,11 @@ napi_value MyObject::New(napi_env env, napi_callback_info info) {
   }
 
   obj->env_ = env;
+
+  // It is important that the below call to napi_wrap() be such that we request
+  // a reference to the wrapped object via the out-parameter, because this
+  // ensures that we test the code path that deals with a reference that is
+  // destroyed from its own finalizer.
   NAPI_CALL(env, napi_wrap(env,
                           _this,
                           obj,
diff --git a/test/addons-napi/8_passing_wrapped/test.js b/test/addons-napi/8_passing_wrapped/test.js
index 3d24fa5d9fd..7793133f775 100644
--- a/test/addons-napi/8_passing_wrapped/test.js
+++ b/test/addons-napi/8_passing_wrapped/test.js
@@ -1,9 +1,16 @@
 'use strict';
+// Flags: --expose-gc
+
 const common = require('../../common');
 const assert = require('assert');
 const addon = require(`./build/${common.buildType}/binding`);
 
-const obj1 = addon.createObject(10);
+let obj1 = addon.createObject(10);
 const obj2 = addon.createObject(20);
 const result = addon.add(obj1, obj2);
 assert.strictEqual(result, 30);
+
+// Make sure the native destructor gets called.
+obj1 = null;
+global.gc();
+assert.strictEqual(addon.finalizeCount(), 1);