diff --git a/src/platform_impl/macos/app_state.rs b/src/platform_impl/macos/app_state.rs index 0083b1bfcc..3927f78b19 100644 --- a/src/platform_impl/macos/app_state.rs +++ b/src/platform_impl/macos/app_state.rs @@ -121,6 +121,7 @@ impl EventHandler for EventLoopHandler { 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(callback: Weak>, window_target: Rc>) { + /// 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( + callback: Weak>, + window_target: Rc>, + ) { *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))); diff --git a/src/platform_impl/macos/appkit/view.rs b/src/platform_impl/macos/appkit/view.rs index b72b712df4..a5b7df03fb 100644 --- a/src/platform_impl/macos/appkit/view.rs +++ b/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); diff --git a/src/platform_impl/macos/event_loop.rs b/src/platform_impl/macos/event_loop.rs index 8e716563ad..711a3b9a0e 100644 --- a/src/platform_impl/macos/event_loop.rs +++ b/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 EventLoop { 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, &RootWindowTarget, &mut ControlFlow)>>, @@ -224,23 +229,46 @@ impl EventLoop { 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 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. + // # 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, &RootWindowTarget, &mut ControlFlow)>>, @@ -274,49 +307,73 @@ impl EventLoop { 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 { diff --git a/src/platform_impl/macos/view.rs b/src/platform_impl/macos/view.rs index 644fcd0a22..f39842c81d 100644 --- a/src/platform_impl/macos/view.rs +++ b/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(), diff --git a/src/platform_impl/macos/window.rs b/src/platform_impl/macos/window.rs index 69921ba78f..6ffe5e8aa3 100644 --- a/src/platform_impl/macos/window.rs +++ b/src/platform_impl/macos/window.rs @@ -23,7 +23,6 @@ use crate::{ icon::Icon, platform::macos::{OptionAsAlt, WindowExtMacOS}, platform_impl::platform::{ - app_state::AppState, appkit::NSWindowOrderingMode, ffi, monitor::{self, MonitorHandle, VideoMode}, @@ -34,7 +33,7 @@ use crate::{ }, window::{ CursorGrabMode, CursorIcon, ImePurpose, ResizeDirection, Theme, UserAttentionType, - WindowAttributes, WindowButtons, WindowId as RootWindowId, WindowLevel, + WindowAttributes, WindowButtons, WindowLevel, }, }; use core_graphics::display::{CGDisplay, CGPoint}; @@ -540,7 +539,8 @@ impl WinitWindow { } pub fn request_redraw(&self) { - AppState::queue_redraw(RootWindowId(self.id())); + let view = self.view(); + view.request_redraw(); } pub fn outer_position(&self) -> Result, NotSupportedError> {