Skip to content

Commit

Permalink
n-api: keep napi_env alive while it has finalizers
Browse files Browse the repository at this point in the history
Manage the napi_env refcount from Finalizer instances, as the
finalizer may refer to the napi_env until it is deleted.

Fixes: #31134
Fixes: node-ffi-napi/node-ffi-napi#48
PR-URL: #31140
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
  • Loading branch information
addaleax authored and targos committed Jan 6, 2020
1 parent 18acacc commit d771221
Show file tree
Hide file tree
Showing 3 changed files with 39 additions and 6 deletions.
28 changes: 23 additions & 5 deletions src/js_native_api_v8.h
Original file line number Diff line number Diff line change
Expand Up @@ -186,26 +186,43 @@ inline v8::Local<v8::Value> 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) {
Expand All @@ -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 {
Expand Down
3 changes: 2 additions & 1 deletion src/node_api.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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<char*>(data),
Expand Down
14 changes: 14 additions & 0 deletions test/node-api/test_buffer/test-external-buffer.js
Original file line number Diff line number Diff line change
@@ -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);

0 comments on commit d771221

Please sign in to comment.