diff --git a/src/context.rs b/src/context.rs index d086e25609..37b65e25a9 100644 --- a/src/context.rs +++ b/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 diff --git a/src/handle.rs b/src/handle.rs index 48ebdda9c3..3435366920 100644 --- a/src/handle.rs +++ b/src/handle.rs @@ -558,7 +558,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)) } @@ -608,13 +623,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. @@ -781,7 +800,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) }; @@ -794,8 +813,10 @@ impl Weak { }; } - 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), + 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; @@ -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 + '_ { + self.map.drain().map(|(_, finalizer)| finalizer) + } } diff --git a/src/isolate.rs b/src/isolate.rs index d43e3b225f..3b7de262ee 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; @@ -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 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(); diff --git a/tests/test_api.rs b/tests/test_api.rs index a525f31fd3..f6382e32e8 100644 --- a/tests/test_api.rs +++ b/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>, + Rc>, + )>(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); }