Regression in 0.28 - NSWindow leaks on macOS

[lunixbochs]

I bisected to 340f951, this appears to be caused by the objc2 port @madsmtm

This caused a worse issue for me downstream than just a memory leak - my windows have strong references to metal layers on them, so after opening/closing enough windows, the system runs out of IOSurface or something and metal breaks system-wide.

I reproduced the issue by modifying examples/window.rs to the following and running cargo run --example window to basically create and drop a window every iteration. With the issue, memory usage grows unbounded. In 0.27.5, memory usage fluctuated but was stable. Xcode debugger suggests the NSWindow objects are being leaked.

#![allow(clippy::single_match)]

use simple_logger::SimpleLogger;
use winit::{
    event::{Event, WindowEvent},
    event_loop::EventLoop,
    window::WindowBuilder,
};

fn main() {
    SimpleLogger::new().init().unwrap();
    let event_loop = EventLoop::new();

    let mut window: Option<winit::window::Window> = None;

    event_loop.run(move |event, window_target, control_flow| {
        control_flow.set_wait();
        println!("{:?}", event);

        match event {
            Event::MainEventsCleared => {
                window.replace(WindowBuilder::new()
                    .with_title("A fantastic window!")
                    .with_inner_size(winit::dpi::LogicalSize::new(128.0, 128.0))
                    .build(&window_target)
                    .unwrap());
            }
            _ => (),
        }
    });
}

[lunixbochs]

The WinitWindow has a retainCount of 1, and the WinitView is holding a reference to it. The WinitView also has a retain count of 1. If I manually release the WinitView with a debugger, the window releases on its own.

[madsmtm]

Oh yeah, we're now holding a strong reference to the window in WinitView (WinitView::_ns_window), could probably fix that by changing it to a weak reference.

[lunixbochs]

I removed that reference and it successfully released the window but I think it still leaked the view

[madsmtm]

Hmm, then it might be stuck in an autoreleasepool - could you try inserting objc2::rc::autoreleasepool?

[lunixbochs]

With this example code, the WinitView is still not releasing. I also removed the view observer from the NSNotificationCenter just in case, but that wasn't it.

fn main() {
    SimpleLogger::new().init().unwrap();
    let event_loop = EventLoop::new();

    let mut window: Option<winit::window::Window> = None;
    autoreleasepool(|_| {
        window.replace(WindowBuilder::new()
            .with_title("A fantastic window!")
            .with_inner_size(winit::dpi::LogicalSize::new(128.0, 128.0))
            .build(&event_loop)
            .unwrap());
    });

    event_loop.run(move |event, window_target, control_flow| {
        control_flow.set_wait();
        println!("{event:?}");

        match event {
            Event::MainEventsCleared => {
                autoreleasepool(|_| {
                    window.take();
                });
            }
            _ => (),
        }
    });
}

[lunixbochs]

This is the diff I'm using on WinitView to break the strong reference to the window. Basically, I put an Option<Id> in the _ns_window ivar because IvarDrop doesn't seem to support WeakId yet, and I drop it when we get view_did_move_to_window, because I don't think we need it after that.

