diff --git a/Sources/GhosttyTerminalView.swift b/Sources/GhosttyTerminalView.swift index a4198e39..fe39e6bf 100644 --- a/Sources/GhosttyTerminalView.swift +++ b/Sources/GhosttyTerminalView.swift @@ -2789,6 +2789,7 @@ final class TerminalSurfaceRegistry { private let lock = NSLock() private let surfaces = NSHashTable.weakObjects() + private var runtimeSurfaceOwners: [UInt: UUID] = [:] private init() {} @@ -2798,6 +2799,26 @@ final class TerminalSurfaceRegistry { surfaces.add(surface) } + func registerRuntimeSurface(_ surface: ghostty_surface_t, ownerId: UUID) { + lock.lock() + defer { lock.unlock() } + runtimeSurfaceOwners[UInt(bitPattern: surface)] = ownerId + } + + func unregisterRuntimeSurface(_ surface: ghostty_surface_t, ownerId: UUID) { + lock.lock() + defer { lock.unlock() } + let key = UInt(bitPattern: surface) + guard runtimeSurfaceOwners[key] == ownerId else { return } + runtimeSurfaceOwners.removeValue(forKey: key) + } + + func runtimeSurfaceOwnerId(_ surface: ghostty_surface_t) -> UUID? { + lock.lock() + defer { lock.unlock() } + return runtimeSurfaceOwners[UInt(bitPattern: surface)] + } + func allSurfaces() -> [TerminalSurface] { lock.lock() let objects = surfaces.allObjects.compactMap { $0 as? TerminalSurface } @@ -2828,10 +2849,11 @@ final class TerminalSurface: Identifiable, ObservableObject { /// Whether the runtime Ghostty surface exists and has not begun teardown. /// - /// Use this before passing `surface` to Ghostty C APIs that dereference the - /// pointer (e.g. `ghostty_surface_inherited_config`, `ghostty_surface_quicklook_font`). - /// A non-nil `surface` alone is not sufficient because the underlying native - /// state may already be closing or closed. + /// Use this as a quick availability check. Before passing `surface` to + /// Ghostty C APIs that dereference the pointer (e.g. + /// `ghostty_surface_inherited_config`, `ghostty_surface_quicklook_font`), + /// call `liveSurfaceForGhosttyAccess(reason:)` so stale freed pointers are + /// rejected and quarantined. var hasLiveSurface: Bool { surface != nil && portalLifecycleState == .live } /// Whether the terminal surface view is currently attached to a window. @@ -2884,6 +2906,7 @@ final class TerminalSurface: Identifiable, ObservableObject { private var desiredFocusState: Bool = true #if DEBUG private var needsConfirmCloseOverrideForTesting: Bool? + private var runtimeSurfaceFreedOutOfBandForTesting = false #endif private enum PortalLifecycleState: String { case live @@ -3074,6 +3097,34 @@ final class TerminalSurface: Identifiable, ObservableObject { return true } + @MainActor + func liveSurfaceForGhosttyAccess(reason: String) -> ghostty_surface_t? { + guard hasLiveSurface, let surface else { return nil } + let registry = TerminalSurfaceRegistry.shared + let registeredOwnerId = registry.runtimeSurfaceOwnerId(surface) + guard registeredOwnerId == id, + cmuxSurfacePointerAppearsLive(surface) else { + let callbackContext = surfaceCallbackContext + surfaceCallbackContext = nil + registry.unregisterRuntimeSurface(surface, ownerId: id) + self.surface = nil + activePortalHostLease = nil + recordTeardownRequest(reason: reason) + markPortalLifecycleClosed(reason: reason) +#if DEBUG + let registeredOwnerToken = registeredOwnerId.map { String($0.uuidString.prefix(5)) } ?? "nil" + dlog( + "surface.lifecycle.stale surface=\(id.uuidString.prefix(5)) " + + "workspace=\(tabId.uuidString.prefix(5)) reason=\(reason) " + + "registryOwner=\(registeredOwnerToken)" + ) +#endif + callbackContext?.release() + return nil + } + return surface + } + private static let portalHostAreaThreshold: CGFloat = 4 private static func portalHostArea(for bounds: CGRect) -> CGFloat { @@ -3259,6 +3310,9 @@ final class TerminalSurface: Identifiable, ObservableObject { surfaceCallbackContext = nil let surfaceToFree = surface + if let surfaceToFree { + TerminalSurfaceRegistry.shared.unregisterRuntimeSurface(surfaceToFree, ownerId: id) + } surface = nil guard let surfaceToFree else { @@ -3266,6 +3320,14 @@ final class TerminalSurface: Identifiable, ObservableObject { return } +#if DEBUG + if runtimeSurfaceFreedOutOfBandForTesting { + runtimeSurfaceFreedOutOfBandForTesting = false + callbackContext?.release() + return + } +#endif + Task { @MainActor in // Keep free behavior aligned with deinit: perform the runtime teardown on // the next main-actor turn so SIGHUP delivery is deterministic but non-reentrant. @@ -3649,6 +3711,7 @@ final class TerminalSurface: Identifiable, ObservableObject { return } guard let createdSurface = surface else { return } + TerminalSurfaceRegistry.shared.registerRuntimeSurface(createdSurface, ownerId: id) recordRuntimeSurfaceCreation() // Session scrollback replay must be one-shot. Reusing it on a later runtime @@ -4066,10 +4129,31 @@ final class TerminalSurface: Identifiable, ObservableObject { return } + TerminalSurfaceRegistry.shared.unregisterRuntimeSurface(surfaceToFree, ownerId: id) surface = nil ghostty_surface_free(surfaceToFree) callbackContext?.release() } + + /// Test-only helper to simulate a stale Swift wrapper whose native surface + /// was already freed out-of-band. + @MainActor + func replaceSurfaceWithFreedPointerForTesting() { + guard !runtimeSurfaceFreedOutOfBandForTesting else { return } + + let callbackContext = surfaceCallbackContext + surfaceCallbackContext = nil + + guard let surfaceToFree = surface else { + callbackContext?.release() + return + } + + TerminalSurfaceRegistry.shared.unregisterRuntimeSurface(surfaceToFree, ownerId: id) + ghostty_surface_free(surfaceToFree) + runtimeSurfaceFreedOutOfBandForTesting = true + callbackContext?.release() + } #endif deinit { @@ -4083,6 +4167,9 @@ final class TerminalSurface: Identifiable, ObservableObject { // before this object is fully deallocated will see nil and bail out, // rather than passing a freed pointer to ghostty_surface_refresh (#432). let surfaceToFree = surface + if let surfaceToFree { + TerminalSurfaceRegistry.shared.unregisterRuntimeSurface(surfaceToFree, ownerId: id) + } surface = nil guard let surfaceToFree else { @@ -4096,6 +4183,14 @@ final class TerminalSurface: Identifiable, ObservableObject { return } +#if DEBUG + if runtimeSurfaceFreedOutOfBandForTesting { + runtimeSurfaceFreedOutOfBandForTesting = false + callbackContext?.release() + return + } +#endif + #if DEBUG let surfaceToken = String(id.uuidString.prefix(5)) let workspaceToken = String(tabId.uuidString.prefix(5)) diff --git a/Sources/TabManager.swift b/Sources/TabManager.swift index 6132a277..cd2310ce 100644 --- a/Sources/TabManager.swift +++ b/Sources/TabManager.swift @@ -2216,8 +2216,9 @@ class TabManager: ObservableObject { workspace: Workspace? ) -> ghostty_surface_config_s? { if let panel = terminalPanelForWorkspaceConfigInheritanceSource(workspace: workspace), - panel.surface.hasLiveSurface, - let sourceSurface = panel.surface.surface { + let sourceSurface = panel.surface.liveSurfaceForGhosttyAccess( + reason: "tabManager.inheritedTerminalConfigForNewWorkspace" + ) { return cmuxInheritedSurfaceConfig( sourceSurface: sourceSurface, context: GHOSTTY_SURFACE_CONTEXT_TAB diff --git a/Sources/Workspace.swift b/Sources/Workspace.swift index 8a231de4..b2730e21 100644 --- a/Sources/Workspace.swift +++ b/Sources/Workspace.swift @@ -21,20 +21,35 @@ func cmuxSurfaceContextName(_ context: ghostty_surface_context_e) -> String { } } +private func cmuxPointerAppearsLive(_ pointer: UnsafeMutableRawPointer?) -> Bool { + guard let pointer, + malloc_zone_from_ptr(pointer) != nil else { + return false + } + return malloc_size(pointer) > 0 +} + +func cmuxSurfacePointerAppearsLive(_ surface: ghostty_surface_t) -> Bool { + // Best-effort check: reject pointers that no longer belong to an active + // malloc zone allocation. A Swift wrapper around `ghostty_surface_t` can + // remain non-nil after the backing native surface has already been freed. + cmuxPointerAppearsLive(surface) +} + func cmuxCurrentSurfaceFontSizePoints(_ surface: ghostty_surface_t) -> Float? { + guard cmuxSurfacePointerAppearsLive(surface) else { + return nil + } + guard let quicklookFont = ghostty_surface_quicklook_font(surface) else { return nil } - // Best-effort check: reject pointers that are not the start of a live - // malloc allocation. ghostty_surface_quicklook_font returns an unretained - // pointer whose lifetime is managed by Ghostty; on Intel Macs the pointer - // can become stale after the internal font is freed (#1496, #1870). - // malloc_size is safe to call with any address (returns 0 for non-malloc - // pointers without dereferencing them). This does not guarantee the memory - // still contains a valid CTFont, but it catches the common case of - // fully-freed or unmapped allocations that would otherwise SIGSEGV. - guard malloc_size(quicklookFont) > 0 else { + // Best-effort check: reject unretained font pointers that no longer belong + // to a live malloc allocation. This does not prove the object is still a + // valid CTFont, but it filters out the common fully-freed/unmapped cases + // that previously crashed on Intel Macs (#1496, #1870). + guard cmuxPointerAppearsLive(quicklookFont) else { return nil } @@ -7161,8 +7176,9 @@ final class Workspace: Identifiable, ObservableObject { private func rememberTerminalConfigInheritanceSource(_ terminalPanel: TerminalPanel) { lastTerminalConfigInheritancePanelId = terminalPanel.id - if terminalPanel.surface.hasLiveSurface, - let sourceSurface = terminalPanel.surface.surface, + if let sourceSurface = terminalPanel.surface.liveSurfaceForGhosttyAccess( + reason: "workspace.rememberConfigInheritanceSource" + ), let runtimePoints = cmuxCurrentSurfaceFontSizePoints(sourceSurface) { let existing = terminalInheritanceFontPointsByPanelId[terminalPanel.id] if existing == nil || abs((existing ?? runtimePoints) - runtimePoints) > 0.05 { @@ -7254,14 +7270,25 @@ final class Workspace: Identifiable, ObservableObject { preferredPanelId: UUID? = nil, inPane preferredPaneId: PaneID? = nil ) -> ghostty_surface_config_s? { + var staleRootedFontFallback: Float? + // Walk candidates in priority order and use the first panel with a live surface. // This avoids returning nil when the top candidate exists but is not attached yet. for terminalPanel in terminalPanelConfigInheritanceCandidates( preferredPanelId: preferredPanelId, inPane: preferredPaneId ) { - guard terminalPanel.surface.hasLiveSurface, - let sourceSurface = terminalPanel.surface.surface else { continue } + let rootedFontFallback = terminalInheritanceFontPointsByPanelId[terminalPanel.id] + guard let sourceSurface = terminalPanel.surface.liveSurfaceForGhosttyAccess( + reason: "workspace.inheritedTerminalConfig" + ) else { + if staleRootedFontFallback == nil, + let rootedFontFallback, + rootedFontFallback > 0 { + staleRootedFontFallback = rootedFontFallback + } + continue + } var config = cmuxInheritedSurfaceConfig( sourceSurface: sourceSurface, context: GHOSTTY_SURFACE_CONTEXT_SPLIT @@ -7281,12 +7308,13 @@ final class Workspace: Identifiable, ObservableObject { return config } - if let fallbackFontPoints = lastTerminalConfigInheritanceFontPoints { + if let fallbackFontPoints = staleRootedFontFallback ?? lastTerminalConfigInheritanceFontPoints { var config = ghostty_surface_config_new() config.font_size = fallbackFontPoints #if DEBUG + let fallbackSource = staleRootedFontFallback != nil ? "quarantinedRootedFont" : "lastKnownFont" dlog( - "zoom.inherit fallback=lastKnownFont context=split font=\(String(format: "%.2f", fallbackFontPoints))" + "zoom.inherit fallback=\(fallbackSource) context=split font=\(String(format: "%.2f", fallbackFontPoints))" ) #endif return config diff --git a/cmuxTests/WorkspaceUnitTests.swift b/cmuxTests/WorkspaceUnitTests.swift index 8f908974..afa1f2b4 100644 --- a/cmuxTests/WorkspaceUnitTests.swift +++ b/cmuxTests/WorkspaceUnitTests.swift @@ -906,6 +906,48 @@ final class WorkspaceTeardownTests: XCTestCase { @MainActor final class WorkspaceSplitWorkingDirectoryTests: XCTestCase { + private func waitForCondition( + timeout: TimeInterval = 2, + pollInterval: TimeInterval = 0.01, + _ condition: () -> Bool + ) -> Bool { + let deadline = Date().addingTimeInterval(timeout) + while Date() < deadline { + if condition() { + return true + } + RunLoop.current.run(until: Date().addingTimeInterval(pollInterval)) + } + return condition() + } + + private func hostTerminalPanelInWindow(_ panel: TerminalPanel) throws -> NSWindow { + let window = NSWindow( + contentRect: NSRect(x: 0, y: 0, width: 420, height: 280), + styleMask: [.titled, .closable], + backing: .buffered, + defer: false + ) + + let contentView = try XCTUnwrap(window.contentView, "Expected content view") + + let hostedView = panel.hostedView + hostedView.frame = contentView.bounds + hostedView.autoresizingMask = [.width, .height] + contentView.addSubview(hostedView) + + window.makeKeyAndOrderFront(nil) + window.displayIfNeeded() + contentView.layoutSubtreeIfNeeded() + XCTAssertTrue( + waitForCondition { + panel.surface.surface != nil + }, + "Expected runtime surface to materialize after hosting panel in a window" + ) + return window + } + func testNewTerminalSplitFallsBackToRequestedWorkingDirectoryWhenReportedDirectoryIsStale() { let workspace = Workspace() guard let sourcePaneId = workspace.bonsplitController.focusedPaneId else { @@ -950,6 +992,72 @@ final class WorkspaceSplitWorkingDirectoryTests: XCTestCase { "Expected split to inherit the source terminal's requested cwd when no reported cwd exists yet" ) } + + func testNewTerminalSplitSkipsFreedInheritedSurfacePointer() throws { +#if DEBUG + let workspace = Workspace() + guard let sourcePanelId = workspace.focusedPanelId, + let sourcePanel = workspace.terminalPanel(for: sourcePanelId) else { + XCTFail("Expected focused terminal panel") + return + } + + let window = try hostTerminalPanelInWindow(sourcePanel) + defer { window.orderOut(nil) } + + XCTAssertNotNil(sourcePanel.surface.surface, "Expected runtime surface before forcing stale pointer") + + sourcePanel.surface.replaceSurfaceWithFreedPointerForTesting() + XCTAssertNotNil( + sourcePanel.surface.surface, + "Expected Swift wrapper to remain non-nil while simulating a stale native surface" + ) + + let splitPanel = workspace.newTerminalSplit( + from: sourcePanelId, + orientation: .horizontal, + focus: false + ) + + XCTAssertNotNil(splitPanel, "Expected split creation to survive a stale inherited surface pointer") + XCTAssertNil(sourcePanel.surface.surface, "Expected stale surface pointer to be quarantined") +#else + throw XCTSkip("Debug-only regression test") +#endif + } + + func testNewTerminalSurfaceSkipsFreedInheritedSurfacePointer() throws { +#if DEBUG + let workspace = Workspace() + guard let sourcePanelId = workspace.focusedPanelId, + let sourcePanel = workspace.terminalPanel(for: sourcePanelId), + let sourcePaneId = workspace.paneId(forPanelId: sourcePanelId) else { + XCTFail("Expected focused terminal panel and pane") + return + } + + let window = try hostTerminalPanelInWindow(sourcePanel) + defer { window.orderOut(nil) } + + XCTAssertNotNil(sourcePanel.surface.surface, "Expected runtime surface before forcing stale pointer") + + sourcePanel.surface.replaceSurfaceWithFreedPointerForTesting() + XCTAssertNotNil( + sourcePanel.surface.surface, + "Expected Swift wrapper to remain non-nil while simulating a stale native surface" + ) + + let createdPanel = workspace.newTerminalSurface( + inPane: sourcePaneId, + focus: false + ) + + XCTAssertNotNil(createdPanel, "Expected terminal creation to survive a stale inherited surface pointer") + XCTAssertNil(sourcePanel.surface.surface, "Expected stale surface pointer to be quarantined") +#else + throw XCTSkip("Debug-only regression test") +#endif + } }