From 2c1b1a7b0fef1a29a5150a6a8f6fef6a0cbab8c4 Mon Sep 17 00:00:00 2001 From: Micah Johnston Date: Sun, 17 Dec 2023 22:57:51 -0600 Subject: [PATCH] Fix window cleanup logic on macOS (#164) Instead of trying to detect when to clean up the window based on the `NSView`'s retain count, require window cleanup to be initiated explicitly via `Window::close`, `WindowHandle::close`, or `[NSWindowDelegate windowShouldClose:]` (in non-parented mode; called when the user clicks the "X" button). This fixes the leaks and use-after-frees that can be caused by the inherent unreliability of the retain count logic. As discussed in #153, this change essentially means that the `NSView` created by Baseview will not be suitable as the top-level view for an Audio Unit, since the Baseview API now requires that child windows be cleaned up by an explicit call to `WindowHandle::close`, and the only reliable signal for cleaning up an Audio Unit view is a call to `[NSView dealloc]`. However, this does not mean that Baseview cannot be used in the context of an Audio Unit; it just means that plugin frameworks must implement a compatibility layer with a wrapper `NSView` (which is the approach taken by JUCE). In order to implement this change: - `WindowState` is stored in an `Rc` rather than a `Box`. - `WindowHandle` holds an `Rc` so that `WindowHandle::close` can directly invoke window cleanup logic. - Since the window can be closed during an event handler, `WindowState::from_view` now returns a clone of the `Rc` held by the `NSView` to ensure that it lives until the end of an event handler. - In the non-parented case, the `NSView` is set as the window delegate, which allows it to receive the `windowShouldClose:` call when the user clicks the "X" button, upon which it will dispatch the `WillClose` event and initiate window cleanup logic. - `Window::open_parented` and `open_blocking` no longer `release` the `NSView` immediately after attaching it. Instead, the `NSView` is released as part of the cleanup logic in `WindowInner::close`. - `Window::resize` now checks if the window is open to avoid using the `NSView` after releasing it. - The overridden `release` method, the `retain_count_after_build` field, the `ParentHandle` struct, and the `close_requested` flag have all been removed. --- src/macos/view.rs | 44 ++------ src/macos/window.rs | 260 +++++++++++++++++--------------------------- 2 files changed, 114 insertions(+), 190 deletions(-) diff --git a/src/macos/view.rs b/src/macos/view.rs index 0354e04c..66dd57ad 100644 --- a/src/macos/view.rs +++ b/src/macos/view.rs @@ -134,7 +134,10 @@ unsafe fn create_view_class() -> &'static Class { accepts_first_mouse as extern "C" fn(&Object, Sel, id) -> BOOL, ); - class.add_method(sel!(release), release as extern "C" fn(&mut Object, Sel)); + class.add_method( + sel!(windowShouldClose:), + window_should_close as extern "C" fn(&Object, Sel, id) -> BOOL, + ); class.add_method(sel!(dealloc), dealloc as extern "C" fn(&mut Object, Sel)); class.add_method( sel!(viewWillMoveToWindow:), @@ -205,37 +208,14 @@ extern "C" fn accepts_first_mouse(_this: &Object, _sel: Sel, _event: id) -> BOOL YES } -extern "C" fn release(this: &mut Object, _sel: Sel) { - // Hack for breaking circular references. We store the value of retainCount - // after build(), and then when retainCount drops back to that value, we - // drop the WindowState, hoping that any circular references it holds back - // to the NSView (e.g. wgpu surfaces) get released. - // - // This is definitely broken, since it can be thwarted by e.g. creating a - // wgpu surface at some point after build() (which will mean the NSView - // never gets dealloced) or dropping a wgpu surface at some point before - // drop() (which will mean the WindowState gets dropped early). - // - // TODO: Find a better solution for circular references. - - unsafe { - let retain_count: usize = msg_send![this, retainCount]; +extern "C" fn window_should_close(this: &Object, _: Sel, _sender: id) -> BOOL { + let state = unsafe { WindowState::from_view(this) }; - let state_ptr: *mut c_void = *this.get_ivar(BASEVIEW_STATE_IVAR); + state.trigger_event(Event::Window(WindowEvent::WillClose)); - if !state_ptr.is_null() { - let retain_count_after_build = WindowState::from_view(this).retain_count_after_build; + state.window_inner.close(); - if retain_count <= retain_count_after_build { - WindowState::stop_and_free(this); - } - } - } - - unsafe { - let superclass = msg_send![this, superclass]; - let () = msg_send![super(this, superclass), release]; - } + NO } extern "C" fn dealloc(this: &mut Object, _sel: Sel) { @@ -446,7 +426,7 @@ extern "C" fn dragging_entered(this: &Object, _sel: Sel, sender: id) -> NSUInteg data: drop_data, }; - on_event(state, event) + on_event(&state, event) } extern "C" fn dragging_updated(this: &Object, _sel: Sel, sender: id) -> NSUInteger { @@ -460,7 +440,7 @@ extern "C" fn dragging_updated(this: &Object, _sel: Sel, sender: id) -> NSUInteg data: drop_data, }; - on_event(state, event) + on_event(&state, event) } extern "C" fn prepare_for_drag_operation(_this: &Object, _sel: Sel, _sender: id) -> BOOL { @@ -491,5 +471,5 @@ extern "C" fn perform_drag_operation(this: &Object, _sel: Sel, sender: id) -> BO extern "C" fn dragging_exited(this: &Object, _sel: Sel, _sender: id) { let state = unsafe { WindowState::from_view(this) }; - on_event(state, MouseEvent::DragLeft); + on_event(&state, MouseEvent::DragLeft); } diff --git a/src/macos/window.rs b/src/macos/window.rs index abef415c..c0412525 100644 --- a/src/macos/window.rs +++ b/src/macos/window.rs @@ -1,8 +1,7 @@ use std::cell::{Cell, RefCell}; use std::ffi::c_void; use std::ptr; -use std::sync::atomic::{AtomicBool, Ordering}; -use std::sync::Arc; +use std::rc::Rc; use cocoa::appkit::{ NSApp, NSApplication, NSApplicationActivationPolicyRegular, NSBackingStoreBuffered, @@ -23,8 +22,8 @@ use raw_window_handle::{ }; use crate::{ - Event, EventStatus, MouseCursor, Size, WindowEvent, WindowHandler, WindowInfo, - WindowOpenOptions, WindowScalePolicy, + Event, EventStatus, MouseCursor, Size, WindowHandler, WindowInfo, WindowOpenOptions, + WindowScalePolicy, }; use super::keyboard::KeyboardState; @@ -34,68 +33,28 @@ use super::view::{create_view, BASEVIEW_STATE_IVAR}; use crate::gl::{GlConfig, GlContext}; pub struct WindowHandle { - raw_window_handle: Option, - close_requested: Arc, - is_open: Arc, + state: Rc, } impl WindowHandle { pub fn close(&mut self) { - if self.raw_window_handle.take().is_some() { - self.close_requested.store(true, Ordering::Relaxed); - } + self.state.window_inner.close(); } pub fn is_open(&self) -> bool { - self.is_open.load(Ordering::Relaxed) + self.state.window_inner.open.get() } } unsafe impl HasRawWindowHandle for WindowHandle { fn raw_window_handle(&self) -> RawWindowHandle { - if let Some(raw_window_handle) = self.raw_window_handle { - if self.is_open.load(Ordering::Relaxed) { - return raw_window_handle; - } - } - - RawWindowHandle::AppKit(AppKitWindowHandle::empty()) - } -} - -struct ParentHandle { - _close_requested: Arc, - is_open: Arc, -} - -impl ParentHandle { - pub fn new(raw_window_handle: RawWindowHandle) -> (Self, WindowHandle) { - let close_requested = Arc::new(AtomicBool::new(false)); - let is_open = Arc::new(AtomicBool::new(true)); - - let handle = WindowHandle { - raw_window_handle: Some(raw_window_handle), - close_requested: Arc::clone(&close_requested), - is_open: Arc::clone(&is_open), - }; - - (Self { _close_requested: close_requested, is_open }, handle) - } - - /* - pub fn parent_did_drop(&self) -> bool { - self.close_requested.load(Ordering::Relaxed) + self.state.window_inner.raw_window_handle() } - */ } -impl Drop for ParentHandle { - fn drop(&mut self) { - self.is_open.store(false, Ordering::Relaxed); - } -} +pub(super) struct WindowInner { + open: Cell, -struct WindowInner { /// Only set if we created the parent window, i.e. we are running in /// parentless mode ns_app: Cell>, @@ -104,12 +63,61 @@ struct WindowInner { ns_window: Cell>, /// Our subclassed NSView ns_view: id, - close_requested: Cell, #[cfg(feature = "opengl")] gl_context: Option, } +impl WindowInner { + pub(super) fn close(&self) { + if self.open.get() { + self.open.set(false); + + unsafe { + // Take back ownership of the NSView's Rc + let state_ptr: *const c_void = *(*self.ns_view).get_ivar(BASEVIEW_STATE_IVAR); + let window_state = Rc::from_raw(state_ptr as *mut WindowState); + + // Cancel the frame timer + if let Some(frame_timer) = window_state.frame_timer.take() { + CFRunLoop::get_current().remove_timer(&frame_timer, kCFRunLoopDefaultMode); + } + + drop(window_state); + + // Close the window if in non-parented mode + if let Some(ns_window) = self.ns_window.take() { + ns_window.close(); + } + + // Ensure that the NSView is detached from the parent window + self.ns_view.removeFromSuperview(); + let () = msg_send![self.ns_view as id, release]; + + // If in non-parented mode, we want to also quit the app altogether + let app = self.ns_app.take(); + if let Some(app) = app { + app.stop_(app); + } + } + } + } + + fn raw_window_handle(&self) -> RawWindowHandle { + if self.open.get() { + let ns_window = self.ns_window.get().unwrap_or(ptr::null_mut()) as *mut c_void; + + let mut handle = AppKitWindowHandle::empty(); + handle.ns_window = ns_window; + handle.ns_view = self.ns_view as *mut c_void; + + return RawWindowHandle::AppKit(handle); + } + + RawWindowHandle::AppKit(AppKitWindowHandle::empty()) + } +} + pub struct Window<'a> { inner: &'a WindowInner, } @@ -140,10 +148,10 @@ impl<'a> Window<'a> { let ns_view = unsafe { create_view(&options) }; let window_inner = WindowInner { + open: Cell::new(true), ns_app: Cell::new(None), ns_window: Cell::new(None), ns_view, - close_requested: Cell::new(false), #[cfg(feature = "opengl")] gl_context: options @@ -151,11 +159,10 @@ impl<'a> Window<'a> { .map(|gl_config| Self::create_gl_context(None, ns_view, gl_config)), }; - let window_handle = Self::init(true, window_inner, window_info, build); + let window_handle = Self::init(window_inner, window_info, build); unsafe { let _: id = msg_send![handle.ns_view as *mut Object, addSubview: ns_view]; - let () = msg_send![ns_view as id, release]; let () = msg_send![pool, drain]; } @@ -216,10 +223,10 @@ impl<'a> Window<'a> { let ns_view = unsafe { create_view(&options) }; let window_inner = WindowInner { + open: Cell::new(true), ns_app: Cell::new(Some(app)), ns_window: Cell::new(Some(ns_window)), ns_view, - close_requested: Cell::new(false), #[cfg(feature = "opengl")] gl_context: options @@ -227,21 +234,19 @@ impl<'a> Window<'a> { .map(|gl_config| Self::create_gl_context(Some(ns_window), ns_view, gl_config)), }; - let _ = Self::init(false, window_inner, window_info, build); + let _ = Self::init(window_inner, window_info, build); unsafe { ns_window.setContentView_(ns_view); + ns_window.setDelegate_(ns_view); - let () = msg_send![ns_view as id, release]; let () = msg_send![pool, drain]; app.run(); } } - fn init( - parented: bool, window_inner: WindowInner, window_info: WindowInfo, build: B, - ) -> WindowHandle + fn init(window_inner: WindowInner, window_info: WindowInfo, build: B) -> WindowHandle where H: WindowHandler + 'static, B: FnOnce(&mut crate::Window) -> H, @@ -250,23 +255,17 @@ impl<'a> Window<'a> { let mut window = crate::Window::new(Window { inner: &window_inner }); let window_handler = Box::new(build(&mut window)); - let (parent_handle, window_handle) = ParentHandle::new(window.raw_window_handle()); - let parent_handle = if parented { Some(parent_handle) } else { None }; - - let retain_count_after_build: usize = - unsafe { msg_send![window_inner.ns_view, retainCount] }; - let ns_view = window_inner.ns_view; - let window_state_ptr = Box::into_raw(Box::new(WindowState { + let window_state = Rc::new(WindowState { window_inner, window_handler: RefCell::new(window_handler), keyboard_state: KeyboardState::new(), frame_timer: Cell::new(None), - retain_count_after_build, window_info: Cell::new(window_info), - _parent_handle: Cell::new(parent_handle), - })); + }); + + let window_state_ptr = Rc::into_raw(Rc::clone(&window_state)); unsafe { (*ns_view).set_ivar(BASEVIEW_STATE_IVAR, window_state_ptr as *const c_void); @@ -274,32 +273,35 @@ impl<'a> Window<'a> { WindowState::setup_timer(window_state_ptr); } - window_handle + WindowHandle { state: window_state } } pub fn close(&mut self) { - self.inner.close_requested.set(true); + self.inner.close(); } pub fn resize(&mut self, size: Size) { - // NOTE: macOS gives you a personal rave if you pass in fractional pixels here. Even though - // the size is in fractional pixels. - let size = NSSize::new(size.width.round(), size.height.round()); + if self.inner.open.get() { + // NOTE: macOS gives you a personal rave if you pass in fractional pixels here. Even + // though the size is in fractional pixels. + let size = NSSize::new(size.width.round(), size.height.round()); - unsafe { NSView::setFrameSize(self.inner.ns_view, size) }; - unsafe { - let _: () = msg_send![self.inner.ns_view, setNeedsDisplay: YES]; - } + unsafe { NSView::setFrameSize(self.inner.ns_view, size) }; + unsafe { + let _: () = msg_send![self.inner.ns_view, setNeedsDisplay: YES]; + } - // When using OpenGL the `NSOpenGLView` needs to be resized separately? Why? Because macOS. - #[cfg(feature = "opengl")] - if let Some(gl_context) = &self.inner.gl_context { - gl_context.resize(size); - } + // When using OpenGL the `NSOpenGLView` needs to be resized separately? Why? Because + // macOS. + #[cfg(feature = "opengl")] + if let Some(gl_context) = &self.inner.gl_context { + gl_context.resize(size); + } - // If this is a standalone window then we'll also need to resize the window itself - if let Some(ns_window) = self.inner.ns_window.get() { - unsafe { NSWindow::setContentSize_(ns_window, size) }; + // If this is a standalone window then we'll also need to resize the window itself + if let Some(ns_window) = self.inner.ns_window.get() { + unsafe { NSWindow::setContentSize_(ns_window, size) }; + } } } @@ -324,22 +326,28 @@ impl<'a> Window<'a> { } pub(super) struct WindowState { - window_inner: WindowInner, + pub(super) window_inner: WindowInner, window_handler: RefCell>, keyboard_state: KeyboardState, frame_timer: Cell>, - _parent_handle: Cell>, - pub retain_count_after_build: usize, /// The last known window info for this window. pub window_info: Cell, } impl WindowState { - /// Returns a reference to the `WindowState` held by a given `NSView` - pub(super) unsafe fn from_view(view: &Object) -> &Self { + /// Gets the `WindowState` held by a given `NSView`. + /// + /// This method returns a cloned `Rc` rather than just a `&WindowState`, since the + /// original `Rc` owned by the `NSView` can be dropped at any time + /// (including during an event handler). + pub(super) unsafe fn from_view(view: &Object) -> Rc { let state_ptr: *const c_void = *view.get_ivar(BASEVIEW_STATE_IVAR); - &*(state_ptr as *const Self) + let state_rc = Rc::from_raw(state_ptr as *const WindowState); + let state = Rc::clone(&state_rc); + let _ = Rc::into_raw(state_rc); + + state } pub(super) fn trigger_event(&self, event: Event) -> EventStatus { @@ -350,37 +358,6 @@ impl WindowState { pub(super) fn trigger_frame(&self) { let mut window = crate::Window::new(Window { inner: &self.window_inner }); self.window_handler.borrow_mut().on_frame(&mut window); - - let mut do_close = false; - - /* FIXME: Is it even necessary to check if the parent dropped the handle - // in MacOS? - // Check if the parent handle was dropped - if let Some(parent_handle) = &self.parent_handle { - if parent_handle.parent_did_drop() { - do_close = true; - self.window.close_requested = false; - } - } - */ - - // Check if the user requested the window to close - if self.window_inner.close_requested.get() { - do_close = true; - self.window_inner.close_requested.set(false); - } - - if do_close { - unsafe { - let ns_window = self.window_inner.ns_window.take(); - if let Some(ns_window) = ns_window { - ns_window.close(); - } else { - // FIXME: How do we close a non-parented window? Is this even - // possible in a DAW host usecase? - } - } - } } pub(super) fn keyboard_state(&self) -> &KeyboardState { @@ -391,7 +368,6 @@ impl WindowState { self.keyboard_state.process_native_event(event) } - /// Don't call until WindowState pointer is stored in view unsafe fn setup_timer(window_state_ptr: *const WindowState) { extern "C" fn timer_callback(_: *mut __CFRunLoopTimer, window_state_ptr: *mut c_void) { unsafe { @@ -415,43 +391,11 @@ impl WindowState { (*window_state_ptr).frame_timer.set(Some(timer)); } - - /// Call when freeing view - pub(super) unsafe fn stop_and_free(ns_view_obj: &mut Object) { - let state_ptr: *const c_void = *ns_view_obj.get_ivar(BASEVIEW_STATE_IVAR); - - // Take back ownership of Box so that it gets dropped - // when it goes out of scope - let window_state = Box::from_raw(state_ptr as *mut WindowState); - - if let Some(frame_timer) = window_state.frame_timer.take() { - CFRunLoop::get_current().remove_timer(&frame_timer, kCFRunLoopDefaultMode); - } - - // Clear ivar before triggering WindowEvent::WillClose. Otherwise, if the - // handler of the event causes another call to release, this function could be - // called again, leading to a double free. - ns_view_obj.set_ivar(BASEVIEW_STATE_IVAR, ptr::null() as *const c_void); - - window_state.trigger_event(Event::Window(WindowEvent::WillClose)); - - // If in non-parented mode, we want to also quit the app altogether - let app = window_state.window_inner.ns_app.take(); - if let Some(app) = app { - app.stop_(app); - } - } } unsafe impl<'a> HasRawWindowHandle for Window<'a> { fn raw_window_handle(&self) -> RawWindowHandle { - let ns_window = self.inner.ns_window.get().unwrap_or(ptr::null_mut()) as *mut c_void; - - let mut handle = AppKitWindowHandle::empty(); - handle.ns_window = ns_window; - handle.ns_view = self.inner.ns_view as *mut c_void; - - RawWindowHandle::AppKit(handle) + self.inner.raw_window_handle() } }