Fix memory leak when GUI window is closed

[dathinaios]

As per #197

The changes made here appear to fix the issue for Mac and Windows but I am not sure this is the correct way to solve it. I haven't tested on Linux, could someone check if the issue also happens there?

[micahrj]

Good work on diagnosing this and coming up with a working fix! Unfortunately I don't think this is really the correct fix.

First, it shouldn't be necessary to remove subviews when closing the window, as subviews don't keep their superview alive, so this change should have the same effect without the calls to subview.removeFromSuperview(). That means that the leak is actually being fixed by the calls to msg_send![subview, release]. It should never be necessary to explicitly loop over and release subviews like this either, as an NSView already releases all of it subviews when it itself is released, so I think this change is just canceling out an unmatched retain call from somewhere else.

It looks like the only place where we call addSubview on Baseview's NSView is in the OpenGL code:

baseview/src/gl/macos.rs

Line 96 in cd4df61

parent_view.addSubview_(view);

Right above that line is a call to retain which should not be necessary:

baseview/src/gl/macos.rs

Line 94 in cd4df61

let () = msg_send![view, retain];

I think the true fix for this leak is to just remove that line.

[dathinaios]

Thank you for the detailed review. I tested the requested changes and they seem to work great 👍

[micahrj]

The windows portion of the change isn't correct either; the purpose of the BV_WINDOW_MUST_CLOSE message is to trigger a DestroyWindow call here:

baseview/src/win/window.rs

Lines 453 to 456 in cd4df61

	BV_WINDOW_MUST_CLOSE => {
	DestroyWindow(hwnd);
	Some(0)
	}

The rationale for doing that is to defer the actual DestroyWindow call until after the current event handler finishes running. So at very least, it is definitely wrong to have both the PostMessageW call and the DestroyWindow call in WindowHandle::close. I think there probably needs to be some larger restructuring of how cleanup is handled on Windows.

[dathinaios]

Yes, I see. I have commented out the PostMessageW call for now. I think it makes sense to have an inelegant deallocation instead of a huge memory leak. We can then leave the issue open until someone gets to restructure the Windows code (unfortunately not something I know how to do as you can tell).

[micahrj]

I would rather merge this PR with just the macOS fix and then implement a proper fix for Windows in a separate PR. The problem with the DestroyWindow call is not that it's inelegant, it's that it is incorrect.

[micahrj]

After doing some local testing on Windows, I observe the same memory leak in the gain_gui_vizia example with and without these changes, so it doesn't look like they fix the issue there anyway.

[micahrj]

I was able to find the root cause of the memory leak on Windows (#199), so the Windows changes in this PR can be removed.

[dathinaios]

Ok, tested with the Gain Vizia example multiple times (Parallels Win 11 on an Apple Silicon Mac):

• Without any of our changes there is a huge memory leak
• With your change only (#199) there is a leak
• With my change only there is a leak
• With both changes there is no memory leak

[dathinaios]

Just to enquire. Was there a reason that this has not been merged? Have been using the change since this PR without issues.