Skip to content

Commit 5cb4644

Browse files
ryantrembghgary
andauthored
Fix issues with reference tracking (#101)
Background refresher: We want to be able to track weak references to arbitrary JS objects (created either from JavaScript or from native). When this is a weak reference, we need to know when the JS object has been GC'd such that the reference evaluates to null. To track when an object is GC'd, we want to register a finalizer callback. However, the only way to know when an object is finalized with JSC is to create a new class and provide a `finalize` callback. Given this, what we do is create a "sentinel" JS object that is attached/linked to the actual JS object of interest, and only referenced by that JS object. This way when the JS object of interest is GC'd, the attached/linked sentinel JS object with a finalizer callback is also GC'd and our finalizer is called and we know the JS object of interest has been GC'd. There are two main issues being addressed in this PR: 1. Since we are relying on the finalizer of the sentinel object (not the actual object of interest), it is possible that the JS object of interest is GC'd, then a new JS object is instantiated at the same memory address, then we finally get the finalizer callback for the sentinel object. When this happens, and we also create a reference to the new JS object, our reference tracking thinks we are creating a reference to an existing JS object, then we finally get the finalizer for the sentinel object, and remove our reference tracking for the new object. Effectively this happens because we are using the memory address of the JS object as a globally and temporally unique identifier, when in fact that same identifier can refer to two different objects at different times, and the sentinel object finalizer can be called after this transition happens. To address this, we augment the reference tracking to use the `NativeInfo` instance memory address as a unique identifier (tracked in our active reference list, and already attached to the sentinel JS object), which is safe because that object is not deleted until after the sentinel is GC'd, so it will definitely be valid (unique) for at least as long as the JS object being tracked. When adding a ref as well as when accessing the underlying value of the ref, we check this new unique identifier to make sure the JS object (at the stored memory address) is actually still the same one. 2. Previously the way we were attaching/linking the sentinel object to the actual JS object was through the prototype chain. This has the benefit of being a well hidden implementation detail (e.g. it won't show up in a JS debugger, etc.), but if the prototype chain of an object is ever assigned to another object, it totally breaks the reference tracking. This could happen if code explicitly sets the prototype of one object to the prototype of another object, but it could also happen if you take a ref to a class and then instantiate the class (the class's prototype chain will implicitly be copied to the object instance). To make this more robust, we now instead attach/link the sentinel by setting it as a property on the object being tracked. We use a symbol as the key, and make it non-enumerable, readonly, and non-deletable. This should make it very hard to accidentally attach/link a sentinel object to a another object. Since there is now an object id check in `napi_ref__::value`, this also exposed another napi bug, where we try to read properties off of a GC'd JS object. To fix this, I'm also taking this napi fix: https://github.com/nodejs/node-addon-api/pull/1607/files --------- Co-authored-by: Gary Hsu <[email protected]>
1 parent 7b1fe94 commit 5cb4644

File tree

4 files changed

+204
-63
lines changed

4 files changed

+204
-63
lines changed

Core/Node-API/Include/Shared/napi/napi-inl.h

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4513,7 +4513,7 @@ template <typename T>
45134513
inline ObjectWrap<T>::~ObjectWrap() {
45144514
// If the JS object still exists at this point, remove the finalizer added
45154515
// through `napi_wrap()`.
4516-
if (!IsEmpty()) {
4516+
if (!IsEmpty() && !_finalized) {
45174517
Object object = Value();
45184518
// It is not valid to call `napi_remove_wrap()` with an empty `object`.
45194519
// This happens e.g. during garbage collection.
@@ -4955,6 +4955,7 @@ inline void ObjectWrap<T>::FinalizeCallback(napi_env env,
49554955
void* /*hint*/) {
49564956
HandleScope scope(env);
49574957
T* instance = static_cast<T*>(data);
4958+
instance->_finalized = true;
49584959
instance->Finalize(Napi::Env(env));
49594960
delete instance;
49604961
}

Core/Node-API/Include/Shared/napi/napi.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2497,6 +2497,7 @@ class ObjectWrap : public InstanceWrap<T>, public Reference<Object> {
24972497
}
24982498

24992499
bool _construction_failed = true;
2500+
bool _finalized = false;
25002501
};
25012502

25022503
class HandleScope {

0 commit comments

Comments
 (0)