feat: Guaranteed finalizers

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.

Closes denoland#1066.

src/context.rs

@@ -174,10 +174,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

@@ -546,7 +546,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))
   }
 
@@ -596,13 +611,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.
@@ -769,7 +788,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)
     };
@@ -782,8 +801,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 => {}
     }
   }
 }
@@ -895,17 +916,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;
@@ -916,4 +939,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;
@@ -849,6 +850,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();