Skip to content
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion Sources/Panels/BrowserHiddenWebViewDiscardManager.swift
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import Foundation
protocol BrowserHiddenWebViewDiscardManagerDelegate: AnyObject {
var hiddenWebViewDiscardSnapshot: BrowserHiddenWebViewDiscardManager.BlockerSnapshot { get }
var hiddenWebViewDiscardHiddenAt: Date? { get }
var hiddenWebViewDiscardLastAutomationActivityAt: Date? { get }
var hiddenWebViewDiscardWebViewInstanceID: UUID { get }

func hiddenWebViewDiscardManagerDidRequestDiscard(
Expand Down Expand Up @@ -101,11 +102,14 @@ final class BrowserHiddenWebViewDiscardManager {
let observedWebViewInstanceID = delegate.hiddenWebViewDiscardWebViewInstanceID
let generation = scheduleGeneration
let hiddenAt = delegate.hiddenWebViewDiscardHiddenAt ?? Date()
let lastAutomationActivityAt = delegate.hiddenWebViewDiscardLastAutomationActivityAt
// Restart the countdown from the latest wake: WebKit pages reconnect and
// re-navigate right after wake, and replacing/releasing a WKWebView in
// that window crashed in WebPageProxy::updateActivityState
// (https://github.com/manaflow-ai/cmux/issues/5261).
let effectiveHiddenAt = lastSystemWakeAt.map { max(hiddenAt, $0) } ?? hiddenAt
let effectiveHiddenAt = [hiddenAt, lastSystemWakeAt, lastAutomationActivityAt]
.compactMap { $0 }
.max() ?? hiddenAt
let elapsed = Date().timeIntervalSince(effectiveHiddenAt)
let remaining = max(0, BrowserHiddenWebViewDiscardPolicy.hiddenDelay - elapsed)
if remaining <= 0 {
Expand Down
34 changes: 34 additions & 0 deletions Sources/Panels/BrowserPanel.swift
Original file line number Diff line number Diff line change
Expand Up @@ -3397,6 +3397,7 @@ final class BrowserPanel: Panel, ObservableObject {
private var backgroundPreloadWindow: NSWindow?
private let visualAutomationCaptureGate = BrowserScreenshotCaptureGate()
private var activeVisualAutomationCaptureCount: Int = 0
private var webViewLastAutomationActivityAt: Date?
private struct PendingInteractiveBrowserPrompt {
let present: (NSWindow, @escaping () -> Void) -> Void
let cancel: () -> Void
Expand Down Expand Up @@ -3788,6 +3789,7 @@ final class BrowserPanel: Panel, ObservableObject {
webViewLastHiddenAt = nil
webViewLastVisibilityChangeAt = nil
webViewLastVisibilityChangeReason = nil
webViewLastAutomationActivityAt = nil
isWebViewVisibleInUI = false
}
hiddenWebViewDiscardManager.resetMetadata()
Expand Down Expand Up @@ -6395,6 +6397,10 @@ extension BrowserPanel: BrowserHiddenWebViewDiscardManagerDelegate {
webViewLastHiddenAt
}

var hiddenWebViewDiscardLastAutomationActivityAt: Date? {
webViewLastAutomationActivityAt
}

var hiddenWebViewDiscardWebViewInstanceID: UUID {
webViewInstanceID
}
Expand Down Expand Up @@ -7580,6 +7586,34 @@ extension BrowserPanel {
}
}

@discardableResult
func beginAutomationCommandLease(reason: String) -> BrowserScreenshotWebViewSnapshotter.OffscreenRenderHostLease? {
webViewLastAutomationActivityAt = Date()
activeVisualAutomationCaptureCount += 1
cancelHiddenWebViewDiscard()
restoreDiscardedWebViewIfNeeded(reason: "\(reason).restore")
refreshWebViewLifecycleState()

guard shouldUseOffscreenRenderHostForVisualAutomation else { return nil }
return BrowserScreenshotWebViewSnapshotter.OffscreenRenderHostLease(
webView: webView,
viewportSize: visualAutomationViewportSize()
)
}
Comment thread
cursor[bot] marked this conversation as resolved.

func endAutomationCommandLease(
_ lease: BrowserScreenshotWebViewSnapshotter.OffscreenRenderHostLease?,
reason: String
) {
lease?.end()
webViewLastAutomationActivityAt = Date()
activeVisualAutomationCaptureCount = max(0, activeVisualAutomationCaptureCount - 1)
refreshWebViewLifecycleState()
if activeVisualAutomationCaptureCount == 0, !isWebViewVisibleInUI {
scheduleHiddenWebViewDiscardIfNeeded(reason: "\(reason).finished")
}
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lease ends during active capture

Medium Severity

endAutomationCommandLease always tears down the automation offscreen host and restores the WKWebView when a socket command finishes, even if activeVisualAutomationCaptureCount is still above zero after decrement. Overlapping browser.screenshot (main actor) with a socket-worker command that held the lease can move the webview back to a hidden portal while PNG capture is still in progress, causing failed or incorrect screenshots.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit e3fd55b. Configure here.


@discardableResult
func ensureVisualAutomationRestoreHostIfNeeded(reason: String) -> Bool {
guard shouldUseOffscreenRenderHostForVisualAutomation else { return false }
Expand Down
259 changes: 127 additions & 132 deletions Sources/Panels/BrowserScreenshotSnapshotter.swift
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,129 @@ enum BrowserScreenshotCaptureBounds {

@MainActor
enum BrowserScreenshotWebViewSnapshotter {
@MainActor
final class OffscreenRenderHostLease {
private weak var webView: WKWebView?
private let previousSuperview: NSView?
private let previousFrame: NSRect
private let previousBounds: NSRect
private let previousAutoresizingMask: NSView.AutoresizingMask
private let previousTranslatesAutoresizingMaskIntoConstraints: Bool
private let restoreAnchor: NSView?
private let restorePosition: NSWindow.OrderingMode
private let window: BrowserScreenshotOffscreenRenderPanel
private var isActive = true

init(webView: WKWebView, viewportSize: NSSize) {
self.webView = webView
previousSuperview = webView.superview
let previousSubviews = previousSuperview?.subviews ?? []
let previousIndex = previousSubviews.firstIndex(of: webView)
previousFrame = webView.frame
previousBounds = webView.bounds
previousAutoresizingMask = webView.autoresizingMask
previousTranslatesAutoresizingMaskIntoConstraints = webView.translatesAutoresizingMaskIntoConstraints

if let previousIndex, previousIndex > 0 {
restoreAnchor = previousSubviews[previousIndex - 1]
restorePosition = .above
} else if let previousIndex, previousIndex == 0, previousSubviews.count > 1 {
restoreAnchor = previousSubviews[1]
restorePosition = .below
} else {
restoreAnchor = nil
restorePosition = .above
}

let normalizedSize = Self.normalizedViewportSize(viewportSize)
let frame = NSRect(
x: -100_000 - normalizedSize.width,
y: -100_000 - normalizedSize.height,
width: normalizedSize.width,
height: normalizedSize.height
)
let window = BrowserScreenshotOffscreenRenderPanel(
contentRect: frame,
styleMask: [.borderless, .nonactivatingPanel],
backing: .buffered,
defer: false
)
window.isReleasedWhenClosed = false
window.identifier = NSUserInterfaceItemIdentifier("cmux.browserVisualAutomationRender")
window.hasShadow = false
window.isOpaque = false
window.backgroundColor = .clear
window.alphaValue = 0.01
window.ignoresMouseEvents = true
window.hidesOnDeactivate = false
window.collectionBehavior = [.transient, .ignoresCycle, .stationary, .canJoinAllSpaces]
window.isExcludedFromWindowsMenu = true
self.window = window

let contentView = NSView(frame: NSRect(origin: .zero, size: normalizedSize))
contentView.wantsLayer = true
webView.removeFromSuperview()
webView.frame = contentView.bounds
webView.autoresizingMask = [.width, .height]
contentView.addSubview(webView)
window.contentView = contentView
window.orderFrontRegardless()
}

func end() {
guard isActive else { return }
isActive = false
if let webView {
Self.restoreWebView(
webView,
to: previousSuperview,
frame: previousFrame,
bounds: previousBounds,
autoresizingMask: previousAutoresizingMask,
translatesAutoresizingMaskIntoConstraints: previousTranslatesAutoresizingMaskIntoConstraints,
anchor: restoreAnchor,
position: restorePosition
)
}
window.orderOut(nil)
window.contentView = nil
window.close()
}

deinit {
guard isActive else { return }
MainActor.assumeIsolated {
end()
}
}
Comment on lines +152 to +157

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 assumeIsolated in deinit is an unchecked crash

@MainActor final class does not guarantee that deinit runs on the main actor in current Swift (SE-0371 "Isolated synchronous deinit" was returned for revision and is not in the language). If the last strong reference to an OffscreenRenderHostLease is released off the main thread when isActive is still true, MainActor.assumeIsolated will preconditionFailure-crash the app.

In the current flow the guard short-circuits in every reachable path (endAutomationCommandLease sets isActive = false before dropping the lease), but the safety net is meant to fire for unexpected drops — precisely the case where thread provenance is unknown. The correct defensive pattern is DispatchQueue.main.async { self.end() } (accepting that cleanup is deferred by one run-loop turn) rather than an unchecked assertion about the current thread.


private static func normalizedViewportSize(_ viewportSize: NSSize) -> NSSize {
BrowserScreenshotWebViewSnapshotter.normalizedViewportSize(viewportSize)
}

private static func restoreWebView(
_ webView: WKWebView,
to superview: NSView?,
frame: NSRect,
bounds: NSRect,
autoresizingMask: NSView.AutoresizingMask,
translatesAutoresizingMaskIntoConstraints: Bool,
anchor: NSView?,
position: NSWindow.OrderingMode
) {
BrowserScreenshotWebViewSnapshotter.restoreWebView(
webView,
to: superview,
frame: frame,
bounds: bounds,
autoresizingMask: autoresizingMask,
translatesAutoresizingMaskIntoConstraints: translatesAutoresizingMaskIntoConstraints,
anchor: anchor,
position: position
)
}
}
Comment on lines +63 to +184

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win

Extract the lease/panel helpers to a dedicated file to stay within the Swift file-size budget.

This new lease abstraction is coherent, but adding it here pushes Sources/Panels/BrowserScreenshotSnapshotter.swift past the 800-line budget. Please move OffscreenRenderHostLease (and, if appropriate, BrowserScreenshotOffscreenRenderPanel) into a focused companion file under Sources/Panels/.

As per coding guidelines, {Sources,CLI,Packages,cmuxTests,cmuxUITests}/**/*.swift should be flagged when a production Swift file exceeds 800 lines, even with coherent responsibility.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@Sources/Panels/BrowserScreenshotSnapshotter.swift` around lines 63 - 184,
Move the OffscreenRenderHostLease class (and the
BrowserScreenshotOffscreenRenderPanel type if it’s declared in the same block)
out of BrowserScreenshotSnapshotter.swift into a new file under Sources/Panels
(e.g., OffscreenRenderHostLease.swift); keep the `@MainActor` attribute, any
imports, and the same access level, and ensure the new file declares the same
types (OffscreenRenderHostLease and BrowserScreenshotOffscreenRenderPanel) so
existing callers (e.g., BrowserScreenshotWebViewSnapshotter.restoreWebView and
any usages in BrowserScreenshotSnapshotter) still compile; after moving, remove
the nested/inline definition from BrowserScreenshotSnapshotter.swift and run a
build to fix any missing import/access issues or to adjust visibility if
necessary.

Source: Coding guidelines


static func captureFullPage(
from webView: WKWebView,
afterScreenUpdates: Bool = true
Expand Down Expand Up @@ -180,73 +303,9 @@ enum BrowserScreenshotWebViewSnapshotter {
expectedURL: URL?,
operation: () async throws -> T
) async throws -> T {
let previousSuperview = webView.superview
let previousSubviews = previousSuperview?.subviews ?? []
let previousIndex = previousSubviews.firstIndex(of: webView)
let previousFrame = webView.frame
let previousBounds = webView.bounds
let previousAutoresizingMask = webView.autoresizingMask
let previousTranslatesAutoresizingMaskIntoConstraints = webView.translatesAutoresizingMaskIntoConstraints
let restoreAnchor: NSView?
let restorePosition: NSWindow.OrderingMode
if let previousIndex, previousIndex > 0 {
restoreAnchor = previousSubviews[previousIndex - 1]
restorePosition = .above
} else if let previousIndex, previousIndex == 0, previousSubviews.count > 1 {
restoreAnchor = previousSubviews[1]
restorePosition = .below
} else {
restoreAnchor = nil
restorePosition = .above
}

let normalizedSize = normalizedViewportSize(viewportSize)
let frame = NSRect(
x: -100_000 - normalizedSize.width,
y: -100_000 - normalizedSize.height,
width: normalizedSize.width,
height: normalizedSize.height
)
let window = BrowserScreenshotOffscreenRenderPanel(
contentRect: frame,
styleMask: [.borderless, .nonactivatingPanel],
backing: .buffered,
defer: false
)
window.isReleasedWhenClosed = false
window.identifier = NSUserInterfaceItemIdentifier("cmux.browserVisualAutomationRender")
window.hasShadow = false
window.isOpaque = false
window.backgroundColor = .clear
window.alphaValue = 0.01
window.ignoresMouseEvents = true
window.hidesOnDeactivate = false
window.collectionBehavior = [.transient, .ignoresCycle, .stationary, .canJoinAllSpaces]
window.isExcludedFromWindowsMenu = true

let contentView = NSView(frame: NSRect(origin: .zero, size: normalizedSize))
contentView.wantsLayer = true
webView.removeFromSuperview()
webView.frame = contentView.bounds
webView.autoresizingMask = [.width, .height]
contentView.addSubview(webView)
window.contentView = contentView
window.orderFrontRegardless()

let lease = OffscreenRenderHostLease(webView: webView, viewportSize: viewportSize)
defer {
restoreWebView(
webView,
to: previousSuperview,
frame: previousFrame,
bounds: previousBounds,
autoresizingMask: previousAutoresizingMask,
translatesAutoresizingMaskIntoConstraints: previousTranslatesAutoresizingMaskIntoConstraints,
anchor: restoreAnchor,
position: restorePosition
)
window.orderOut(nil)
window.contentView = nil
window.close()
lease.end()
}

try await prepareForVisualCapture(webView, expectedURL: expectedURL)
Expand All @@ -261,79 +320,15 @@ enum BrowserScreenshotWebViewSnapshotter {
operation: @escaping (@escaping (Result<T, Error>) -> Void) -> Void,
completion: @escaping (Result<T, Error>) -> Void
) {
let previousSuperview = webView.superview
let previousSubviews = previousSuperview?.subviews ?? []
let previousIndex = previousSubviews.firstIndex(of: webView)
let previousFrame = webView.frame
let previousBounds = webView.bounds
let previousAutoresizingMask = webView.autoresizingMask
let previousTranslatesAutoresizingMaskIntoConstraints = webView.translatesAutoresizingMaskIntoConstraints
let restoreAnchor: NSView?
let restorePosition: NSWindow.OrderingMode
if let previousIndex, previousIndex > 0 {
restoreAnchor = previousSubviews[previousIndex - 1]
restorePosition = .above
} else if let previousIndex, previousIndex == 0, previousSubviews.count > 1 {
restoreAnchor = previousSubviews[1]
restorePosition = .below
} else {
restoreAnchor = nil
restorePosition = .above
}

let normalizedSize = normalizedViewportSize(viewportSize)
let frame = NSRect(
x: -100_000 - normalizedSize.width,
y: -100_000 - normalizedSize.height,
width: normalizedSize.width,
height: normalizedSize.height
)
let window = BrowserScreenshotOffscreenRenderPanel(
contentRect: frame,
styleMask: [.borderless, .nonactivatingPanel],
backing: .buffered,
defer: false
)
window.isReleasedWhenClosed = false
window.identifier = NSUserInterfaceItemIdentifier("cmux.browserVisualAutomationRender")
window.hasShadow = false
window.isOpaque = false
window.backgroundColor = .clear
window.alphaValue = 0.01
window.ignoresMouseEvents = true
window.hidesOnDeactivate = false
window.collectionBehavior = [.transient, .ignoresCycle, .stationary, .canJoinAllSpaces]
window.isExcludedFromWindowsMenu = true

let contentView = NSView(frame: NSRect(origin: .zero, size: normalizedSize))
contentView.wantsLayer = true
webView.removeFromSuperview()
webView.frame = contentView.bounds
webView.autoresizingMask = [.width, .height]
contentView.addSubview(webView)
window.contentView = contentView
window.orderFrontRegardless()

let lease = OffscreenRenderHostLease(webView: webView, viewportSize: viewportSize)
var didFinish = false
var timeoutTimer: Timer?
let finish: (Result<T, Error>) -> Void = { result in
guard !didFinish else { return }
didFinish = true
timeoutTimer?.invalidate()
timeoutTimer = nil
restoreWebView(
webView,
to: previousSuperview,
frame: previousFrame,
bounds: previousBounds,
autoresizingMask: previousAutoresizingMask,
translatesAutoresizingMaskIntoConstraints: previousTranslatesAutoresizingMaskIntoConstraints,
anchor: restoreAnchor,
position: restorePosition
)
window.orderOut(nil)
window.contentView = nil
window.close()
lease.end()
completion(result)
}

Expand Down
Loading
Loading