From c109b3c83ebaab0724bda4c013da6ce364f35625 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Mon, 30 Dec 2019 21:26:35 +0100 Subject: [PATCH 1/3] n-api: keep napi_env alive while it has finalizers Manage the napi_env refcount from Finalizer instances, as the finalizer may refer to the napi_env until it is deleted. Fixes: https://github.com/nodejs/node/issues/31134 Fixes: https://github.com/node-ffi-napi/node-ffi-napi/issues/48 --- src/js_native_api_v8.h | 5 ++++- test/node-api/test_buffer/test-external-buffer.js | 14 ++++++++++++++ 2 files changed, 18 insertions(+), 1 deletion(-) create mode 100644 test/node-api/test_buffer/test-external-buffer.js diff --git a/src/js_native_api_v8.h b/src/js_native_api_v8.h index 534b09851f8c8e..86d656264dcbbb 100644 --- a/src/js_native_api_v8.h +++ b/src/js_native_api_v8.h @@ -195,9 +195,12 @@ class Finalizer { _finalize_callback(finalize_callback), _finalize_data(finalize_data), _finalize_hint(finalize_hint) { + _env->Ref(); } - ~Finalizer() = default; + ~Finalizer() { + _env->Unref(); + } public: static Finalizer* New(napi_env env, diff --git a/test/node-api/test_buffer/test-external-buffer.js b/test/node-api/test_buffer/test-external-buffer.js new file mode 100644 index 00000000000000..4f019d4fb93284 --- /dev/null +++ b/test/node-api/test_buffer/test-external-buffer.js @@ -0,0 +1,14 @@ +'use strict'; + +const common = require('../../common'); +const binding = require(`./build/${common.buildType}/test_buffer`); +const assert = require('assert'); + +// Regresion test for https://github.com/nodejs/node/issues/31134: +// Buffers with finalizers are allowed to remain alive until +// Environment cleanup without crashing the process. +// The test stores the buffer on `process` to make sure it lives as long as +// the Context does. + +process.externalBuffer = binding.newExternalBuffer(); +assert.strictEqual(process.externalBuffer.toString(), binding.theText); From ac502fab4fdb165d6d485219e84e18b30fb5e6a8 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Mon, 30 Dec 2019 22:54:29 +0100 Subject: [PATCH 2/3] fixup! n-api: keep napi_env alive while it has finalizers --- src/js_native_api_v8.h | 27 +++++++++++++++++++++------ src/node_api.cc | 3 ++- 2 files changed, 23 insertions(+), 7 deletions(-) diff --git a/src/js_native_api_v8.h b/src/js_native_api_v8.h index 86d656264dcbbb..0d5424cccaddf0 100644 --- a/src/js_native_api_v8.h +++ b/src/js_native_api_v8.h @@ -186,29 +186,43 @@ inline v8::Local V8LocalValueFromJsValue(napi_value v) { // Adapter for napi_finalize callbacks. class Finalizer { + public: + // Some Finalizers are run during shutdown when the napi_env is destroyed, + // and some need to keep an explicit reference to the napi_env because they + // are run independently. + enum EnvReferenceMode { + kNoEnvReference, + kKeepEnvReference + }; + protected: Finalizer(napi_env env, napi_finalize finalize_callback, void* finalize_data, - void* finalize_hint) + void* finalize_hint, + EnvReferenceMode refmode = kNoEnvReference) : _env(env), _finalize_callback(finalize_callback), _finalize_data(finalize_data), - _finalize_hint(finalize_hint) { - _env->Ref(); + _finalize_hint(finalize_hint), + _has_env_reference(refmode == kKeepEnvReference) { + if (_has_env_reference) + _env->Ref(); } ~Finalizer() { - _env->Unref(); + if (_has_env_reference) + _env->Unref(); } public: static Finalizer* New(napi_env env, napi_finalize finalize_callback = nullptr, void* finalize_data = nullptr, - void* finalize_hint = nullptr) { + void* finalize_hint = nullptr, + EnvReferenceMode refmode = kNoEnvReference) { return new Finalizer( - env, finalize_callback, finalize_data, finalize_hint); + env, finalize_callback, finalize_data, finalize_hint, refmode); } static void Delete(Finalizer* finalizer) { @@ -221,6 +235,7 @@ class Finalizer { void* _finalize_data; void* _finalize_hint; bool _finalize_ran = false; + bool _has_env_reference = false; }; class TryCatch : public v8::TryCatch { diff --git a/src/node_api.cc b/src/node_api.cc index 8df4559c6cddc6..5e467dded5bdb7 100644 --- a/src/node_api.cc +++ b/src/node_api.cc @@ -732,7 +732,8 @@ napi_status napi_create_external_buffer(napi_env env, // The finalizer object will delete itself after invoking the callback. v8impl::Finalizer* finalizer = v8impl::Finalizer::New( - env, finalize_cb, nullptr, finalize_hint); + env, finalize_cb, nullptr, finalize_hint, + v8impl::Finalizer::kKeepEnvReference); auto maybe = node::Buffer::New(isolate, static_cast(data), From cacbfe8671fbfd9224ca6b53570e9a3434919a4e Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Fri, 3 Jan 2020 05:10:51 +0100 Subject: [PATCH 3/3] fixup! n-api: keep napi_env alive while it has finalizers --- test/node-api/test_buffer/test-external-buffer.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/node-api/test_buffer/test-external-buffer.js b/test/node-api/test_buffer/test-external-buffer.js index 4f019d4fb93284..4f220f1d21e9ce 100644 --- a/test/node-api/test_buffer/test-external-buffer.js +++ b/test/node-api/test_buffer/test-external-buffer.js @@ -4,7 +4,7 @@ const common = require('../../common'); const binding = require(`./build/${common.buildType}/test_buffer`); const assert = require('assert'); -// Regresion test for https://github.com/nodejs/node/issues/31134: +// Regression test for https://github.com/nodejs/node/issues/31134 // Buffers with finalizers are allowed to remain alive until // Environment cleanup without crashing the process. // The test stores the buffer on `process` to make sure it lives as long as