FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

node-api: stop ref gc during environment teardown · nodejs/node@6e01855 · GitHub

/ node Public

Commit 6e01855

Browse files
gabrielschulhof
authored and
targos
committed
node-api: stop ref gc during environment teardown
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 Backport-PR-URL: #37802 Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Michael Dawson <midawson@redhat.com>
1 parent b5b1ad3 commit 6e01855

4 files changed

Lines changed: 86 additions & 1 deletion

File tree

‎src/js_native_api_v8.cc‎

Lines changed: 27 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -270,6 +270,20 @@ class RefBase : protected Finalizer, RefTracker {
270270

271271
protected:
272272
inline void Finalize(bool is_env_teardown = false) override {
273+
// In addition to being called during environment teardown, this method is
274+
// also the entry point for the garbage collector. During environment
275+
// teardown we have to remove the garbage collector's reference to this
276+
// method so that, if, as part of the user's callback, JS gets executed,
277+
// resulting in a garbage collection pass, this method is not re-entered as
278+
// part of that pass, because that'll cause a double free (as seen in
279+
// https://github.com/nodejs/node/issues/37236).
280+
//
281+
// Since this class does not have access to the V8 persistent reference,
282+
// this method is overridden in the `Reference` class below. Therein the
283+
// weak callback is removed, ensuring that the garbage collector does not
284+
// re-enter this method, and the method chains up to continue the process of
285+
// environment-teardown-induced finalization.
286+
273287
// During environment teardown we have to convert a strong reference to
274288
// a weak reference to force the deferring behavior if the user's finalizer
275289
// happens to delete this reference so that the code in this function that
@@ -278,9 +292,10 @@ class RefBase : protected Finalizer, RefTracker {
278292
if (is_env_teardown && RefCount() > 0) _refcount = 0;
279293

280294
if (_finalize_callback != nullptr) {
281-
_env->CallFinalizer(_finalize_callback, _finalize_data, _finalize_hint);
282295
// This ensures that we never call the finalizer twice.
296+
napi_finalize fini = _finalize_callback;
283297
_finalize_callback = nullptr;
298+
_env->CallFinalizer(fini, _finalize_data, _finalize_hint);
284299
}
285300

286301
// this is safe because if a request to delete the reference
@@ -355,6 +370,17 @@ class Reference : public RefBase {
355370
}
356371
}
357372

373+
protected:
374+
inline void Finalize(bool is_env_teardown = false) override {
375+
// During env teardown, `~napi_env()` alone is responsible for finalizing.
376+
// Thus, we don't want any stray gc passes to trigger a second call to
377+
// `Finalize()`, so let's reset the persistent here.
378+
if (is_env_teardown) _persistent.ClearWeak();
379+
380+
// Chain up to perform the rest of the finalization.
381+
RefBase::Finalize(is_env_teardown);
382+
}
383+
358384
private:
359385
// The N-API finalizer callback may make calls into the engine. V8's heap is
360386
// not in a consistent state during the weak callback, and therefore it does

‎test/node-api/test_env_teardown_gc/binding.c‎

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,37 @@
1+
#include <stdlib.h>
2+
#include <node_api.h>
3+
#include "../../js-native-api/common.h"
4+
5+
static void MyObject_fini(napi_env env, void* data, void* hint) {
6+
napi_ref* ref = data;
7+
napi_value global;
8+
napi_value cleanup;
9+
NAPI_CALL_RETURN_VOID(env, napi_get_global(env, &global));
10+
NAPI_CALL_RETURN_VOID(
11+
env, napi_get_named_property(env, global, "cleanup", &cleanup));
12+
napi_status status = napi_call_function(env, global, cleanup, 0, NULL, NULL);
13+
// We may not be allowed to call into JS, in which case a pending exception
14+
// will be returned.
15+
NAPI_ASSERT_RETURN_VOID(env,
16+
status == napi_ok || status == napi_pending_exception,
17+
"Unexpected status for napi_call_function");
18+
NAPI_CALL_RETURN_VOID(env, napi_delete_reference(env, *ref));
19+
free(ref);
20+
}
21+
22+
static napi_value MyObject(napi_env env, napi_callback_info info) {
23+
napi_value js_this;
24+
napi_ref* ref = malloc(sizeof(*ref));
25+
NAPI_CALL(env, napi_get_cb_info(env, info, NULL, NULL, &js_this, NULL));
26+
NAPI_CALL(env, napi_wrap(env, js_this, ref, MyObject_fini, NULL, ref));
27+
return NULL;
28+
}
29+
30+
NAPI_MODULE_INIT() {
31+
napi_value ctor;
32+
NAPI_CALL(
33+
env, napi_define_class(
34+
env, "MyObject", NAPI_AUTO_LENGTH, MyObject, NULL, 0, NULL, &ctor));
35+
NAPI_CALL(env, napi_set_named_property(env, exports, "MyObject", ctor));
36+
return exports;
37+
}

‎test/node-api/test_env_teardown_gc/binding.gyp‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
1+
{
2+
'targets': [
3+
{
4+
'target_name': 'binding',
5+
'sources': [ 'binding.c' ]
6+
}
7+
]
8+
}

‎test/node-api/test_env_teardown_gc/test.js‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,14 @@
1+
'use strict';
2+
// Flags: --expose-gc
3+
4+
process.env.NODE_TEST_KNOWN_GLOBALS = 0;
5+
6+
const common = require('../../common');
7+
const binding = require(`./build/${common.buildType}/binding`);
8+
9+
global.it = new binding.MyObject();
10+
11+
global.cleanup = () => {
12+
delete global.it;
13+
global.gc();
14+
};

0 commit comments

Comments
 (0)
Back | FazBrowse Home | New Git URL