Feat/gc drop finalizer parity by Flamki · Pull Request #43 · boa-dev/oscars

Flamki · 2026-03-09T12:00:42Z

This PR tightens mark-sweep teardown/finalization semantics with minimal scope.

What changed

Added a typed-erased finalizer hook to the GC vtable (FinalizeFn) so GcBox<T> finalizers can be dispatched correctly from erased pointers.
Updated collector teardown (sweep_all_queues) to run finalize_fn before drop_fn for:
- root_queue
- pending_root_queue
- ephemeron_queue
- pending_ephemeron_queue
Updated dead-root sweep path to use the new vtable finalizer dispatch.
Updated ephemeron finalizer hook to use Trace::run_finalizer(...).
Added regression tests:
- collector_drop_runs_finalizers_for_live_gc_values
- collector_drop_runs_ephemeron_value_finalizers_for_live_values

Why

Previous teardown behavior reliably dropped live tracked allocations, but finalizer execution for erased GC values was incomplete/inconsistent. This patch keeps the PR focused to mark-sweep internals and guarantees finalizers are run during collector teardown where safe.

Scope

mark-sweep internals + tests only
no public API changes
no allocator policy/threshold changes

Validation

cargo fmt --all -- --check
cargo test --workspace
cargo clippy --workspace --all-features --all-targets
cargo clippy --workspace --no-default-features --all-targets
cargo doc -v --document-private-items --all-features

jedel1043 · 2026-03-11T07:29:38Z

oscars/src/collectors/mark_sweep/mod.rs

+            unsafe { gc_box.finalize_fn()(node) };
+            unsafe { gc_box.drop_fn()(node) };


This is incorrect, and possibly unsound. You cannot finalize objects then drop them immediately after, because their finalize implementation could revive those objects to make them reachable again. If you immediately drop them after finalizing, you would have a reference to unallocated memory.

GC usually happens in phases to ensure soundness:
mark -> finalize -> mark again -> drop

Flamki · 2026-03-11T10:12:02Z

Thanks for the review — fixed.

I rebased onto latest main and updated teardown sweeping to avoid finalize-then-immediate-drop unsoundness.

What changed in sweep_all_queues():

Phase 1: finalize all tracked roots/ephemerons.
Phase 2: re-check rootedness on root queues.
Phase 3: only if nothing was revived, run drop + free_slot.

So if finalization revives any root, teardown now avoids immediate drop/free of those values.

Validation rerun locally:

cargo fmt --all -- --check
cargo test --workspace
cargo clippy --workspace --all-features --all-targets
cargo clippy --workspace --no-default-features --all-targets
cargo doc -v --document-private-items --all-features

jedel1043 · 2026-03-11T15:46:02Z

oscars/src/collectors/mark_sweep/mod.rs

+
+        // Phase 2: if finalization revived any roots, do not drop/free now.
+        // Dropping immediately after revival would be unsound.
+        let revived_rooted = roots


This is still invalid because this doesn't run a marking phase, so the check for rootedness check is just returning roots again. See https://github.com/boa-dev/boa/blob/a49348586d90b790936dc6ab651da6aff3f60e67/core/gc/src/lib.rs#L226-L277 for a correct implementation, and note how mark_heap needs to run twice.

Flamki · 2026-03-11T19:05:48Z

Thanks for the pointer — agreed the previous rootedness re-check was not enough.

I updated teardown to follow the phase order from your reference: mark -> finalize -> mark again -> drop/free. In sweep_all_queues(), we now:

run an initial mark pass,
finalize only currently unreachable nodes,
run a second mark pass to account for resurrection,
drop/free only nodes still unreachable after that second mark.

I also added a regression test (collector_drop_does_not_drop_values_revived_by_finalizer) for resurrection during collector drop, and reran local validation (fmt, workspace tests, clippy, docs, and miri for oscars).

jedel1043 · 2026-03-12T02:56:08Z

oscars/src/collectors/mark_sweep/mod.rs

    // arena3 uses`free_slot` calls to reclaim memory.
    // arena2 uses a bitmap (`mark_dropped`) and reclaims automatically
