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

ArturKovacs · maroider · francesca64 · web-flow · commit ffe2143d14b1 · 2021-03-08T19:56:39.000+01:00
* 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>
diff --git 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
@@ -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
@@ -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.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<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,
         }));
@@ -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();
@@ -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() {
@@ -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
@@ -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<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,
diff --git a/src/platform_impl/macos/observer.rs b/src/platform_impl/macos/observer.rs
