From e9e4813449a623708bee5dfbc282767293b534be Mon Sep 17 00:00:00 2001 From: Ralph Kuepper Date: Mon, 24 Aug 2026 19:30:09 +0200 Subject: [PATCH] fix(windows): keep WinUI callbacks behind scanned keys Resolve widget, lifecycle, and timer callback pointers from GC-scanned storage at invocation time so Reactor closures cannot retain stale addresses after an evacuating collection.\n\nFixes #8734 --- .../8734-winui-callback-indirection.md | 3 + crates/perry-ui-windows-winui/src/app.rs | 104 +++++--- crates/perry-ui-windows-winui/src/widgets.rs | 233 ++++++++++++++---- 3 files changed, 258 insertions(+), 82 deletions(-) create mode 100644 changelog.d/8734-winui-callback-indirection.md diff --git a/changelog.d/8734-winui-callback-indirection.md b/changelog.d/8734-winui-callback-indirection.md new file mode 100644 index 0000000000..06e36a9f89 --- /dev/null +++ b/changelog.d/8734-winui-callback-indirection.md @@ -0,0 +1,3 @@ +### Fixed + +- Fixed WinUI widget, application-exit, and timer callbacks becoming stale after an evacuating garbage collection. Windows Reactor closures now retain stable widget/slot keys and resolve callbacks from GC-scanned storage immediately before invocation; timer callbacks likewise remain in a scanned key-indexed table instead of being copied into opaque `DispatcherTimer` closures. diff --git a/crates/perry-ui-windows-winui/src/app.rs b/crates/perry-ui-windows-winui/src/app.rs index b022c72009..49963528d3 100644 --- a/crates/perry-ui-windows-winui/src/app.rs +++ b/crates/perry-ui-windows-winui/src/app.rs @@ -33,6 +33,7 @@ thread_local! { static APPS: RefCell> = const { RefCell::new(Vec::new()) }; static ON_ACTIVATE: RefCell> = const { RefCell::new(None) }; static ON_TERMINATE: RefCell> = const { RefCell::new(None) }; + static TIMER_CALLBACKS: RefCell> = const { RefCell::new(Vec::new()) }; static PENDING_TIMERS: RefCell> = const { RefCell::new(Vec::new()) }; static ACTIVE_TIMERS: RefCell> = const { RefCell::new(Vec::new()) }; static RUNTIME_PUMP_STARTED: RefCell = const { RefCell::new(false) }; @@ -45,14 +46,11 @@ thread_local! { /// `widgets::NODES`, not an address), two optional size pairs and a /// `PresenterKind` — no JS value, so it is not a GC root. /// -/// KNOWN RESIDUAL (not fixable by scanning): `start_runtime_pump` drains -/// `PENDING_TIMERS` into `DispatcherTimer` closures that own a COPY of the raw -/// pointer, and `app_run` moves `ON_TERMINATE` into an `on_exit` closure the -/// same way. Those copies live inside boxed Rust closures owned by Windows -/// Reactor, where no scanner can reach or rewrite them, so an evacuating -/// collection would leave them stale. Making the closures re-read a scanned -/// slot (the indirection `perry-ui-macos` gets from its handle-keyed callback -/// maps) is the real fix and is a follow-up, not a relocation. +/// The lifecycle slots and `TIMER_CALLBACKS` are the sole owners of raw +/// callback pointers. Reactor closures capture stable keys and re-read these +/// scanned slots at invocation time, so an evacuating collection's rewritten +/// address is always observed. `PENDING_TIMERS` holds only durations and +/// indices into `TIMER_CALLBACKS`. pub(crate) fn scan_winui_app_gc_roots(visitor: &mut perry_ffi::GcRootVisitor<'_>) { for slot in [&ON_ACTIVATE, &ON_TERMINATE] { slot.with(|slot| { @@ -63,8 +61,8 @@ pub(crate) fn scan_winui_app_gc_roots(visitor: &mut perry_ffi::GcRootVisitor<'_> } }); } - PENDING_TIMERS.with(|timers| { - for (_, callback) in timers.borrow_mut().iter_mut() { + TIMER_CALLBACKS.with(|callbacks| { + for callback in callbacks.borrow_mut().iter_mut() { if *callback != 0 { visitor.visit_usize_slot(callback); } @@ -80,6 +78,32 @@ fn closure_ptr(value: f64) -> usize { unsafe { js_nanbox_get_pointer(value) as usize } } +fn app_callback(slot: &'static std::thread::LocalKey>>) -> usize { + slot.with(|slot| slot.borrow().unwrap_or(0)) +} + +fn invoke_app_callback(slot: &'static std::thread::LocalKey>>) { + let callback = app_callback(slot); + if callback != 0 { + unsafe { + js_closure_call0(callback as *const u8); + } + } +} + +fn timer_callback(key: usize) -> usize { + TIMER_CALLBACKS.with(|callbacks| callbacks.borrow().get(key).copied().unwrap_or(0)) +} + +fn invoke_timer_callback(key: usize) { + let callback = timer_callback(key); + if callback != 0 { + unsafe { + js_closure_call0(callback as *const u8); + } + } +} + fn with_app_mut(handle: i64, f: impl FnOnce(&mut AppState)) { APPS.with(|apps| { if let Some(app) = apps.borrow_mut().get_mut(handle.saturating_sub(1) as usize) { @@ -130,11 +154,7 @@ pub fn app_run(app_handle: i64) { }; crate::widgets::set_root(state.root); - if let Some(callback) = ON_ACTIVATE.with(|slot| *slot.borrow()) { - unsafe { - js_closure_call0(callback as *const u8); - } - } + invoke_app_callback(&ON_ACTIVATE); let constraints = InnerConstraints { min_width: state.min_size.map(|v| v.0), @@ -148,10 +168,8 @@ pub fn app_run(app_handle: i64) { .inner_constraints(constraints) .presenter(state.presenter) .backdrop(Backdrop::Mica); - if let Some(callback) = ON_TERMINATE.with(|slot| slot.borrow_mut().take()) { - app = app.on_exit(move || unsafe { - js_closure_call0(callback as *const u8); - }); + if app_callback(&ON_TERMINATE) != 0 { + app = app.on_exit(move || invoke_app_callback(&ON_TERMINATE)); } if let Err(error) = app.render(crate::widgets::render_root) { eprintln!("[perry-winui] application failed: {error}"); @@ -220,10 +238,15 @@ pub fn set_timer(interval_ms: f64, callback: f64) { perry_ui_windows::app::set_timer(interval_ms, callback); return; } + let callback_key = TIMER_CALLBACKS.with(|callbacks| { + let mut callbacks = callbacks.borrow_mut(); + callbacks.push(closure_ptr(callback)); + callbacks.len() - 1 + }); PENDING_TIMERS.with(|timers| { timers.borrow_mut().push(( Duration::from_secs_f64((interval_ms.max(1.0)) / 1000.0), - closure_ptr(callback), + callback_key, )); }); } @@ -251,15 +274,14 @@ pub(crate) fn start_runtime_pump() { }) { active.push(timer); } - PENDING_TIMERS.with(|pending| { - for (interval, callback) in pending.borrow_mut().drain(..) { - if let Ok(timer) = DispatcherTimer::new(interval, move || unsafe { - js_closure_call0(callback as *const u8); - }) { - active.push(timer); - } + let pending = PENDING_TIMERS.with(|pending| std::mem::take(&mut *pending.borrow_mut())); + for (interval, callback_key) in pending { + if let Ok(timer) = + DispatcherTimer::new(interval, move || invoke_timer_callback(callback_key)) + { + active.push(timer); } - }); + } }); } @@ -328,3 +350,29 @@ pub fn app_set_activation_policy(app_handle: i64, value_ptr: *const u8) { perry_ui_windows::app::app_set_activation_policy(app_handle, value_ptr); } } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn reactor_callback_keys_observe_rewritten_app_slots() { + ON_ACTIVATE.with(|slot| *slot.borrow_mut() = Some(0x101)); + ON_TERMINATE.with(|slot| *slot.borrow_mut() = Some(0x111)); + assert_eq!(app_callback(&ON_ACTIVATE), 0x101); + assert_eq!(app_callback(&ON_TERMINATE), 0x111); + ON_ACTIVATE.with(|slot| *slot.borrow_mut() = Some(0x202)); + ON_TERMINATE.with(|slot| *slot.borrow_mut() = Some(0x222)); + assert_eq!(app_callback(&ON_ACTIVATE), 0x202); + assert_eq!(app_callback(&ON_TERMINATE), 0x222); + + let key = TIMER_CALLBACKS.with(|callbacks| { + let mut callbacks = callbacks.borrow_mut(); + callbacks.push(0x333); + callbacks.len() - 1 + }); + assert_eq!(timer_callback(key), 0x333); + TIMER_CALLBACKS.with(|callbacks| callbacks.borrow_mut()[key] = 0x444); + assert_eq!(timer_callback(key), 0x444); + } +} diff --git a/crates/perry-ui-windows-winui/src/widgets.rs b/crates/perry-ui-windows-winui/src/widgets.rs index b075ee69b5..a12669f6ed 100644 --- a/crates/perry-ui-windows-winui/src/widgets.rs +++ b/crates/perry-ui-windows-winui/src/widgets.rs @@ -147,14 +147,10 @@ thread_local! { /// `js_nanbox_get_pointer`), so each stored slot is a GC root that an /// evacuating collection must rewrite. /// -/// KNOWN RESIDUAL (not fixable by scanning): `render_handle` works on a CLONE -/// of the node and captures the unboxed pointer by value into the `move` -/// closures it hands to Windows Reactor (`fluent_button(..).on_click(move || -/// invoke0(selected))`, `apply_common`'s `on_tapped`, and the per-widget -/// handlers). Those captured copies live inside boxed Rust closures owned by -/// the element tree, which no scanner can reach or rewrite. Re-reading the -/// scanned `NODES` slot at invoke time — the indirection `perry-ui-macos` gets -/// from its handle-keyed callback maps — is the real fix and is a follow-up. +/// Reactor callbacks capture only a widget handle and `CallbackSlot`, then +/// re-read the corresponding slot below at invocation time. The raw pointer +/// therefore remains in this scanned table instead of escaping into a boxed +/// Rust closure that the collector cannot visit or rewrite. pub(crate) fn scan_winui_widgets_gc_roots(visitor: &mut perry_ffi::GcRootVisitor<'_>) { NODES.with(|nodes| { for node in nodes.borrow_mut().iter_mut() { @@ -200,11 +196,55 @@ fn callback_ptr(value: f64) -> usize { unsafe { js_nanbox_get_pointer(value) as usize } } +#[derive(Clone, Copy, Debug, Eq, PartialEq)] +enum CallbackSlot { + CommonOnClick, + ButtonClick, + TextFieldChange, + SecureFieldChange, + ToggleChange, + SliderChange, +} + fn read_string(ptr: *const u8) -> String { unsafe { perry_ffi::copy_string_from_raw(ptr) }.to_owned() } -fn invoke0(callback: usize) { +fn node_callback(handle: i64, slot: CallbackSlot) -> usize { + NODES.with(|nodes| { + let nodes = nodes.borrow(); + let Some(node) = nodes.get(handle.saturating_sub(1) as usize) else { + return 0; + }; + match slot { + CallbackSlot::CommonOnClick => node.common.on_click, + CallbackSlot::ButtonClick if node.common.on_click != 0 => node.common.on_click, + CallbackSlot::ButtonClick => match &node.kind { + NodeKind::Button { callback, .. } => *callback, + _ => 0, + }, + CallbackSlot::TextFieldChange => match &node.kind { + NodeKind::TextField { callback, .. } => *callback, + _ => 0, + }, + CallbackSlot::SecureFieldChange => match &node.kind { + NodeKind::SecureField { callback, .. } => *callback, + _ => 0, + }, + CallbackSlot::ToggleChange => match &node.kind { + NodeKind::Toggle { callback, .. } => *callback, + _ => 0, + }, + CallbackSlot::SliderChange => match &node.kind { + NodeKind::Slider { callback, .. } => *callback, + _ => 0, + }, + } + }) +} + +fn invoke_node0(handle: i64, slot: CallbackSlot) { + let callback = node_callback(handle, slot); if callback != 0 { unsafe { js_closure_call0(callback as *const u8); @@ -212,7 +252,8 @@ fn invoke0(callback: usize) { } } -fn invoke1(callback: usize, value: f64) { +fn invoke_node1(handle: i64, slot: CallbackSlot, value: f64) { + let callback = node_callback(handle, slot); if callback != 0 { unsafe { js_closure_call1(callback as *const u8, value); @@ -220,14 +261,16 @@ fn invoke1(callback: usize, value: f64) { } } -fn invoke_string(callback: usize, value: &str) { - if callback == 0 { - return; - } +fn invoke_node_string(handle: i64, slot: CallbackSlot, value: &str) { unsafe { let string = js_string_from_bytes(value.as_ptr(), value.len() as u32); let boxed = js_nanbox_string(string as i64); - js_closure_call1(callback as *const u8, boxed); + // String allocation may collect. Resolve the callback only afterwards + // so even that collection's rewritten slot is observed. + let callback = node_callback(handle, slot); + if callback != 0 { + js_closure_call1(callback as *const u8, boxed); + } } } @@ -307,7 +350,7 @@ pub(crate) fn render_root(cx: &mut RenderCx) -> Element { .into() } -fn apply_common(modifiers: &mut Modifiers, common: &Common) { +fn apply_common(modifiers: &mut Modifiers, handle: i64, common: &Common) { modifiers.width = common.width; modifiers.height = common.height; modifiers.opacity = common.opacity; @@ -329,11 +372,12 @@ fn apply_common(modifiers: &mut Modifiers, common: &Common) { }); } if common.on_click != 0 { - let callback = common.on_click; - modifiers + let handlers = modifiers .pointer_handlers - .get_or_insert_with(Default::default) - .on_tapped = Some(Callback::new(move |()| invoke0(callback))); + .get_or_insert_with(Default::default); + handlers.on_tapped = Some(Callback::new(move |()| { + invoke_node0(handle, CallbackSlot::CommonOnClick) + })); } } @@ -373,27 +417,23 @@ fn render_handle(handle: i64) -> Element { view.font_size = *font_size; view.font_weight = *font_weight; view.modifiers.font_family = font_family.clone(); - apply_common(&mut view.modifiers, &node.common); + apply_common(&mut view.modifiers, handle, &node.common); view.into() } NodeKind::Button { label, - callback, + callback: _, bordered, } => { - let selected = if node.common.on_click != 0 { - node.common.on_click - } else { - *callback - }; - let mut view = fluent_button(label.clone()).on_click(move || invoke0(selected)); + let mut view = fluent_button(label.clone()) + .on_click(move || invoke_node0(handle, CallbackSlot::ButtonClick)); view.style = if *bordered { ButtonStyle::Default } else { ButtonStyle::Subtle }; view.is_enabled = node.common.enabled; - apply_common(&mut view.modifiers, &node.common); + apply_common(&mut view.modifiers, handle, &node.common); // `Button::on_click` already owns the generic callback. view.modifiers.pointer_handlers = None; view.into() @@ -401,26 +441,26 @@ fn render_handle(handle: i64) -> Element { NodeKind::VStack { spacing } | NodeKind::LazyVStack { spacing } => { let children = render_children(&node.common.children, node.common.detaches_hidden); let mut view = fluent_vstack(children).spacing(*spacing); - apply_common(&mut view.modifiers, &node.common); + apply_common(&mut view.modifiers, handle, &node.common); view.into() } NodeKind::HStack { spacing } => { let children = render_children(&node.common.children, node.common.detaches_hidden); let mut view = fluent_hstack(children).spacing(*spacing); - apply_common(&mut view.modifiers, &node.common); + apply_common(&mut view.modifiers, handle, &node.common); view.into() } NodeKind::ZStack => { let children = render_children(&node.common.children, node.common.detaches_hidden); let mut view = grid(children); - apply_common(&mut view.modifiers, &node.common); + apply_common(&mut view.modifiers, handle, &node.common); view.into() } NodeKind::Spacer => { let mut view = Border::default(); view.modifiers.min_width = Some(8.0); view.modifiers.min_height = Some(8.0); - apply_common(&mut view.modifiers, &node.common); + apply_common(&mut view.modifiers, handle, &node.common); view.into() } NodeKind::Divider => { @@ -428,76 +468,76 @@ fn render_handle(handle: i64) -> Element { view.modifiers.height = Some(1.0); view.modifiers.horizontal_alignment = Some(HorizontalAlignment::Stretch); view.modifiers.background = Some(Brush::Solid(Color::rgb(128, 128, 128))); - apply_common(&mut view.modifiers, &node.common); + apply_common(&mut view.modifiers, handle, &node.common); view.into() } NodeKind::TextField { value, placeholder, - callback, + callback: _, borderless: _, font_size, } => { - let cb = *callback; let mut view = TextBox::new(value.clone()) .placeholder(placeholder.clone()) .on_changed(move |value: String| { set_textfield_value(handle, value.clone()); - invoke_string(cb, &value); + invoke_node_string(handle, CallbackSlot::TextFieldChange, &value); }); view.is_enabled = node.common.enabled; view.modifiers.font_size = *font_size; - apply_common(&mut view.modifiers, &node.common); + apply_common(&mut view.modifiers, handle, &node.common); view.into() } NodeKind::SecureField { value, placeholder, - callback, + callback: _, } => { - let cb = *callback; let mut view = PasswordBox::new() .value(value.clone()) .placeholder(placeholder.clone()) .on_changed(move |value: String| { set_securefield_value(handle, value.clone()); - invoke_string(cb, &value); + invoke_node_string(handle, CallbackSlot::SecureFieldChange, &value); }); view.is_enabled = node.common.enabled; - apply_common(&mut view.modifiers, &node.common); + apply_common(&mut view.modifiers, handle, &node.common); view.into() } NodeKind::Toggle { label, on, - callback, + callback: _, } => { - let cb = *callback; let mut view = ToggleSwitch::new(*on) .header(label.clone()) .on_changed(move |value| { set_toggle_value(handle, value); - invoke1(cb, if value { 1.0 } else { 0.0 }); + invoke_node1( + handle, + CallbackSlot::ToggleChange, + if value { 1.0 } else { 0.0 }, + ); }); view.is_enabled = node.common.enabled; - apply_common(&mut view.modifiers, &node.common); + apply_common(&mut view.modifiers, handle, &node.common); view.into() } NodeKind::Slider { min, max, value, - callback, + callback: _, } => { - let cb = *callback; let mut view = Slider::new(*value) .range(*min, *max) .on_changed(move |value| { set_slider_value(handle, value); - invoke1(cb, value); + invoke_node1(handle, CallbackSlot::SliderChange, value); }); view.is_enabled = node.common.enabled; - apply_common(&mut view.modifiers, &node.common); + apply_common(&mut view.modifiers, handle, &node.common); view.into() } NodeKind::ScrollView { .. } => { @@ -509,13 +549,13 @@ fn render_handle(handle: i64) -> Element { .map(render_handle) .unwrap_or(Element::Empty); let mut view = ScrollViewer::new(child); - apply_common(&mut view.modifiers, &node.common); + apply_common(&mut view.modifiers, handle, &node.common); view.into() } NodeKind::Form => { let children = render_children(&node.common.children, node.common.detaches_hidden); let mut view = fluent_vstack(children).spacing(12.0); - apply_common(&mut view.modifiers, &node.common); + apply_common(&mut view.modifiers, handle, &node.common); view.into() } NodeKind::Section { title } => { @@ -525,7 +565,7 @@ fn render_handle(handle: i64) -> Element { node.common.detaches_hidden, )); let mut view = fluent_vstack(children).spacing(8.0); - apply_common(&mut view.modifiers, &node.common); + apply_common(&mut view.modifiers, handle, &node.common); view.into() } NodeKind::Progress { value } => { @@ -534,7 +574,7 @@ fn render_handle(handle: i64) -> Element { } else { ProgressBar::new(value.clamp(0.0, 1.0)).range(0.0, 1.0) }; - apply_common(&mut view.modifiers, &node.common); + apply_common(&mut view.modifiers, handle, &node.common); view.into() } }; @@ -1404,4 +1444,89 @@ mod tests { assert_eq!(node.common.children, vec![first, second]); assert_eq!(render_handle(parent).kind_name(), "StackPanel"); } + + #[test] + fn reactor_callback_keys_observe_rewritten_node_slots() { + let handle = register(NodeKind::Button { + label: "move-safe".into(), + callback: 0x111, + bordered: true, + }); + + assert_eq!(node_callback(handle, CallbackSlot::ButtonClick), 0x111); + with_node_mut(handle, |node| { + if let NodeKind::Button { callback, .. } = &mut node.kind { + *callback = 0x222; + } + }); + assert_eq!(node_callback(handle, CallbackSlot::ButtonClick), 0x222); + + // The generic on-click slot has precedence for Button and is itself + // resolved from the live node rather than copied into Reactor. + with_node_mut(handle, |node| node.common.on_click = 0x333); + assert_eq!(node_callback(handle, CallbackSlot::ButtonClick), 0x333); + assert_eq!(node_callback(handle, CallbackSlot::CommonOnClick), 0x333); + + let textfield = register(NodeKind::TextField { + value: String::new(), + placeholder: String::new(), + callback: 0x444, + borderless: false, + font_size: None, + }); + let securefield = register(NodeKind::SecureField { + value: String::new(), + placeholder: String::new(), + callback: 0x555, + }); + let toggle = register(NodeKind::Toggle { + label: String::new(), + on: false, + callback: 0x666, + }); + let slider = register(NodeKind::Slider { + min: 0.0, + max: 1.0, + value: 0.5, + callback: 0x777, + }); + assert_eq!( + node_callback(textfield, CallbackSlot::TextFieldChange), + 0x444 + ); + assert_eq!( + node_callback(securefield, CallbackSlot::SecureFieldChange), + 0x555 + ); + assert_eq!(node_callback(toggle, CallbackSlot::ToggleChange), 0x666); + assert_eq!(node_callback(slider, CallbackSlot::SliderChange), 0x777); + + for (handle, rewritten) in [ + (textfield, 0x844), + (securefield, 0x855), + (toggle, 0x866), + (slider, 0x877), + ] { + with_node_mut(handle, |node| { + let callback = match &mut node.kind { + NodeKind::TextField { callback, .. } + | NodeKind::SecureField { callback, .. } + | NodeKind::Toggle { callback, .. } + | NodeKind::Slider { callback, .. } => callback, + _ => unreachable!(), + }; + *callback = rewritten; + }); + } + assert_eq!( + node_callback(textfield, CallbackSlot::TextFieldChange), + 0x844 + ); + assert_eq!( + node_callback(securefield, CallbackSlot::SecureFieldChange), + 0x855 + ); + assert_eq!(node_callback(toggle, CallbackSlot::ToggleChange), 0x866); + assert_eq!(node_callback(slider, CallbackSlot::SliderChange), 0x877); + } }