macos: ensure pump_events returns for each RedrawRequested event

This also aims to be more rigorous with catching panics during
run_ondemand and pump_events to be absolutely sure that we
can't return to the caller without clearing the application
event handler from the global AppState (which could lead to
undefined behaviour).

Similar to the windows backend, this and adds lots of `// # Safety`
comments about how the callback lifetime is erased and how
it _must_ be cleared before returning to the app.

Author: rib

src/platform_impl/macos/app_state.rs

@@ -121,6 +121,7 @@ impl<T> EventHandler for EventLoopHandler<T> {
 struct Handler {
     stop_app_on_launch: AtomicBool,
     stop_app_before_wait: AtomicBool,
+    stop_app_on_redraw: AtomicBool,
     launched: AtomicBool,
     running: AtomicBool,
     in_callback: AtomicBool,
@@ -215,6 +216,8 @@ impl Handler {
         self.running.store(false, Ordering::Relaxed);
         *self.control_flow_prev.lock().unwrap() = ControlFlow::default();
         *self.control_flow.lock().unwrap() = ControlFlow::default();
+        self.set_stop_app_on_redraw_requested(false);
+        self.set_stop_app_before_wait(false);
     }
 
     pub fn request_stop_app_on_launch(&self) {
@@ -242,6 +245,19 @@ impl Handler {
         self.stop_app_before_wait.load(Ordering::Relaxed)
     }
 
+    pub fn set_stop_app_on_redraw_requested(&self, stop_on_redraw: bool) {
+        // Relaxed ordering because we don't actually have multiple threads involved, we just want
+        // interior mutability
+        self.stop_app_on_redraw
+            .store(stop_on_redraw, Ordering::Relaxed);
+    }
+
+    pub fn should_stop_app_on_redraw_requested(&self) -> bool {
+        // Relaxed ordering because we don't actually have multiple threads involved, we just want
+        // interior mutability
+        self.stop_app_on_redraw.load(Ordering::Relaxed)
+    }
+
     fn get_control_flow_and_update_prev(&self) -> ControlFlow {
         let control_flow = self.control_flow.lock().unwrap();
         *self.control_flow_prev.lock().unwrap() = *control_flow;
@@ -343,13 +359,30 @@ impl Handler {
 pub(crate) enum AppState {}
 
 impl AppState {
-    pub fn set_callback<T>(callback: Weak<Callback<T>>, window_target: Rc<RootWindowTarget<T>>) {
+    /// Associate the application's event callback with the (global static) Handler state
+    ///
+    /// # 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_ondemand`,
+    /// `pump_events`) _must_ pair a call to `set_callback` with
+    /// a call to `clear_callback` before returning to avoid undefined behaviour.
+    pub unsafe fn set_callback<T>(
+        callback: Weak<Callback<T>>,
+        window_target: Rc<RootWindowTarget<T>>,
+    ) {
         *HANDLER.callback.lock().unwrap() = Some(Box::new(EventLoopHandler {
             callback,
             window_target,
         }));
     }
 
+    pub fn clear_callback() {
+        HANDLER.callback.lock().unwrap().take();
+    }
+
     pub fn is_launched() -> bool {
         HANDLER.is_launched()
     }
@@ -369,12 +402,12 @@ impl AppState {
         HANDLER.set_stop_app_before_wait(stop_before_wait);
     }
 
-    pub fn control_flow() -> ControlFlow {
-        HANDLER.get_old_and_new_control_flow().1
+    pub fn set_stop_app_on_redraw_requested(stop_on_redraw: bool) {
+        HANDLER.set_stop_app_on_redraw_requested(stop_on_redraw);
     }
 
-    pub fn clear_callback() {
-        HANDLER.callback.lock().unwrap().take();
+    pub fn control_flow() -> ControlFlow {
+        HANDLER.get_old_and_new_control_flow().1
     }
 
     pub fn exit() -> i32 {
@@ -487,8 +520,17 @@ impl AppState {
         HANDLER.set_in_callback(false);
     }
 
-    // This is called from multiple threads at present
-    pub fn queue_redraw(window_id: WindowId) {
+    // Queue redraw on the main thread
+    //
+    // Only call this if not running on the main thread.
+    //
+    // Prefer to call `WinitView::queue_redraw()` which will only call this API
+    // if needed.
+    //
+    // Currently this wil simily lead to a `RedrawRequested` event being dispatched
+    // via `AppState::cleared()` but this should almost certainly be updated to
+    // queue a `drawRect` callback for the view via `view.setNeedsDisplay(true)`
+    pub fn queue_redraw_on_main(window_id: WindowId) {
         let mut pending_redraw = HANDLER.redraw();
         if !pending_redraw.contains(&window_id) {
             pending_redraw.push(window_id);
@@ -506,6 +548,12 @@ impl AppState {
             HANDLER
                 .handle_nonuser_event(EventWrapper::StaticEvent(Event::RedrawRequested(window_id)));
             HANDLER.set_in_callback(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
+            if HANDLER.should_stop_app_on_redraw_requested() {
+                AppState::stop();
+            }
         }
     }
 
@@ -548,6 +596,12 @@ impl AppState {
             HANDLER.handle_nonuser_event(event);
         }
         HANDLER.handle_nonuser_event(EventWrapper::StaticEvent(Event::MainEventsCleared));
+
+        // TODO(rib)
+        // Instead of directly dispatching RedrawRequested events here we should put a reference
+        // to the View or Window in `HANDLER.pending_redraw` so we can can instead call
+        // `view.setNeedsDisplay(true)` from the main thread to queue a regular `drawRect`
+        // callback (so RedrawRequested would only ever be driven via `drawRect`)
         for window_id in HANDLER.should_redraw() {
             HANDLER
                 .handle_nonuser_event(EventWrapper::StaticEvent(Event::RedrawRequested(window_id)));

src/platform_impl/macos/appkit/view.rs

@@ -59,6 +59,9 @@ extern_methods!(
     }
 
     unsafe impl NSView {
+        #[sel(setNeedsDisplay:)]
+        pub fn setNeedsDisplay(&self, needs_display: bool);
+
         #[sel(setWantsBestResolutionOpenGLSurface:)]
         pub fn setWantsBestResolutionOpenGLSurface(&self, value: bool);
 

src/platform_impl/macos/event_loop.rs

@@ -5,7 +5,7 @@ use std::{
     marker::PhantomData,
     mem,
     os::raw::c_void,
-    panic::{catch_unwind, resume_unwind, RefUnwindSafe, UnwindSafe},
+    panic::{catch_unwind, resume_unwind, AssertUnwindSafe, RefUnwindSafe, UnwindSafe},
     ptr,
     rc::{Rc, Weak},
     sync::mpsc,
@@ -203,10 +203,15 @@ impl<T> EventLoop<T> {
             return Err(ExternalError::AlreadyRunning);
         }
 
-        // 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.
+        // # Safety
+        // We are erasing the lifetime of the application callback here so that we
+        // can (temporarily) store it within 'static global `AppState` that's
+        // accessible to objc delegate callbacks.
+        //
+        // The safety of this depends on on making sure to also clear the callback
+        // from the global `AppState` 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<RefCell<dyn FnMut(Event<'_, T>, &RootWindowTarget<T>, &mut ControlFlow)>>,
@@ -224,23 +229,46 @@ impl<T> EventLoop<T> {
             let weak_cb: Weak<_> = Rc::downgrade(&callback);
             drop(callback);
 
-            AppState::set_callback(weak_cb, Rc::clone(&self.window_target));
-
-            if AppState::is_launched() {
-                debug_assert!(!AppState::is_running());
-                AppState::start_running(); // Set is_running = true + dispatch `NewEvents(Init)` + `Resumed`
+            // # Safety
+            // We make sure to call `AppState::clear_callback` before returning
+            unsafe {
+                AppState::set_callback(weak_cb, Rc::clone(&self.window_target));
             }
-            AppState::set_stop_app_before_wait(false);
-            unsafe { app.run() };
 
-            if let Some(panic) = self.panic_info.take() {
-                drop(self._callback.take());
-                AppState::clear_callback();
-                resume_unwind(panic);
+            // catch panics to make sure we can't unwind without clearing the set callback
+            // (which would leave the global `AppState` in an undefined, unsafe state)
+            let catch_result = catch_unwind(AssertUnwindSafe(|| {
+                if AppState::is_launched() {
+                    debug_assert!(!AppState::is_running());
+                    AppState::start_running(); // Set is_running = true + dispatch `NewEvents(Init)` + `Resumed`
+                }
+                AppState::set_stop_app_before_wait(false);
+                unsafe { app.run() };
+
+                // While the app is running it's possible that we catch a panic
+                // to avoid unwinding across an objective-c ffi boundary, which
+                // will lead to us stopping the `NSApp` and saving the
+                // `PanicInfo` so that we can resume the unwind at a controlled,
+                // safe point in time.
+                if let Some(panic) = self.panic_info.take() {
+                    resume_unwind(panic);
+                }
+
+                AppState::exit()
+            }));
+
+            // # Safety
+            // This pairs up with the `unsafe` call to `set_callback` above and ensures that
+            // we always clear the application callback from the global `AppState` before
+            // returning
+            drop(self._callback.take());
+            AppState::clear_callback();
+
+            match catch_result {
+                Ok(exit_code) => exit_code,
+                Err(payload) => resume_unwind(payload),
             }
-            AppState::exit()
         });
-        drop(self._callback.take());
 
         if exit_code == 0 {
             Ok(())
@@ -253,10 +281,15 @@ 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.
+        // # Safety
+        // We are erasing the lifetime of the application callback here so that we
+        // can (temporarily) store it within 'static global `AppState` that's
+        // accessible to objc delegate callbacks.
+        //
+        // The safety of this depends on on making sure to also clear the callback
+        // from the global `AppState` 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<RefCell<dyn FnMut(Event<'_, T>, &RootWindowTarget<T>, &mut ControlFlow)>>,
@@ -274,49 +307,73 @@ impl<T> EventLoop<T> {
             let weak_cb: Weak<_> = Rc::downgrade(&callback);
             drop(callback);
 
-            AppState::set_callback(weak_cb, Rc::clone(&self.window_target));
-
-            // Note: there are two possible `Init` conditions we have to handle - either the
-            // `NSApp` is not yet launched, or else the `EventLoop` is not yet running.
-
-            // As a special case, if the `NSApp` hasn't been launched yet then we at least run
-            // the loop until it has fully launched.
-            if !AppState::is_launched() {
-                debug_assert!(!AppState::is_running());
+            // # Safety
+            // We will make sure to call `AppState::clear_callback` before returning
+            // to ensure that we don't hold on to the callback beyond its (erased)
+            // lifetime
+            unsafe {
+                AppState::set_callback(weak_cb, Rc::clone(&self.window_target));
+            }
 
-                AppState::request_stop_on_launch();
-                unsafe {
-                    app.run();
+            // catch panics to make sure we can't unwind without clearing the set callback
+            // (which would leave the global `AppState` in an undefined, unsafe state)
+            let catch_result = catch_unwind(AssertUnwindSafe(|| {
+                // As a special case, if the `NSApp` hasn't been launched yet then we at least run
+                // the loop until it has fully launched.
+                if !AppState::is_launched() {
+                    debug_assert!(!AppState::is_running());
+
+                    AppState::request_stop_on_launch();
+                    unsafe {
+                        app.run();
+                    }
+
+                    // Note: we dispatch `NewEvents(Init)` + `Resumed` events after the `NSApp` has launched
+                } else if !AppState::is_running() {
+                    // Even though the NSApp may have been launched, it's possible we aren't running
+                    // if the `EventLoop` was run before and has since exited. This indicates that
+                    // we just starting to re-run the same `EventLoop` again.
+                    AppState::start_running(); // Set is_running = true + dispatch `NewEvents(Init)` + `Resumed`
+                } else {
+                    // Make sure we can't block any external loop indefinitely by stopping the NSApp
+                    // and returning after dispatching any `RedrawRequested` event or whenever the
+                    // `RunLoop` needs to wait for new events from the OS
+                    AppState::set_stop_app_on_redraw_requested(true);
+                    AppState::set_stop_app_before_wait(true);
+                    unsafe {
+                        app.run();
+                    }
                 }
 
-                // Note: we dispatch `NewEvents(Init)` + `Resumed` events after the `NSApp` has launched
-            } else if !AppState::is_running() {
-                AppState::start_running(); // Set is_running = true + dispatch `NewEvents(Init)` + `Resumed`
-            } else {
-                AppState::set_stop_app_before_wait(true);
-                unsafe {
-                    app.run();
+                // While the app is running it's possible that we catch a panic
+                // to avoid unwinding across an objective-c ffi boundary, which
+                // will lead to us stopping the `NSApp` and saving the
+                // `PanicInfo` so that we can resume the unwind at a controlled,
+                // safe point in time.
+                if let Some(panic) = self.panic_info.take() {
+                    resume_unwind(panic);
                 }
-            }
 
-            if let Some(panic) = self.panic_info.take() {
-                drop(self._callback.take());
-                AppState::clear_callback();
-                resume_unwind(panic);
+                if let ControlFlow::ExitWithCode(code) = AppState::control_flow() {
+                    AppState::exit();
+                    PumpStatus::Exit(code)
+                } 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 global `AppState` before
+            // returning
+            AppState::clear_callback();
+            drop(self._callback.take());
+
+            match catch_result {
+                Ok(pump_status) => pump_status,
+                Err(payload) => resume_unwind(payload),
             }
-        });
-
-        let status = if let ControlFlow::ExitWithCode(code) = AppState::control_flow() {
-            AppState::exit();
-            PumpStatus::Exit(code)
-        } else {
-            PumpStatus::Continue
-        };
-
-        AppState::clear_callback();
-        drop(self._callback.take());
-
-        status
+        })
     }
 
     pub fn create_proxy(&self) -> EventLoopProxy<T> {

src/platform_impl/macos/view.rs

@@ -4,8 +4,8 @@ use std::{boxed::Box, os::raw::*, ptr, str, sync::Mutex};
 
 use objc2::declare::{Ivar, IvarDrop};
 use objc2::foundation::{
-    NSArray, NSAttributedString, NSAttributedStringKey, NSCopying, NSMutableAttributedString,
-    NSObject, NSPoint, NSRange, NSRect, NSSize, NSString, NSUInteger,
+    is_main_thread, NSArray, NSAttributedString, NSAttributedStringKey, NSCopying,
+    NSMutableAttributedString, NSObject, NSPoint, NSRange, NSRect, NSSize, NSString, NSUInteger,
 };
 use objc2::rc::{Id, Owned, Shared, WeakId};
 use objc2::runtime::{Object, Sel};
@@ -884,6 +884,15 @@ impl WinitView {
         WindowId(self.window().id())
     }
 
+    pub(super) fn request_redraw(&self) {
+        if is_main_thread() {
+            // Request a callback via `drawRect`
+            self.setNeedsDisplay(true);
+        } else {
+            AppState::queue_redraw_on_main(self.window_id());
+        }
+    }
+
     fn queue_event(&self, event: WindowEvent<'static>) {
         let event = Event::WindowEvent {
             window_id: self.window_id(),