diff --git a/CHANGELOG.md b/CHANGELOG.md index 5f0b622b..3e5000e7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,7 @@ - On Android, unimplemented events are marked as unhandled on the native event loop. - On Windows, added `WindowBuilderExtWindows::with_menu` to set a custom menu at window creation time. - On Android, bump `ndk` and `ndk-glue` to 0.3: use predefined constants for event `ident`. +- On macOS, fix objects captured by the event loop closure not being dropped on panic. - On Windows, fixed `WindowEvent::ThemeChanged` not properly firing and fixed `Window::theme` returning the wrong theme. - On Web, added support for `DeviceEvent::MouseMotion` to listen for relative mouse movements. - Added `Window::drag_window`. Implemented on Windows, macOS, X11 and Wayland. diff --git a/Cargo.toml b/Cargo.toml index 2db7219d..68e85a1c 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -49,6 +49,7 @@ cocoa = "0.24" core-foundation = "0.9" core-graphics = "0.22" dispatch = "0.2.0" +scopeguard = "1.1" [target.'cfg(target_os = "macos")'.dependencies.core-video-sys] version = "0.1.4" diff --git a/src/platform_impl/macos/app_state.rs b/src/platform_impl/macos/app_state.rs index cf2f8280..b72cc57b 100644 --- a/src/platform_impl/macos/app_state.rs +++ b/src/platform_impl/macos/app_state.rs @@ -1,9 +1,10 @@ use std::{ + cell::{RefCell, RefMut}, collections::VecDeque, fmt::{self, Debug}, hint::unreachable_unchecked, mem, - rc::Rc, + rc::{Rc, Weak}, sync::{ atomic::{AtomicBool, Ordering}, Mutex, MutexGuard, @@ -12,19 +13,18 @@ use std::{ }; use cocoa::{ - appkit::{NSApp, NSEventType::NSApplicationDefined, NSWindow}, + appkit::{NSApp, NSWindow}, base::{id, nil}, - foundation::{NSAutoreleasePool, NSPoint, NSSize}, + foundation::{NSAutoreleasePool, NSSize}, }; -use objc::runtime::YES; - use crate::{ dpi::LogicalSize, event::{Event, StartCause, WindowEvent}, event_loop::{ControlFlow, EventLoopWindowTarget as RootWindowTarget}, platform_impl::platform::{ event::{EventProxy, EventWrapper}, + event_loop::{post_dummy_event, PanicInfo}, observer::EventLoopWaker, util::{IdRef, Never}, window::get_window_id, @@ -52,11 +52,31 @@ pub trait EventHandler: Debug { } struct EventLoopHandler { - callback: Box, &RootWindowTarget, &mut ControlFlow)>, + callback: Weak, &RootWindowTarget, &mut ControlFlow)>>, will_exit: bool, window_target: Rc>, } +impl EventLoopHandler { + fn with_callback(&mut self, f: F) + where + F: FnOnce( + &mut EventLoopHandler, + RefMut<'_, dyn FnMut(Event<'_, T>, &RootWindowTarget, &mut ControlFlow)>, + ), + { + if let Some(callback) = self.callback.upgrade() { + let callback = callback.borrow_mut(); + (f)(self, callback); + } else { + panic!( + "Tried to dispatch an event, but the event loop that \ + owned the event handler callback seems to be destroyed" + ); + } + } +} + impl Debug for EventLoopHandler { fn fmt(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result { formatter @@ -68,23 +88,27 @@ impl Debug for EventLoopHandler { impl EventHandler for EventLoopHandler { fn handle_nonuser_event(&mut self, event: Event<'_, Never>, control_flow: &mut ControlFlow) { - (self.callback)(event.userify(), &self.window_target, control_flow); - self.will_exit |= *control_flow == ControlFlow::Exit; - if self.will_exit { - *control_flow = ControlFlow::Exit; - } + self.with_callback(|this, mut callback| { + (callback)(event.userify(), &this.window_target, control_flow); + this.will_exit |= *control_flow == ControlFlow::Exit; + if this.will_exit { + *control_flow = ControlFlow::Exit; + } + }); } fn handle_user_events(&mut self, control_flow: &mut ControlFlow) { - let mut will_exit = self.will_exit; - for event in self.window_target.p.receiver.try_iter() { - (self.callback)(Event::UserEvent(event), &self.window_target, control_flow); - will_exit |= *control_flow == ControlFlow::Exit; - if will_exit { - *control_flow = ControlFlow::Exit; + self.with_callback(|this, mut callback| { + let mut will_exit = this.will_exit; + for event in this.window_target.p.receiver.try_iter() { + (callback)(Event::UserEvent(event), &this.window_target, control_flow); + will_exit |= *control_flow == ControlFlow::Exit; + if will_exit { + *control_flow = ControlFlow::Exit; + } } - } - self.will_exit = will_exit; + this.will_exit = will_exit; + }); } } @@ -229,20 +253,12 @@ pub static INTERRUPT_EVENT_LOOP_EXIT: AtomicBool = AtomicBool::new(false); pub enum AppState {} impl AppState { - // This function extends lifetime of `callback` to 'static as its side effect - pub unsafe fn set_callback(callback: F, window_target: Rc>) - where - F: FnMut(Event<'_, T>, &RootWindowTarget, &mut ControlFlow), - { + pub fn set_callback( + callback: Weak, &RootWindowTarget, &mut ControlFlow)>>, + window_target: Rc>, + ) { *HANDLER.callback.lock().unwrap() = Some(Box::new(EventLoopHandler { - // This transmute is always safe, in case it was reached through `run`, since our - // lifetime will be already 'static. In other cases caller should ensure that all data - // they passed to callback will actually outlive it, some apps just can't move - // everything to event loop, so this is something that they should care about. - callback: mem::transmute::< - Box, &RootWindowTarget, &mut ControlFlow)>, - Box, &RootWindowTarget, &mut ControlFlow)>, - >(Box::new(callback)), + callback, will_exit: false, window_target, })); @@ -265,8 +281,11 @@ impl AppState { HANDLER.set_in_callback(false); } - pub fn wakeup() { - if !HANDLER.is_ready() { + pub fn wakeup(panic_info: Weak) { + let panic_info = panic_info + .upgrade() + .expect("The panic info must exist here. This failure indicates a developer error."); + if panic_info.is_panicking() || !HANDLER.is_ready() { return; } let start = HANDLER.get_start_time().unwrap(); @@ -318,8 +337,11 @@ impl AppState { HANDLER.events().append(&mut wrappers); } - pub fn cleared() { - if !HANDLER.is_ready() { + pub fn cleared(panic_info: Weak) { + let panic_info = panic_info + .upgrade() + .expect("The panic info must exist here. This failure indicates a developer error."); + if panic_info.is_panicking() || !HANDLER.is_ready() { return; } if !HANDLER.get_in_callback() { @@ -357,21 +379,9 @@ impl AppState { && !dialog_open && !dialog_is_closing { - let _: () = msg_send![app, stop: nil]; - - let dummy_event: id = msg_send![class!(NSEvent), - otherEventWithType: NSApplicationDefined - location: NSPoint::new(0.0, 0.0) - modifierFlags: 0 - timestamp: 0 - windowNumber: 0 - context: nil - subtype: 0 - data1: 0 - data2: 0 - ]; + let () = msg_send![app, stop: nil]; // To stop event loop immediately, we need to post some event here. - let _: () = msg_send![app, postEvent: dummy_event atStart: YES]; + post_dummy_event(app); } pool.drain(); diff --git a/src/platform_impl/macos/event_loop.rs b/src/platform_impl/macos/event_loop.rs index 5b3d96b0..afebcd47 100644 --- a/src/platform_impl/macos/event_loop.rs +++ b/src/platform_impl/macos/event_loop.rs @@ -1,14 +1,24 @@ use std::{ - collections::VecDeque, marker::PhantomData, mem, os::raw::c_void, process, ptr, rc::Rc, + any::Any, + cell::{Cell, RefCell}, + collections::VecDeque, + marker::PhantomData, + mem, + os::raw::c_void, + panic::{catch_unwind, resume_unwind, RefUnwindSafe, UnwindSafe}, + process, ptr, + rc::{Rc, Weak}, sync::mpsc, }; use cocoa::{ - appkit::NSApp, - base::{id, nil}, - foundation::NSAutoreleasePool, + appkit::{NSApp, NSEventType::NSApplicationDefined}, + base::{id, nil, YES}, + foundation::{NSAutoreleasePool, NSPoint}, }; +use scopeguard::defer; + use crate::{ event::Event, event_loop::{ControlFlow, EventLoopClosed, EventLoopWindowTarget as RootWindowTarget}, @@ -23,6 +33,34 @@ use crate::{ }, }; +#[derive(Default)] +pub struct PanicInfo { + inner: Cell>>, +} + +// WARNING: +// As long as this struct is used through its `impl`, it is UnwindSafe. +// (If `get_mut` is called on `inner`, unwind safety may get broken.) +impl UnwindSafe for PanicInfo {} +impl RefUnwindSafe for PanicInfo {} +impl PanicInfo { + pub fn is_panicking(&self) -> bool { + let inner = self.inner.take(); + let result = inner.is_some(); + self.inner.set(inner); + result + } + /// Overwrites the curret state if the current state is not panicking + pub fn set_panic(&self, p: Box) { + if !self.is_panicking() { + self.inner.set(Some(p)); + } + } + pub fn take(&self) -> Option> { + self.inner.take() + } +} + pub struct EventLoopWindowTarget { pub sender: mpsc::Sender, // this is only here to be cloned elsewhere pub receiver: mpsc::Receiver, @@ -50,6 +88,15 @@ impl EventLoopWindowTarget { pub struct EventLoop { window_target: Rc>, + 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. + _callback: Option, &RootWindowTarget, &mut ControlFlow)>>>, _delegate: IdRef, } @@ -72,12 +119,15 @@ impl EventLoop { let _: () = msg_send![pool, drain]; delegate }; - setup_control_flow_observers(); + let panic_info: Rc = Default::default(); + setup_control_flow_observers(Rc::downgrade(&panic_info)); EventLoop { window_target: Rc::new(RootWindowTarget { p: Default::default(), _marker: PhantomData, }), + panic_info, + _callback: None, _delegate: delegate, } } @@ -98,14 +148,37 @@ impl EventLoop { where F: FnMut(Event<'_, T>, &RootWindowTarget, &mut ControlFlow), { + // This transmute is always safe, in case it was reached through `run`, since our + // lifetime will be already 'static. In other cases caller should ensure that all data + // they passed to callback will actually outlive it, some apps just can't move + // everything to event loop, so this is something that they should care about. + let callback = unsafe { + mem::transmute::< + Rc, &RootWindowTarget, &mut ControlFlow)>>, + Rc, &RootWindowTarget, &mut ControlFlow)>>, + >(Rc::new(RefCell::new(callback))) + }; + + self._callback = Some(Rc::clone(&callback)); + unsafe { let pool = NSAutoreleasePool::new(nil); + defer!(pool.drain()); let app = NSApp(); assert_ne!(app, nil); - AppState::set_callback(callback, Rc::clone(&self.window_target)); - let _: () = msg_send![app, run]; + + // 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); + mem::drop(callback); + + AppState::set_callback(weak_cb, Rc::clone(&self.window_target)); + let () = msg_send![app, run]; + + if let Some(panic) = self.panic_info.take() { + resume_unwind(panic); + } AppState::exit(); - pool.drain(); } } @@ -114,6 +187,56 @@ impl EventLoop { } } +#[inline] +pub unsafe fn post_dummy_event(target: id) { + let event_class = class!(NSEvent); + let dummy_event: id = msg_send![ + event_class, + otherEventWithType: NSApplicationDefined + location: NSPoint::new(0.0, 0.0) + modifierFlags: 0 + timestamp: 0 + windowNumber: 0 + context: nil + subtype: 0 + data1: 0 + data2: 0 + ]; + let () = msg_send![target, postEvent: dummy_event atStart: YES]; +} + +/// Catches panics that happen inside `f` and when a panic +/// happens, stops the `sharedApplication` +#[inline] +pub fn stop_app_on_panic R + UnwindSafe, R>( + panic_info: Weak, + f: F, +) -> Option { + match catch_unwind(f) { + Ok(r) => Some(r), + Err(e) => { + // It's important that we set the panic before requesting a `stop` + // because some callback are still called during the `stop` message + // and we need to know in those callbacks if the application is currently + // panicking + { + let panic_info = panic_info.upgrade().unwrap(); + panic_info.set_panic(e); + } + unsafe { + let app_class = class!(NSApplication); + let app: id = msg_send![app_class, sharedApplication]; + let () = msg_send![app, stop: nil]; + + // Posting a dummy event to get `stop` to take effect immediately. + // See: https://stackoverflow.com/questions/48041279/stopping-the-nsapplication-main-event-loop/48064752#48064752 + post_dummy_event(app); + } + None + } + } +} + pub struct Proxy { sender: mpsc::Sender, source: CFRunLoopSourceRef, diff --git a/src/platform_impl/macos/observer.rs b/src/platform_impl/macos/observer.rs index aa7f5362..5f92c55b 100644 --- a/src/platform_impl/macos/observer.rs +++ b/src/platform_impl/macos/observer.rs @@ -1,6 +1,17 @@ -use std::{self, os::raw::*, ptr, time::Instant}; +use std::{ + self, + os::raw::*, + panic::{AssertUnwindSafe, UnwindSafe}, + ptr, + rc::Weak, + time::Instant, +}; -use crate::platform_impl::platform::{app_state::AppState, ffi}; +use crate::platform_impl::platform::{ + app_state::AppState, + event_loop::{stop_app_on_panic, PanicInfo}, + ffi, +}; #[link(name = "CoreFoundation", kind = "framework")] extern "C" { @@ -85,9 +96,20 @@ pub type CFRunLoopObserverCallBack = extern "C" fn(observer: CFRunLoopObserverRef, activity: CFRunLoopActivity, info: *mut c_void); pub type CFRunLoopTimerCallBack = extern "C" fn(timer: CFRunLoopTimerRef, info: *mut c_void); -pub enum CFRunLoopObserverContext {} pub enum CFRunLoopTimerContext {} +/// This mirrors the struct with the same name from Core Foundation. +/// https://developer.apple.com/documentation/corefoundation/cfrunloopobservercontext?language=objc +#[allow(non_snake_case)] +#[repr(C)] +pub struct CFRunLoopObserverContext { + pub version: CFIndex, + pub info: *mut c_void, + pub retain: Option *const c_void>, + pub release: Option, + pub copyDescription: Option CFStringRef>, +} + #[allow(non_snake_case)] #[repr(C)] pub struct CFRunLoopSourceContext { @@ -103,21 +125,42 @@ pub struct CFRunLoopSourceContext { pub perform: Option, } +unsafe fn control_flow_handler(panic_info: *mut c_void, f: F) +where + F: FnOnce(Weak) + UnwindSafe, +{ + let info_from_raw = Weak::from_raw(panic_info as *mut PanicInfo); + // Asserting unwind safety on this type should be fine because `PanicInfo` is + // `RefUnwindSafe` and `Rc` is `UnwindSafe` if `T` is `RefUnwindSafe`. + let panic_info = AssertUnwindSafe(Weak::clone(&info_from_raw)); + // `from_raw` takes ownership of the data behind the pointer. + // But if this scope takes ownership of the weak pointer, then + // the weak pointer will get free'd at the end of the scope. + // However we want to keep that weak reference around after the function. + std::mem::forget(info_from_raw); + + stop_app_on_panic(Weak::clone(&panic_info), move || f(panic_info.0)); +} + // begin is queued with the highest priority to ensure it is processed before other observers extern "C" fn control_flow_begin_handler( _: CFRunLoopObserverRef, activity: CFRunLoopActivity, - _: *mut c_void, + panic_info: *mut c_void, ) { - #[allow(non_upper_case_globals)] - match activity { - kCFRunLoopAfterWaiting => { - //trace!("Triggered `CFRunLoopAfterWaiting`"); - AppState::wakeup(); - //trace!("Completed `CFRunLoopAfterWaiting`"); - } - kCFRunLoopEntry => unimplemented!(), // not expected to ever happen - _ => unreachable!(), + unsafe { + control_flow_handler(panic_info, |panic_info| { + #[allow(non_upper_case_globals)] + match activity { + kCFRunLoopAfterWaiting => { + //trace!("Triggered `CFRunLoopAfterWaiting`"); + AppState::wakeup(panic_info); + //trace!("Completed `CFRunLoopAfterWaiting`"); + } + kCFRunLoopEntry => unimplemented!(), // not expected to ever happen + _ => unreachable!(), + } + }); } } @@ -126,17 +169,21 @@ extern "C" fn control_flow_begin_handler( extern "C" fn control_flow_end_handler( _: CFRunLoopObserverRef, activity: CFRunLoopActivity, - _: *mut c_void, + panic_info: *mut c_void, ) { - #[allow(non_upper_case_globals)] - match activity { - kCFRunLoopBeforeWaiting => { - //trace!("Triggered `CFRunLoopBeforeWaiting`"); - AppState::cleared(); - //trace!("Completed `CFRunLoopBeforeWaiting`"); - } - kCFRunLoopExit => (), //unimplemented!(), // not expected to ever happen - _ => unreachable!(), + unsafe { + control_flow_handler(panic_info, |panic_info| { + #[allow(non_upper_case_globals)] + match activity { + kCFRunLoopBeforeWaiting => { + //trace!("Triggered `CFRunLoopBeforeWaiting`"); + AppState::cleared(panic_info); + //trace!("Completed `CFRunLoopBeforeWaiting`"); + } + kCFRunLoopExit => (), //unimplemented!(), // not expected to ever happen + _ => unreachable!(), + } + }); } } @@ -152,6 +199,7 @@ impl RunLoop { flags: CFOptionFlags, priority: CFIndex, handler: CFRunLoopObserverCallBack, + context: *mut CFRunLoopObserverContext, ) { let observer = CFRunLoopObserverCreate( ptr::null_mut(), @@ -159,24 +207,33 @@ impl RunLoop { ffi::TRUE, // Indicates we want this to run repeatedly priority, // The lower the value, the sooner this will run handler, - ptr::null_mut(), + context, ); CFRunLoopAddObserver(self.0, observer, kCFRunLoopCommonModes); } } -pub fn setup_control_flow_observers() { +pub fn setup_control_flow_observers(panic_info: Weak) { unsafe { + let mut context = CFRunLoopObserverContext { + info: Weak::into_raw(panic_info) as *mut _, + version: 0, + retain: None, + release: None, + copyDescription: None, + }; let run_loop = RunLoop::get(); run_loop.add_observer( kCFRunLoopEntry | kCFRunLoopAfterWaiting, CFIndex::min_value(), control_flow_begin_handler, + &mut context as *mut _, ); run_loop.add_observer( kCFRunLoopExit | kCFRunLoopBeforeWaiting, CFIndex::max_value(), control_flow_end_handler, + &mut context as *mut _, ); } }