Mark header of rooted ephemerons when tracing (#3049)

* Mark header of rooted ephemerons when tracing

* Add additional assertions

* Use assert_eq instead of expect

* Apply review

* Add test for fix

boa_gc/src/internals/ephemeron_box.rs

@@ -204,7 +204,7 @@ impl<K: Trace + ?Sized, V: Trace> ErasedEphemeronBox for EphemeronBox<K, V> {
             // 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);
         }
     }
 }

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.

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!(wrap.upgrade().as_deref().map(String::as_str), Some("foo"));
-            assert_eq!(eph.value().expect("weak is still live"), 3);
+            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<Cell<bool>>,
+    }
+
+    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());
+    });
+}