RustAudio / baseview

low-level window system interface for audio plugin UIs
Apache License 2.0
259 stars 54 forks source link

Fix window cleanup logic on macOS #164

Closed glowcoil closed 7 months ago

glowcoil commented 7 months ago

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:

Engid commented 7 months ago

@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.

glowcoil commented 7 months ago

@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.

glowcoil commented 7 months ago

Tested this PR using the gain_gui_vizia example plugin in the NIH-plug repository. On both the current master branch (https://github.com/robbert-vdh/nih-plug/commit/e211029979a4a4aaece4e0bc3362c312f4ef92e4) and the vizia-update branch (https://github.com/robbert-vdh/nih-plug/commit/cd5b2592a80c2981f038c36fd0c530771a2d7414), 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.

greatest-ape commented 5 months ago

Excellent!