fix(desktop): tear down a HUD whose renderer dies before first paint (#108230)
A window born show:false is revealed only by success-shaped events (ready-to-show, did-finish-load). When the HUD's main frame fails to load or its render process dies before first paint, neither fires, the 4s fallback is never even scheduled, and the transparent window stays hidden forever while broadcastHudState(true) keeps every toggle reading open. wireWindowReveal now grows a failure branch (onRevealFailed) that fires exactly once for a main-frame did-fail-load or a pre-reveal render-process-gone, cancels a pending fallback, and disarms the reveal. The HUD passes a handler that tears the window down through the bounded requestHudClose, so the existing 'closed' handler owns the one teardown path (snap shortcut, main-window restore, broadcastHudState(false)) and the toggles converge to closed. The log-only post-reveal lifecycle (#81290) is untouched: a crash after a successful reveal is still diagnosable, not resurrected. wireWindowReveal moves from main.ts into window-reveal.ts (same signature, event wiring now unit-testable against fake emitters).
This commit is contained in:
Hermes Agent
brooklyn!
@@ -540,7 +540,7 @@ import {
|
||||
import { registerWindowControlIpc, windowControlState } from './window-controls'
|
||||
import { createWindowOpenHandler } from './window-open-policy'
|
||||
import { installWindowRendererLifecycle } from './window-renderer-lifecycle'
|
||||
import { createWindowRevealController } from './window-reveal'
|
||||
import { wireWindowReveal } from './window-reveal'
|
||||
import {
|
||||
bindGeometryPersistence,
|
||||
computeWindowOptions,
|
||||
@@ -12959,23 +12959,8 @@ function installPreviewGuestPreload() {
|
||||
// though the renderer finished loading. Keep the themed path as the preferred
|
||||
// reveal, then fall back a few seconds after the renderer loads. `show` and
|
||||
// `onRevealed` carry the caller's reveal action and post-visible work; whichever
|
||||
// path wins runs them exactly once.
|
||||
function wireWindowReveal(win, { show, onRevealed }: { show?: () => void; onRevealed?: () => void } = {}) {
|
||||
const controller = createWindowRevealController(
|
||||
{
|
||||
isDestroyed: () => win.isDestroyed(),
|
||||
isVisible: () => win.isVisible(),
|
||||
show: show ?? (() => win.show())
|
||||
},
|
||||
{ onRevealed }
|
||||
)
|
||||
|
||||
win.once('ready-to-show', controller.reveal)
|
||||
win.webContents.once('did-finish-load', controller.scheduleFallback)
|
||||
win.on('closed', controller.dispose)
|
||||
|
||||
return controller
|
||||
}
|
||||
// path wins runs them exactly once. Callers that pass `onRevealFailed` also get
|
||||
// the pre-paint failure branch (see window-reveal.ts).
|
||||
|
||||
// Secondary "session windows" — one extra OS window per chat so a user can
|
||||
// work with multiple chats side by side. The registry guarantees one window
|
||||
@@ -13939,6 +13924,17 @@ function spawnHudWindow(sessionId, profile) {
|
||||
// Compositor overlay adapters (Hyprland float+pin today). Electron
|
||||
// alwaysOnTop is already set; this is the dialect some WMs actually hear.
|
||||
void promoteHudOverlay({ title: HUD_WINDOW_TITLE })
|
||||
},
|
||||
// #108230: the HUD is born `show: false` + transparent, and its renderer
|
||||
// lifecycle is deliberately log-only (#81290 — a dead renderer should be
|
||||
// diagnosable, not resurrected). But a load failure or renderer crash
|
||||
// BEFORE first paint leaves a hidden window every toggle claims is open.
|
||||
// Tear it down instead: requestHudClose is bounded, and the 'closed'
|
||||
// handler below owns the one teardown path (snap shortcut, main-window
|
||||
// restore, broadcastHudState(false)) so the toggles converge to closed.
|
||||
onRevealFailed: reason => {
|
||||
rememberLog(`[renderer:hud] window never revealed; tearing it down (${reason})`)
|
||||
destroyHudWindow(win)
|
||||
}
|
||||
})
|
||||
|
||||
|
||||
@@ -2,7 +2,7 @@ import assert from 'node:assert/strict'
|
||||
|
||||
import { test } from 'vitest'
|
||||
|
||||
import { createWindowRevealController } from './window-reveal'
|
||||
import { createWindowRevealController, wireWindowReveal } from './window-reveal'
|
||||
|
||||
function createHarness({ visible = false }: { visible?: boolean } = {}) {
|
||||
let destroyed = false
|
||||
@@ -186,3 +186,174 @@ test('reveals without an onRevealed callback', () => {
|
||||
assert.equal(controller.reveal(), true)
|
||||
assert.equal(shown, true)
|
||||
})
|
||||
|
||||
// ── Reveal failure branch (#108230) ─────────────────────────────────────────
|
||||
//
|
||||
// A window created `show: false` is only revealed by success-shaped events
|
||||
// (ready-to-show / did-finish-load). When the main frame fails to load or the
|
||||
// render process dies before first paint, none of those fire — the fallback
|
||||
// timer is never even scheduled — so the window stays hidden forever while
|
||||
// callers keep claiming it is open. The failure branch hands such windows to
|
||||
// `onRevealFailed` exactly once and disarms every other path.
|
||||
|
||||
type FailureHarness = {
|
||||
emit(event: 'closed' | 'did-finish-load' | 'did-fail-load' | 'render-process-gone', ...args: unknown[]): void
|
||||
emitReadyToShow(): void
|
||||
failures: string[]
|
||||
revealCalls: number
|
||||
scheduledCallback: (() => void) | null
|
||||
showCalls: number
|
||||
visible: () => boolean
|
||||
}
|
||||
|
||||
function createFailureHarness(): FailureHarness {
|
||||
const windowListeners = new Map<string, Array<(...args: any[]) => void>>()
|
||||
const webContentsListeners = new Map<string, Array<(...args: any[]) => void>>()
|
||||
let visible = false
|
||||
const failures: string[] = []
|
||||
let revealCalls = 0
|
||||
let scheduledCallback: (() => void) | null = null
|
||||
|
||||
const win = {
|
||||
isDestroyed: () => false,
|
||||
isVisible: () => visible,
|
||||
show: () => {
|
||||
visible = true
|
||||
},
|
||||
once: (event: string, listener: (...args: any[]) => void) => {
|
||||
windowListeners.set(`once:${event}`, [...(windowListeners.get(`once:${event}`) ?? []), listener])
|
||||
},
|
||||
on: (event: string, listener: (...args: any[]) => void) => {
|
||||
windowListeners.set(`on:${event}`, [...(windowListeners.get(`on:${event}`) ?? []), listener])
|
||||
},
|
||||
webContents: {
|
||||
once: (event: string, listener: (...args: any[]) => void) => {
|
||||
webContentsListeners.set(`once:${event}`, [...(webContentsListeners.get(`once:${event}`) ?? []), listener])
|
||||
},
|
||||
on: (event: string, listener: (...args: any[]) => void) => {
|
||||
webContentsListeners.set(`on:${event}`, [...(webContentsListeners.get(`on:${event}`) ?? []), listener])
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
wireWindowReveal(win, {
|
||||
onRevealed: () => {
|
||||
revealCalls += 1
|
||||
},
|
||||
onRevealFailed: reason => {
|
||||
failures.push(reason)
|
||||
},
|
||||
setTimer: callback => {
|
||||
scheduledCallback = callback
|
||||
|
||||
return 1 as unknown as ReturnType<typeof setTimeout>
|
||||
},
|
||||
clearTimer: () => {}
|
||||
})
|
||||
|
||||
const emit = (
|
||||
target: 'window' | 'webContents',
|
||||
kind: 'once' | 'on',
|
||||
event: string,
|
||||
...args: unknown[]
|
||||
) => {
|
||||
const key = `${kind}:${event}`
|
||||
const map = target === 'window' ? windowListeners : webContentsListeners
|
||||
|
||||
for (const listener of [...(map.get(key) ?? [])]) {
|
||||
listener(...args)
|
||||
}
|
||||
}
|
||||
|
||||
return {
|
||||
emit: (event, ...args) => {
|
||||
if (event === 'closed') {
|
||||
emit('window', 'on', event, ...args)
|
||||
} else if (event === 'did-finish-load') {
|
||||
emit('webContents', 'once', event, ...args)
|
||||
} else {
|
||||
emit('webContents', 'on', event, ...args)
|
||||
}
|
||||
},
|
||||
emitReadyToShow: () => emit('window', 'once', 'ready-to-show'),
|
||||
failures,
|
||||
get revealCalls() {
|
||||
return revealCalls
|
||||
},
|
||||
get scheduledCallback() {
|
||||
return scheduledCallback
|
||||
},
|
||||
showCalls: 0,
|
||||
visible: () => visible
|
||||
}
|
||||
}
|
||||
|
||||
test('a main-frame load failure before reveal fires onRevealFailed once and disarms the reveal', () => {
|
||||
const harness = createFailureHarness()
|
||||
|
||||
// The main frame failed: did-finish-load never fires, so no fallback is
|
||||
// scheduled — without the failure branch nothing would ever happen again.
|
||||
harness.emit('did-fail-load', {}, -3, 'ABORTED', 'file:///hud', true)
|
||||
|
||||
assert.equal(harness.failures.length, 1)
|
||||
assert.match(harness.failures[0]!, /main frame/)
|
||||
|
||||
// Disarmed: a late ready-to-show must not show a dead window, and a second
|
||||
// failure must not re-fire the teardown.
|
||||
harness.emitReadyToShow()
|
||||
assert.equal(harness.visible(), false)
|
||||
assert.equal(harness.revealCalls, 0)
|
||||
|
||||
harness.emit('render-process-gone', {}, { reason: 'oom' })
|
||||
assert.equal(harness.failures.length, 1)
|
||||
})
|
||||
|
||||
test('a subframe load failure never tears the window down', () => {
|
||||
const harness = createFailureHarness()
|
||||
|
||||
harness.emit('did-fail-load', {}, -3, 'ABORTED', 'file:///subframe.js', false)
|
||||
harness.emitReadyToShow()
|
||||
|
||||
assert.deepEqual(harness.failures, [])
|
||||
assert.equal(harness.visible(), true)
|
||||
})
|
||||
|
||||
test('a render-process crash before first paint fires onRevealFailed', () => {
|
||||
const harness = createFailureHarness()
|
||||
|
||||
harness.emit('render-process-gone', {}, { reason: 'crashed' })
|
||||
|
||||
assert.equal(harness.failures.length, 1)
|
||||
assert.match(harness.failures[0]!, /render process gone/)
|
||||
})
|
||||
|
||||
test('a crash or load failure AFTER reveal leaves the window alone', () => {
|
||||
const harness = createFailureHarness()
|
||||
|
||||
harness.emitReadyToShow()
|
||||
assert.equal(harness.visible(), true)
|
||||
|
||||
// Post-reveal, the window's own lifecycle owns recovery — the reveal
|
||||
// controller must not resurrect its failure branch for a shown window.
|
||||
harness.emit('render-process-gone', {}, { reason: 'crashed' })
|
||||
harness.emit('did-fail-load', {}, -3, 'ABORTED', 'file:///hud', true)
|
||||
|
||||
assert.deepEqual(harness.failures, [])
|
||||
assert.equal(harness.revealCalls, 1)
|
||||
})
|
||||
|
||||
test('a scheduled fallback is cancelled when the load fails first', () => {
|
||||
const harness = createFailureHarness()
|
||||
|
||||
// did-finish-load arrived (fallback scheduled at 4s), then the process died
|
||||
// before first paint: the timer must not later reveal a dead window.
|
||||
harness.emit('did-finish-load')
|
||||
assert.ok(harness.scheduledCallback)
|
||||
|
||||
harness.emit('render-process-gone', {}, { reason: 'kill' })
|
||||
harness.scheduledCallback?.()
|
||||
|
||||
assert.equal(harness.failures.length, 1)
|
||||
assert.equal(harness.visible(), false)
|
||||
assert.equal(harness.revealCalls, 0)
|
||||
})
|
||||
|
||||
@@ -8,6 +8,8 @@ type TimerHandle = ReturnType<typeof setTimeout>
|
||||
|
||||
type WindowRevealOptions = {
|
||||
onRevealed?: () => void
|
||||
/** Called once when the window fails to reach first paint (see fail below). */
|
||||
onRevealFailed?: (reason: string) => void
|
||||
delayMs?: number
|
||||
setTimer?: (callback: () => void, delayMs: number) => TimerHandle
|
||||
clearTimer?: (timer: TimerHandle) => void
|
||||
@@ -19,6 +21,7 @@ export function createWindowRevealController(
|
||||
window: WindowRevealTarget,
|
||||
{
|
||||
onRevealed = () => {},
|
||||
onRevealFailed = () => {},
|
||||
delayMs = WINDOW_REVEAL_FALLBACK_MS,
|
||||
setTimer = (callback, delay) => setTimeout(callback, delay),
|
||||
clearTimer = timer => clearTimeout(timer)
|
||||
@@ -26,6 +29,7 @@ export function createWindowRevealController(
|
||||
) {
|
||||
let disposed = false
|
||||
let revealed = false
|
||||
let failed = false
|
||||
let fallbackTimer: TimerHandle | null = null
|
||||
|
||||
const cancelFallback = () => {
|
||||
@@ -38,7 +42,7 @@ export function createWindowRevealController(
|
||||
}
|
||||
|
||||
const reveal = () => {
|
||||
if (disposed || revealed || window.isDestroyed()) {
|
||||
if (disposed || revealed || failed || window.isDestroyed()) {
|
||||
return false
|
||||
}
|
||||
|
||||
@@ -55,7 +59,7 @@ export function createWindowRevealController(
|
||||
}
|
||||
|
||||
const scheduleFallback = () => {
|
||||
if (disposed || revealed || fallbackTimer !== null || window.isDestroyed()) {
|
||||
if (disposed || revealed || failed || fallbackTimer !== null || window.isDestroyed()) {
|
||||
return
|
||||
}
|
||||
|
||||
@@ -65,6 +69,28 @@ export function createWindowRevealController(
|
||||
}, delayMs)
|
||||
}
|
||||
|
||||
/**
|
||||
* The reveal gate has no success-shaped event left to hang on: the main
|
||||
* frame failed to load or the render process died before first paint, so
|
||||
* neither `ready-to-show` nor `did-finish-load` will ever fire — and a
|
||||
* `show: false` window would stay hidden forever while callers keep
|
||||
* claiming it is open (#108230). Hand it to `onRevealFailed` once and
|
||||
* disarm every other path. A window that already revealed keeps its own
|
||||
* caller-chosen lifecycle: a post-reveal crash is not this branch's
|
||||
* business.
|
||||
*/
|
||||
const fail = (reason: string) => {
|
||||
if (disposed || revealed || failed || window.isDestroyed()) {
|
||||
return false
|
||||
}
|
||||
|
||||
failed = true
|
||||
cancelFallback()
|
||||
onRevealFailed(reason)
|
||||
|
||||
return true
|
||||
}
|
||||
|
||||
const dispose = () => {
|
||||
disposed = true
|
||||
cancelFallback()
|
||||
@@ -72,7 +98,84 @@ export function createWindowRevealController(
|
||||
|
||||
return {
|
||||
dispose,
|
||||
fail,
|
||||
reveal,
|
||||
scheduleFallback
|
||||
}
|
||||
}
|
||||
|
||||
/** Minimal event-emitter shape wireWindowReveal listens on (a BrowserWindow
|
||||
* satisfies it; tests pass fakes). */
|
||||
type RevealEvents = {
|
||||
once(event: string, listener: (...args: unknown[]) => void): unknown
|
||||
on(event: string, listener: (...args: unknown[]) => void): unknown
|
||||
}
|
||||
|
||||
export type WindowRevealWindow = WindowRevealTarget &
|
||||
RevealEvents & {
|
||||
webContents: RevealEvents & {
|
||||
on(
|
||||
event: 'did-fail-load',
|
||||
listener: (
|
||||
event: unknown,
|
||||
errorCode: number,
|
||||
errorDescription: string,
|
||||
validatedURL: string,
|
||||
isMainFrame: boolean
|
||||
) => void
|
||||
): unknown
|
||||
on(event: 'render-process-gone', listener: (event: unknown, details: { reason?: string }) => void): unknown
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Wire a `show: false` window's reveal to the load lifecycle: reveal on
|
||||
* `ready-to-show`, arm the bounded fallback on `did-finish-load`, and — only
|
||||
* for callers that pass `onRevealFailed` — catch the two pre-paint death
|
||||
* modes (`did-fail-load` on the main frame, `render-process-gone`) so a
|
||||
* failed window is handed to the caller once instead of staying hidden
|
||||
* forever.
|
||||
*/
|
||||
export function wireWindowReveal(
|
||||
win: WindowRevealWindow,
|
||||
{
|
||||
show,
|
||||
onRevealed,
|
||||
onRevealFailed,
|
||||
...timing
|
||||
}: {
|
||||
show?: () => void
|
||||
onRevealed?: () => void
|
||||
onRevealFailed?: (reason: string) => void
|
||||
} & Omit<WindowRevealOptions, 'onRevealed' | 'onRevealFailed'> = {}
|
||||
) {
|
||||
const controller = createWindowRevealController(
|
||||
{
|
||||
isDestroyed: () => win.isDestroyed(),
|
||||
isVisible: () => win.isVisible(),
|
||||
show: show ?? (() => win.show())
|
||||
},
|
||||
{ onRevealed, onRevealFailed, ...timing }
|
||||
)
|
||||
|
||||
win.once('ready-to-show', controller.reveal)
|
||||
win.webContents.once('did-finish-load', controller.scheduleFallback)
|
||||
win.on('closed', controller.dispose)
|
||||
|
||||
if (onRevealFailed) {
|
||||
win.webContents.on(
|
||||
'did-fail-load',
|
||||
(_event, errorCode, errorDescription, _validatedURL, isMainFrame) => {
|
||||
if (isMainFrame) {
|
||||
controller.fail(`main frame failed to load (${errorCode}: ${errorDescription})`)
|
||||
}
|
||||
}
|
||||
)
|
||||
|
||||
win.webContents.on('render-process-gone', (_event, details) => {
|
||||
controller.fail(`render process gone (${details?.reason ?? 'unknown'})`)
|
||||
})
|
||||
}
|
||||
|
||||
return controller
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user