Skip to content

Commit

Permalink
node-api: stop ref gc during environment teardown
Browse files Browse the repository at this point in the history
A gc may happen during environment teardown. Thus, during finalization
initiated by environment teardown we must remove the V8 finalizer
before calling the Node-API finalizer.

Fixes: #37236
PR-URL: #37616
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
  • Loading branch information
gabrielschulhof authored and ruyadorno committed Mar 24, 2021
1 parent bfe3f21 commit d1a3e0e
Show file tree
Hide file tree
Showing 4 changed files with 86 additions and 1 deletion.
28 changes: 27 additions & 1 deletion src/js_native_api_v8.cc
Original file line number Diff line number Diff line change
Expand Up @@ -270,6 +270,20 @@ class RefBase : protected Finalizer, RefTracker {

protected:
inline void Finalize(bool is_env_teardown = false) override {
// In addition to being called during environment teardown, this method is
// also the entry point for the garbage collector. During environment
// teardown we have to remove the garbage collector's reference to this
// method so that, if, as part of the user's callback, JS gets executed,
// resulting in a garbage collection pass, this method is not re-entered as
// part of that pass, because that'll cause a double free (as seen in
// https://github.com/nodejs/node/issues/37236).
//
// Since this class does not have access to the V8 persistent reference,
// this method is overridden in the `Reference` class below. Therein the
// weak callback is removed, ensuring that the garbage collector does not
// re-enter this method, and the method chains up to continue the process of
// environment-teardown-induced finalization.

// During environment teardown we have to convert a strong reference to
// a weak reference to force the deferring behavior if the user's finalizer
// happens to delete this reference so that the code in this function that
Expand All @@ -278,9 +292,10 @@ class RefBase : protected Finalizer, RefTracker {
if (is_env_teardown && RefCount() > 0) _refcount = 0;

if (_finalize_callback != nullptr) {
_env->CallFinalizer(_finalize_callback, _finalize_data, _finalize_hint);
// This ensures that we never call the finalizer twice.
napi_finalize fini = _finalize_callback;
_finalize_callback = nullptr;
_env->CallFinalizer(fini, _finalize_data, _finalize_hint);
}

// this is safe because if a request to delete the reference
Expand Down Expand Up @@ -355,6 +370,17 @@ class Reference : public RefBase {
}
}

protected:
inline void Finalize(bool is_env_teardown = false) override {
// During env teardown, `~napi_env()` alone is responsible for finalizing.
// Thus, we don't want any stray gc passes to trigger a second call to
// `Finalize()`, so let's reset the persistent here.
if (is_env_teardown) _persistent.ClearWeak();

// Chain up to perform the rest of the finalization.
RefBase::Finalize(is_env_teardown);
}

private:
// The N-API finalizer callback may make calls into the engine. V8's heap is
// not in a consistent state during the weak callback, and therefore it does
Expand Down
37 changes: 37 additions & 0 deletions test/node-api/test_env_teardown_gc/binding.c
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
#include <stdlib.h>
#include <node_api.h>
#include "../../js-native-api/common.h"

static void MyObject_fini(napi_env env, void* data, void* hint) {
napi_ref* ref = data;
napi_value global;
napi_value cleanup;
NODE_API_CALL_RETURN_VOID(env, napi_get_global(env, &global));
NODE_API_CALL_RETURN_VOID(
env, napi_get_named_property(env, global, "cleanup", &cleanup));
napi_status status = napi_call_function(env, global, cleanup, 0, NULL, NULL);
// We may not be allowed to call into JS, in which case a pending exception
// will be returned.
NODE_API_ASSERT_RETURN_VOID(env,
status == napi_ok || status == napi_pending_exception,
"Unexpected status for napi_call_function");
NODE_API_CALL_RETURN_VOID(env, napi_delete_reference(env, *ref));
free(ref);
}

static napi_value MyObject(napi_env env, napi_callback_info info) {
napi_value js_this;
napi_ref* ref = malloc(sizeof(*ref));
NODE_API_CALL(env, napi_get_cb_info(env, info, NULL, NULL, &js_this, NULL));
NODE_API_CALL(env, napi_wrap(env, js_this, ref, MyObject_fini, NULL, ref));
return NULL;
}

NAPI_MODULE_INIT() {
napi_value ctor;
NODE_API_CALL(
env, napi_define_class(
env, "MyObject", NAPI_AUTO_LENGTH, MyObject, NULL, 0, NULL, &ctor));
NODE_API_CALL(env, napi_set_named_property(env, exports, "MyObject", ctor));
return exports;
}
8 changes: 8 additions & 0 deletions test/node-api/test_env_teardown_gc/binding.gyp
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
{
'targets': [
{
'target_name': 'binding',
'sources': [ 'binding.c' ]
}
]
}
14 changes: 14 additions & 0 deletions test/node-api/test_env_teardown_gc/test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
'use strict';
// Flags: --expose-gc

process.env.NODE_TEST_KNOWN_GLOBALS = 0;

const common = require('../../common');
const binding = require(`./build/${common.buildType}/binding`);

global.it = new binding.MyObject();

global.cleanup = () => {
delete global.it;
global.gc();
};

0 comments on commit d1a3e0e

Please sign in to comment.