From d7712213a43faca487d044d48534342de1e0ed0c Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Mon, 30 Dec 2019 21:26:35 +0100 Subject: [PATCH] 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 PR-URL: https://github.com/nodejs/node/pull/31140 Reviewed-By: Jiawen Geng Reviewed-By: Rich Trott --- src/js_native_api_v8.h | 28 +++++++++++++++---- src/node_api.cc | 3 +- .../test_buffer/test-external-buffer.js | 14 ++++++++++ 3 files changed, 39 insertions(+), 6 deletions(-) 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..0d5424cccaddf0 100644 --- a/src/js_native_api_v8.h +++ b/src/js_native_api_v8.h @@ -186,26 +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) { + _finalize_hint(finalize_hint), + _has_env_reference(refmode == kKeepEnvReference) { + if (_has_env_reference) + _env->Ref(); } - ~Finalizer() = default; + ~Finalizer() { + 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) { @@ -218,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), 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..4f220f1d21e9ce --- /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'); + +// 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 +// the Context does. + +process.externalBuffer = binding.newExternalBuffer(); +assert.strictEqual(process.externalBuffer.toString(), binding.theText);