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: #19673 Backport-PR-URL: #19447 PR-URL: #19718 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
nodejs · Apr 16, 2018 · abd9fd6 · abd9fd6
1 parent e6ccdfb
commit abd9fd6
Show file tree

Hide file tree

Showing 4 changed files with 33 additions and 6 deletions.
diff --git a/src/node_api.cc b/src/node_api.cc
@@ -481,9 +481,10 @@ class Reference : private Finalizer {
     // Check before calling the finalize callback, because the callback might
     // delete it.
     bool delete_self = reference->_delete_self;
+    napi_env env = reference->_env;
 
     if (reference->_finalize_callback != nullptr) {
-      NAPI_CALL_INTO_MODULE_THROW(reference->_env,
+      NAPI_CALL_INTO_MODULE_THROW(env,
         reference->_finalize_callback(
             reference->_env,
             reference->_finalize_data,

diff --git 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
@@ -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
@@ -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);