Skip to content

Conversation

@ryantrem
Copy link
Member

@ryantrem ryantrem commented Jan 17, 2025

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

@ryantrem ryantrem requested a review from bghgary January 17, 2025 18:29
@ryantrem ryantrem marked this pull request as draft January 17, 2025 18:30
@ryantrem ryantrem marked this pull request as ready for review January 17, 2025 18:31
ryantrem and others added 2 commits January 21, 2025 10:40
Co-authored-by: Gary Hsu <bghgary@users.noreply.github.com>
@ryantrem ryantrem enabled auto-merge (squash) January 21, 2025 18:42
@ryantrem ryantrem merged commit 5cb4644 into BabylonJS:main Jan 21, 2025
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants