Repository navigation
Fix browser panes stuck black after a failed discard-restore #7533
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
Show all changes
42 commits
Select commit
Hold shift + click to select a range
0804a92
Add failing regression tests for discarded browser webview restore re…
austinywang 17684f9
Fix browser panes stuck black after a failed discard-restore (#7504)
austinywang cfe0bb7
Merge remote-tracking branch 'origin/main' into issue-7504-browser-di…
austinywang 07b21c4
Fix remote queued discard restore state
austinywang 1dcab64
Handle discarded restore review edge cases
austinywang 449e8cd
Ignore about:blank commits when tracking discarded-restore recovery
austinywang 40b3647
Merge remote-tracking branch 'origin/main' into issue-7504-browser-di…
austinywang 1e042d2
Refresh Swift file length budget for BrowserNavigationDelegate downlo…
austinywang e190d94
Mark pure lifecycle formatter helpers nonisolated
austinywang 6394ab3
Fix download and no-URL edge cases in discarded webview restore
austinywang 9f9ad5b
Seed committed flag for adopted prewarmed webviews; move commit predi…
austinywang 64273c9
Merge remote-tracking branch 'origin/main' into issue-7504-browser-di…
austinywang 4d71ea1
Gate blank-shell heal off during pending WebContent crash recovery
austinywang 2f2bef9
Cache the lifecycle-payload ISO8601 formatter
austinywang d8cfb97
Merge remote-tracking branch 'origin/main' into issue-7504-browser-di…
austinywang 70e89e7
Scope committed-document tracking to each discarded restore attempt
austinywang 7b5b343
ci: keep staged macOS jobs alive when web jobs skip
austinywang 74be6ba
ci: align staged macOS job gates with upstream fix and update guard test
austinywang 2555004
Keep restore-stall detection armed after an about:blank commit
austinywang 6c25969
Let explicit reloads restart pending restores; attribute restore call…
austinywang 09b0dac
Never blank-shell-heal over an explicit user Stop
austinywang 90101c2
Merge remote-tracking branch 'origin/main' into issue-7504-browser-di…
austinywang 8012061
Make explicit Stop sticky for discarded restores; move restore flow t…
austinywang 32ed2aa
Never blank-shell-heal over the browser error page
austinywang 7bb6a31
Merge remote-tracking branch 'origin/main' into issue-7504-browser-di…
austinywang cb2851c
Add queued remote restore regression test
austinywang c3667e7
Deduplicate queued remote discard restores
austinywang 7b9eda6
Complete policy-cancelled discard restores
austinywang 2610eff
Handle policy-cancelled browser restores explicitly
austinywang 03ebf17
Keep intent fallback restores retryable
austinywang 2623e8d
Complete insecure HTTP prompt restores
austinywang 8a6307b
Preserve current restore attempts on stale cancels
austinywang 7ba6262
Defer external prompt restore completion
austinywang 6345987
Clear stale restore navigation on stalls
austinywang 620bd08
Tokenize browser restore policy cancels
austinywang 9167d38
Keep insecure HTTP restore prompts retryable
austinywang daa077b
Avoid browser panel budget growth
austinywang 3042ee2
Preserve restore tokens through policy prompts
austinywang 1acb3b4
Scope restore downloads to attempts
austinywang bf21072
Merge remote-tracking branch 'origin/main' into issue-7504-browser-di…
austinywang 74d3507
Complete terminal restore handling for nil-target tabs
austinywang 348e3ee
Fix PR comment CI guard regressions
austinywang File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,318 @@ | ||
| import Foundation | ||
| import WebKit | ||
|
|
||
| extension BrowserPanel { | ||
| func shouldTreatCommitAsDiscardedRestoreCommit(from webView: WKWebView) -> Bool { | ||
| guard navigationDelegate?.activeErrorPageDisplayURL == nil else { return false } | ||
| guard let committedURL = webView.url else { return false } | ||
| return !Self.isAboutBlankURL(committedURL) | ||
| } | ||
|
|
||
| func noteDiscardedWebViewRestoreNavigationStarted() { | ||
| if hiddenWebViewDiscardManager.isDiscardedForMemory { | ||
| // Each restore attempt tracks its own commit. Without this reset, a | ||
| // previous attempt's error-page commit would satisfy the stall | ||
| // detector forever and a silently stalled retry could never re-arm. | ||
| hasCommittedDocumentSinceWebViewReplacement = false | ||
| currentDiscardRestoreAttemptID = UUID() | ||
| } | ||
| hiddenWebViewDiscardManager.noteRestoreNavigationStarted(reason: "navigation") | ||
| refreshWebViewLifecycleState() | ||
| } | ||
|
|
||
| func noteDiscardedWebViewRestoreNavigationCommitted(reason: String = "navigation_commit") { | ||
| guard hiddenWebViewDiscardManager.noteRestoreNavigationCommitted(reason: reason) else { | ||
| return | ||
| } | ||
| pendingDiscardRestoreNavigation = nil | ||
| currentDiscardRestoreAttemptID = nil | ||
| refreshWebViewLifecycleState() | ||
| } | ||
|
|
||
| func noteDiscardedWebViewRestoreNavigationDidNotCommit(reason: String) { | ||
| hiddenWebViewDiscardManager.noteRestoreNavigationDidNotCommit(reason: reason) | ||
| pendingDiscardRestoreNavigation = nil | ||
| refreshWebViewLifecycleState() | ||
| } | ||
|
|
||
| /// Whether a WebKit failure/cancel callback belongs to the navigation the | ||
| /// discard-restore bookkeeping is tracking. WebKit can deliver an older | ||
| /// provisional load's cancellation after a newer attempt has already | ||
| /// started; clearing the pending state for that stale callback would let a | ||
| /// visibility touch hijack the in-flight navigation with a restore reload. | ||
| /// A nil callback navigation or no tracked navigation matches conservatively. | ||
| func isDiscardRestoreBookkeepingNavigation(_ navigation: WKNavigation?) -> Bool { | ||
| guard let tracked = pendingDiscardRestoreNavigation else { return true } | ||
| guard let navigation else { return true } | ||
| return navigation === tracked | ||
| } | ||
|
|
||
| /// Restore touch for a possibly-discarded pane: detects stalled restore | ||
| /// attempts, honors an explicit user Stop, restores through the discard | ||
| /// manager, and falls back to blank-shell healing. | ||
| @discardableResult | ||
| func restoreDiscardedWebViewIfNeeded( | ||
| reason: String, | ||
| cachePolicy: URLRequest.CachePolicy = .useProtocolCachePolicy, | ||
| allowBlankShellHeal: Bool = true, | ||
| forceRestartPendingRestore: Bool = false | ||
| ) -> Bool { | ||
| if Self.isRestoreStalled( | ||
| isRestoreNavigationPending: hiddenWebViewDiscardManager.isRestoreNavigationPending, | ||
| isWebViewLoading: webView.isLoading, | ||
| isMainFrameProvisionalNavigationActive: isMainFrameProvisionalNavigationActive, | ||
| hasPendingRemoteNavigation: hasPendingRemoteNavigation, | ||
| hasCommittedDocument: hasCommittedDocumentSinceWebViewReplacement | ||
| ) { | ||
| noteDiscardedWebViewRestoreNavigationDidNotCommit(reason: "\(reason).stalled") | ||
| } | ||
|
austinywang marked this conversation as resolved.
|
||
|
|
||
| if forceRestartPendingRestore { | ||
| userStoppedLoadSinceWebViewReplacement = false | ||
| } | ||
| // Stop is sticky for discarded restores too: routine visibility touches | ||
| // must not restart a stopped load; explicit reload is the override. | ||
| guard !userStoppedLoadSinceWebViewReplacement else { return false } | ||
|
|
||
| if Self.isQueuedRemoteRestoreInFlight( | ||
| isDiscardedForMemory: hiddenWebViewDiscardManager.isDiscardedForMemory, | ||
| hasPendingRemoteNavigation: hasPendingRemoteNavigation, | ||
| forceRestartPendingRestore: forceRestartPendingRestore | ||
| ) { | ||
| return true | ||
| } | ||
|
|
||
| let restoreURL = restoredHistoryCurrentURL ?? currentURL | ||
| guard let restoreURL, !Self.isAboutBlankURL(restoreURL) else { | ||
| return reactivateDiscardedPaneWithoutRestorableURL(reason: reason) | ||
| } | ||
|
|
||
| if hiddenWebViewDiscardManager.restoreIfNeeded(reason: reason, force: forceRestartPendingRestore, performRestore: { | ||
| shouldRenderWebView = true | ||
| navigateWithoutInsecureHTTPPrompt( | ||
| to: restoreURL, | ||
| recordTypedNavigation: false, | ||
| preserveRestoredSessionHistory: true, | ||
| cachePolicy: cachePolicy | ||
| ) | ||
| }) { | ||
|
austinywang marked this conversation as resolved.
|
||
| return true | ||
| } | ||
|
|
||
| guard allowBlankShellHeal else { return false } | ||
| return healBlankRestoredWebViewIfNeeded(reason: reason, cachePolicy: cachePolicy) | ||
| } | ||
|
|
||
| /// Re-navigates a rendered-but-empty web view (for example after a failed | ||
| /// discard restore whose state was already consumed) back to its intent URL. | ||
| @discardableResult | ||
| private func healBlankRestoredWebViewIfNeeded( | ||
| reason _: String, | ||
| cachePolicy: URLRequest.CachePolicy | ||
| ) -> Bool { | ||
| let intentURL = restoredHistoryCurrentURL ?? currentURL | ||
| let isNavigationBlockedPendingConsent = intentURL.map { browserShouldBlockInsecureHTTPURL($0) } ?? false | ||
| guard Self.shouldHealBlankShell( | ||
| shouldRenderWebView: shouldRenderWebView, | ||
| isClosing: isClosingWebViewLifecycle, | ||
| hasPendingRemoteNavigation: hasPendingRemoteNavigation, | ||
| isWebViewLoading: webView.isLoading, | ||
| isMainFrameProvisionalNavigationActive: isMainFrameProvisionalNavigationActive, | ||
| hasCommittedDocument: hasCommittedDocumentSinceWebViewReplacement, | ||
| isNavigationBlockedPendingConsent: isNavigationBlockedPendingConsent, | ||
| hasRecoverableWebContentTermination: hasRecoverableWebContentTermination, | ||
| userStoppedLoad: userStoppedLoadSinceWebViewReplacement, | ||
| isShowingErrorPage: navigationDelegate?.activeErrorPageDisplayURL != nil, | ||
| intentURL: intentURL | ||
| ) else { | ||
| return false | ||
| } | ||
| guard let intentURL else { return false } | ||
| navigateWithoutInsecureHTTPPrompt( | ||
| to: intentURL, | ||
| recordTypedNavigation: false, | ||
| preserveRestoredSessionHistory: true, | ||
| cachePolicy: cachePolicy | ||
| ) | ||
| return true | ||
| } | ||
|
|
||
| /// Restore fallback for a discarded pane with no restorable document (nil | ||
| /// or about:blank restore URL): navigating would wait on a commit that | ||
| /// ``shouldTreatCommitAsDiscardedRestoreCommit(from:)`` ignores, leaving the | ||
| /// manager pending forever, so reactivate in place instead. | ||
| func reactivateDiscardedPaneWithoutRestorableURL(reason: String) -> Bool { | ||
| guard reactivateDiscardedWebViewWithoutNavigation(reason: "\(reason).no_restore_url") else { | ||
| return false | ||
| } | ||
| refreshNavigationAvailability() | ||
| refreshWebViewLifecycleState() | ||
| return true | ||
| } | ||
|
|
||
| /// ISO8601DateFormatter is documented thread-safe; cached so the polled | ||
| /// lifecycle-payload path stays allocation-free. | ||
| private nonisolated(unsafe) static let webViewLifecycleTimestampFormatter: ISO8601DateFormatter = { | ||
| let formatter = ISO8601DateFormatter() | ||
| formatter.formatOptions = [.withInternetDateTime, .withFractionalSeconds] | ||
| return formatter | ||
| }() | ||
|
|
||
| nonisolated static func webViewLifecycleTimestamp(_ date: Date?) -> Any { | ||
| guard let date else { return NSNull() } | ||
| return webViewLifecycleTimestampFormatter.string(from: date) | ||
| } | ||
|
|
||
| nonisolated static func webViewHiddenDurationMilliseconds( | ||
| hiddenAt: Date?, | ||
| visible: Bool, | ||
| now: Date | ||
| ) -> Any { | ||
| guard !visible, let hiddenAt else { return NSNull() } | ||
| return max(0, Int((now.timeIntervalSince(hiddenAt) * 1000.0).rounded())) | ||
| } | ||
|
|
||
| nonisolated static func isAboutBlankURL(_ url: URL?) -> Bool { | ||
| guard let url else { return false } | ||
| let value = url.absoluteString.trimmingCharacters(in: .whitespacesAndNewlines) | ||
| return value.caseInsensitiveCompare("about:blank") == .orderedSame | ||
| } | ||
|
|
||
| nonisolated static func shouldHealBlankShell( | ||
| shouldRenderWebView: Bool, | ||
| isClosing: Bool, | ||
| hasPendingRemoteNavigation: Bool, | ||
| isWebViewLoading: Bool, | ||
| isMainFrameProvisionalNavigationActive: Bool, | ||
| hasCommittedDocument: Bool, | ||
| isNavigationBlockedPendingConsent: Bool, | ||
| hasRecoverableWebContentTermination: Bool, | ||
| userStoppedLoad: Bool, | ||
| isShowingErrorPage: Bool, | ||
| intentURL: URL? | ||
| ) -> Bool { | ||
| guard shouldRenderWebView else { return false } | ||
| guard !isClosing else { return false } | ||
| guard !hasPendingRemoteNavigation else { return false } | ||
| guard !isWebViewLoading else { return false } | ||
| guard !isMainFrameProvisionalNavigationActive else { return false } | ||
| guard !hasCommittedDocument else { return false } | ||
| guard !isNavigationBlockedPendingConsent else { return false } | ||
| // A crashed WebContent process waits for the user's explicit Reload; | ||
| // auto-healing here would bypass that gate and can re-enter the crash. | ||
| guard !hasRecoverableWebContentTermination else { return false } | ||
| // A load the user explicitly stopped before first commit must stay | ||
| // stopped; healing on reveal would silently undo the Stop. | ||
| guard !userStoppedLoad else { return false } | ||
| // The browser's own error page commits as about:blank; it is content | ||
| // awaiting the user's Reload, not a blank shell to heal over. | ||
| guard !isShowingErrorPage else { return false } | ||
| guard let intentURL else { return false } | ||
| return !isAboutBlankURL(intentURL) | ||
| } | ||
|
|
||
| /// Whether a discarded pane's restore is already queued waiting for the | ||
| /// remote workspace proxy endpoint. A queued remote restore never enters | ||
| /// performNavigation, so isRestoreNavigationPending stays false; without | ||
| /// this check every later restore touch would re-run the restore closure | ||
| /// and re-queue the navigation instead of treating the queue as in-flight. | ||
| /// An explicit reload (force) still restarts the queued restore. | ||
| nonisolated static func isQueuedRemoteRestoreInFlight( | ||
| isDiscardedForMemory: Bool, | ||
| hasPendingRemoteNavigation: Bool, | ||
| forceRestartPendingRestore: Bool | ||
| ) -> Bool { | ||
| guard isDiscardedForMemory, hasPendingRemoteNavigation else { return false } | ||
| return !forceRestartPendingRestore | ||
| } | ||
|
|
||
| nonisolated static func isRestoreStalled( | ||
| isRestoreNavigationPending: Bool, | ||
| isWebViewLoading: Bool, | ||
| isMainFrameProvisionalNavigationActive: Bool, | ||
| hasPendingRemoteNavigation: Bool, | ||
| hasCommittedDocument: Bool | ||
| ) -> Bool { | ||
| guard isRestoreNavigationPending else { return false } | ||
| guard !isWebViewLoading else { return false } | ||
| guard !isMainFrameProvisionalNavigationActive else { return false } | ||
| guard !hasPendingRemoteNavigation else { return false } | ||
| return !hasCommittedDocument | ||
| } | ||
| } | ||
|
|
||
| extension BrowserPanel { | ||
| /// Whether browser native/SwiftUI fills should draw over the window root | ||
| /// backdrop. Mirrors terminal/markdown panel background decisions. | ||
| static func drawsConfiguredWebViewBackground( | ||
| isBlankPage: Bool, | ||
| usesTransparentBackground: Bool = false | ||
| ) -> Bool { | ||
| drawsWebViewBackground( | ||
| isBlankPage: isBlankPage, | ||
| usesTransparentBackground: usesTransparentBackground, | ||
| opacity: GhosttyApp.shared.defaultBackgroundOpacity, | ||
| usesGhosttyGlassStyle: GhosttyApp.shared.defaultBackgroundBlur.isMacOSGlassStyle, | ||
| usesTransparentWindow: WindowBackgroundComposition.policy | ||
| .shouldUseTransparentBackgroundWindow(glassEffectAvailable: false) | ||
| ) | ||
| } | ||
|
|
||
| nonisolated static func isBlankBrowserPageURL(_ url: URL?) -> Bool { | ||
| guard let url else { return true } | ||
| let value = url.absoluteString.trimmingCharacters(in: .whitespacesAndNewlines) | ||
| return value.caseInsensitiveCompare("about:blank") == .orderedSame | ||
| } | ||
|
|
||
| nonisolated static func isBlankBrowserPage( | ||
| liveURL: URL?, | ||
| currentURL: URL?, | ||
| pendingNavigationURL: URL?, | ||
| isMainFrameProvisionalNavigationActive: Bool | ||
| ) -> Bool { | ||
| if isMainFrameProvisionalNavigationActive, | ||
| !isBlankBrowserPageURL(pendingNavigationURL) { | ||
| return false | ||
| } | ||
| if !isBlankBrowserPageURL(pendingNavigationURL), | ||
| isBlankBrowserPageURL(liveURL), | ||
| isBlankBrowserPageURL(currentURL) { | ||
| return false | ||
| } | ||
| return isBlankBrowserPageURL(liveURL) && isBlankBrowserPageURL(currentURL) | ||
| } | ||
|
|
||
| nonisolated static func drawsWebViewBackground( | ||
| isBlankPage: Bool, | ||
| usesTransparentBackground: Bool = false, | ||
| opacity: Double, | ||
| usesGhosttyGlassStyle: Bool, | ||
| usesTransparentWindow: Bool | ||
| ) -> Bool { | ||
| if usesTransparentBackground { | ||
| return drawsWebViewBackground( | ||
| opacity: opacity, | ||
| usesGhosttyGlassStyle: usesGhosttyGlassStyle, | ||
| usesTransparentWindow: usesTransparentWindow | ||
| ) | ||
| } | ||
| guard isBlankPage else { return true } | ||
| return drawsWebViewBackground( | ||
| opacity: opacity, | ||
| usesGhosttyGlassStyle: usesGhosttyGlassStyle, | ||
| usesTransparentWindow: usesTransparentWindow | ||
| ) | ||
| } | ||
|
|
||
| nonisolated static func drawsWebViewBackground( | ||
| opacity: Double, | ||
| usesGhosttyGlassStyle: Bool, | ||
| usesTransparentWindow: Bool | ||
| ) -> Bool { | ||
| !PanelAppearance.shouldUseClearContentBackground( | ||
| opacity: opacity, | ||
| usesGhosttyGlassStyle: usesGhosttyGlassStyle, | ||
| usesTransparentWindow: usesTransparentWindow | ||
| ) | ||
| } | ||
| } | ||
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.