Adjust CLI executor to handle new finalization jobs

jedel1043 · jedel1043 · commit ccf30215ebb4 · 2026-03-16T19:31:10.000-06:00
diff --git a/cli/src/executor.rs b/cli/src/executor.rs
@@ -20,6 +20,7 @@ pub(crate) struct Executor {
     async_jobs: RefCell<VecDeque<NativeAsyncJob>>,
     timeout_jobs: RefCell<BTreeMap<JsInstant, Vec<TimeoutJob>>>,
     generic_jobs: RefCell<VecDeque<GenericJob>>,
+    finalization_registry_jobs: RefCell<VecDeque<NativeAsyncJob>>,
 
     printer: SharedExternalPrinterLogger,
 }
@@ -31,6 +32,7 @@ impl Executor {
             async_jobs: RefCell::default(),
             timeout_jobs: RefCell::default(),
             generic_jobs: RefCell::default(),
+            finalization_registry_jobs: RefCell::default(),
             printer,
         }
     }
@@ -96,6 +98,9 @@ impl JobExecutor for Executor {
                     .push(job);
             }
             Job::GenericJob(job) => self.generic_jobs.borrow_mut().push_back(job),
+            Job::FinalizationRegistryCleanupJob(job) => {
+                self.finalization_registry_jobs.borrow_mut().push_back(job);
+            }
             job => self.printer.print(format!("unsupported job type {job:?}")),
         }
     }
