feat(engine): add WeakJsObject weak reference type - #5437
Conversation
Introduce a WeakJsObject<T> type in boa_engine that lets embedders hold a weak reference to a JsObject without keeping it alive across garbage collections. This is the object-level counterpart of boa_gc::WeakGc. The mechanism already existed internally: the JS-level WeakRef builtin constructs a WeakGc<VTableObject<T>> via WeakGc::new(target.inner()), but JsObject::inner() is pub(crate), so embedders had no public API to build a weak object reference. WeakJsObject wraps that construction and exposes new / upgrade / is_upgradable, mirroring WeakGc's surface, plus Clone, PartialEq, Eq and Hash. Debug is implemented by hand because VTableObject intentionally does not implement Debug. Closes boa-dev#5420 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Cover the weak reference behaviour of WeakJsObject: upgrading while the referent is live, upgrade returning None after the referent is collected, clone sharing the same referent, equality by referent identity, and construction via the From<&JsObject> conversion. The collection test uses boa_gc::force_collect to drive the garbage collector, mirroring the existing WeakGc tests in core/gc. Refs boa-dev#5420 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The issue only asked for a straightforward WeakJsObject wrapper, and the Eq/Hash impls (mirrored from WeakGc) are unsound to expose on a public API: forwarding to WeakGc compares and hashes by the live referent, so once the object is collected two references to it stop comparing equal and change their hash. That breaks Eq reflexivity (a == a becomes false) and the Hash/Eq invariant for a collected key, making WeakJsObject unusable as a long-lived HashMap/HashSet key, which is the archetypal use of a weak reference. Drop these impls from the initial surface; they can be added back later without a breaking change once a collection-stable identity is designed. To compare weak references, upgrade them and compare the JsObjects. Also expand the tests: an upgraded handle keeps the referent alive across a later collection, all weak references to one object die together, and the generic works with a typed (non-erased) object. Refs boa-dev#5420 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Test262 conformance changes
Tested main commit: |
jedel1043
left a comment
There was a problem hiding this comment.
Mostly looks good, I just have a couple of nitpicks.
| // `VTableObject` deliberately does not implement `Debug` to avoid recursing into the object graph | ||
| // (which could overflow the stack), so we cannot derive `Debug` here. We provide a minimal, | ||
| // non-recursive implementation instead. | ||
| impl<T: NativeObject> Debug for WeakJsObject<T> { | ||
| fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { | ||
| f.debug_struct("WeakJsObject").finish_non_exhaustive() | ||
| } | ||
| } |
There was a problem hiding this comment.
This was already solved by the Debug implementation on JsObject, which uses a recursion limiter
boa/core/engine/src/object/jsobject.rs
Lines 1290 to 1325 in 6ef5370
You should use that instead.
There was a problem hiding this comment.
Thanks for the pointer. Upgrades and delegates to JsObject's Debug now in commit 2137fbf.
| #[cfg(test)] | ||
| mod tests { | ||
| use super::{JsObject, WeakJsObject}; | ||
| use boa_gc::force_collect; | ||
|
|
||
| #[test] | ||
| fn upgrade_while_referent_is_live() { | ||
| let object = JsObject::with_null_proto(); | ||
| let weak = WeakJsObject::new(&object); | ||
|
|
||
| assert!(weak.is_upgradable()); | ||
| let upgraded = weak.upgrade().expect("referent is still alive"); | ||
| assert_eq!(upgraded, object); | ||
| } | ||
|
|
||
| #[test] | ||
| fn upgrade_returns_none_after_referent_is_collected() { | ||
| let object = JsObject::with_null_proto(); | ||
| let weak = WeakJsObject::new(&object); | ||
|
|
||
| // While the strong reference is alive the weak one can be upgraded, even across a collection. | ||
| force_collect(); | ||
| assert!(weak.is_upgradable()); | ||
| assert!(weak.upgrade().is_some()); | ||
|
|
||
| // Once the last strong reference is gone the referent can be collected. | ||
| drop(object); | ||
| force_collect(); | ||
|
|
||
| assert!(!weak.is_upgradable()); | ||
| assert!(weak.upgrade().is_none()); | ||
| } | ||
|
|
||
| #[test] | ||
| fn clone_points_to_the_same_referent() { | ||
| let object = JsObject::with_null_proto(); | ||
| let weak = WeakJsObject::new(&object); | ||
| let cloned = weak.clone(); | ||
|
|
||
| assert_eq!( | ||
| weak.upgrade().expect("live"), | ||
| cloned.upgrade().expect("live") | ||
| ); | ||
| } | ||
|
|
||
| #[test] | ||
| fn upgraded_handle_keeps_referent_alive_across_collection() { | ||
| let object = JsObject::with_null_proto(); | ||
| let weak = WeakJsObject::new(&object); | ||
| let strong = weak.upgrade().expect("referent is still alive"); | ||
|
|
||
| // Drop the original binding; only the upgraded handle roots the referent now. | ||
| drop(object); | ||
| force_collect(); | ||
| assert!(weak.is_upgradable()); | ||
| assert_eq!( | ||
| weak.upgrade().expect("kept alive by the upgraded handle"), | ||
| strong | ||
| ); | ||
|
|
||
| // Once the upgraded handle is gone too, the referent can be collected. | ||
| drop(strong); | ||
| force_collect(); | ||
| assert!(!weak.is_upgradable()); | ||
| assert!(weak.upgrade().is_none()); | ||
| } | ||
|
|
||
| #[test] | ||
| fn all_weaks_to_same_referent_die_together() { | ||
| let object = JsObject::with_null_proto(); | ||
| let first = WeakJsObject::new(&object); | ||
| let cloned = first.clone(); | ||
| let second = WeakJsObject::new(&object); | ||
|
|
||
| drop(object); | ||
| force_collect(); | ||
|
|
||
| assert!(!first.is_upgradable()); | ||
| assert!(!cloned.is_upgradable()); | ||
| assert!(!second.is_upgradable()); | ||
| } | ||
|
|
||
| #[test] | ||
| fn works_with_a_typed_object() { | ||
| use crate::builtins::OrdinaryObject; | ||
|
|
||
| let object: JsObject<OrdinaryObject> = JsObject::new_unique(None, OrdinaryObject); | ||
| let weak: WeakJsObject<OrdinaryObject> = WeakJsObject::new(&object); | ||
|
|
||
| let upgraded: JsObject<OrdinaryObject> = weak.upgrade().expect("referent is still alive"); | ||
| assert_eq!(upgraded, object); | ||
|
|
||
| drop(object); | ||
| drop(upgraded); | ||
| force_collect(); | ||
| assert!(!weak.is_upgradable()); | ||
| assert!(weak.upgrade().is_none()); | ||
| } | ||
|
|
||
| #[test] | ||
| fn built_from_reference_via_conversion() { | ||
| let object = JsObject::with_null_proto(); | ||
| let weak: WeakJsObject = (&object).into(); | ||
|
|
||
| assert_eq!(weak.upgrade().expect("live"), object); | ||
| } | ||
| } |
There was a problem hiding this comment.
Instead of adding tests that are kind of already covered by the WeakGc tests on boa_gc, you should convert these into a nice usage example inside examples.
There was a problem hiding this comment.
Thanks for the suggestion! Added an example in commit 3355d72.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5437 +/- ##
===========================================
+ Coverage 47.24% 62.74% +15.49%
===========================================
Files 476 535 +59
Lines 46892 59091 +12199
===========================================
+ Hits 22154 37074 +14920
+ Misses 24738 22017 -2721 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…ject Delegate WeakJsObject's Debug to JsObject's own Debug impl instead of the placeholder finish_non_exhaustive(). JsObject's Debug uses a RecursionLimiter to avoid overflowing the stack on cyclic object graphs, which was the reason VTableObject skips Debug in the first place, so upgrading and printing the referent (or None when collected) is both safe and more useful. Addresses review feedback on boa-dev#5437. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The unit tests largely duplicated the WeakGc tests in boa_gc, so drop them and add examples/src/bin/weak_js_object.rs instead. The example shows the intended embedder use case: a Rust-side registry that remembers JS objects by id through weak references without keeping them alive, upgrading while a strong handle exists and failing to upgrade once the referent is collected. Addresses review feedback on boa-dev#5437. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This Pull Request closes #5420.
It adds a public
WeakJsObjecttype toboa_engine, giving embedders a weak reference to aJsObjectthat does not keep the object alive across garbage collections. This is the object-level counterpart ofboa_gc::WeakGc.It changes the following:
WeakJsObject<T>incore/engine/src/object/weak.rswithnew,upgradeandis_upgradable, mirroring the surface ofWeakGc, plusClone,DebugandFrom<&JsObject>. The mechanism already existed internally (theWeakRefbuiltin builds aWeakGc<VTableObject<T>>viaWeakGc::new(target.inner())), butJsObject::inner()ispub(crate), so embedders had no public way to construct one.PartialEq/Eq/Hash. Forwarding them toWeakGccompares and hashes by the live referent, so once the object is collected two references to it stop comparing equal and change their hash, which breaksEqreflexivity and theHash/Eqinvariant for a collected key. These can be added later without a breaking change if a collection-stable identity is wanted.Noneafter collection, an upgraded handle keeping the referent alive across a later collection, multiple weak references dropping together, and a typed (non-erased) object.