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<WindowState>` 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<WindowState>` 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

@@ -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) {
+extern "C" fn window_should_close(this: &Object, _: Sel, _sender: id) -> BOOL {
-    // Hack for breaking circular references. We store the value of retainCount
+    let state = unsafe { WindowState::from_view(this) };
-    // 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 {
+    state.trigger_event(Event::Window(WindowEvent::WillClose));
-        let retain_count: usize = msg_send![this, retainCount];
 
-        let state_ptr: *mut c_void = *this.get_ivar(BASEVIEW_STATE_IVAR);
+    state.window_inner.close();
 
-        if !state_ptr.is_null() {
+    NO
-            let retain_count_after_build = WindowState::from_view(this).retain_count_after_build;
-
-            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];
-    }
 }
 
 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);
 }

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::rc::Rc;
-use std::sync::Arc;
 
 use cocoa::appkit::{
     NSApp, NSApplication, NSApplicationActivationPolicyRegular, NSBackingStoreBuffered,
@@ -23,8 +22,8 @@ use raw_window_handle::{
 };
 
 use crate::{
-    Event, EventStatus, MouseCursor, Size, WindowEvent, WindowHandler, WindowInfo,
+    Event, EventStatus, MouseCursor, Size, WindowHandler, WindowInfo, WindowOpenOptions,
-    WindowOpenOptions, WindowScalePolicy,
+    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<RawWindowHandle>,
+    state: Rc<WindowState>,
-    close_requested: Arc<AtomicBool>,
-    is_open: Arc<AtomicBool>,
 }
 
 impl WindowHandle {
     pub fn close(&mut self) {
-        if self.raw_window_handle.take().is_some() {
+        self.state.window_inner.close();
-            self.close_requested.store(true, Ordering::Relaxed);
-        }
     }
 
     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 {
+        self.state.window_inner.raw_window_handle()
-            if self.is_open.load(Ordering::Relaxed) {
-                return raw_window_handle;
-            }
-        }
-
-        RawWindowHandle::AppKit(AppKitWindowHandle::empty())
     }
 }
 
-struct ParentHandle {
+pub(super) struct WindowInner {
-    _close_requested: Arc<AtomicBool>,
+    open: Cell<bool>,
-    is_open: Arc<AtomicBool>,
-}
 
-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)
-    }
-    */
-}
-
-impl Drop for ParentHandle {
-    fn drop(&mut self) {
-        self.is_open.store(false, Ordering::Relaxed);
-    }
-}
-
-struct WindowInner {
     /// Only set if we created the parent window, i.e. we are running in
     /// parentless mode
     ns_app: Cell<Option<id>>,
@@ -104,12 +63,61 @@ struct WindowInner {
     ns_window: Cell<Option<id>>,
     /// Our subclassed NSView
     ns_view: id,
-    close_requested: Cell<bool>,
 
     #[cfg(feature = "opengl")]
     gl_context: Option<GlContext>,
 }
 
+impl WindowInner {
+    pub(super) fn close(&self) {
+        if self.open.get() {
+            self.open.set(false);
+
+            unsafe {
+                // Take back ownership of the NSView's Rc<WindowState>
+                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<H, B>(
+    fn init<H, B>(window_inner: WindowInner, window_info: WindowInfo, build: B) -> WindowHandle
-        parented: bool, 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,16 +273,17 @@ 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
+        if self.inner.open.get() {
-        //       the size is in fractional pixels.
+            // 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) };
@@ -291,7 +291,8 @@ impl<'a> Window<'a> {
                 let _: () = msg_send![self.inner.ns_view, setNeedsDisplay: YES];
             }
 
-        // When using OpenGL the `NSOpenGLView` needs to be resized separately? Why? Because macOS.
+            // 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);
@@ -302,6 +303,7 @@ impl<'a> Window<'a> {
                 unsafe { NSWindow::setContentSize_(ns_window, size) };
             }
         }
+    }
 
     pub fn set_mouse_cursor(&mut self, _mouse_cursor: MouseCursor) {
         todo!()
@@ -324,22 +326,28 @@ impl<'a> Window<'a> {
 }
 
 pub(super) struct WindowState {
-    window_inner: WindowInner,
+    pub(super) window_inner: WindowInner,
     window_handler: RefCell<Box<dyn WindowHandler>>,
     keyboard_state: KeyboardState,
     frame_timer: Cell<Option<CFRunLoopTimer>>,
-    _parent_handle: Cell<Option<ParentHandle>>,
-    pub retain_count_after_build: usize,
     /// The last known window info for this window.
     pub window_info: Cell<WindowInfo>,
 }
 
 impl WindowState {
-    /// Returns a reference to the `WindowState` held by a given `NSView`
+    /// Gets the `WindowState` held by a given `NSView`.
-    pub(super) unsafe fn from_view(view: &Object) -> &Self {
+    ///
+    /// This method returns a cloned `Rc<WindowState>` rather than just a `&WindowState`, since the
+    /// original `Rc<WindowState>` owned by the `NSView` can be dropped at any time
+    /// (including during an event handler).
+    pub(super) unsafe fn from_view(view: &Object) -> Rc<WindowState> {
         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<WindowState> 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;
+        self.inner.raw_window_handle()
-
-        let mut handle = AppKitWindowHandle::empty();
-        handle.ns_window = ns_window;
-        handle.ns_view = self.inner.ns_view as *mut c_void;
-
-        RawWindowHandle::AppKit(handle)
     }
 }