Implement FinalizationRegistry pt. 3

jedel1043 · jedel1043 · commit 51a739ebe88d · 2026-03-15T11:41:24.000-06:00
diff --git a/core/engine/src/builtins/finalization_registry/mod.rs b/core/engine/src/builtins/finalization_registry/mod.rs
@@ -25,13 +25,15 @@ use super::{BuiltInConstructor, BuiltInObject, IntrinsicObject, builder::BuiltIn
 
 /// On GG collection, sends a message to a [`FinalizationRegistry`] indicating that it needs to
 /// be collected.
-#[derive(Trace, Clone)]
+#[derive(Trace)]
 #[boa_gc(unsafe_empty_trace)]
-struct CleanupSignaler(async_channel::WeakSender<()>);
+struct CleanupSignaler(Cell<Option<async_channel::WeakSender<()>>>);
 
 impl Finalize for CleanupSignaler {
     fn finalize(&self) {
-        if let Some(sender) = self.0.upgrade() {
+        if let Some(sender) = self.0.take()
+            && let Some(sender) = sender.upgrade()
+        {
             // We don't need to handle errors:
             // - If the channel is full, the `FinalizationRegistry` has already
             //   been enqueued for cleanup.
@@ -52,7 +54,7 @@ pub(crate) struct RegistryCell {
 
 /// Boa's implementation of ECMAScript's [`FinalizationRegistry`] builtin object.
 ///
-/// FinalizationRegistry provides a way to request that a cleanup callback get called at some point
+/// `FinalizationRegistry` provides a way to request that a cleanup callback get called at some point
 /// when a value registered with the registry has been reclaimed (garbage-collected).
 ///
 /// [`FinalizationRegistry`]: https://tc39.es/ecma262/#sec-finalization-registry-objects
@@ -178,7 +180,7 @@ impl BuiltInConstructor for FinalizationRegistry {
                         async move |context| inner_cleanup(weak_registry, receiver, context).await,
                     )));
 
-                result.map(|_| JsValue::undefined())
+                result.map(|()| JsValue::undefined())
             }
 
             context.enqueue_job(Job::FinalizationRegistryCleanupJob(NativeAsyncJob::new(
@@ -249,7 +251,9 @@ impl FinalizationRegistry {
         let cell = RegistryCell {
             target: Ephemeron::new(
                 target_obj.inner(),
-                CleanupSignaler(registry.cleanup_notifier.clone().downgrade()),
+                CleanupSignaler(Cell::new(Some(
+                    registry.cleanup_notifier.clone().downgrade(),
+                ))),
             ),
             held_value: held_value.clone(),
             unregister_token,
@@ -302,14 +306,18 @@ impl FinalizationRegistry {
             // a. If cell.[[UnregisterToken]] is not empty and SameValue(cell.[[UnregisterToken]], unregisterToken) is true, then
             if let Some(tok) = cell.unregister_token.as_ref()
                 && let Some(tok) = tok.upgrade()
-                && Gc::ptr_eq(&tok, &unregister_token)
+                && Gc::ptr_eq(&tok, unregister_token)
             {
                 // i. Remove cell from finalizationRegistry.[[Cells]].
                 let cell = registry.cells.swap_remove(i);
-                if let Some(value) = cell.target.value() {
-                    // Remove the inner signaler to avoid notifying a registry that doesn't
-                    // have dead entries.
-                    value.0.take();
+                let _key = cell.target.key();
+                // TODO: it might be better to add a special ref for the value that
+                // also preserves the original key instead.
+                // SAFETY: the original key is alive per our previous call to `key`,
+                // so if this returns `Some`, then the value cannot be collected
+                // until `key` gets dropped.
+                unsafe {
+                    cell.target.value_ref().and_then(|v| v.0.take());
                 }
 
                 // ii. Set removed to true.
@@ -353,7 +361,9 @@ impl FinalizationRegistry {
                 break Ok(());
             }
             // 3. While finalizationRegistry.[[Cells]] contains a Record cell such that cell.[[WeakRefTarget]] is empty, an implementation may perform the following steps:
-            if !obj.borrow().data().cells[i].target.has_value() {
+            if obj.borrow().data().cells[i].target.has_value() {
+                i += 1;
+            } else {
                 // a. Choose any such cell.
                 // b. Remove cell from finalizationRegistry.[[Cells]].
                 let cell = obj.borrow_mut().data_mut().cells.swap_remove(i);
@@ -368,8 +378,6 @@ impl FinalizationRegistry {
                 if let Err(err) = result {
                     break Err(err);
                 }
-            } else {
-                i += 1;
             }
         };
 
diff --git a/core/engine/src/job.rs b/core/engine/src/job.rs
@@ -535,23 +535,23 @@ pub enum Job {
     ///
     /// See [`GenericJob`] for more information.
     GenericJob(GenericJob),
-    /// A job that will eventually cleanup a FinalizationRegistry.
+    /// A job that will eventually cleanup a `FinalizationRegistry`.
     ///
     /// This job differs slightly from the [spec]; originally it's defined
-    /// as being enqueued exactly when a FinalizationRegistry needs to call
+    /// as being enqueued exactly when a `FinalizationRegistry` needs to call
     /// `FinalizationRegistry::cleanup`, but here it's defined as an async
     /// job that suspends execution until it receives a signal from the engine
-    /// that the FinalizationRegistry needs to be cleaned up.
+    /// that the `FinalizationRegistry` needs to be cleaned up.
     ///
     /// # Execution
     ///
     /// 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
-    ///   dead, or if the application is shutting down.
+    /// > 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`].
@@ -657,6 +657,7 @@ impl JobExecutor for IdleJobExecutor {
 pub struct SimpleJobExecutor {
     promise_jobs: RefCell<VecDeque<PromiseJob>>,
     async_jobs: RefCell<VecDeque<NativeAsyncJob>>,
+    finalization_registry_jobs: RefCell<VecDeque<NativeAsyncJob>>,
     timeout_jobs: RefCell<BTreeMap<JsInstant, Vec<TimeoutJob>>>,
     generic_jobs: RefCell<VecDeque<GenericJob>>,
     stop: Arc<AtomicBool>,
@@ -714,6 +715,9 @@ impl JobExecutor for SimpleJobExecutor {
                     .push(t);
             }
             Job::GenericJob(g) => self.generic_jobs.borrow_mut().push_back(g),
+            Job::FinalizationRegistryCleanupJob(fr) => {
+                self.finalization_registry_jobs.borrow_mut().push_back(fr);
+            }
         }
     }
 
@@ -726,6 +730,7 @@ impl JobExecutor for SimpleJobExecutor {
         Self: Sized,
     {
         let mut group = FutureGroup::new();
+        let mut fr_group = FutureGroup::new();
         loop {
             if self.stop.load(Ordering::Relaxed) {
                 self.stop.store(false, Ordering::Relaxed);
@@ -737,6 +742,10 @@ impl JobExecutor for SimpleJobExecutor {
                 group.insert(job.call(context));
             }
 
+            for job in mem::take(&mut *self.finalization_registry_jobs.borrow_mut()) {
+                fr_group.insert(job.call(context));
+            }
+
             // Dispatch all past-due timeout jobs before the termination check.
             {
                 let now = context.borrow().clock().now();
@@ -763,7 +772,14 @@ impl JobExecutor for SimpleJobExecutor {
             }
 
             if self.is_empty() && group.is_empty() {
-                break;
+                match future::poll_once(fr_group.next()).await.flatten() {
+                    Some(Err(err)) => {
+                        self.clear();
+                        return Err(err);
+                    }
+                    _ if !self.is_empty() => {}
+                    _ => break,
+                }
             }
 
             if let Some(Err(err)) = future::poll_once(group.next()).await.flatten() {
diff --git a/core/gc/src/pointers/ephemeron.rs b/core/gc/src/pointers/ephemeron.rs
@@ -22,19 +22,7 @@ pub struct Ephemeron<K: Trace + ?Sized + 'static, V: Trace + 'static> {
     inner_ptr: NonNull<EphemeronBox<K, V>>,
 }
 
-
-impl<K: Trace + ?Sized, V: Trace + Clone> Ephemeron<K, V> {
-    /// Gets the stored value of this `Ephemeron`, or `None` if the key was already garbage collected.
-    ///
-    /// This needs to return a clone of the value because holding a reference to it between
-    /// garbage collection passes could drop the underlying allocation, causing an Use After Free.
-    #[must_use]
-    pub fn value(&self) -> Option<V> {
-        // SAFETY: this is safe because `Ephemeron` is tracked to always point to a valid pointer
-        // `inner_ptr`.
-        unsafe { self.inner_ptr.as_ref().value().cloned() }
-    }
-
+impl<K: Trace + ?Sized, V: Trace> Ephemeron<K, V> {
     /// Gets the stored key of this `Ephemeron`, or `None` if the key was already garbage collected.
     #[inline]
     #[must_use]
@@ -52,6 +40,20 @@ impl<K: Trace + ?Sized, V: Trace + Clone> Ephemeron<K, V> {
         Some(unsafe { Gc::from_raw(key_ptr) })
     }
 
+    /// Gets a reference to the stored value of this `Ephemeron`, or `None` if the
+    /// key was already garbage collected.
+    ///
+    /// # Safety
+    ///
+    /// The key must be kept alive while this reference is active. Otherwise,
+    /// the reference may point to deallocated memory if the key gets collected.
+    #[must_use]
+    pub unsafe fn value_ref(&self) -> Option<&V> {
+        // SAFETY: this is safe because `Ephemeron` is tracked to always point to a valid pointer
+        // `inner_ptr`.
+        unsafe { self.inner_ptr.as_ref().value() }
+    }
+
     /// Checks if the [`Ephemeron`] has a value.
     #[must_use]
     pub fn has_value(&self) -> bool {
@@ -61,6 +63,19 @@ impl<K: Trace + ?Sized, V: Trace + Clone> Ephemeron<K, V> {
     }
 }
 
+impl<K: Trace + ?Sized, V: Trace + Clone> Ephemeron<K, V> {
+    /// Gets the stored value of this `Ephemeron`, or `None` if the key was already garbage collected.
+    ///
+    /// This needs to return a clone of the value because holding a reference to it between
+    /// garbage collection passes could drop the underlying allocation, causing an Use After Free.
+    #[must_use]
+    pub fn value(&self) -> Option<V> {
+        // SAFETY: this is safe because `Ephemeron` is tracked to always point to a valid pointer
+        // `inner_ptr`.
+        unsafe { self.inner_ptr.as_ref().value().cloned() }
+    }
+}
+
 impl<K: Trace + ?Sized, V: Trace> Ephemeron<K, V> {
     /// Creates a new `Ephemeron`.
     #[must_use]
diff --git a/test262_config.toml b/test262_config.toml
@@ -7,7 +7,6 @@ flags = []
 features = [
     ### Unimplemented features:
 
-    "FinalizationRegistry",
     "symbols-as-weakmap-keys",
     "Intl.DisplayNames",
     "Intl.RelativeTimeFormat",
@@ -88,5 +87,5 @@ tests = [
     "test/intl402/Locale/constructor-options-canonicalized.js",
     # TODO: Remove this once regress fixes the named groups parsing issue.
     "test/built-ins/RegExp/named-groups/non-unicode-property-names-valid.js",
-    
+
 ]
