Commit a0a5873
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
PR-URL: #19718
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>1 parent c1b83fc commit a0a5873
4 files changed
Lines changed: 33 additions & 6 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
@@ -467,9 +467,10 @@ class Reference : private Finalizer { | |||
| 467 | 467 | // Check before calling the finalize callback, because the callback might | |
| 468 | 468 | // delete it. | |
| 469 | 469 | bool delete_self = reference->_delete_self; | |
| 470 | + napi_env env = reference->_env; | ||
| 470 | 471 | ||
| 471 | 472 | if (reference->_finalize_callback != nullptr) { | |
| 472 | - NAPI_CALL_INTO_MODULE_THROW(reference->_env, | ||
| 473 | + NAPI_CALL_INTO_MODULE_THROW(env, | ||
| 473 | 474 | reference->_finalize_callback( | |
| 474 | 475 | reference->_env, | |
| 475 | 476 | reference->_finalize_data, | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
@@ -1,7 +1,9 @@ | |||
| 1 | 1 | #include "myobject.h" | |
| 2 | 2 | #include "../common.h" | |
| 3 | 3 | ||
| 4 | - napi_value CreateObject(napi_env env, napi_callback_info info) { | ||
| 4 | + extern size_t finalize_count; | ||
| 5 | + | ||
| 6 | + static napi_value CreateObject(napi_env env, napi_callback_info info) { | ||
| 5 | 7 | size_t argc = 1; | |
| 6 | 8 | napi_value args[1]; | |
| 7 | 9 | 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) { | |||
| 12 | 14 | return instance; | |
| 13 | 15 | } | |
| 14 | 16 | ||
| 15 | - napi_value Add(napi_env env, napi_callback_info info) { | ||
| 17 | + static napi_value Add(napi_env env, napi_callback_info info) { | ||
| 16 | 18 | size_t argc = 2; | |
| 17 | 19 | napi_value args[2]; | |
| 18 | 20 | 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) { | |||
| 29 | 31 | return sum; | |
| 30 | 32 | } | |
| 31 | 33 | ||
| 32 | - napi_value Init(napi_env env, napi_value exports) { | ||
| 34 | + static napi_value FinalizeCount(napi_env env, napi_callback_info info) { | ||
| 35 | + napi_value return_value; | ||
| 36 | + NAPI_CALL(env, napi_create_uint32(env, finalize_count, &return_value)); | ||
| 37 | + return return_value; | ||
| 38 | + } | ||
| 39 | + | ||
| 40 | + static napi_value Init(napi_env env, napi_value exports) { | ||
| 33 | 41 | MyObject::Init(env); | |
| 34 | 42 | ||
| 35 | 43 | napi_property_descriptor desc[] = { | |
| 36 | 44 | DECLARE_NAPI_PROPERTY("createObject", CreateObject), | |
| 37 | 45 | DECLARE_NAPI_PROPERTY("add", Add), | |
| 46 | + DECLARE_NAPI_PROPERTY("finalizeCount", FinalizeCount), | ||
| 38 | 47 | }; | |
| 39 | 48 | ||
| 40 | 49 | NAPI_CALL(env, | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
@@ -1,9 +1,14 @@ | |||
| 1 | 1 | #include "myobject.h" | |
| 2 | 2 | #include "../common.h" | |
| 3 | 3 | ||
| 4 | + size_t finalize_count = 0; | ||
| 5 | + | ||
| 4 | 6 | MyObject::MyObject() : env_(nullptr), wrapper_(nullptr) {} | |
| 5 | 7 | ||
| 6 | - MyObject::~MyObject() { napi_delete_reference(env_, wrapper_); } | ||
| 8 | + MyObject::~MyObject() { | ||
| 9 | + finalize_count++; | ||
| 10 | + napi_delete_reference(env_, wrapper_); | ||
| 11 | + } | ||
| 7 | 12 | ||
| 8 | 13 | void MyObject::Destructor( | |
| 9 | 14 | napi_env env, void* nativeObject, void* /*finalize_hint*/) { | |
@@ -45,6 +50,11 @@ napi_value MyObject::New(napi_env env, napi_callback_info info) { | |||
| 45 | 50 | } | |
| 46 | 51 | ||
| 47 | 52 | obj->env_ = env; | |
| 53 | + | ||
| 54 | + // It is important that the below call to napi_wrap() be such that we request | ||
| 55 | + // a reference to the wrapped object via the out-parameter, because this | ||
| 56 | + // ensures that we test the code path that deals with a reference that is | ||
| 57 | + // destroyed from its own finalizer. | ||
| 48 | 58 | NAPI_CALL(env, napi_wrap(env, | |
| 49 | 59 | _this, | |
| 50 | 60 | obj, | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
@@ -1,9 +1,16 @@ | |||
| 1 | 1 | 'use strict'; | |
| 2 | + // Flags: --expose-gc | ||
| 3 | + | ||
| 2 | 4 | const common = require('../../common'); | |
| 3 | 5 | const assert = require('assert'); | |
| 4 | 6 | const addon = require(`./build/${common.buildType}/binding`); | |
| 5 | 7 | ||
| 6 | - const obj1 = addon.createObject(10); | ||
| 8 | + let obj1 = addon.createObject(10); | ||
| 7 | 9 | const obj2 = addon.createObject(20); | |
| 8 | 10 | const result = addon.add(obj1, obj2); | |
| 9 | 11 | assert.strictEqual(result, 30); | |
| 12 | + | ||
| 13 | + // Make sure the native destructor gets called. | ||
| 14 | + obj1 = null; | ||
| 15 | + global.gc(); | ||
| 16 | + assert.strictEqual(addon.finalizeCount(), 1); | ||
0 commit comments