Skip to content

Commit

Permalink
Retain ApplicationDelegate in NSWindowDelegate and NSView
Browse files Browse the repository at this point in the history
The delegate is only weakly referenced by NSApplication, so getting it
from there may fail if the event loop has been dropped.

Fixes #3668.
  • Loading branch information
madsmtm authored and kchibisov committed Jun 10, 2024
1 parent ebd6454 commit c801b69
Show file tree
Hide file tree
Showing 6 changed files with 55 additions and 23 deletions.
4 changes: 4 additions & 0 deletions src/changelog/unreleased.md
Original file line number Diff line number Diff line change
Expand Up @@ -39,3 +39,7 @@ The migration guide could reference other migration examples in the current
changelog entry.

## Unreleased

### Fixed

- On macOS, fix panic on exit when dropping windows outside the event loop.
8 changes: 5 additions & 3 deletions src/platform_impl/macos/app_delegate.rs
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,8 @@ pub(super) struct State {
wait_timeout: Cell<Option<Instant>>,
pending_events: RefCell<VecDeque<QueuedEvent>>,
pending_redraw: RefCell<Vec<WindowId>>,
// NOTE: This is strongly referenced by our `NSWindowDelegate` and our `NSView` subclass, and
// as such should be careful to not add fields that, in turn, strongly reference those.
}

