From 9ca9cff09a494025419ccf9b7e08b4ea36a27fdd Mon Sep 17 00:00:00 2001 From: Jacob Fu <141651335+FuJacob@users.noreply.github.com> Date: Mon, 1 Jun 2026 22:48:23 -0700 Subject: [PATCH] Fix an overlay crash and two dead settings (pre-release audit, wave 2) 1. Crash on a non-finite caret rect. Some host AX implementations (Chromium, Electron, mid-scroll or under load) return NaN/Inf bounds. The existing guards (isNull, != .zero, isEmpty) do not reject non-finite rects, so the value flowed into NSWindow.setFrame, which raises NSInvalidArgumentException and crashes the app on the hottest path (every suggestion shown). Reject non-finite rects at the AX ingest boundary (cocoaRect / validatedCocoaTextRect now collapse them to .zero, which also removes a latent Int(.nan) trap in OverlayState.detail), plus a last-resort finite guard before each OverlayController.setFrame. 2. Two Settings controls were silently dead. AppDelegate built the dependency graph as a local `let environment` and copied the individual services out, but never retained the environment itself. The environment owns the only subscriptions wiring the focus-poll-interval setting (to updatePollInterval) and the global-toggle hotkey rebind (to refreshToggleTap); when the environment deallocated as init returned, both were cancelled, so changing the poll interval did nothing and rebinding the hotkey did nothing until the next relaunch. Retain the environment for the app lifetime. Deliberately deferred: PermissionManager / PermissionGuidanceController have isolated deinits (the macOS 14 double-free class), but they are app-lifetime singletons that never dealloc, so this is latent only and the fix is non-trivial (the deinits do real cleanup, unlike the empty nonisolated-deinit precedent). --- Cotabby/App/Core/AppDelegate.swift | 9 +++++++-- Cotabby/Services/UI/OverlayController.swift | 15 ++++++++++++++- Cotabby/Support/AXHelper.swift | 15 +++++++++++++++ CotabbyTests/AXTextGeometryResolverTests.swift | 16 ++++++++++++++++ 4 files changed, 52 insertions(+), 3 deletions(-) diff --git a/Cotabby/App/Core/AppDelegate.swift b/Cotabby/App/Core/AppDelegate.swift index d5c6587d..19fd1531 100644 --- a/Cotabby/App/Core/AppDelegate.swift +++ b/Cotabby/App/Core/AppDelegate.swift @@ -31,6 +31,11 @@ final class AppDelegate: NSObject, NSApplicationDelegate { private let activationIndicatorController: ActivationIndicatorController private let focusDebugOverlayController: FocusDebugOverlayController? + /// Retained for the app's lifetime because the environment owns its own `cancellables` (the only + /// subscriptions wiring the focus-poll-interval setting and the global-toggle hotkey rebind to the + /// runtime). If the environment deallocated when `init` returned, those subscriptions would be + /// cancelled and both settings would silently stop working until the next relaunch. + private let environment: CotabbyAppEnvironment private var cancellables = Set() private var didStartServices = false @@ -38,9 +43,9 @@ final class AppDelegate: NSObject, NSApplicationDelegate { CotabbyLogger.bootstrap() // Build the dependency graph once up front so every scene/view observes the same - // long-lived objects for the entire app session. `CotabbyAppEnvironment` is a composition - // helper here; the app delegate retains the root objects it needs directly. + // long-lived objects for the entire app session. let environment = CotabbyAppEnvironment() + self.environment = environment permissionManager = environment.permissionManager runtimeModel = environment.runtimeModel modelDownloadManager = environment.modelDownloadManager diff --git a/Cotabby/Services/UI/OverlayController.swift b/Cotabby/Services/UI/OverlayController.swift index d8ed617b..2b0791b8 100644 --- a/Cotabby/Services/UI/OverlayController.swift +++ b/Cotabby/Services/UI/OverlayController.swift @@ -1,5 +1,6 @@ import AppKit import Foundation +import Logging import SwiftUI /// File overview: @@ -183,6 +184,13 @@ final class OverlayController: SuggestionOverlayControlling { let frame = layout.panelFrame(for: contentSize, caretRect: geometry.caretRect) + // Last-resort guard: AppKit raises on a non-finite frame. The AX ingest boundary already + // rejects NaN/Inf rects, so reaching here means the layout math produced one; skip the show + // rather than crash on the hottest path. + guard AXHelper.rectHasFiniteComponents(frame) else { + CotabbyLogger.suggestion.warning("Skipped inline overlay: computed a non-finite frame") + return + } panel.setFrame(frame.integral, display: true) panel.orderFrontRegardless() } @@ -231,7 +239,12 @@ final class OverlayController: SuggestionOverlayControlling { panel.contentView = contentView } - panel.setFrame(layout.panelFrame, display: true) + let panelFrame = layout.panelFrame + guard AXHelper.rectHasFiniteComponents(panelFrame) else { + CotabbyLogger.suggestion.warning("Skipped mirror overlay: computed a non-finite frame") + return + } + panel.setFrame(panelFrame, display: true) panel.orderFrontRegardless() } diff --git a/Cotabby/Support/AXHelper.swift b/Cotabby/Support/AXHelper.swift index fccb3fd0..c78a35c0 100644 --- a/Cotabby/Support/AXHelper.swift +++ b/Cotabby/Support/AXHelper.swift @@ -590,6 +590,9 @@ enum AXHelper { /// Use this for element-level rects (AXFrame) that are reliably in Cocoa points. /// For text-range rects (BoundsForRange, TextMarker), use `validatedCocoaTextRect` instead. static func cocoaRect(fromAccessibilityRect rect: CGRect) -> CGRect { + guard rectHasFiniteComponents(rect) else { + return .zero + } guard !rect.isNull, rect != .zero else { return rect } @@ -605,6 +608,15 @@ enum AXHelper { return legacyDesktopUnionFlip(rect) } + /// True only when every component of `rect` is a finite number. Some host AX implementations + /// (Chromium/Electron, especially mid-scroll or under load) return NaN/Inf bounds; AppKit raises + /// on a non-finite window frame and `Int(.nan)` traps, so such rects are rejected here at the AX + /// ingest boundary before they can reach geometry math or `NSWindow.setFrame`. + static func rectHasFiniteComponents(_ rect: CGRect) -> Bool { + rect.origin.x.isFinite && rect.origin.y.isFinite + && rect.size.width.isFinite && rect.size.height.isFinite + } + /// Converts a text-range AX rect to Cocoa coordinates, using the element's AXFrame (already /// in Cocoa coordinates) as a ground-truth anchor to detect whether pixel-to-point scaling /// is needed. This replaces the old bundle-ID heuristic with empirical geometric validation: @@ -615,6 +627,9 @@ enum AXHelper { fromAccessibilityRect textRect: CGRect, anchorFrame cocoaAnchorFrame: CGRect? ) -> CGRect { + guard rectHasFiniteComponents(textRect) else { + return .zero + } guard !textRect.isNull, textRect != .zero else { return textRect } diff --git a/CotabbyTests/AXTextGeometryResolverTests.swift b/CotabbyTests/AXTextGeometryResolverTests.swift index 6fd8458a..c34fc75c 100644 --- a/CotabbyTests/AXTextGeometryResolverTests.swift +++ b/CotabbyTests/AXTextGeometryResolverTests.swift @@ -146,4 +146,20 @@ final class AXTextGeometryResolverTests: XCTestCase { XCTAssertTrue(resolver.rectIsNearAnchor(rect, anchor: nil)) XCTAssertTrue(resolver.rectIsNearAnchor(rect, anchor: .zero)) } + + // MARK: - Non-finite AX rect rejection (crash guard) + + func test_rectHasFiniteComponents_rejectsNaNAndInfinity() { + XCTAssertTrue(AXHelper.rectHasFiniteComponents(CGRect(x: 1, y: 2, width: 3, height: 4))) + XCTAssertFalse(AXHelper.rectHasFiniteComponents(CGRect(x: CGFloat.nan, y: 0, width: 10, height: 10))) + XCTAssertFalse(AXHelper.rectHasFiniteComponents(CGRect(x: 0, y: 0, width: CGFloat.infinity, height: 10))) + XCTAssertFalse(AXHelper.rectHasFiniteComponents(CGRect(x: 0, y: CGFloat.nan, width: 10, height: CGFloat.nan))) + } + + func test_validatedCocoaTextRect_collapsesNonFiniteRectToZero() { + // A NaN/Inf AX rect must collapse to .zero, never propagate toward NSWindow.setFrame (a crash). + let nan = CGRect(x: CGFloat.nan, y: 10, width: 20, height: 14) + XCTAssertEqual(AXHelper.validatedCocoaTextRect(fromAccessibilityRect: nan, anchorFrame: nil), .zero) + XCTAssertEqual(AXHelper.cocoaRect(fromAccessibilityRect: nan), .zero) + } }