-    fn sweep_all_queues(&self) {
+    fn sweep_all_queues(&self) -> bool {


I just realized this only runs when the collector itself gets dropped, so it doesn't require a two phase mark & drop, it just requires that we run all finalizers first (since finalizers could reference other objects in the heap), then all drops after that.

Flamki · 2026-03-12T14:47:26Z

Thanks for the clarification, that makes sense.

I updated the latest commit to keep collector-drop teardown simple: it now runs finalizers for all tracked values first, then does drop/free for all tracked values. I removed the extra mark/reachability logic from this path and also resolved the tests.rs conflict on the branch.

I reran the full local checks (fmt, workspace tests, both clippy configs, docs, and miri for oscars), all passing.

jedel1043 · 2026-03-12T17:06:19Z

LGTM. Pinging @nekevss to do the final review

nekevss · 2026-03-12T17:38:50Z

oscars/src/collectors/mark_sweep/mod.rs

+            if map.is_alive() {
+                true
+            } else {
+                unsafe {


nit: safety comment

nekevss · 2026-03-12T17:42:19Z

oscars/src/collectors/mark_sweep/mod.rs

+                    let _ = rust_alloc::boxed::Box::from_raw(map_ptr.as_ptr());
+                }
+                false
+            }


suggestion: maybe clean up this section a bit

Suggested change

}

let is_map_alive = map.is_alive();

if !is_map_alive {

unsafe {

let _ = rust_alloc::boxed::Box::from_raw(map_ptr.as_ptr());

}

}

is_map_alive

nekevss · 2026-03-12T17:50:18Z

oscars/src/collectors/mark_sweep/mod.rs

+                true
+            } else {
+                unsafe {
+                    let _ = rust_alloc::boxed::Box::from_raw(map_ptr.as_ptr());


nit: can you please add a note that we reclaim the weak_map with rust_alloc, because it's allocated with rust_alloc and not alloc.

Ideally we eventually have that value better tied to alloc, and not rust_alloc, but in the meantime, we should really note it better, because it's going to be easy to lose track and get confused if we don't

nekevss · 2026-03-12T17:50:38Z

oscars/src/collectors/mark_sweep/tests.rs

 }
+
+#[test]
+fn collector_drop_runs_destructors_for_live_gc_values() {


praise: nice tests! Thanks for adding them

Flamki force-pushed the feat/gc-drop-finalizer-parity branch from 7cabb2f to 503ee7d Compare March 9, 2026 12:05

jedel1043 requested changes Mar 11, 2026

View reviewed changes

Flamki force-pushed the feat/gc-drop-finalizer-parity branch from 503ee7d to b236f85 Compare March 11, 2026 10:11

jedel1043 reviewed Mar 11, 2026

View reviewed changes

jedel1043 reviewed Mar 12, 2026

View reviewed changes

Flamki added 5 commits March 12, 2026 16:51

mark_sweep: run destructors/finalizers on collector drop

2b56d3c

mark_sweep: avoid dropping revived values at teardown

fc89c45

mark_sweep: defer weak map reclaim until safe teardown

332485e

mark_sweep: use mark-finalize-mark-drop at teardown

2e24123

mark_sweep: simplify collector-drop teardown phases

5ca8967

Flamki force-pushed the feat/gc-drop-finalizer-parity branch from 26703be to 5ca8967 Compare March 12, 2026 14:45

tests: make gc_alloc_vec_survives_collection miri-stable

2452a85

jedel1043 requested a review from nekevss March 12, 2026 17:06

jedel1043 approved these changes Mar 12, 2026

View reviewed changes

nekevss requested changes Mar 12, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat/gc drop finalizer parity#43

Feat/gc drop finalizer parity#43
Flamki wants to merge 6 commits intoboa-dev:mainfrom
Flamki:feat/gc-drop-finalizer-parity

Flamki commented Mar 9, 2026

Uh oh!

jedel1043 Mar 11, 2026 •

edited

Loading

Uh oh!

jedel1043 Mar 11, 2026

Uh oh!

Flamki commented Mar 11, 2026

Uh oh!

jedel1043 Mar 11, 2026

Uh oh!

Flamki commented Mar 11, 2026

Uh oh!

jedel1043 Mar 12, 2026

Uh oh!

Flamki commented Mar 12, 2026

Uh oh!

jedel1043 commented Mar 12, 2026

Uh oh!

nekevss Mar 12, 2026

Uh oh!

nekevss Mar 12, 2026

Uh oh!

nekevss Mar 12, 2026

Uh oh!

nekevss Mar 12, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

		unsafe { gc_box.finalize_fn()(node) };
		unsafe { gc_box.drop_fn()(node) };

-            }
+            let is_map_alive = map.is_alive();
+            if !is_map_alive {
+                unsafe {
+                    let _ = rust_alloc::boxed::Box::from_raw(map_ptr.as_ptr());
+                }
+            }
+            is_map_alive

Conversation

Flamki commented Mar 9, 2026

What changed

Why

Scope

Validation

Uh oh!

jedel1043 Mar 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jedel1043 Mar 11, 2026

Choose a reason for hiding this comment

Uh oh!

Flamki commented Mar 11, 2026

Uh oh!

jedel1043 Mar 11, 2026

Choose a reason for hiding this comment

Uh oh!

Flamki commented Mar 11, 2026

Uh oh!

jedel1043 Mar 12, 2026

Choose a reason for hiding this comment

Uh oh!

Flamki commented Mar 12, 2026

Uh oh!

jedel1043 commented Mar 12, 2026

Uh oh!

nekevss Mar 12, 2026

Choose a reason for hiding this comment

Uh oh!

nekevss Mar 12, 2026

Choose a reason for hiding this comment

Uh oh!

nekevss Mar 12, 2026

Choose a reason for hiding this comment

Uh oh!

nekevss Mar 12, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jedel1043 Mar 11, 2026 •

edited

Loading