diff --git a/src/platform_impl/macos/view.rs b/src/platform_impl/macos/view.rs
index e3a8b05e..146ff219 100644
--- a/src/platform_impl/macos/view.rs
+++ b/src/platform_impl/macos/view.rs
@@ -136,7 +136,7 @@ declare_class!(
     #[derive(Debug)]
     #[allow(non_snake_case)]
     pub(super) struct WinitView {
-        _ns_window: IvarDrop<Id<WinitWindow, Shared>>,
+        _ns_window: IvarDrop<Option<Id<WinitWindow, Shared>>>,
         pub(super) state: IvarDrop<Box<ViewState>>,
         marked_text: IvarDrop<Id<NSMutableAttributedString, Owned>>,
         accepts_first_mouse: bool,
@@ -156,6 +156,7 @@ declare_class!(
         ) -> Option<&mut Self> {
             let this: Option<&mut Self> = unsafe { msg_send![super(self), init] };
             this.map(|this| {
+                println!("WinitView::init_with_id: {:?}", this.__inner);
                 let state = ViewState {
                     cursor_state: Default::default(),
                     ime_position: LogicalPosition::new(0.0, 0.0),
@@ -167,7 +168,7 @@ declare_class!(
                     forward_key_to_app: false,
                 };
 
-                Ivar::write(&mut this._ns_window, window.retain());
+                Ivar::write(&mut this._ns_window, Some(window.retain()));
                 Ivar::write(&mut this.state, Box::new(state));
                 Ivar::write(&mut this.marked_text, NSMutableAttributedString::new());
                 Ivar::write(&mut this.accepts_first_mouse, accepts_first_mouse);
@@ -203,6 +204,9 @@ declare_class!(
             if let Some(tracking_rect) = self.state.tracking_rect.take() {
                 self.removeTrackingRect(tracking_rect);
             }
+            // break the strong reference
+            // we don't need this ivar once we've been attached to a window
+            (*self._ns_window).take();
 
             let rect = self.visibleRect();
             let tracking_rect = self.add_tracking_rect(rect, false);
@@ -873,11 +877,15 @@ impl WinitView {
         // (which is incompatible with `frameDidChange:`)
         //
         // unsafe { msg_send_id![self, window] }
-        (*self._ns_window).clone()
+
+        match &*self._ns_window {
+            Some(window) => window.clone(),
+            None => unsafe { msg_send_id![self, window] },
+        }
     }
 
     fn window_id(&self) -> WindowId {
-        WindowId(self._ns_window.id())
+        WindowId(self.window().id())
     }
 
     fn queue_event(&self, event: WindowEvent<'static>) {

[madsmtm]

Wow, that took me a while to find... The issue is that objc2 didn't run dealloc on custom structs in declare_class!, which I fixed a while ago in madsmtm/objc2@077a964.

This fix is included in objc2 v0.3.0-beta.4, but we're currently using v0.3.0-beta.3 because I haven't taken the time to update it yet - see also #2724.

[madsmtm]

To fix it in the interim (because the objc2 upgrade might be a bit of trouble) we can avoid IvarDrop in WinitView and WinitWindow and implement dealloc manually instead.

[lunixbochs]

For my own purposes, because I'm comfortable patching the crates I'm shipping, would you recommend just bumping my tree to objc2-0.3.0-beta.5 for now, or will that cause other issues? (The leak is impacting beta users today and I'm trying to both get a fix out now and avoid rolling back winit)

Edit: ah I see that seems to break a lot of stuff, dealloc it is then. Do I need to call superclass dealloc?

[madsmtm]

Yes, that's the problem (that we didn't do that before)

[lunixbochs]

Hrm is it easier to cherry-pick that bugfix as objc2-0.3.0-beta3.1 or something? I started doing the ivar thing and got annoyed with it pretty quickly.

[lunixbochs]

I think cherry-picking that objc2 commit fixed it for me, I'm going to go with a cargo patch on my side, IMO you should just see if you can get that commit into a hotfixed objc2 version for winit

[madsmtm]

Well, the problem is that pre-release versions are not patch-able like this as far as I know (which I wish I'd known when I started using them, but oh well, live and learn).

[lunixbochs]

It also feels weird for a pre-release version of objc2 to be a core dependency of a non-pre-release version of winit.

Well, for anyone hitting this, my mitigation is the patch in this issue: #2722 (comment)
Plus a cargo patch for objc2:

[patch.crates-io]
objc2 = { git = "https://github.com/talonvoice/objc2", branch = "talon" }

[lunixbochs]

okay, my fix may have introduced a crash around the NSView?

[madsmtm]

You might be able to use IvarDrop<Box<WeakId<...>>> to work around WeakId not being usable directly inside IvarDrop yet.