feat: Guaranteed finalizers (#1076)

Currently, when a finalizer callback is registered, it is not
guaranteed to be called if there is a global reference to the
corresponding object that survives the isolate. This is because the
finalizer callback takes a `&mut Isolate`, and so it must be called
before the isolate is fully destroyed, but all existing globals
(including possibly the one being currently finalized) are still
usable while there still exists a mutable reference to the isolate.

However, there are still use cases for having finalizers that are
guaranteed to run regardless of any remaining globals, but that don't
require any interaction with the isolate. This change adds them.

This change also changes the context annex to use a guaranteed
finalizer, fixing a bug with context slots not being freed if there
were any globals to the context at the time the isolate is dropped.

src/context.rs

@@ -180,10 +180,10 @@ impl Context {
       // We also check above that `isolate` is the context's isolate.
       let self_ref_handle = unsafe { UnsafeRefHandle::new(self, isolate) };
 
-      Weak::with_finalizer(
+      Weak::with_guaranteed_finalizer(
         isolate,
         self_ref_handle,
-        Box::new(move |_| {
+        Box::new(move || {
           // SAFETY: The lifetimes of references to the annex returned by this
           // method are always tied to the context, and because this is the
           // context's finalizer, we know there are no living references to

src/handle.rs

@@ -558,7 +558,22 @@ impl<T> Weak<T> {
   ) -> Self {
     let HandleInfo { data, host } = handle.get_handle_info();
     host.assert_match_isolate(isolate);
-    let finalizer_id = isolate.get_finalizer_map_mut().add(finalizer);
+    let finalizer_id = isolate
+      .get_finalizer_map_mut()
+      .add(FinalizerCallback::Regular(finalizer));
+    Self::new_raw(isolate, data, Some(finalizer_id))
+  }
+
+  pub fn with_guaranteed_finalizer(
+    isolate: &mut Isolate,
+    handle: impl Handle<Data = T>,
+    finalizer: Box<dyn FnOnce()>,
+  ) -> Self {
+    let HandleInfo { data, host } = handle.get_handle_info();
+    host.assert_match_isolate(isolate);
+    let finalizer_id = isolate
+      .get_finalizer_map_mut()
+      .add(FinalizerCallback::Guaranteed(finalizer));
     Self::new_raw(isolate, data, Some(finalizer_id))
   }
 
@@ -608,13 +623,17 @@ impl<T> Weak<T> {
     &self,
     finalizer: Box<dyn FnOnce(&mut Isolate)>,
   ) -> Self {
-    self.clone_raw(Some(finalizer))
+    self.clone_raw(Some(FinalizerCallback::Regular(finalizer)))
   }
 
-  fn clone_raw(
+  pub fn clone_with_guaranteed_finalizer(
     &self,
-    finalizer: Option<Box<dyn FnOnce(&mut Isolate)>>,
+    finalizer: Box<dyn FnOnce()>,
   ) -> Self {
+    self.clone_raw(Some(FinalizerCallback::Guaranteed(finalizer)))
+  }
+
+  fn clone_raw(&self, finalizer: Option<FinalizerCallback>) -> Self {
     if let Some(data) = self.get_pointer() {
       // SAFETY: We're in the isolate's thread, because Weak<T> isn't Send or
       // Sync.
@@ -781,7 +800,7 @@ impl<T> Weak<T> {
       let ptr = v8__WeakCallbackInfo__GetParameter(wci);
       &*(ptr as *mut WeakData<T>)
     };
-    let finalizer: Option<Box<dyn FnOnce(&mut Isolate)>> = {
+    let finalizer: Option<FinalizerCallback> = {
       let finalizer_id = weak_data.finalizer_id.unwrap();
       isolate.get_finalizer_map_mut().map.remove(&finalizer_id)
     };
@@ -794,8 +813,10 @@ impl<T> Weak<T> {
       };
     }
 
-    if let Some(finalizer) = finalizer {
-      finalizer(isolate);
+    match finalizer {
+      Some(FinalizerCallback::Regular(finalizer)) => finalizer(isolate),
+      Some(FinalizerCallback::Guaranteed(finalizer)) => finalizer(),
+      None => {}
     }
   }
 }
@@ -907,17 +928,19 @@ struct WeakCallbackInfo(Opaque);
 
 type FinalizerId = usize;
 
+pub(crate) enum FinalizerCallback {
+  Regular(Box<dyn FnOnce(&mut Isolate)>),
+  Guaranteed(Box<dyn FnOnce()>),
+}
+
 #[derive(Default)]
 pub(crate) struct FinalizerMap {
-  map: std::collections::HashMap<FinalizerId, Box<dyn FnOnce(&mut Isolate)>>,
+  map: std::collections::HashMap<FinalizerId, FinalizerCallback>,
   next_id: FinalizerId,
 }
 
 impl FinalizerMap {
-  pub(crate) fn add(
-    &mut self,
-    finalizer: Box<dyn FnOnce(&mut Isolate)>,
-  ) -> FinalizerId {
+  fn add(&mut self, finalizer: FinalizerCallback) -> FinalizerId {
     let id = self.next_id;
     // TODO: Overflow.
     self.next_id += 1;
@@ -928,4 +951,10 @@ impl FinalizerMap {
   pub(crate) fn is_empty(&self) -> bool {
     self.map.is_empty()
   }
+
+  pub(crate) fn drain(
+    &mut self,
+  ) -> impl Iterator<Item = FinalizerCallback> + '_ {
+    self.map.drain().map(|(_, finalizer)| finalizer)
+  }
 }

src/isolate.rs

@@ -1,6 +1,7 @@
 use crate::PromiseResolver;
 // Copyright 2019-2021 the Deno authors. All rights reserved. MIT license.
 use crate::function::FunctionCallbackInfo;
+use crate::handle::FinalizerCallback;
 use crate::handle::FinalizerMap;
 use crate::isolate_create_params::raw;
 use crate::isolate_create_params::CreateParams;
@@ -887,6 +888,13 @@ impl Isolate {
     annex.create_param_allocations = Box::new(());
     annex.slots.clear();
 
+    // Run through any remaining guaranteed finalizers.
+    for finalizer in annex.finalizer_map.drain() {
+      if let FinalizerCallback::Guaranteed(callback) = finalizer {
+        callback();
+      }
+    }
+
     // Subtract one from the Arc<IsolateAnnex> reference count.
     Arc::from_raw(annex);
     self.set_data(0, null_mut());

tests/slots.rs

@@ -294,6 +294,34 @@ fn dropped_context_slots() {
   assert!(dropped.get());
 }
 
+#[test]
+fn dropped_context_slots_on_kept_context() {
+  use std::cell::Cell;
+
+  struct DropMarker(Rc<Cell<bool>>);
+  impl Drop for DropMarker {
+    fn drop(&mut self) {
+      println!("Dropping the drop marker");
+      self.0.set(true);
+    }
+  }
+
+  let mut isolate = CoreIsolate::new(Default::default());
+  let dropped = Rc::new(Cell::new(false));
+  let _global_context;
+  {
+    let scope = &mut v8::HandleScope::new(isolate.deref_mut());
+    let context = v8::Context::new(scope);
+
+    context.set_slot(scope, DropMarker(dropped.clone()));
+
+    _global_context = v8::Global::new(scope, context);
+  }
+
+  drop(isolate);
+  assert!(dropped.get());
+}
+
 #[test]
 fn clear_all_context_slots() {
   setup();

tests/test_api.rs

@@ -7162,6 +7162,75 @@ fn finalizers() {
   assert!(finalizer_called.get());
 }
 
+#[test]
+fn guaranteed_finalizers() {
+  // Test that guaranteed finalizers behave the same as regular finalizers for
+  // everything except being guaranteed.
+
+  use std::cell::Cell;
+  use std::ops::Deref;
+  use std::rc::Rc;
+
+  let _setup_guard = setup();
+
+  let isolate = &mut v8::Isolate::new(Default::default());
+  let scope = &mut v8::HandleScope::new(isolate);
+  let context = v8::Context::new(scope);
+  let scope = &mut v8::ContextScope::new(scope, context);
+
+  // The finalizer for a dropped Weak is never called.
+  {
+    {
+      let scope = &mut v8::HandleScope::new(scope);
+      let local = v8::Object::new(scope);
+      let _ = v8::Weak::with_guaranteed_finalizer(
+        scope,
+        &local,
+        Box::new(|| unreachable!()),
+      );
+    }
+
+    let scope = &mut v8::HandleScope::new(scope);
+    eval(scope, "gc()").unwrap();
+  }
+
+  let finalizer_called = Rc::new(Cell::new(false));
+  let weak = {
+    let scope = &mut v8::HandleScope::new(scope);
+    let local = v8::Object::new(scope);
+
+    // We use a channel to send data into the finalizer without having to worry
+    // about lifetimes.
+    let (tx, rx) = std::sync::mpsc::sync_channel::<(
+      Rc<v8::Weak<v8::Object>>,
+      Rc<Cell<bool>>,
+    )>(1);
+
+    let weak = Rc::new(v8::Weak::with_guaranteed_finalizer(
+      scope,
+      &local,
+      Box::new(move || {
+        let (weak, finalizer_called) = rx.try_recv().unwrap();
+        finalizer_called.set(true);
+        assert!(weak.is_empty());
+      }),
+    ));
+
+    tx.send((weak.clone(), finalizer_called.clone())).unwrap();
+
+    assert!(!weak.is_empty());
+    assert_eq!(weak.deref(), &local);
+    assert_eq!(weak.to_local(scope), Some(local));
+
+    weak
+  };
+
+  let scope = &mut v8::HandleScope::new(scope);
+  eval(scope, "gc()").unwrap();
+  assert!(weak.is_empty());
+  assert!(finalizer_called.get());
+}
+
 #[test]
 fn weak_from_global() {
   let _setup_guard = setup();
@@ -7369,8 +7438,8 @@ fn finalizer_on_global_object() {
 
 #[test]
 fn finalizer_on_kept_global() {
-  // If a global is kept alive after an isolate is dropped, any finalizers won't
-  // be called.
+  // If a global is kept alive after an isolate is dropped, regular finalizers
+  // won't be called, but guaranteed ones will.
 
   use std::cell::Cell;
   use std::rc::Rc;
@@ -7378,8 +7447,10 @@ fn finalizer_on_kept_global() {
   let _setup_guard = setup();
 
   let global;
-  let weak;
-  let finalized = Rc::new(Cell::new(false));
+  let weak1;
+  let weak2;
+  let regular_finalized = Rc::new(Cell::new(false));
+  let guaranteed_finalized = Rc::new(Cell::new(false));
 
   {
     let isolate = &mut v8::Isolate::new(Default::default());
@@ -7389,19 +7460,30 @@ fn finalizer_on_kept_global() {
 
     let object = v8::Object::new(scope);
     global = v8::Global::new(scope, &object);
-    weak = v8::Weak::with_finalizer(
+    weak1 = v8::Weak::with_finalizer(
       scope,
       &object,
       Box::new({
-        let finalized = finalized.clone();
+        let finalized = regular_finalized.clone();
         move |_| finalized.set(true)
       }),
-    )
+    );
+    weak2 = v8::Weak::with_guaranteed_finalizer(
+      scope,
+      &object,
+      Box::new({
+        let guaranteed_finalized = guaranteed_finalized.clone();
+        move || guaranteed_finalized.set(true)
+      }),
+    );
   }
 
-  assert!(weak.is_empty());
-  assert!(!finalized.get());
-  drop(weak);
+  assert!(weak1.is_empty());
+  assert!(weak2.is_empty());
+  assert!(!regular_finalized.get());
+  assert!(guaranteed_finalized.get());
+  drop(weak1);
+  drop(weak2);
   drop(global);
 }