Closed lunixbochs closed 1 year ago
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.
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.
I removed that reference and it successfully released the window but I think it still leaked the view
Hmm, then it might be stuck in an autoreleasepool - could you try inserting objc2::rc::autoreleasepool
?
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();
});
}
_ => (),
}
});
}
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>) {
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 https://github.com/madsmtm/objc2/commit/077a964661a04b67f6640fa97529b860ab5adb1d.
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 https://github.com/rust-windowing/winit/issues/2724.
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.
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?
Yes, that's the problem (that we didn't do that before)
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.
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
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).
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: https://github.com/rust-windowing/winit/issues/2722#issuecomment-1460743705 Plus a cargo patch for objc2:
[patch.crates-io]
objc2 = { git = "https://github.com/talonvoice/objc2", branch = "talon" }
okay, my fix may have introduced a crash around the NSView?
You might be able to use IvarDrop<Box<WeakId<...>>>
to work around WeakId
not being usable directly inside IvarDrop
yet.
I bisected to 340f951d10c74169c258a120530b1eadf3a3e9fe, 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 runningcargo 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 theNSWindow
objects are being leaked.