Fix for closure-captured values not being dropped on panic (#1853)

* 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>

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.

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"

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<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
@@ -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.with_callback(|this, mut callback| {
-        self.will_exit |= *control_flow == ControlFlow::Exit;
+            (callback)(event.userify(), &this.window_target, control_flow);
-        if self.will_exit {
+            this.will_exit |= *control_flow == ControlFlow::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;
+        self.with_callback(|this, mut callback| {
-        for event in self.window_target.p.receiver.try_iter() {
+            let mut will_exit = this.will_exit;
-            (self.callback)(Event::UserEvent(event), &self.window_target, control_flow);
+            for event in this.window_target.p.receiver.try_iter() {
-            will_exit |= *control_flow == ControlFlow::Exit;
+                (callback)(Event::UserEvent(event), &this.window_target, control_flow);
-            if will_exit {
+                will_exit |= *control_flow == ControlFlow::Exit;
-                *control_flow = ControlFlow::Exit;
+                if will_exit {
+                    *control_flow = ControlFlow::Exit;
+                }
             }
-        }
+            this.will_exit = will_exit;
-        self.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 fn set_callback<T>(
-    pub unsafe fn set_callback<F, T>(callback: F, window_target: Rc<RootWindowTarget<T>>)
+        callback: Weak<RefCell<dyn FnMut(Event<'_, T>, &RootWindowTarget<T>, &mut ControlFlow)>>,
-    where
+        window_target: Rc<RootWindowTarget<T>>,
-        F: FnMut(Event<'_, T>, &RootWindowTarget<T>, &mut ControlFlow),
+    ) {
-    {
         *HANDLER.callback.lock().unwrap() = Some(Box::new(EventLoopHandler {
-            // This transmute is always safe, in case it was reached through `run`, since our
+            callback,
-            // 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)),
             will_exit: false,
             window_target,
         }));
@@ -265,8 +281,11 @@ impl AppState {
         HANDLER.set_in_callback(false);
     }
 
-    pub fn wakeup() {
+    pub fn wakeup(panic_info: Weak<PanicInfo>) {
-        if !HANDLER.is_ready() {
+        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() {
+    pub fn cleared(panic_info: Weak<PanicInfo>) {
-        if !HANDLER.is_ready() {
+        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 () = 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
-                    ];
                     // 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();
 

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,
+    appkit::{NSApp, NSEventType::NSApplicationDefined},
-    base::{id, nil},
+    base::{id, nil, YES},
-    foundation::NSAutoreleasePool,
+    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<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>,
@@ -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,
 }
 
@@ -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,
         }
     }
@@ -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();
         }
     }
 
@@ -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,

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<extern "C" fn(info: *const c_void) -> *const c_void>,
+    pub release: Option<extern "C" fn(info: *const c_void)>,
+    pub copyDescription: Option<extern "C" fn(info: *const c_void) -> CFStringRef>,
+}
+
 #[allow(non_snake_case)]
 #[repr(C)]
 pub struct CFRunLoopSourceContext {
@@ -103,21 +125,42 @@ pub struct CFRunLoopSourceContext {
     pub perform: Option<extern "C" fn(*mut c_void)>,
 }
 
+unsafe fn control_flow_handler<F>(panic_info: *mut c_void, f: F)
+where
+    F: FnOnce(Weak<PanicInfo>) + 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<T>` 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)]
+    unsafe {
-    match activity {
+        control_flow_handler(panic_info, |panic_info| {
-        kCFRunLoopAfterWaiting => {
+            #[allow(non_upper_case_globals)]
-            //trace!("Triggered `CFRunLoopAfterWaiting`");
+            match activity {
-            AppState::wakeup();
+                kCFRunLoopAfterWaiting => {
-            //trace!("Completed `CFRunLoopAfterWaiting`");
+                    //trace!("Triggered `CFRunLoopAfterWaiting`");
-        }
+                    AppState::wakeup(panic_info);
-        kCFRunLoopEntry => unimplemented!(), // not expected to ever happen
+                    //trace!("Completed `CFRunLoopAfterWaiting`");
-        _ => unreachable!(),
+                }
+                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)]
+    unsafe {
-    match activity {
+        control_flow_handler(panic_info, |panic_info| {
-        kCFRunLoopBeforeWaiting => {
+            #[allow(non_upper_case_globals)]
-            //trace!("Triggered `CFRunLoopBeforeWaiting`");
+            match activity {
-            AppState::cleared();
+                kCFRunLoopBeforeWaiting => {
-            //trace!("Completed `CFRunLoopBeforeWaiting`");
+                    //trace!("Triggered `CFRunLoopBeforeWaiting`");
-        }
+                    AppState::cleared(panic_info);
-        kCFRunLoopExit => (), //unimplemented!(), // not expected to ever happen
+                    //trace!("Completed `CFRunLoopBeforeWaiting`");
-        _ => unreachable!(),
+                }
+                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<PanicInfo>) {
     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 _,
         );
     }
 }