From c460930ed2a630bb37237e6c446b924986c5ae85 Mon Sep 17 00:00:00 2001 From: Andreu Botella Date: Tue, 20 Sep 2022 05:27:22 +0200 Subject: [PATCH] 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 #1066. --- src/context.rs | 4 ++-- src/handle.rs | 53 ++++++++++++++++++++++++++++++++++++++------------ src/isolate.rs | 8 ++++++++ tests/slots.rs | 28 ++++++++++++++++++++++++++ 4 files changed, 79 insertions(+), 14 deletions(-) diff --git a/src/context.rs b/src/context.rs index 4140df602d..57f535e703 100644 --- a/src/context.rs +++ b/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 diff --git a/src/handle.rs b/src/handle.rs index 4eec98dbbe..7953de2a97 100644 --- a/src/handle.rs +++ b/src/handle.rs @@ -546,7 +546,22 @@ impl Weak { ) -> 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, + finalizer: Box, + ) -> 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 Weak { &self, finalizer: Box, ) -> 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>, + finalizer: Box, ) -> Self { + self.clone_raw(Some(FinalizerCallback::Guaranteed(finalizer))) + } + + fn clone_raw(&self, finalizer: Option) -> Self { if let Some(data) = self.get_pointer() { // SAFETY: We're in the isolate's thread, because Weak isn't Send or // Sync. @@ -769,7 +788,7 @@ impl Weak { let ptr = v8__WeakCallbackInfo__GetParameter(wci); &*(ptr as *mut WeakData) }; - let finalizer: Option> = { + let finalizer: Option = { let finalizer_id = weak_data.finalizer_id.unwrap(); isolate.get_finalizer_map_mut().map.remove(&finalizer_id) }; @@ -782,8 +801,10 @@ impl Weak { }; } - 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), + Guaranteed(Box), +} + #[derive(Default)] pub(crate) struct FinalizerMap { - map: std::collections::HashMap>, + map: std::collections::HashMap, next_id: FinalizerId, } impl FinalizerMap { - pub(crate) fn add( - &mut self, - finalizer: Box, - ) -> 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 + '_ { + self.map.drain().map(|(_, finalizer)| finalizer) + } } diff --git a/src/isolate.rs b/src/isolate.rs index 2f92a6996d..bd70fe43cc 100644 --- a/src/isolate.rs +++ b/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 reference count. Arc::from_raw(annex); self.set_data(0, null_mut()); diff --git a/tests/slots.rs b/tests/slots.rs index 30489eb8f8..9ebf56d443 100644 --- a/tests/slots.rs +++ b/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>); + 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();