declare_class!(
Expand All @@ -71,7 +73,7 @@ declare_class!(
unsafe impl NSObjectProtocol for ApplicationDelegate {}

unsafe impl NSApplicationDelegate for ApplicationDelegate {
// Note: This will, globally, only be run once, no matter how many
// NOTE: This will, globally, only be run once, no matter how many
// `EventLoop`s the user creates.
#[method(applicationDidFinishLaunching:)]
fn did_finish_launching(&self, _sender: Option<&AnyObject>) {
Expand Down Expand Up @@ -106,7 +108,7 @@ declare_class!(
// In this case we still want to consider Winit's `EventLoop` to be "running",
// so we call `start_running()` above.
if self.ivars().stop_on_launch.get() {
// Note: the original idea had been to only stop the underlying `RunLoop`
// NOTE: the original idea had been to only stop the underlying `RunLoop`
// for the app but that didn't work as expected (`-[NSApplication run]`
// effectively ignored the attempt to stop the RunLoop and re-started it).
//
Expand Down Expand Up @@ -188,7 +190,7 @@ impl ApplicationDelegate {

/// Clears the `running` state and resets the `control_flow` state when an `EventLoop` exits.
///
/// Note: that if the `NSApplication` has been launched then that state is preserved,
/// NOTE: that if the `NSApplication` has been launched then that state is preserved,
/// and we won't need to re-launch the app if subsequent EventLoops are run.
pub fn internal_exit(&self) {
self.handle_event(Event::LoopExiting);
Expand Down
6 changes: 4 additions & 2 deletions src/platform_impl/macos/event_loop.rs
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,10 @@ impl ActiveEventLoop {
RootWindowTarget { p, _marker: PhantomData }
}

pub(super) fn app_delegate(&self) -> &ApplicationDelegate {
&self.delegate
}

pub fn create_custom_cursor(&self, source: CustomCursorSource) -> RootCustomCursor {
RootCustomCursor { inner: CustomCursor::new(source.inner) }
}
Expand Down Expand Up @@ -133,9 +137,7 @@ impl ActiveEventLoop {
pub(crate) fn owned_display_handle(&self) -> OwnedDisplayHandle {
OwnedDisplayHandle
}
}

impl ActiveEventLoop {
pub(crate) fn hide_application(&self) {
NSApplication::sharedApplication(self.mtm).hide(None)
}
Expand Down
28 changes: 20 additions & 8 deletions src/platform_impl/macos/view.rs
Original file line number Diff line number Diff line change
Expand Up @@ -110,8 +110,11 @@ fn get_left_modifier_code(key: &Key) -> KeyCode {
}
}

#[derive(Debug, Default)]
#[derive(Debug)]
pub struct ViewState {
/// Strong reference to the global application state.
app_delegate: Id<ApplicationDelegate>,

cursor_state: RefCell<CursorState>,
ime_position: Cell<NSPoint>,
ime_size: Cell<NSSize>,
Expand Down Expand Up @@ -204,8 +207,7 @@ declare_class!(

// It's a workaround for https://github.com/rust-windowing/winit/issues/2640, don't replace with `self.window_id()`.
if let Some(window) = self.ivars()._ns_window.load() {
let app_delegate = ApplicationDelegate::get(MainThreadMarker::from(self));
app_delegate.handle_redraw(window.id());
self.ivars().app_delegate.handle_redraw(window.id());
}

#[allow(clippy::let_unit_value)]
Expand Down Expand Up @@ -773,16 +775,28 @@ declare_class!(

impl WinitView {
pub(super) fn new(
app_delegate: &ApplicationDelegate,
window: &WinitWindow,
accepts_first_mouse: bool,
option_as_alt: OptionAsAlt,
) -> Id<Self> {
let mtm = MainThreadMarker::from(window);
let this = mtm.alloc().set_ivars(ViewState {
app_delegate: app_delegate.retain(),
cursor_state: Default::default(),
ime_position: Default::default(),
ime_size: Default::default(),
modifiers: Default::default(),
phys_modifiers: Default::default(),
tracking_rect: Default::default(),
ime_state: Default::default(),
input_source: Default::default(),
ime_allowed: Default::default(),
forward_key_to_app: Default::default(),
marked_text: Default::default(),
accepts_first_mouse,
_ns_window: WeakId::new(&window.retain()),
option_as_alt: Cell::new(option_as_alt),
..Default::default()
});
let this: Id<Self> = unsafe { msg_send_id![super(this), init] };

Expand Down Expand Up @@ -818,13 +832,11 @@ impl WinitView {
}

fn queue_event(&self, event: WindowEvent) {
let app_delegate = ApplicationDelegate::get(MainThreadMarker::from(self));
app_delegate.queue_window_event(self.window().id(), event);
self.ivars().app_delegate.queue_window_event(self.window().id(), event);
}

fn queue_device_event(&self, event: DeviceEvent) {
let app_delegate = ApplicationDelegate::get(MainThreadMarker::from(self));
app_delegate.queue_device_event(event);
self.ivars().app_delegate.queue_device_event(event);
}

fn scale_factor(&self) -> f64 {
Expand Down
4 changes: 3 additions & 1 deletion src/platform_impl/macos/window.rs
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,9 @@ impl Window {
attributes: WindowAttributes,
) -> Result<Self, RootOsError> {
let mtm = window_target.mtm;
let delegate = autoreleasepool(|_| WindowDelegate::new(attributes, mtm))?;
let delegate = autoreleasepool(|_| {
WindowDelegate::new(window_target.app_delegate(), attributes, mtm)
})?;
Ok(Window {
window: MainThreadBound::new(delegate.window().retain(), mtm),
delegate: MainThreadBound::new(delegate, mtm),
Expand Down
28 changes: 19 additions & 9 deletions src/platform_impl/macos/window_delegate.rs
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,9 @@ impl Default for PlatformSpecificWindowAttributes {

#[derive(Debug)]
pub(crate) struct State {
/// Strong reference to the global application state.
app_delegate: Id<ApplicationDelegate>,

window: Id<WinitWindow>,

current_theme: Cell<Option<Theme>>,
Expand Down Expand Up @@ -442,7 +445,11 @@ declare_class!(
}
);

fn new_window(attrs: &WindowAttributes, mtm: MainThreadMarker) -> Option<Id<WinitWindow>> {
fn new_window(
app_delegate: &ApplicationDelegate,
attrs: &WindowAttributes,
mtm: MainThreadMarker,
) -> Option<Id<WinitWindow>> {
autoreleasepool(|_| {
let screen = match attrs.fullscreen.clone().map(Into::into) {
Some(Fullscreen::Borderless(Some(monitor)))
Expand Down Expand Up @@ -579,6 +586,7 @@ fn new_window(attrs: &WindowAttributes, mtm: MainThreadMarker) -> Option<Id<Wini
}

let view = WinitView::new(
app_delegate,
&window,
attrs.platform_specific.accepts_first_mouse,
attrs.platform_specific.option_as_alt,
Expand Down Expand Up @@ -618,8 +626,12 @@ fn new_window(attrs: &WindowAttributes, mtm: MainThreadMarker) -> Option<Id<Wini
}

impl WindowDelegate {
pub fn new(attrs: WindowAttributes, mtm: MainThreadMarker) -> Result<Id<Self>, RootOsError> {
let window = new_window(&attrs, mtm)
pub(super) fn new(
app_delegate: &ApplicationDelegate,
attrs: WindowAttributes,
mtm: MainThreadMarker,
) -> Result<Id<Self>, RootOsError> {
let window = new_window(app_delegate, &attrs, mtm)
.ok_or_else(|| os_error!(OsError::CreationError("couldn't create `NSWindow`")))?;

#[cfg(feature = "rwh_06")]
Expand Down Expand Up @@ -660,6 +672,7 @@ impl WindowDelegate {
};

let delegate = mtm.alloc().set_ivars(State {
app_delegate: app_delegate.retain(),
window: window.retain(),
current_theme: Cell::new(current_theme),
previous_position: Cell::new(None),
Expand Down Expand Up @@ -756,8 +769,7 @@ impl WindowDelegate {
}

pub(crate) fn queue_event(&self, event: WindowEvent) {
let app_delegate = ApplicationDelegate::get(MainThreadMarker::from(self));
app_delegate.queue_window_event(self.window().id(), event);
self.ivars().app_delegate.queue_window_event(self.window().id(), event);
}

fn queue_static_scale_factor_changed_event(&self) {
Expand All @@ -770,8 +782,7 @@ impl WindowDelegate {
let content_size = self.window().contentRectForFrameRect(self.window().frame()).size;
let content_size = LogicalSize::new(content_size.width, content_size.height);

let app_delegate = ApplicationDelegate::get(MainThreadMarker::from(self));
app_delegate.queue_static_scale_factor_changed_event(
self.ivars().app_delegate.queue_static_scale_factor_changed_event(
self.window().retain(),
content_size.to_physical(scale_factor),
scale_factor,
Expand Down Expand Up @@ -833,8 +844,7 @@ impl WindowDelegate {
}

pub fn request_redraw(&self) {
let app_delegate = ApplicationDelegate::get(MainThreadMarker::from(self));
app_delegate.queue_redraw(self.window().id());
self.ivars().app_delegate.queue_redraw(self.window().id());
}

#[inline]
Expand Down

0 comments on commit c801b69

Please sign in to comment.