Skip to content

Commit 89fa0f3

Browse files
authored
Refactor napi_env to use Ref-counted NapiEnv (#23940)
### What does this PR do? Replaces raw napi_env pointers with WTF::Ref<NapiEnv> for improved memory management and safety. Updates related classes, function signatures, and finalizer handling to use reference counting. Adds ref/deref methods to NapiEnv and integrates them in Zig and C++ code paths, ensuring proper lifecycle management for N-API environments. ### How did you verify your code works?
1 parent 72f1ffd commit 89fa0f3

File tree

14 files changed

+168
-139
lines changed

14 files changed

+168
-139
lines changed

src/bun.js/api/FFI.h

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,7 @@ typedef _Bool bool;
3939
#define false 0
4040

4141
#ifndef SRC_JS_NATIVE_API_TYPES_H_
42-
typedef struct napi_env__ *napi_env;
42+
typedef struct NapiEnv *napi_env;
4343
typedef int64_t napi_value;
4444
typedef enum {
4545
napi_ok,
@@ -67,7 +67,7 @@ typedef enum {
6767
} napi_status;
6868
BUN_FFI_IMPORT void* NapiHandleScope__open(void* napi_env, bool detached);
6969
BUN_FFI_IMPORT void NapiHandleScope__close(void* napi_env, void* handleScope);
70-
BUN_FFI_IMPORT extern struct napi_env__ Bun__thisFFIModuleNapiEnv;
70+
BUN_FFI_IMPORT extern struct NapiEnv Bun__thisFFIModuleNapiEnv;
7171
#endif
7272

7373

src/bun.js/bindings/BunProcess.cpp

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -637,7 +637,7 @@ JSC_DEFINE_HOST_FUNCTION(Process_functionDlopen, (JSC::JSGlobalObject * globalOb
637637
auto env = globalObject->makeNapiEnv(nmodule);
638638
env->filename = filename_cstr;
639639

640-
auto encoded = reinterpret_cast<EncodedJSValue>(napi_register_module_v1(env, reinterpret_cast<napi_value>(exportsValue)));
640+
auto encoded = reinterpret_cast<EncodedJSValue>(napi_register_module_v1(env.ptr(), reinterpret_cast<napi_value>(exportsValue)));
641641
if (env->throwPendingException()) {
642642
return {};
643643
}
@@ -656,7 +656,7 @@ JSC_DEFINE_HOST_FUNCTION(Process_functionDlopen, (JSC::JSGlobalObject * globalOb
656656
// TODO: think about the finalizer here
657657
// currently we do not dealloc napi modules so we don't have to worry about it right now
658658
auto* meta = new Bun::NapiModuleMeta(globalObject->m_pendingNapiModuleDlopenHandle);
659-
Bun::NapiExternal* napi_external = Bun::NapiExternal::create(vm, globalObject->NapiExternalStructure(), meta, nullptr, env, nullptr);
659+
Bun::NapiExternal* napi_external = Bun::NapiExternal::create(vm, globalObject->NapiExternalStructure(), meta, nullptr, nullptr, env.ptr());
660660
bool success = resultObject->putDirect(vm, WebCore::builtinNames(vm).napiDlopenHandlePrivateName(), napi_external, JSC::PropertyAttribute::DontDelete | JSC::PropertyAttribute::ReadOnly);
661661
ASSERT(success);
662662
RETURN_IF_EXCEPTION(scope, {});

src/bun.js/bindings/NapiRef.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,7 @@ void NapiRef::unref()
3737
void NapiRef::clear()
3838
{
3939
NAPI_LOG("ref clear %p", this);
40-
finalizer.call(env, nativeObject);
40+
finalizer.call(env.ptr(), nativeObject);
4141
globalObject.clear();
4242
weakValueRef.clear();
4343
strongRef.clear();

src/bun.js/bindings/ZigGlobalObject.cpp

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -3517,10 +3517,10 @@ GlobalObject::PromiseFunctions GlobalObject::promiseHandlerID(Zig::FFIFunction h
35173517
}
35183518
}
35193519

3520-
napi_env GlobalObject::makeNapiEnv(const napi_module& mod)
3520+
Ref<NapiEnv> GlobalObject::makeNapiEnv(const napi_module& mod)
35213521
{
3522-
m_napiEnvs.append(std::make_unique<napi_env__>(this, mod));
3523-
return m_napiEnvs.last().get();
3522+
m_napiEnvs.append(NapiEnv::create(this, mod));
3523+
return m_napiEnvs.last();
35243524
}
35253525

35263526
napi_env GlobalObject::makeNapiEnvForFFI()
@@ -3534,7 +3534,7 @@ napi_env GlobalObject::makeNapiEnvForFFI()
35343534
.nm_priv = nullptr,
35353535
.reserved = {},
35363536
});
3537-
return out;
3537+
return &out.leakRef();
35383538
}
35393539

35403540
bool GlobalObject::hasNapiFinalizers() const

src/bun.js/bindings/ZigGlobalObject.h

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -724,8 +724,8 @@ class GlobalObject : public Bun::GlobalScope {
724724
// De-optimization once `require("module").runMain` is written to
725725
bool hasOverriddenModuleRunMain = false;
726726

727-
WTF::Vector<std::unique_ptr<napi_env__>> m_napiEnvs;
728-
napi_env makeNapiEnv(const napi_module&);
727+
WTF::Vector<WTF::Ref<NapiEnv>> m_napiEnvs;
728+
Ref<NapiEnv> makeNapiEnv(const napi_module&);
729729
napi_env makeNapiEnvForFFI();
730730
bool hasNapiFinalizers() const;
731731

src/bun.js/bindings/napi.cpp

Lines changed: 21 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -698,7 +698,7 @@ void Napi::executePendingNapiModule(Zig::GlobalObject* globalObject)
698698
ASSERT(globalObject->m_pendingNapiModule);
699699

700700
auto& mod = *globalObject->m_pendingNapiModule;
701-
napi_env env = globalObject->makeNapiEnv(mod);
701+
Ref<NapiEnv> env = globalObject->makeNapiEnv(mod);
702702
auto keyStr = WTF::String::fromUTF8(mod.nm_modname);
703703
JSValue pendingNapiModule = globalObject->m_pendingNapiModuleAndExports[0].get();
704704
JSObject* object = (pendingNapiModule && pendingNapiModule.isObject()) ? pendingNapiModule.getObject()
@@ -727,7 +727,7 @@ void Napi::executePendingNapiModule(Zig::GlobalObject* globalObject)
727727
JSValue resultValue;
728728

729729
if (mod.nm_register_func) {
730-
resultValue = toJS(mod.nm_register_func(env, toNapi(object, globalObject)));
730+
resultValue = toJS(mod.nm_register_func(env.ptr(), toNapi(object, globalObject)));
731731
} else {
732732
JSValue errorInstance = createError(globalObject, makeString("Module has no declared entry point."_s));
733733
globalObject->m_pendingNapiModuleAndExports[0].set(vm, globalObject, errorInstance);
@@ -751,7 +751,7 @@ void Napi::executePendingNapiModule(Zig::GlobalObject* globalObject)
751751
auto* meta = new Bun::NapiModuleMeta(globalObject->m_pendingNapiModuleDlopenHandle);
752752

753753
// TODO: think about the finalizer here
754-
Bun::NapiExternal* napi_external = Bun::NapiExternal::create(vm, globalObject->NapiExternalStructure(), meta, nullptr, env, nullptr);
754+
Bun::NapiExternal* napi_external = Bun::NapiExternal::create(vm, globalObject->NapiExternalStructure(), meta, nullptr, nullptr, env.ptr());
755755

756756
bool success = resultValue.getObject()->putDirect(vm, WebCore::builtinNames(vm).napiDlopenHandlePrivateName(), napi_external, JSC::PropertyAttribute::DontDelete | JSC::PropertyAttribute::ReadOnly);
757757
ASSERT(success);
@@ -791,7 +791,7 @@ static void wrap_cleanup(napi_env env, void* data, void* hint)
791791
{
792792
auto* ref = reinterpret_cast<NapiRef*>(data);
793793
ASSERT(ref->boundCleanup != nullptr);
794-
ref->boundCleanup->deactivate(env);
794+
ref->boundCleanup->deactivate(*env);
795795
ref->boundCleanup = nullptr;
796796
ref->callFinalizer();
797797
}
@@ -842,7 +842,7 @@ extern "C" napi_status napi_wrap(napi_env env,
842842
NAPI_RETURN_EARLY_IF_FALSE(env, existing_wrap == nullptr, napi_invalid_arg);
843843

844844
// create a new weak reference (refcount 0)
845-
auto* ref = new NapiRef(env, 0, Bun::NapiFinalizer { finalize_cb, finalize_hint });
845+
auto* ref = new NapiRef(*env, 0, Bun::NapiFinalizer { finalize_cb, finalize_hint });
846846
// In case the ref's finalizer is never called, we'll add a finalizer to execute on exit.
847847
const auto& bound_cleanup = env->addFinalizer(wrap_cleanup, native_object, ref);
848848
ref->boundCleanup = &bound_cleanup;
@@ -852,7 +852,7 @@ extern "C" napi_status napi_wrap(napi_env env,
852852
napi_instance->napiRef = ref;
853853
} else {
854854
// wrap the ref in an external so that it can serve as a JSValue
855-
auto* external = Bun::NapiExternal::create(JSC::getVM(globalObject), globalObject->NapiExternalStructure(), ref, nullptr, env, nullptr);
855+
auto* external = Bun::NapiExternal::create(JSC::getVM(globalObject), globalObject->NapiExternalStructure(), ref, nullptr, nullptr, env);
856856
jsc_object->putDirect(vm, propertyName, JSValue(external));
857857
}
858858

@@ -1082,7 +1082,7 @@ extern "C" napi_status napi_create_reference(napi_env env, napi_value value,
10821082
can_be_weak = false;
10831083
}
10841084

1085-
auto* ref = new NapiRef(env, initial_refcount, Bun::NapiFinalizer {});
1085+
auto* ref = new NapiRef(*env, initial_refcount, Bun::NapiFinalizer {});
10861086
ref->setValueInitial(val, can_be_weak);
10871087

10881088
*result = toNapi(ref);
@@ -1119,14 +1119,14 @@ extern "C" napi_status napi_add_finalizer(napi_env env, napi_value js_object,
11191119

11201120
if (result) {
11211121
// If they're expecting a Ref, use the ref.
1122-
auto* ref = new NapiRef(env, 0, Bun::NapiFinalizer { finalize_cb, finalize_hint });
1122+
auto* ref = new NapiRef(*env, 0, Bun::NapiFinalizer { finalize_cb, finalize_hint });
11231123
// TODO(@heimskr): consider detecting whether the value can't be weak, as we do in napi_create_reference.
11241124
ref->setValueInitial(object, true);
11251125
ref->nativeObject = native_object;
11261126
*result = toNapi(ref);
11271127
} else {
11281128
// Otherwise, it's cheaper to just call .addFinalizer.
1129-
vm.heap.addFinalizer(object, [env, finalize_cb, native_object, finalize_hint](JSCell* cell) -> void {
1129+
vm.heap.addFinalizer(object, [env = WTF::Ref<NapiEnv>(*env), finalize_cb, native_object, finalize_hint](JSCell* cell) -> void {
11301130
NAPI_LOG("finalizer %p", finalize_hint);
11311131
env->doFinalizer(finalize_cb, native_object, finalize_hint);
11321132
});
@@ -1991,7 +1991,7 @@ extern "C" napi_status napi_create_external_buffer(napi_env env, size_t length,
19911991

19921992
Zig::GlobalObject* globalObject = toJS(env);
19931993

1994-
auto arrayBuffer = ArrayBuffer::createFromBytes({ reinterpret_cast<const uint8_t*>(data), length }, createSharedTask<void(void*)>([env, finalize_hint, finalize_cb](void* p) {
1994+
auto arrayBuffer = ArrayBuffer::createFromBytes({ reinterpret_cast<const uint8_t*>(data), length }, createSharedTask<void(void*)>([env = WTF::Ref<NapiEnv>(*env), finalize_hint, finalize_cb](void* p) {
19951995
NAPI_LOG("external buffer finalizer");
19961996
env->doFinalizer(finalize_cb, p, finalize_hint);
19971997
}));
@@ -2303,7 +2303,7 @@ extern "C" napi_status napi_create_external(napi_env env, void* data,
23032303
JSC::VM& vm = JSC::getVM(globalObject);
23042304

23052305
auto* structure = globalObject->NapiExternalStructure();
2306-
JSValue value = Bun::NapiExternal::create(vm, structure, data, finalize_hint, env, finalize_cb);
2306+
JSValue value = Bun::NapiExternal::create(vm, structure, data, finalize_hint, finalize_cb, env);
23072307
JSC::EnsureStillAliveScope ensureStillAlive(value);
23082308
*result = toNapi(value, globalObject);
23092309
NAPI_RETURN_SUCCESS(env);
@@ -2902,4 +2902,14 @@ extern "C" bool NapiEnv__getAndClearPendingException(napi_env env, JSC::EncodedJ
29022902
return false;
29032903
}
29042904

2905+
extern "C" void NapiEnv__ref(napi_env env)
2906+
{
2907+
env->ref();
2908+
}
2909+
2910+
extern "C" void NapiEnv__deref(napi_env env)
2911+
{
2912+
env->deref();
2913+
}
2914+
29052915
}

src/bun.js/bindings/napi.h

Lines changed: 18 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -168,17 +168,24 @@ static bool equal(napi_async_cleanup_hook_handle one, napi_async_cleanup_hook_ha
168168
} while (0)
169169

170170
// Named this way so we can manipulate napi_env values directly (since napi_env is defined as a pointer to struct napi_env__)
171-
struct napi_env__ {
171+
struct NapiEnv : public WTF::RefCounted<NapiEnv> {
172+
WTF_MAKE_STRUCT_TZONE_ALLOCATED(NapiEnv);
173+
172174
public:
173-
napi_env__(Zig::GlobalObject* globalObject, const napi_module& napiModule)
175+
NapiEnv(Zig::GlobalObject* globalObject, const napi_module& napiModule)
174176
: m_globalObject(globalObject)
175177
, m_napiModule(napiModule)
176178
, m_vm(JSC::getVM(globalObject))
177179
{
178180
napi_internal_register_cleanup_zig(this);
179181
}
180182

181-
~napi_env__()
183+
static Ref<NapiEnv> create(Zig::GlobalObject* globalObject, const napi_module& napiModule)
184+
{
185+
return adoptRef(*new NapiEnv(globalObject, napiModule));
186+
}
187+
188+
~NapiEnv()
182189
{
183190
delete[] filename;
184191
}
@@ -434,12 +441,12 @@ struct napi_env__ {
434441
}
435442
}
436443

437-
void deactivate(napi_env env) const
444+
void deactivate(NapiEnv& env) const
438445
{
439-
if (env->isFinishingFinalizers()) {
446+
if (env.isFinishingFinalizers()) {
440447
active = false;
441448
} else {
442-
env->removeFinalizer(*this);
449+
env.removeFinalizer(*this);
443450
// At this point the BoundFinalizer has been destroyed, but because we're not doing anything else here it's safe.
444451
// https://isocpp.org/wiki/faq/freestore-mgmt#delete-this
445452
}
@@ -451,7 +458,7 @@ struct napi_env__ {
451458
}
452459

453460
struct Hash {
454-
std::size_t operator()(const napi_env__::BoundFinalizer& bound) const
461+
std::size_t operator()(const NapiEnv::BoundFinalizer& bound) const
455462
{
456463
constexpr std::hash<void*> hasher;
457464
constexpr std::ptrdiff_t magic = 0x9e3779b9;
@@ -659,7 +666,7 @@ class NapiRef {
659666
void unref();
660667
void clear();
661668

662-
NapiRef(napi_env env, uint32_t count, Bun::NapiFinalizer finalizer)
669+
NapiRef(Ref<NapiEnv>&& env, uint32_t count, Bun::NapiFinalizer finalizer)
663670
: env(env)
664671
, globalObject(JSC::Weak<JSC::JSGlobalObject>(env->globalObject()))
665672
, finalizer(WTFMove(finalizer))
@@ -708,7 +715,7 @@ class NapiRef {
708715
// calling the finalizer
709716
Bun::NapiFinalizer saved_finalizer = this->finalizer;
710717
this->finalizer.clear();
711-
saved_finalizer.call(env, nativeObject, !env->mustDeferFinalizers() || !env->inGC());
718+
saved_finalizer.call(env.ptr(), nativeObject, !env->mustDeferFinalizers() || !env->inGC());
712719
}
713720

714721
~NapiRef()
@@ -728,12 +735,12 @@ class NapiRef {
728735
weakValueRef.clear();
729736
}
730737

731-
napi_env env = nullptr;
738+
WTF::Ref<NapiEnv> env;
732739
JSC::Weak<JSC::JSGlobalObject> globalObject;
733740
NapiWeakValue weakValueRef;
734741
JSC::Strong<JSC::Unknown> strongRef;
735742
Bun::NapiFinalizer finalizer;
736-
const napi_env__::BoundFinalizer* boundCleanup = nullptr;
743+
const NapiEnv::BoundFinalizer* boundCleanup = nullptr;
737744
void* nativeObject = nullptr;
738745
uint32_t refCount = 0;
739746
bool releaseOnWeaken = false;

src/bun.js/bindings/napi_external.cpp

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -5,15 +5,15 @@ namespace Bun {
55

66
NapiExternal::~NapiExternal()
77
{
8-
ASSERT(m_env);
9-
m_finalizer.call(m_env, m_value, !m_env->mustDeferFinalizers());
8+
auto* env = m_env.get();
9+
m_finalizer.call(env, m_value, env && !env->mustDeferFinalizers());
1010
}
1111

1212
void NapiExternal::destroy(JSC::JSCell* cell)
1313
{
1414
static_cast<NapiExternal*>(cell)->~NapiExternal();
1515
}
1616

17-
const ClassInfo NapiExternal::s_info = { "External"_s, &Base::s_info, nullptr, nullptr, CREATE_METHOD_TABLE(NapiExternal) };
17+
const ClassInfo NapiExternal::s_info = { "NapiExternal"_s, &Base::s_info, nullptr, nullptr, CREATE_METHOD_TABLE(NapiExternal) };
1818

1919
}

src/bun.js/bindings/napi_external.h

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -22,8 +22,9 @@ class NapiExternal : public JSC::JSDestructibleObject {
2222
using Base = JSC::JSDestructibleObject;
2323

2424
public:
25-
NapiExternal(JSC::VM& vm, JSC::Structure* structure)
25+
NapiExternal(JSC::VM& vm, JSC::Structure* structure, WTF::RefPtr<NapiEnv> env)
2626
: Base(vm, structure)
27+
, m_env(env)
2728
{
2829
}
2930

@@ -53,11 +54,11 @@ class NapiExternal : public JSC::JSDestructibleObject {
5354
JSC::TypeInfo(JSC::ObjectType, StructureFlags), info());
5455
}
5556

56-
static NapiExternal* create(JSC::VM& vm, JSC::Structure* structure, void* value, void* finalizer_hint, napi_env env, napi_finalize callback)
57+
static NapiExternal* create(JSC::VM& vm, JSC::Structure* structure, void* value, void* finalizer_hint, napi_finalize callback, WTF::RefPtr<NapiEnv> env = nullptr)
5758
{
58-
NapiExternal* accessor = new (NotNull, JSC::allocateCell<NapiExternal>(vm)) NapiExternal(vm, structure);
59+
NapiExternal* accessor = new (NotNull, JSC::allocateCell<NapiExternal>(vm)) NapiExternal(vm, structure, env);
5960

60-
accessor->finishCreation(vm, value, finalizer_hint, env, callback);
61+
accessor->finishCreation(vm, value, finalizer_hint, callback);
6162

6263
#if ASSERT_ENABLED
6364
if (auto* callFrame = vm.topCallFrame) {
@@ -81,11 +82,10 @@ class NapiExternal : public JSC::JSDestructibleObject {
8182
return accessor;
8283
}
8384

84-
void finishCreation(JSC::VM& vm, void* value, void* finalizer_hint, napi_env env, napi_finalize callback)
85+
void finishCreation(JSC::VM& vm, void* value, void* finalizer_hint, napi_finalize callback)
8586
{
8687
Base::finishCreation(vm);
8788
m_value = value;
88-
m_env = env;
8989
m_finalizer = NapiFinalizer { callback, finalizer_hint };
9090
}
9191

@@ -95,7 +95,7 @@ class NapiExternal : public JSC::JSDestructibleObject {
9595

9696
void* m_value;
9797
NapiFinalizer m_finalizer;
98-
napi_env m_env;
98+
WTF::RefPtr<NapiEnv> m_env;
9999

100100
#if ASSERT_ENABLED
101101
String sourceOriginURL = String();

src/bun.js/bindings/napi_finalizer.cpp

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -5,14 +5,14 @@
55

66
namespace Bun {
77

8-
void NapiFinalizer::call(napi_env env, void* data, bool immediate)
8+
void NapiFinalizer::call(WTF::RefPtr<NapiEnv> env, void* data, bool immediate)
99
{
1010
if (m_callback) {
1111
NAPI_LOG_CURRENT_FUNCTION;
1212
if (immediate) {
13-
m_callback(env, data, m_hint);
13+
m_callback(env.get(), data, m_hint);
1414
} else {
15-
napi_internal_enqueue_finalizer(env, m_callback, data, m_hint);
15+
napi_internal_enqueue_finalizer(env.get(), m_callback, data, m_hint);
1616
}
1717
}
1818
}

0 commit comments

Comments
 (0)