Skip to content

Commit

Permalink
Fix window cleanup logic on macOS (RustAudio#164)
Browse files Browse the repository at this point in the history
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 RustAudio#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.
  • Loading branch information
micahrj authored Dec 18, 2023
1 parent fdb43ea commit 2c1b1a7
Show file tree
Hide file tree
Showing 2 changed files with 114 additions and 190 deletions.
44 changes: 12 additions & 32 deletions src/macos/view.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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:),
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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 {
Expand All @@ -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 {
Expand Down Expand Up @@ -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);
}
Loading

0 comments on commit 2c1b1a7

Please sign in to comment.