Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix window cleanup logic on macOS #164

Merged
merged 1 commit into from
Dec 18, 2023

Conversation

micahrj
Copy link
Member

@micahrj micahrj commented Dec 17, 2023

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.

@micahrj micahrj marked this pull request as ready for review December 17, 2023 21:29
@micahrj micahrj requested review from robbert-vdh and Engid December 17, 2023 21:29
@Engid
Copy link
Contributor

Engid commented Dec 17, 2023

@glowcoil Beautiful code and great detailed descriptions here! thank you. I just have a basic question so please forgive me: Were you able to change the Arc's in window.rs to an Rc because as you described

In the non-parented case, the NSView is set as the window delegate, which allows it to receive the windowShouldClose

..? In other words, how can I tell that its safe to switch to Rc? Thank you.

Copy link
Contributor

@Engid Engid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Optimistically approving but hoping to hear back from @maxjvh with some QA testing confirmation

@micahrj
Copy link
Member Author

micahrj commented Dec 17, 2023

@Engid The Arcs and atomics were actually never necessary in the first place, since the entire macOS backend has always been completely single-threaded. I think they were just copied unchanged from the X11 backend, which is multithreaded.

@micahrj
Copy link
Member Author

micahrj commented Dec 18, 2023

Tested this PR using the gain_gui_vizia example plugin in the NIH-plug repository. On both the current master branch (robbert-vdh/nih-plug@e211029) and the vizia-update branch (robbert-vdh/nih-plug@cd5b259), when running in Bitwig on macOS, the WindowHandler does not get dropped when the editor window is closed. After updating Vizia to use this branch of Baseview, the WindowHandler is successfully dropped when the editor window is closed.

@micahrj micahrj merged commit 2c1b1a7 into RustAudio:master Dec 18, 2023
3 checks passed
@micahrj micahrj deleted the macos-close-fixes branch December 18, 2023 04:57
@greatest-ape
Copy link
Contributor

Excellent!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants