From a5dbd3ee52f6b800d0afd252dc72a44df7b6f245 Mon Sep 17 00:00:00 2001 From: Mads Marquart Date: Wed, 28 Feb 2024 04:33:47 +0100 Subject: [PATCH] macOS: Refactor event handler storage (#3532) This makes our use of `unsafe` to make the event handler temporarily 'static be local to a module, in a way that's (hopefully) much easier to reason about. --- src/platform_impl/macos/app_delegate.rs | 158 +++++++---------------- src/platform_impl/macos/event_handler.rs | 136 +++++++++++++++++++ src/platform_impl/macos/event_loop.rs | 152 +++++----------------- src/platform_impl/macos/mod.rs | 1 + 4 files changed, 213 insertions(+), 234 deletions(-) create mode 100644 src/platform_impl/macos/event_handler.rs diff --git a/src/platform_impl/macos/app_delegate.rs b/src/platform_impl/macos/app_delegate.rs index bb7e14d2..a3358efd 100644 --- a/src/platform_impl/macos/app_delegate.rs +++ b/src/platform_impl/macos/app_delegate.rs @@ -1,7 +1,7 @@ use std::cell::{Cell, RefCell}; use std::collections::VecDeque; use std::mem; -use std::rc::{Rc, Weak}; +use std::rc::Weak; use std::sync::{Arc, Mutex}; use std::time::Instant; @@ -11,13 +11,14 @@ use objc2::rc::Id; use objc2::runtime::AnyObject; use objc2::{declare_class, msg_send_id, mutability, ClassType, DeclaredClass}; -use super::event_loop::{stop_app_immediately, PanicInfo}; +use super::event_handler::EventHandler; +use super::event_loop::{stop_app_immediately, ActiveEventLoop, PanicInfo}; use super::observer::{EventLoopWaker, RunLoop}; use super::window::WinitWindow; use super::{menu, WindowId, DEVICE_ID}; use crate::dpi::PhysicalSize; use crate::event::{DeviceEvent, Event, InnerSizeWriter, StartCause, WindowEvent}; -use crate::event_loop::{ActiveEventLoop as RootWindowTarget, ControlFlow}; +use crate::event_loop::{ActiveEventLoop as RootActiveEventLoop, ControlFlow}; use crate::window::WindowId as RootWindowId; #[derive(Debug, Default)] @@ -25,10 +26,7 @@ pub(super) struct State { activation_policy: NSApplicationActivationPolicy, default_menu: bool, activate_ignoring_other_apps: bool, - /// Whether the application is currently executing a callback. - in_callback: Cell, - /// The lifetime-erased callback. - callback: RefCell>, + event_handler: EventHandler, stop_on_launch: Cell, stop_before_wait: Cell, stop_after_wait: Cell, @@ -146,34 +144,14 @@ impl ApplicationDelegate { } } - /// Associate the application's event callback with the application delegate. - /// - /// # Safety - /// This is ignoring the lifetime of the application callback (which may not be 'static) - /// and can lead to undefined behaviour if the callback is not cleared before the end of - /// its real lifetime. - /// - /// All public APIs that take an event callback (`run`, `run_on_demand`, - /// `pump_events`) _must_ pair a call to `set_callback` with - /// a call to `clear_callback` before returning to avoid undefined behaviour. - #[allow(clippy::type_complexity)] - pub unsafe fn set_callback( + /// Place the event handler in the application delegate for the duration + /// of the given closure. + pub fn set_event_handler( &self, - callback: Weak, &RootWindowTarget)>>, - window_target: Rc, - ) { - *self.ivars().callback.borrow_mut() = Some(EventLoopHandler { - callback, - window_target, - }); - } - - pub fn clear_callback(&self) { - self.ivars().callback.borrow_mut().take(); - } - - fn have_callback(&self) -> bool { - self.ivars().callback.borrow().is_some() + handler: impl FnMut(Event, &RootActiveEventLoop), + closure: impl FnOnce() -> R, + ) -> R { + self.ivars().event_handler.set(handler, closure) } /// If `pump_events` is called to progress the event loop then we @@ -204,16 +182,13 @@ impl ApplicationDelegate { /// Note: that if the `NSApplication` has been launched then that state is preserved, /// and we won't need to re-launch the app if subsequent EventLoops are run. pub fn internal_exit(&self) { - self.set_in_callback(true); self.handle_event(Event::LoopExiting); - self.set_in_callback(false); self.set_is_running(false); self.set_stop_on_redraw(false); self.set_stop_before_wait(false); self.set_stop_after_wait(false); self.set_wait_timeout(None); - self.clear_callback(); } pub fn is_launched(&self) -> bool { @@ -240,10 +215,6 @@ impl ApplicationDelegate { self.ivars().exit.get() } - fn set_in_callback(&self, value: bool) { - self.ivars().in_callback.set(value) - } - pub fn set_control_flow(&self, value: ControlFlow) { self.ivars().control_flow.set(value) } @@ -285,13 +256,12 @@ impl ApplicationDelegate { pub fn handle_redraw(&self, window_id: WindowId) { let mtm = MainThreadMarker::from(self); // Redraw request might come out of order from the OS. - // -> Don't go back into the callback when our callstack originates from there - if !self.ivars().in_callback.get() { + // -> Don't go back into the event handler when our callstack originates from there + if !self.ivars().event_handler.in_use() { self.handle_event(Event::WindowEvent { window_id: RootWindowId(window_id), event: WindowEvent::RedrawRequested, }); - self.ivars().in_callback.set(false); // `pump_events` will request to stop immediately _after_ dispatching RedrawRequested events // as a way to ensure that `pump_events` can't block an external loop indefinitely @@ -311,19 +281,17 @@ impl ApplicationDelegate { } fn handle_event(&self, event: Event) { - if let Some(ref mut callback) = *self.ivars().callback.borrow_mut() { - callback.handle_event(event) - } + self.ivars() + .event_handler + .handle_event(event, &ActiveEventLoop::new_root(self.retain())) } /// dispatch `NewEvents(Init)` + `Resumed` pub fn dispatch_init_events(&self) { - self.set_in_callback(true); self.handle_event(Event::NewEvents(StartCause::Init)); // NB: For consistency all platforms must emit a 'resumed' event even though macOS // applications don't themselves have a formal suspend/resume lifecycle. self.handle_event(Event::Resumed); - self.set_in_callback(false); } // Called by RunLoopObserver after finishing waiting for new events @@ -333,12 +301,8 @@ impl ApplicationDelegate { .upgrade() .expect("The panic info must exist here. This failure indicates a developer error."); - // Return when in callback due to https://github.com/rust-windowing/winit/issues/1779 - if panic_info.is_panicking() - || self.ivars().in_callback.get() - || !self.have_callback() - || !self.is_running() - { + // Return when in event handler due to https://github.com/rust-windowing/winit/issues/1779 + if panic_info.is_panicking() || !self.ivars().event_handler.ready() || !self.is_running() { return; } @@ -369,9 +333,7 @@ impl ApplicationDelegate { } }; - self.set_in_callback(true); self.handle_event(Event::NewEvents(cause)); - self.set_in_callback(false); } // Called by RunLoopObserver before waiting for new events @@ -381,18 +343,13 @@ impl ApplicationDelegate { .upgrade() .expect("The panic info must exist here. This failure indicates a developer error."); - // Return when in callback due to https://github.com/rust-windowing/winit/issues/1779 - // XXX: how does it make sense that `in_callback()` can ever return `true` here if we're - // about to return to the `CFRunLoop` to poll for new events? - if panic_info.is_panicking() - || self.ivars().in_callback.get() - || !self.have_callback() - || !self.is_running() - { + // Return when in event handler due to https://github.com/rust-windowing/winit/issues/1779 + // XXX: how does it make sense that `event_handler.ready()` can ever return `false` here if + // we're about to return to the `CFRunLoop` to poll for new events? + if panic_info.is_panicking() || !self.ivars().event_handler.ready() || !self.is_running() { return; } - self.set_in_callback(true); self.handle_event(Event::UserEvent(HandlePendingUserEvents)); let events = mem::take(&mut *self.ivars().pending_events.borrow_mut()); @@ -415,32 +372,30 @@ impl ApplicationDelegate { suggested_size, scale_factor, } => { - if let Some(ref mut callback) = *self.ivars().callback.borrow_mut() { - let new_inner_size = Arc::new(Mutex::new(suggested_size)); - let scale_factor_changed_event = Event::WindowEvent { - window_id: RootWindowId(window.id()), - event: WindowEvent::ScaleFactorChanged { - scale_factor, - inner_size_writer: InnerSizeWriter::new(Arc::downgrade( - &new_inner_size, - )), - }, - }; + let new_inner_size = Arc::new(Mutex::new(suggested_size)); + let scale_factor_changed_event = Event::WindowEvent { + window_id: RootWindowId(window.id()), + event: WindowEvent::ScaleFactorChanged { + scale_factor, + inner_size_writer: InnerSizeWriter::new(Arc::downgrade( + &new_inner_size, + )), + }, + }; - callback.handle_event(scale_factor_changed_event); + self.handle_event(scale_factor_changed_event); - let physical_size = *new_inner_size.lock().unwrap(); - drop(new_inner_size); - let logical_size = physical_size.to_logical(scale_factor); - let size = NSSize::new(logical_size.width, logical_size.height); - window.setContentSize(size); + let physical_size = *new_inner_size.lock().unwrap(); + drop(new_inner_size); + let logical_size = physical_size.to_logical(scale_factor); + let size = NSSize::new(logical_size.width, logical_size.height); + window.setContentSize(size); - let resized_event = Event::WindowEvent { - window_id: RootWindowId(window.id()), - event: WindowEvent::Resized(physical_size), - }; - callback.handle_event(resized_event); - } + let resized_event = Event::WindowEvent { + window_id: RootWindowId(window.id()), + event: WindowEvent::Resized(physical_size), + }; + self.handle_event(resized_event); } } } @@ -454,7 +409,6 @@ impl ApplicationDelegate { } self.handle_event(Event::AboutToWait); - self.set_in_callback(false); if self.exiting() { let app = NSApplication::sharedApplication(mtm); @@ -493,30 +447,6 @@ pub(crate) enum QueuedEvent { #[derive(Debug)] pub(crate) struct HandlePendingUserEvents; -#[derive(Debug)] -struct EventLoopHandler { - #[allow(clippy::type_complexity)] - callback: Weak, &RootWindowTarget)>>, - window_target: Rc, -} - -impl EventLoopHandler { - fn handle_event(&mut self, event: Event) { - // `NSApplication` and our app delegate are global state and so it's possible - // that we could get a delegate callback after the application has exit an - // `EventLoop`. If the loop has been exit then our weak `self.callback` - // will fail to upgrade. - // - // We don't want to panic or output any verbose logging if we fail to - // upgrade the weak reference since it might be valid that the application - // re-starts the `NSApplication` after exiting a Winit `EventLoop` - if let Some(callback) = self.callback.upgrade() { - let mut callback = callback.borrow_mut(); - (callback)(event, &self.window_target); - } - } -} - /// Returns the minimum `Option`, taking into account that `None` /// equates to an infinite timeout, not a zero timeout (so can't just use /// `Option::min`) diff --git a/src/platform_impl/macos/event_handler.rs b/src/platform_impl/macos/event_handler.rs new file mode 100644 index 00000000..947682a7 --- /dev/null +++ b/src/platform_impl/macos/event_handler.rs @@ -0,0 +1,136 @@ +use std::cell::RefCell; +use std::fmt; +use std::mem; + +use super::app_delegate::HandlePendingUserEvents; +use crate::event::Event; +use crate::event_loop::ActiveEventLoop as RootActiveEventLoop; + +struct EventHandlerData { + #[allow(clippy::type_complexity)] + handler: Box, &RootActiveEventLoop) + 'static>, +} + +impl fmt::Debug for EventHandlerData { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + f.debug_struct("EventHandlerData").finish_non_exhaustive() + } +} + +#[derive(Debug, Default)] +pub(crate) struct EventHandler { + /// This can be in the following states: + /// - Not registered by the event loop (None). + /// - Present (Some(handler)). + /// - Currently executing the handler / in use (RefCell borrowed). + inner: RefCell>, +} + +impl EventHandler { + /// Set the event loop handler for the duration of the given closure. + /// + /// This is similar to using the `scoped-tls` or `scoped-tls-hkt` crates + /// to store the handler in a thread local, such that it can be accessed + /// from within the closure. + pub(crate) fn set<'handler, R>( + &self, + handler: impl FnMut(Event, &RootActiveEventLoop) + 'handler, + closure: impl FnOnce() -> R, + ) -> R { + // SAFETY: We extend the lifetime of the handler here so that we can + // store it in `EventHandler`'s `RefCell`. + // + // This is sound, since we make sure to unset the handler again at the + // end of this function, and as such the lifetime isn't actually + // extended beyond `'handler`. + let handler = unsafe { + mem::transmute::< + Box, &RootActiveEventLoop) + 'handler>, + Box, &RootActiveEventLoop) + 'static>, + >(Box::new(handler)) + }; + + match self.inner.try_borrow_mut().as_deref_mut() { + Ok(Some(_)) => { + unreachable!("tried to set handler while another was already set"); + } + Ok(data @ None) => { + *data = Some(EventHandlerData { handler }); + } + Err(_) => { + unreachable!("tried to set handler that is currently in use"); + } + } + + struct ClearOnDrop<'a>(&'a EventHandler); + + impl Drop for ClearOnDrop<'_> { + fn drop(&mut self) { + match self.0.inner.try_borrow_mut().as_deref_mut() { + Ok(data @ Some(_)) => { + *data = None; + } + Ok(None) => { + log::error!("tried to clear handler, but no handler was set"); + } + Err(_) => { + // Note: This is not expected to ever happen, this + // module generally controls the `RefCell`, and + // prevents it from ever being borrowed outside of it. + // + // But if it _does_ happen, it is a serious error, and + // we must abort the process, it'd be unsound if we + // weren't able to unset the handler. + eprintln!("tried to clear handler that is currently in use"); + std::process::abort(); + } + } + } + } + + let _clear_on_drop = ClearOnDrop(self); + + // Note: The RefCell should not be borrowed while executing the + // closure, that'd defeat the whole point. + closure() + + // `_clear_on_drop` will be dropped here, or when unwinding, ensuring + // soundness. + } + + pub(crate) fn in_use(&self) -> bool { + self.inner.try_borrow().is_err() + } + + pub(crate) fn ready(&self) -> bool { + matches!(self.inner.try_borrow().as_deref(), Ok(Some(_))) + } + + pub(crate) fn handle_event( + &self, + event: Event, + event_loop: &RootActiveEventLoop, + ) { + match self.inner.try_borrow_mut().as_deref_mut() { + Ok(Some(EventHandlerData { handler })) => { + // It is important that we keep the reference borrowed here, + // so that `in_use` can properly detect that the handler is + // still in use. + // + // If the handler unwinds, the `RefMut` will ensure that the + // handler is no longer borrowed. + (handler)(event, event_loop); + } + Ok(None) => { + // `NSApplication`, our app delegate and this handler are all + // global state and so it's not impossible that we could get + // an event after the application has exited the `EventLoop`. + log::error!("tried to run event handler, but no handler was set"); + } + Err(_) => { + // Prevent re-entrancy. + panic!("tried to handle event while another event is currently being handled"); + } + } + } +} diff --git a/src/platform_impl/macos/event_loop.rs b/src/platform_impl/macos/event_loop.rs index 984291a6..1e0e34ee 100644 --- a/src/platform_impl/macos/event_loop.rs +++ b/src/platform_impl/macos/event_loop.rs @@ -1,11 +1,10 @@ use std::{ any::Any, - cell::{Cell, RefCell}, + cell::Cell, collections::VecDeque, marker::PhantomData, - mem, os::raw::c_void, - panic::{catch_unwind, resume_unwind, AssertUnwindSafe, RefUnwindSafe, UnwindSafe}, + panic::{catch_unwind, resume_unwind, RefUnwindSafe, UnwindSafe}, ptr, rc::{Rc, Weak}, sync::mpsc, @@ -79,6 +78,15 @@ pub struct ActiveEventLoop { } impl ActiveEventLoop { + pub(super) fn new_root(delegate: Id) -> RootWindowTarget { + let mtm = MainThreadMarker::from(&*delegate); + let p = Self { delegate, mtm }; + RootWindowTarget { + p, + _marker: PhantomData, + } + } + pub fn create_custom_cursor(&self, source: CustomCursorSource) -> RootCustomCursor { RootCustomCursor { inner: CustomCursor::new(source.inner), @@ -188,17 +196,8 @@ pub struct EventLoop { sender: mpsc::Sender, receiver: Rc>, - window_target: Rc, + window_target: RootWindowTarget, panic_info: Rc, - - /// We make sure that the callback closure is dropped during a panic - /// by making the event loop own it. - /// - /// Every other reference should be a Weak reference which is only upgraded - /// into a strong reference in order to call the callback but then the - /// strong reference should be dropped as soon as possible. - #[allow(clippy::type_complexity)] - _callback: Option, &RootWindowTarget)>>>, } #[derive(Debug, Copy, Clone, PartialEq, Eq, Hash)] @@ -257,12 +256,11 @@ impl EventLoop { delegate: delegate.clone(), sender, receiver: Rc::new(receiver), - window_target: Rc::new(RootWindowTarget { + window_target: RootWindowTarget { p: ActiveEventLoop { delegate, mtm }, _marker: PhantomData, - }), + }, panic_info, - _callback: None, }) } @@ -270,56 +268,25 @@ impl EventLoop { &self.window_target } - pub fn run(mut self, callback: F) -> Result<(), EventLoopError> + pub fn run(mut self, handler: F) -> Result<(), EventLoopError> where F: FnMut(Event, &RootWindowTarget), { - self.run_on_demand(callback) + self.run_on_demand(handler) } // NB: we don't base this on `pump_events` because for `MacOs` we can't support // `pump_events` elegantly (we just ask to run the loop for a "short" amount of // time and so a layered implementation would end up using a lot of CPU due to // redundant wake ups. - pub fn run_on_demand(&mut self, callback: F) -> Result<(), EventLoopError> + pub fn run_on_demand(&mut self, handler: F) -> Result<(), EventLoopError> where F: FnMut(Event, &RootWindowTarget), { - let callback = map_user_event(callback, self.receiver.clone()); + let handler = map_user_event(handler, self.receiver.clone()); - // # Safety - // We are erasing the lifetime of the application callback here so that we - // can (temporarily) store it within 'static app delegate that's - // accessible to objc delegate callbacks. - // - // The safety of this depends on on making sure to also clear the callback - // from the app delegate before we return from here, ensuring that we don't - // retain a reference beyond the real lifetime of the callback. - let callback = unsafe { - mem::transmute::< - Rc, &RootWindowTarget)>>, - Rc, &RootWindowTarget)>>, - >(Rc::new(RefCell::new(callback))) - }; - - self._callback = Some(Rc::clone(&callback)); - - autoreleasepool(|_| { - // A bit of juggling with the callback references to make sure - // that `self.callback` is the only owner of the callback. - let weak_cb: Weak<_> = Rc::downgrade(&callback); - drop(callback); - - // # Safety - // We make sure to call `delegate.clear_callback` before returning - unsafe { - self.delegate - .set_callback(weak_cb, Rc::clone(&self.window_target)); - } - - // catch panics to make sure we can't unwind without clearing the set callback - // (which would leave the app delegate in an undefined, unsafe state) - let catch_result = catch_unwind(AssertUnwindSafe(|| { + self.delegate.set_event_handler(handler, || { + autoreleasepool(|_| { // clear / normalize pump_events state self.delegate.set_wait_timeout(None); self.delegate.set_stop_before_wait(false); @@ -331,6 +298,8 @@ impl EventLoop { self.delegate.set_is_running(true); self.delegate.dispatch_init_events(); } + + // SAFETY: We do not run the application re-entrantly unsafe { self.app.run() }; // While the app is running it's possible that we catch a panic @@ -343,73 +312,28 @@ impl EventLoop { } self.delegate.internal_exit() - })); - - // # Safety - // This pairs up with the `unsafe` call to `set_callback` above and ensures that - // we always clear the application callback from the app delegate before returning. - drop(self._callback.take()); - self.delegate.clear_callback(); - - if let Err(payload) = catch_result { - resume_unwind(payload) - } + }) }); Ok(()) } - pub fn pump_events(&mut self, timeout: Option, callback: F) -> PumpStatus + pub fn pump_events(&mut self, timeout: Option, handler: F) -> PumpStatus where F: FnMut(Event, &RootWindowTarget), { - let callback = map_user_event(callback, self.receiver.clone()); + let handler = map_user_event(handler, self.receiver.clone()); - // # Safety - // We are erasing the lifetime of the application callback here so that we - // can (temporarily) store it within 'static global app delegate that's - // accessible to objc delegate callbacks. - // - // The safety of this depends on on making sure to also clear the callback - // from the app delegate before we return from here, ensuring that we don't - // retain a reference beyond the real lifetime of the callback. - - let callback = unsafe { - mem::transmute::< - Rc, &RootWindowTarget)>>, - Rc, &RootWindowTarget)>>, - >(Rc::new(RefCell::new(callback))) - }; - - self._callback = Some(Rc::clone(&callback)); - - autoreleasepool(|_| { - // A bit of juggling with the callback references to make sure - // that `self.callback` is the only owner of the callback. - let weak_cb: Weak<_> = Rc::downgrade(&callback); - drop(callback); - - // # Safety - // We will make sure to call `delegate.clear_callback` before returning - // to ensure that we don't hold on to the callback beyond its (erased) - // lifetime - unsafe { - self.delegate - .set_callback(weak_cb, Rc::clone(&self.window_target)); - } - - // catch panics to make sure we can't unwind without clearing the set callback - // (which would leave the app delegate in an undefined, unsafe state) - let catch_result = catch_unwind(AssertUnwindSafe(|| { + self.delegate.set_event_handler(handler, || { + autoreleasepool(|_| { // As a special case, if the application hasn't been launched yet then we at least run // the loop until it has fully launched. if !self.delegate.is_launched() { debug_assert!(!self.delegate.is_running()); self.delegate.set_stop_on_launch(); - unsafe { - self.app.run(); - } + // SAFETY: We do not run the application re-entrantly + unsafe { self.app.run() }; // Note: we dispatch `NewEvents(Init)` + `Resumed` events after the application has launched } else if !self.delegate.is_running() { @@ -438,9 +362,8 @@ impl EventLoop { } } self.delegate.set_stop_on_redraw(true); - unsafe { - self.app.run(); - } + // SAFETY: We do not run the application re-entrantly + unsafe { self.app.run() }; } // While the app is running it's possible that we catch a panic @@ -458,18 +381,7 @@ impl EventLoop { } else { PumpStatus::Continue } - })); - - // # Safety - // This pairs up with the `unsafe` call to `set_callback` above and ensures that - // we always clear the application callback from the app delegate before returning - self.delegate.clear_callback(); - drop(self._callback.take()); - - match catch_result { - Ok(pump_status) => pump_status, - Err(payload) => resume_unwind(payload), - } + }) }) } diff --git a/src/platform_impl/macos/mod.rs b/src/platform_impl/macos/mod.rs index 6ad30834..2d0d306e 100644 --- a/src/platform_impl/macos/mod.rs +++ b/src/platform_impl/macos/mod.rs @@ -5,6 +5,7 @@ mod app; mod app_delegate; mod cursor; mod event; +mod event_handler; mod event_loop; mod ffi; mod menu;