diff --git a/boa_gc/src/internals/ephemeron_box.rs b/boa_gc/src/internals/ephemeron_box.rs index 68cbb0af976..fa356c4e650 100644 --- a/boa_gc/src/internals/ephemeron_box.rs +++ b/boa_gc/src/internals/ephemeron_box.rs @@ -204,7 +204,7 @@ impl ErasedEphemeronBox for EphemeronBox { // SAFETY: `data` comes from an `into_raw` call, so this pointer is safe to pass to // `from_raw`. let contents = unsafe { Box::from_raw(data.as_ptr()) }; - contents.value.finalize(); + Trace::run_finalizer(&contents.value); } } } diff --git a/boa_gc/src/lib.rs b/boa_gc/src/lib.rs index fb6e9b53fbb..d70e1a3dece 100644 --- a/boa_gc/src/lib.rs +++ b/boa_gc/src/lib.rs @@ -286,7 +286,7 @@ impl Collector { let unreachables = Self::mark_heap(&gc.strong_start, &gc.weak_start, &gc.weak_map_start); // Only finalize if there are any unreachable nodes. - if !unreachables.strong.is_empty() || unreachables.weak.is_empty() { + if !unreachables.strong.is_empty() || !unreachables.weak.is_empty() { // Finalize all the unreachable nodes. // SAFETY: All passed pointers are valid, since we won't deallocate until `Self::sweep`. unsafe { Self::finalize(unreachables) }; @@ -341,7 +341,7 @@ impl Collector { while let Some(node) = strong.get() { // SAFETY: node must be valid as this phase cannot drop any node. let node_ref = unsafe { node.as_ref() }; - if node_ref.header.roots() > 0 { + if node_ref.header.roots() != 0 { // SAFETY: the reference to node must be valid as it is rooted. Passing // invalid references can result in Undefined Behavior unsafe { @@ -367,17 +367,22 @@ impl Collector { // === Weak mark phase === // + // // 1. Get the naive list of ephemerons that are supposedly dead or their key is dead and // trace all the ephemerons that have roots and their keys are live. Also remove from // this list the ephemerons that are marked but their value is dead. while let Some(eph) = weak.get() { // SAFETY: node must be valid as this phase cannot drop any node. let eph_ref = unsafe { eph.as_ref() }; + let header = eph_ref.header(); + if header.roots() != 0 { + header.mark(); + } // SAFETY: the garbage collector ensures `eph_ref` always points to valid data. if unsafe { !eph_ref.trace() } { pending_ephemerons.push(eph); } - weak = &eph_ref.header().next; + weak = &header.next; } // 2. Trace all the weak pointers in the live weak maps to make sure they do not get swept. @@ -432,7 +437,7 @@ impl Collector { for node in unreachables.strong { // SAFETY: The caller must ensure all pointers inside `unreachables.strong` are valid. let node = unsafe { node.as_ref() }; - Trace::run_finalizer(&node.value()); + Trace::run_finalizer(node.value()); } for node in unreachables.weak { // SAFETY: The caller must ensure all pointers inside `unreachables.weak` are valid. diff --git a/boa_gc/src/test/weak.rs b/boa_gc/src/test/weak.rs index 2635565848b..dac35ff7515 100644 --- a/boa_gc/src/test/weak.rs +++ b/boa_gc/src/test/weak.rs @@ -1,3 +1,5 @@ +use std::{cell::Cell, rc::Rc}; + use super::run_test; use crate::{ force_collect, test::Harness, Ephemeron, Finalize, Gc, GcBox, GcRefCell, Trace, WeakGc, @@ -65,18 +67,22 @@ fn eph_allocation_chains() { let weak = WeakGc::new(&cloned_gc); let wrap = Gc::new(weak); - assert_eq!(*wrap.upgrade().expect("weak is live"), "foo"); + assert_eq!(wrap.upgrade().as_deref().map(String::as_str), Some("foo")); let eph = Ephemeron::new(&wrap, 3); drop(cloned_gc); force_collect(); - - assert_eq!(eph.value().expect("weak is still live"), 3); + assert_eq!(wrap.upgrade().as_deref().map(String::as_str), Some("foo")); + assert_eq!(eph.value(), Some(3)); drop(gc_value); force_collect(); + assert!(wrap.upgrade().is_none()); + assert_eq!(eph.value(), Some(3)); + drop(wrap); + force_collect(); assert!(eph.value().is_none()); } }); @@ -91,7 +97,7 @@ fn eph_basic_alloc_dump_test() { let eph = Ephemeron::new(&gc_value, 4); let _fourth = Gc::new("tail"); - assert_eq!(eph.value().expect("must be live"), 4); + assert_eq!(eph.value(), Some(4)); }); } @@ -218,3 +224,36 @@ fn eph_self_referential_chain() { Harness::assert_exact_bytes_allocated(root_size); }); } + +#[test] +fn eph_finalizer() { + #[derive(Clone, Trace)] + struct S { + #[unsafe_ignore_trace] + inner: Rc>, + } + + impl Finalize for S { + fn finalize(&self) { + self.inner.set(true); + } + } + + run_test(|| { + let val = S { + inner: Rc::new(Cell::new(false)), + }; + + let key = Gc::new(50u32); + let eph = Ephemeron::new(&key, Gc::new(val.clone())); + assert!(eph.has_value()); + // finalize hasn't been run + assert!(!val.inner.get()); + + drop(key); + force_collect(); + assert!(!eph.has_value()); + // finalize ran when collecting + assert!(val.inner.get()); + }); +}