macos: fix small memory leak in surface tree when closing splits

This fixes a small memory leak I found where the `SplitNode.Leaf` was
not being deinitialized properly when closing a split. It would get
deinitialized the next time a split was made or the window was closed,
so the leak wasn't big. The surface view underneath the split was also
properly deinitialized because we forced it, so again, the leak was
quite small.

But conceptually this is a big problem, because when we change the
surface tree we expect the deinit chain to propagate properly through
the whole thing, _including_ to the SurfaceView.

This fixes that by removing the `id(node)` call. I don't find this to be
necessary anymore. I don't know when that happened but we've changed
quite a lot in our split system since it was introduced. I'm also not
100% sure why the `id(node)` was causing a strong reference to begin
with... which bothers me a bit.

AI note: While I manually hunted this down, I started up Claude Code and
Codex in separate tabs to also hunt for the memory leak. They both
failed to find it and offered solutions that didn't work.
This commit is contained in:
Mitchell Hashimoto
2025-06-02 13:57:33 -07:00
parent 957ddd00dd
commit d1f1be8833
4 changed files with 7 additions and 33 deletions

View File

@@ -149,10 +149,8 @@ class BaseTerminalController: NSWindowController,
///
/// Subclasses should call super first.
func surfaceTreeDidChange(from: Ghostty.SplitNode?, to: Ghostty.SplitNode?) {
// If our surface tree becomes nil then ensure all surfaces
// in the old tree have closed.
// If our surface tree becomes nil then we have no focused surface.
if (to == nil) {
from?.close()
focusedSurface = nil
}
}

View File

@@ -102,19 +102,6 @@ extension Ghostty {
}
}
/// Close the surface associated with this node. This will likely deinitialize the
/// surface. At this point, the surface view in this node tree can never be used again.
func close() {
switch (self) {
case .leaf(let leaf):
leaf.surface.close()
case .split(let container):
container.topLeft.close()
container.bottomRight.close()
}
}
/// Returns true if any surface in the split stack requires quit confirmation.
func needsConfirmQuit() -> Bool {
switch (self) {
@@ -224,7 +211,7 @@ extension Ghostty {
self.app = app
self.surface = SurfaceView(app, baseConfig: baseConfig, uuid: uuid)
}
// MARK: - Hashable
func hash(into hasher: inout Hasher) {

View File

@@ -75,7 +75,6 @@ extension Ghostty {
.onReceive(pubZoom) { onZoom(notification: $0) }
}
}
.id(node) // Needed for change detection on node
} else {
// On these events we want to reset the split state and call it.
let pubSplit = center.publisher(for: Notification.ghosttyNewSplit, object: zoomedSurface!)
@@ -289,7 +288,7 @@ extension Ghostty {
let neighbors: SplitNode.Neighbors
@Binding var node: SplitNode?
@StateObject var container: SplitNode.Container
@ObservedObject var container: SplitNode.Container
var body: some View {
SplitView(
@@ -331,7 +330,6 @@ extension Ghostty {
}
// Closing
container.topLeft.close()
node = container.bottomRight
switch (node) {
@@ -362,7 +360,6 @@ extension Ghostty {
}
// Closing
container.bottomRight.close()
node = container.topLeft
switch (node) {

View File

@@ -279,22 +279,14 @@ extension Ghostty {
// Remove ourselves from secure input if we have to
SecureInput.shared.removeScoped(ObjectIdentifier(self))
guard let surface = self.surface else { return }
ghostty_surface_free(surface)
}
/// Close the surface early. This will free the associated Ghostty surface and the view will
/// no longer render. The view can never be used again. This is a way for us to free the
/// Ghostty resources while references may still be held to this view. I've found that SwiftUI
/// tends to hold this view longer than it should so we free the expensive stuff explicitly.
func close() {
// Remove any notifications associated with this surface
let identifiers = Array(self.notificationIdentifiers)
UNUserNotificationCenter.current().removeDeliveredNotifications(withIdentifiers: identifiers)
guard let surface = self.surface else { return }
ghostty_surface_free(surface)
self.surface = nil
// Free our core surface resources
if let surface = self.surface {
ghostty_surface_free(surface)
}
}
func focusDidChange(_ focused: Bool) {