From f3e1dc571999742458ec6861a6f358601011580b Mon Sep 17 00:00:00 2001 From: whoisaldo Date: Thu, 27 Aug 2026 13:14:41 -0400 Subject: [PATCH] Fix three iPad crashes and two silent freezes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit All found by the code-review pass over the merged revamp and confirmed against the tree. Crash on every host restart. The 4 Hz stats flush computed fragment deltas with wrapping subtraction, but the assembler zeroes its counters whenever it resets — which is exactly what a stream-epoch bump does. Two ~2^64 deltas were then added with a trapping +, so the app died the moment the host restarted its pipeline: supervisor auto-restart, VDD toggle, settings change, encoder crash recovery. The client killed itself on the very path M6 built to survive. It now re-baselines when a counter goes backwards. Crash on a fast swipe. TouchRelay squared Int32 deltas of coordinates spanning 0...65535; a flick across ~71% of the width overflows and Swift traps. Widened to Int64. Crash on connect and disconnect. FrameSlot.onFrameStored was a plain stored closure, written on the MainActor by the Metal view's attach and detach while the VideoToolbox decode thread called it — a race on the closure's own retain/release. It is now swapped under a lock and copied out before being called. Auto-reconnect never fired. The flush loop skipped the rest of the tick when no datagrams had arrived, and the only call to the liveness watchdog sat below that guard. A dead host sends neither media nor heartbeats, so the watchdog was silenced in precisely the situation it exists for: the bar kept reading ON AIR over a frozen frame and the reconnect cycle from #17 could never start. The watchdog now runs every tick. Large frames were dropped forever. The assembler capped frames at 1024 fragments while the host fragments against, and the wire parser accepts, 3066. A 1440p or 4K scene-change IDR lands in that gap: every fragment discarded, no sync sample, so every later frame discarded too, and the client asks for a keyframe that comes back equally large. Permanent freeze with healthy heartbeats on both ends. The cap now IS the protocol cap; maxPendingBytes remains the real memory guard. Also: the three-finger HUD gesture could only ever show the HUD, because it called scheduleHUDDismiss() which unconditionally sets showHUD = true; both tap paths now share one toggle. And Info.plist still declared Syne-Variable.ttf, deleted in #22. --- .../App/ConnectionManager.swift | 94 +++++++++++++------ ios/EternalMonitor/App/DisplayView.swift | 29 +++--- ios/EternalMonitor/Info.plist | 2 +- ios/EternalMonitor/Input/TouchRelay.swift | 9 +- .../Network/FrameAssembler.swift | 12 ++- .../FrameAssemblerTests.swift | 19 ++++ ios/EternalMonitorTests/TouchRelayTests.swift | 12 +++ 7 files changed, 126 insertions(+), 51 deletions(-) diff --git a/ios/EternalMonitor/App/ConnectionManager.swift b/ios/EternalMonitor/App/ConnectionManager.swift index 9c9c501..548589b 100644 --- a/ios/EternalMonitor/App/ConnectionManager.swift +++ b/ios/EternalMonitor/App/ConnectionManager.swift @@ -408,32 +408,39 @@ final class ConnectionManager: ObservableObject { try? await Task.sleep(nanoseconds: 250_000_000) guard let self, !Task.isCancelled else { return } let snapshot = self.streamCounters.drain() - guard snapshot.datagrams > 0 || snapshot.assembled > 0 else { continue } - - let firstDatagrams = self.debugState.datagramsReceived == 0 && snapshot.datagrams > 0 - let firstAssembled = self.debugState.assembledFrames == 0 && snapshot.assembled > 0 - self.debugState.datagramsReceived += snapshot.datagrams - self.debugState.datagramBytesReceived += snapshot.datagramBytes - self.debugState.assembledFrames += snapshot.assembled - self.debugState.assembledFrameBytes += snapshot.assembledBytes - self.debugState.decodePackets += snapshot.parsed - - if firstDatagrams { - self.record( - .info, "udp", - "UDP data flowing (\(snapshot.datagrams) datagrams, \(snapshot.datagramBytes) bytes in first batch)" - ) - // Data is flowing — grant a fresh full window (once) so a slow/jittery - // network gets time to finish reassembly + decode instead of being cut - // off mid-handshake. - if self.state == .connecting && !self.didExtendTimeout { - self.didExtendTimeout = true - self.armConnectTimeout(seconds: Self.connectionTimeoutSeconds) + + if snapshot.datagrams > 0 || snapshot.assembled > 0 { + let firstDatagrams = + self.debugState.datagramsReceived == 0 && snapshot.datagrams > 0 + let firstAssembled = + self.debugState.assembledFrames == 0 && snapshot.assembled > 0 + self.debugState.datagramsReceived += snapshot.datagrams + self.debugState.datagramBytesReceived += snapshot.datagramBytes + self.debugState.assembledFrames += snapshot.assembled + self.debugState.assembledFrameBytes += snapshot.assembledBytes + self.debugState.decodePackets += snapshot.parsed + + if firstDatagrams { + self.record( + .info, "udp", + "UDP data flowing (\(snapshot.datagrams) datagrams, \(snapshot.datagramBytes) bytes in first batch)" + ) + // Data is flowing — grant a fresh full window (once) so a slow/jittery + // network gets time to finish reassembly + decode instead of being cut + // off mid-handshake. + if self.state == .connecting && !self.didExtendTimeout { + self.didExtendTimeout = true + self.armConnectTimeout(seconds: Self.connectionTimeoutSeconds) + } + } + if firstAssembled { + self.record(.info, "assembly", "First frame payload assembled (\(snapshot.assembledBytes) bytes)") } } - if firstAssembled { - self.record(.info, "assembly", "First frame payload assembled (\(snapshot.assembledBytes) bytes)") - } + + // Runs on EVERY tick, including silent ones: a dead host sends + // neither media nor heartbeats, so skipping this when nothing + // arrived is exactly when the liveness watchdog is needed. self.refreshStatsAndWatchdog() } } @@ -451,8 +458,17 @@ final class ConnectionManager: ObservableObject { if let rtt = clock.rttUs { next.rttMs = Double(rtt) / 1000.0 } if clock.offsetUs != nil { next.e2eMs = lagMs } if let counters = frameAssembler?.counters.withLock({ $0 }) { - let deltaReceived = counters.fragsReceived &- prevCounters.fragsReceived - let deltaLost = counters.fragsLost &- prevCounters.fragsLost + // The assembler zeroes its counters whenever it resets (host + // pipeline restart bumps the stream epoch). A value BELOW last + // tick's therefore means "restarted", not "wrapped" — re-baseline, + // because subtracting through zero produced two ~2^64 deltas whose + // sum overflowed and killed the app on every host restart. + if counters.fragsReceived < prevCounters.fragsReceived + || counters.fragsLost < prevCounters.fragsLost { + prevCounters = FrameAssembler.Counters() + } + let deltaReceived = counters.fragsReceived - prevCounters.fragsReceived + let deltaLost = counters.fragsLost - prevCounters.fragsLost prevCounters = counters let total = deltaReceived + deltaLost next.lossPercent = total == 0 ? 0 : Double(deltaLost) * 100.0 / Double(total) @@ -692,15 +708,33 @@ final class PixelBufferBox: @unchecked Sendable { final class FrameSlot: @unchecked Sendable { private let lock = os.OSAllocatedUnfairLock(initialState: nil) + + /// Holder so the redraw callback can be swapped under a lock. The Metal + /// view attaches and detaches it on the MainActor while the VideoToolbox + /// decode thread is calling it; an unguarded stored closure raced on its + /// own retain/release there, which crashes on connect and on teardown. + private struct CallbackHolder: @unchecked Sendable { + var callback: (() -> Void)? + } + private let stored = os.OSAllocatedUnfairLock( + initialState: CallbackHolder() + ) + /// Fired (on the storing thread — the VideoToolbox callback) every time a - /// frame lands, so the renderer can schedule an on-demand redraw. Assigned - /// once by the Metal view before streaming starts. - var onFrameStored: (() -> Void)? + /// frame lands, so the renderer can schedule an on-demand redraw. + var onFrameStored: (() -> Void)? { + get { stored.withLock { $0.callback } } + set { stored.withLock { $0.callback = newValue } } + } func set(_ buffer: CVPixelBuffer) { let box = PixelBufferBox(buffer) lock.withLock { $0 = box } - onFrameStored?() + // Copy the closure out under the lock, then call it outside: holding + // the lock across arbitrary caller code invites deadlock, and a local + // strong reference keeps a concurrent detach from freeing it mid-call. + let callback = stored.withLock { $0.callback } + callback?() } func take() -> CVPixelBuffer? { diff --git a/ios/EternalMonitor/App/DisplayView.swift b/ios/EternalMonitor/App/DisplayView.swift index 2bbcb12..0836f25 100644 --- a/ios/EternalMonitor/App/DisplayView.swift +++ b/ios/EternalMonitor/App/DisplayView.swift @@ -18,25 +18,15 @@ struct DisplayView: View { // PC; a three-finger tap (via the relay layer) drives the HUD. MetalView() .ignoresSafeArea() - TouchRelayView(onToggleHUD: { - showHUD.toggle() - scheduleHUDDismiss() - }) - .ignoresSafeArea() + TouchRelayView(onToggleHUD: { toggleHUD() }) + .ignoresSafeArea() } else { // One gesture, one meaning: tap toggles the HUD. (The old // single-tap-hide + triple-tap-toggle pair made every triple // tap race its own first tap.) MetalView() .ignoresSafeArea() - .onTapGesture { - if showHUD { - hudDismissTask?.cancel() - withAnimation(.easeOut(duration: 0.25)) { showHUD = false } - } else { - scheduleHUDDismiss() - } - } + .onTapGesture { toggleHUD() } } // Viewfinder registration marks frame the picture (fade with the HUD). @@ -236,6 +226,19 @@ struct DisplayView: View { // MARK: - Auto-hide HUD + /// Show the HUD (auto-hiding after a few seconds) or hide it now. Both the + /// plain tap and the three-finger relay tap route here: calling + /// `scheduleHUDDismiss()` to "toggle" could only ever show it, since that + /// function unconditionally sets `showHUD = true`. + private func toggleHUD() { + if showHUD { + hudDismissTask?.cancel() + withAnimation(.easeOut(duration: 0.25)) { showHUD = false } + } else { + scheduleHUDDismiss() + } + } + private func scheduleHUDDismiss() { hudDismissTask?.cancel() withAnimation(.easeOut(duration: 0.25)) { showHUD = true } diff --git a/ios/EternalMonitor/Info.plist b/ios/EternalMonitor/Info.plist index b73bcc2..d9019e1 100644 --- a/ios/EternalMonitor/Info.plist +++ b/ios/EternalMonitor/Info.plist @@ -24,7 +24,7 @@ UIAppFonts - Syne-Variable.ttf + Syne-Bold.ttf JetBrainsMono-Regular.ttf JetBrainsMono-Medium.ttf diff --git a/ios/EternalMonitor/Input/TouchRelay.swift b/ios/EternalMonitor/Input/TouchRelay.swift index baff039..ccc7949 100644 --- a/ios/EternalMonitor/Input/TouchRelay.swift +++ b/ios/EternalMonitor/Input/TouchRelay.swift @@ -177,9 +177,12 @@ struct TouchRelayMachine { switch mode { case .pending(let start, _): guard let point else { return [] } - let dx = Int32(point.x) - Int32(start.x) - let dy = Int32(point.y) - Int32(start.y) - if dx * dx + dy * dy > Self.dragSlop * Self.dragSlop { + // Int64: the coordinates span 0...65535, so squaring a large flick + // overflows Int32 and Swift traps rather than wrapping. + let dx = Int64(point.x) - Int64(start.x) + let dy = Int64(point.y) - Int64(start.y) + let slop = Int64(Self.dragSlop) + if dx * dx + dy * dy > slop * slop { // Committed to a drag: press at the start point, catch up. mode = .leftDown(isPencil: false) var outputs = edge(Phase.began, kind: Kind.touch, buttons: 1, at: start, timeUs: timeUs) diff --git a/ios/EternalMonitor/Network/FrameAssembler.swift b/ios/EternalMonitor/Network/FrameAssembler.swift index 465e083..24a87e9 100644 --- a/ios/EternalMonitor/Network/FrameAssembler.swift +++ b/ios/EternalMonitor/Network/FrameAssembler.swift @@ -14,10 +14,14 @@ final class FrameAssembler { var onFrameAssembled: ((Data, _ seq: UInt32, _ captureTimestampUs: UInt64, _ isKeyframe: Bool) -> Void)? var onDiagnostic: ((String) -> Void)? - /// A frame may span at most this many fragments (~1.4 MB at 1384-byte - /// payloads). The u16 field allows 65535 (≈90 MB) — a memory bomb, not a - /// video frame. - static let maxFragmentCount: UInt16 = 1024 + /// A frame may span at most this many fragments. This MUST match the + /// protocol cap the host fragments against and the wire parser enforces: + /// a tighter value here silently discards legitimately large access units + /// (a 1440p/4K scene-change IDR runs past 1024 fragments), and because the + /// decoder then never receives a sync sample, every later frame is dropped + /// too — a permanent freeze with healthy heartbeats. `maxPendingBytes` is + /// the real memory guard; the u16 field alone would allow ≈90 MB. + static let maxFragmentCount: UInt16 = MediaHeader.maxFragCount /// At most this many partial frames in flight; the oldest is dropped first. static let maxPendingFrames = 8 /// Hard ceiling on buffered fragment bytes across all partial frames. diff --git a/ios/EternalMonitorTests/FrameAssemblerTests.swift b/ios/EternalMonitorTests/FrameAssemblerTests.swift index 83b2a01..3a9a09a 100644 --- a/ios/EternalMonitorTests/FrameAssemblerTests.swift +++ b/ios/EternalMonitorTests/FrameAssemblerTests.swift @@ -57,6 +57,25 @@ final class FrameAssemblerTests: XCTestCase { XCTAssertEqual(completed.count, 2, "stale-epoch fragment must be dropped") } + func testAcceptsFramesUpToTheProtocolFragmentCap() { + // A 1440p/4K scene-change IDR runs past 1024 fragments. The host + // fragments against the protocol cap and the wire parser accepts up to + // it, so an assembler cap below that silently dropped every fragment + // of such a frame — and with no sync sample the decoder then discarded + // everything after it too. + XCTAssertEqual( + FrameAssembler.maxFragmentCount, MediaHeader.maxFragCount, + "the assembler cap must match the protocol cap the host sends against" + ) + + let count: UInt16 = 2000 + for index in 0..