@@ -106,12 +111,17 @@ impl JobExecutor for Executor {
 
     async fn run_jobs_async(self: Rc<Self>, context: &RefCell<&mut Context>) -> JsResult<()> {
         let mut group = FutureGroup::new();
+        let mut fr_group = FutureGroup::new();
 
         loop {
             for job in mem::take(&mut *self.async_jobs.borrow_mut()) {
                 group.insert(job.call(context));
             }
 
+            for job in mem::take(&mut *self.finalization_registry_jobs.borrow_mut()) {
+                fr_group.insert(job.call(context));
+            }
+
             if let Some(Err(e)) = future::poll_once(group.next()).await.flatten() {
                 self.printer.print(uncaught_job_error(&e));
             }
@@ -120,7 +130,16 @@ impl JobExecutor for Executor {
             // event loop is cancelled almost immediately after the channel with
             // the reader gets closed.
             if self.is_empty() && group.is_empty() {
-                return Ok(());
+                // Run finalizers with a lower priority than every other type of
+                // job.
+                if let Some(Err(e)) = future::poll_once(fr_group.next()).await.flatten() {
+                    self.printer.print(uncaught_job_error(&e));
+                }
+
+                // Finalizers could enqueue new jobs, so we cannot just exit.
+                if self.is_empty() {
+                    return Ok(());
+                }
             }
 
             {
diff --git a/cli/src/main.rs b/cli/src/main.rs
@@ -17,7 +17,7 @@ use crate::logger::SharedExternalPrinterLogger;
 use async_channel::Sender;
 use boa_engine::JsValue;
 use boa_engine::error::JsErasedError;
-use boa_engine::job::{JobExecutor, NativeAsyncJob};
+use boa_engine::job::NativeAsyncJob;
 use boa_engine::{
     Context, JsError, Source,
     builtins::promise::PromiseState,
@@ -35,9 +35,7 @@ use color_eyre::{
 };
 use colored::Colorize;
 use debug::init_boa_debug_object;
-use futures_lite::future;
 use rustyline::{EditMode, Editor, config::Config, error::ReadlineError};
-use std::cell::RefCell;
 use std::time::{Duration, Instant};
 use std::{
     fs::OpenOptions,
@@ -671,8 +669,7 @@ fn main() -> Result<()> {
     });
     context.enqueue_job(eval_loop.into());
 
-    let result = future::block_on(executor.run_jobs_async(&RefCell::new(context)))
-        .map_err(|e| e.into_erased(context));
+    let result = context.run_jobs().map_err(|e| e.into_erased(context));
 
     handle.join().expect("failed to join thread");
 
diff --git a/core/engine/src/builtins/finalization_registry/tests.rs b/core/engine/src/builtins/finalization_registry/tests.rs
@@ -1,21 +1,89 @@
-use indoc::indoc;
+mod miri {
 
-use crate::{TestAction, run_test_actions};
+    use indoc::indoc;
 
-#[test]
-fn finalization_registry_simple() {
-    run_test_actions([
-        TestAction::run(indoc! {r#"
+    use crate::{TestAction, run_test_actions};
+
+    #[test]
+    fn finalization_registry_simple() {
+        run_test_actions([
+            TestAction::run(indoc! {r#"
             let counter = 0;
             const registry = new FinalizationRegistry(() => {
                 counter++;
             });
 
             registry.register(["foo"]);
         "#}),
-        TestAction::assert_eq("counter", 0),
-        TestAction::inspect_context(|_| boa_gc::force_collect()),
-        TestAction::inspect_context(|ctx| ctx.run_jobs().unwrap()),
-        TestAction::assert_eq("counter", 1),
-    ]);
+            TestAction::assert_eq("counter", 0),
+            TestAction::inspect_context(|_| boa_gc::force_collect()),
+            TestAction::inspect_context(|ctx| ctx.run_jobs().unwrap()),
+            // Callback should run at least once
+            TestAction::assert_eq("counter", 1),
+        ]);
+    }
+
+    #[test]
+    fn finalization_registry_unregister() {
+        run_test_actions([
+            TestAction::run(indoc! {r#"
+            let counter = 0;
+            const registry = new FinalizationRegistry(() => {
+                counter++;
+            });
+
+            {
+                let array = ["foo"];
+                registry.register(array, undefined, array);
+                registry.unregister(array);
+            }
+
+        "#}),
+            TestAction::assert_eq("counter", 0),
+            TestAction::inspect_context(|_| boa_gc::force_collect()),
+            TestAction::inspect_context(|ctx| ctx.run_jobs().unwrap()),
+            // Callback shouldn't run
+            TestAction::assert_eq("counter", 0),
+        ]);
+    }
+
+    #[test]
+    fn finalization_registry_held_value_handover() {
+        run_test_actions([
+            TestAction::run(indoc! {r#"
+            let counter = 0;
+            const registry = new FinalizationRegistry((value) => {
+                counter += value.increment;
+            });
+
+            registry.register(["foo"], { increment: 5 });
+        "#}),
+            TestAction::assert_eq("counter", 0),
+            TestAction::inspect_context(|_| boa_gc::force_collect()),
+            TestAction::inspect_context(|ctx| ctx.run_jobs().unwrap()),
+            // Registry should handover the held value as argument
+            TestAction::assert_eq("counter", 5),
+        ]);
+    }
+
+    #[test]
+    fn finalization_registry_unrelated_unregister_token() {
+        run_test_actions([
+            TestAction::run(indoc! {r#"
+            let counter = 0;
+
+            const registry = new FinalizationRegistry((value) => {
+                counter += 1;
+            });
+
+            registry.register(["foo"], undefined, {});
+            registry.unregister({});
+        "#}),
+            TestAction::assert_eq("counter", 0),
+            TestAction::inspect_context(|_| boa_gc::force_collect()),
+            TestAction::inspect_context(|ctx| ctx.run_jobs().unwrap()),
+            // Object should not have been unregistered if the token is not the correct one
+            TestAction::assert_eq("counter", 1),
+        ]);
+    }
 }
diff --git a/core/engine/src/job.rs b/core/engine/src/job.rs
@@ -547,17 +547,17 @@ pub enum Job {
     ///
     /// As described on the [spec's section about execution][execution],
     ///
-    /// > Because calling HostEnqueueFinalizationRegistryCleanupJob is optional,
-    /// > registered objects in a FinalizationRegistry do not necessarily hold
-    /// > that FinalizationRegistry live. Implementations may omit FinalizationRegistry
-    /// > callbacks for any reason, e.g., if the FinalizationRegistry itself becomes
+    /// > Because calling `HostEnqueueFinalizationRegistryCleanupJob` is optional,
+    /// > registered objects in a `FinalizationRegistry` do not necessarily hold
+    /// > that `FinalizationRegistry` live. Implementations may omit `FinalizationRegistry`
+    /// > callbacks for any reason, e.g., if the `FinalizationRegistry` itself becomes
     /// > dead, or if the application is shutting down.
     ///
     /// For this reason, it is recommended to exclude `FinalizationRegistry` cleanup
-    /// jobs from any condition that returns from [`JobExecutor::run_jobs`].
+    /// jobs from any condition that exits from [`JobExecutor::run_jobs`].
     ///
-    /// By the same token, it is recommended to execute [`FinalizationRegistryCleanubJob`]
-    /// separately from all other enqueued [`NativeAsyncJob`]s, prioritizing the
+    /// By the same token, it is recommended to execute `FinalizationRegistry` cleanup
+    /// jobs separately from all other enqueued [`NativeAsyncJob`]s, prioritizing the
     /// execution of all other jobs if possible.
     ///
     /// [spec]: https://tc39.es/ecma262/#sec-weakref-host-hooks

Original file line number	Diff line number	Diff line change
`@@ -20,6 +20,7 @@ pub(crate) struct Executor {`
`20`	`20`	`async_jobs: RefCell<VecDeque<NativeAsyncJob>>,`
`21`	`21`	`timeout_jobs: RefCell<BTreeMap<JsInstant, Vec<TimeoutJob>>>,`
`22`	`22`	`generic_jobs: RefCell<VecDeque<GenericJob>>,`
	`23`	`+ finalization_registry_jobs: RefCell<VecDeque<NativeAsyncJob>>,`
`23`	`24`
`24`	`25`	`printer: SharedExternalPrinterLogger,`
`25`	`26`	`}`
`@@ -31,6 +32,7 @@ impl Executor {`
`31`	`32`	`async_jobs: RefCell::default(),`
`32`	`33`	`timeout_jobs: RefCell::default(),`
`33`	`34`	`generic_jobs: RefCell::default(),`
	`35`	`+ finalization_registry_jobs: RefCell::default(),`
`34`	`36`	`printer,`
`35`	`37`	`}`
`36`	`38`	`}`
`@@ -96,6 +98,9 @@ impl JobExecutor for Executor {`
`96`	`98`	`.push(job);`
`97`	`99`	`}`
`98`	`100`	`Job::GenericJob(job) => self.generic_jobs.borrow_mut().push_back(job),`
	`101`	`+ Job::FinalizationRegistryCleanupJob(job) => {`
	`102`	`+ self.finalization_registry_jobs.borrow_mut().push_back(job);`
	`103`	`+ }`
`99`	`104`	`job => self.printer.print(format!("unsupported job type {job:?}")),`
`100`	`105`	`}`
`101`	`106`	`}`
`@@ -106,12 +111,17 @@ impl JobExecutor for Executor {`
`106`	`111`
`107`	`112`	`async fn run_jobs_async(self: Rc<Self>, context: &RefCell<&mut Context>) -> JsResult<()> {`
`108`	`113`	`let mut group = FutureGroup::new();`
	`114`	`+ let mut fr_group = FutureGroup::new();`
`109`	`115`
`110`	`116`	`loop {`
`111`	`117`	`for job in mem::take(&mut *self.async_jobs.borrow_mut()) {`
`112`	`118`	`group.insert(job.call(context));`
`113`	`119`	`}`
`114`	`120`
	`121`	`+ for job in mem::take(&mut *self.finalization_registry_jobs.borrow_mut()) {`
	`122`	`+ fr_group.insert(job.call(context));`
	`123`	`+ }`
	`124`	`+`
`115`	`125`	`if let Some(Err(e)) = future::poll_once(group.next()).await.flatten() {`
`116`	`126`	`self.printer.print(uncaught_job_error(&e));`
`117`	`127`	`}`
`@@ -120,7 +130,16 @@ impl JobExecutor for Executor {`
`120`	`130`	`// event loop is cancelled almost immediately after the channel with`
`121`	`131`	`// the reader gets closed.`
`122`	`132`	`if self.is_empty() && group.is_empty() {`
`123`		`- return Ok(());`
	`133`	`+ // Run finalizers with a lower priority than every other type of`
	`134`	`+ // job.`
	`135`	`+ if let Some(Err(e)) = future::poll_once(fr_group.next()).await.flatten() {`
	`136`	`+ self.printer.print(uncaught_job_error(&e));`
	`137`	`+ }`
	`138`	`+`
	`139`	`+ // Finalizers could enqueue new jobs, so we cannot just exit.`
	`140`	`+ if self.is_empty() {`
	`141`	`+ return Ok(());`
	`142`	`+ }`
`124`	`143`	`}`
`125`	`144`
`126`	`145`	`{`