Skip to content

Commit

Permalink
Fix for closure-captured values not being dropped on panic (#1853)
Browse files Browse the repository at this point in the history 
* Fix for #1850

* Update changelog

* Fix for compilation warnings

* Apply suggestions from code review

Co-authored-by: Markus Røyset <maroider@protonmail.com>

* Improve code quality

* Change Arc<Mutex> to Rc<RefCell>

* Panicking in the user callback is now well defined

* Address feedback

* Fix nightly warning

* The panic info is now not a global.

* Apply suggestions from code review

Co-authored-by: Francesca Lovebloom <francesca@brainiumstudios.com>

* Address feedback

Co-authored-by: Markus Røyset <maroider@protonmail.com>
Co-authored-by: Francesca Lovebloom <francesca@brainiumstudios.com>
  • Loading branch information
@ArturKovacs @maroider @francesca64
3 people committed Mar 8, 2021
1 parent 9847039 commit ffe2143
Show file tree
Hide file tree
Showing 5 changed files with 275 additions and 83 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
1 change: 1 addition & 0 deletions Cargo.toml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down
110 changes: 60 additions & 50 deletions src/platform_impl/macos/app_state.rs
View file
Original file line number Diff line number Diff line change
@@ -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,
Expand All @@ -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,
Expand Down Expand Up @@ -52,11 +52,31 @@ pub trait EventHandler: Debug {
}

struct EventLoopHandler<T: 'static> {
callback: Box<dyn FnMut(Event<'_, T>, &RootWindowTarget<T>, &mut ControlFlow)>,
callback: Weak<RefCell<dyn FnMut(Event<'_, T>, &RootWindowTarget<T>, &mut ControlFlow)>>,
will_exit: bool,
window_target: Rc<RootWindowTarget<T>>,
}

impl<T> EventLoopHandler<T> {
fn with_callback<F>(&mut self, f: F)
where
F: FnOnce(
&mut EventLoopHandler<T>,
RefMut<'_, dyn FnMut(Event<'_, T>, &RootWindowTarget<T>, &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<T> Debug for EventLoopHandler<T> {
fn fmt(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result {
formatter
Expand All @@ -68,23 +88,27 @@ impl<T> Debug for EventLoopHandler<T> {

impl<T> EventHandler for EventLoopHandler<T> {
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;
});
}
}

Expand Down Expand Up @@ -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<F, T>(callback: F, window_target: Rc<RootWindowTarget<T>>)
where
F: FnMut(Event<'_, T>, &RootWindowTarget<T>, &mut ControlFlow),
{
pub fn set_callback<T>(
callback: Weak<RefCell<dyn FnMut(Event<'_, T>, &RootWindowTarget<T>, &mut ControlFlow)>>,
window_target: Rc<RootWindowTarget<T>>,
) {
*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<dyn FnMut(Event<'_, T>, &RootWindowTarget<T>, &mut ControlFlow)>,
Box<dyn FnMut(Event<'_, T>, &RootWindowTarget<T>, &mut ControlFlow)>,
>(Box::new(callback)),
callback,
will_exit: false,
window_target,
}));
Expand All @@ -265,8 +281,11 @@ impl AppState {
HANDLER.set_in_callback(false);
}

pub fn wakeup() {
if !HANDLER.is_ready() {
pub fn wakeup(panic_info: Weak<PanicInfo>) {
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();
Expand Down Expand Up @@ -318,8 +337,11 @@ impl AppState {
HANDLER.events().append(&mut wrappers);
}

pub fn cleared() {
if !HANDLER.is_ready() {
pub fn cleared(panic_info: Weak<PanicInfo>) {
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() {
Expand Down Expand Up @@ -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();

Expand Down
139 changes: 131 additions & 8 deletions src/platform_impl/macos/event_loop.rs
View file
Original file line number Diff line number Diff line change
@@ -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},
Expand All @@ -23,6 +33,34 @@ use crate::{
},
};

#[derive(Default)]
pub struct PanicInfo {
inner: Cell<Option<Box<dyn Any + Send + 'static>>>,
}

// 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<dyn Any + Send + 'static>) {
if !self.is_panicking() {
self.inner.set(Some(p));
}
}
pub fn take(&self) -> Option<Box<dyn Any + Send + 'static>> {
self.inner.take()
}
}

pub struct EventLoopWindowTarget<T: 'static> {
pub sender: mpsc::Sender<T>, // this is only here to be cloned elsewhere
pub receiver: mpsc::Receiver<T>,
Expand Down Expand Up @@ -50,6 +88,15 @@ impl<T: 'static> EventLoopWindowTarget<T> {

pub struct EventLoop<T: 'static> {
window_target: Rc<RootWindowTarget<T>>,
panic_info: Rc<PanicInfo>,

/// 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<Rc<RefCell<dyn FnMut(Event<'_, T>, &RootWindowTarget<T>, &mut ControlFlow)>>>,
_delegate: IdRef,
}

Expand All @@ -72,12 +119,15 @@ impl<T> EventLoop<T> {
let _: () = msg_send![pool, drain];
delegate
};
setup_control_flow_observers();
let panic_info: Rc<PanicInfo> = 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,
}
}
Expand All @@ -98,14 +148,37 @@ impl<T> EventLoop<T> {
where
F: FnMut(Event<'_, T>, &RootWindowTarget<T>, &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<RefCell<dyn FnMut(Event<'_, T>, &RootWindowTarget<T>, &mut ControlFlow)>>,
Rc<RefCell<dyn FnMut(Event<'_, T>, &RootWindowTarget<T>, &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();
}
}

Expand All @@ -114,6 +187,56 @@ impl<T> EventLoop<T> {
}
}

#[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<F: FnOnce() -> R + UnwindSafe, R>(
panic_info: Weak<PanicInfo>,
f: F,
) -> Option<R> {
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<T> {
sender: mpsc::Sender<T>,
source: CFRunLoopSourceRef,
Expand Down
Loading

0 comments on commit ffe2143

Please sign in to comment.