Skip to content

fix(desktop): crash on boot when mainWindow is destroyed mid-startup (Object has been destroyed) - #38589

Open
xxxigm wants to merge 2 commits into
NousResearch:mainfrom
xxxigm:fix/desktop-window-destroyed-boot
Open

xxxigm wants to merge 2 commits into
NousResearch:mainfrom
xxxigm:fix/desktop-window-destroyed-boot

Conversation

@xxxigm

@xxxigm xxxigm commented Jun 4, 2026

Copy link
Copy Markdown
Contributor

What does this PR do?

Fixes a desktop boot crash where startup fails right after the (remote) backend reports ready:

Desktop boot failed: Object has been destroyed
TypeError: Object has been destroyed
    at getWindowState (.../electron/main.cjs)

Fixes #38468

Root cause

startHermes() resolves its connection object with ...getWindowState() once the backend is ready (for both the remote and local branches). getWindowState() / getWindowButtonPosition() read the window with optional chaining:

isFullscreen: Boolean(mainWindow?.isFullScreen?.())
// and
return mainWindow?.getWindowButtonPosition?.() || WINDOW_BUTTON_POSITION

But a destroyed Electron BrowserWindow is still a non-null object — optional chaining only guards null/undefined, not a destroyed window. Calling any native method on it throws Object has been destroyed.

So when the window is torn down during the async boot (the reporter's logs show updater relaunch — [updates] launched updater … exiting desktop; the same applies to gateway reconnect), spreading getWindowState() throws, and the boot's .catch reports Desktop boot failed: Object has been destroyed even though the backend is healthy and reachable.

This matches the rest of the file, which already guards every other window access with if (!mainWindow || mainWindow.isDestroyed()) return.

Changes

  • New dependency-free electron/window-state.cjs with isWindowLive(), getWindowState(), getWindowButtonPosition() that check isDestroyed() (and swallow residual throws) before touching the window, returning inert defaults otherwise.
  • main.cjs delegates getWindowState() / getWindowButtonPosition() to it. No behavioural change for a live window.

How to test

cd apps/desktop && npm run test:desktop:platforms   # 29 passed (incl. 6 new)

Tests added

electron/window-state.test.cjs: getWindowState / getWindowButtonPosition never throw for a destroyed or missing window and fall back to inert defaults, while still reflecting a live window (full-screen + traffic-light position).

xxxigm added 2 commits June 4, 2026 07:48
startHermes() resolves with `...getWindowState()` once the backend is ready.
getWindowState()/getWindowButtonPosition() queried mainWindow via optional
chaining (`mainWindow?.isFullScreen?.()`), but a destroyed BrowserWindow is
still a non-null object — optional chaining doesn't guard it, and the native
call throws "Object has been destroyed". When the window is torn down during
boot (updater relaunch, gateway reconnect), that exception rejected the entire
boot with "Desktop boot failed: Object has been destroyed", even though the
remote backend was healthy and ready.

Move the geometry reads into a dependency-free window-state.cjs that checks
isDestroyed() (and swallows residual throws) before touching the window,
falling back to inert defaults. main.cjs now delegates to it.

Fixes NousResearch#38468
Add window-state.test.cjs (run via test:desktop:platforms): getWindowState and
getWindowButtonPosition never throw for a destroyed or missing window and fall
back to inert defaults, while still reflecting a live window.
@xxxigm
xxxigm requested a review from a team June 4, 2026 00:49
@alt-glitch alt-glitch added type/bug Something isn't working P3 Low — cosmetic, nice to have labels Jun 4, 2026

@austinpickett austinpickett left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Hermes Agent Review — ✅ Approve

Checked out locally and ran the new test suite — 6/6 pass:

node --test apps/desktop/electron/window-state.test.cjs
✔ isWindowLive distinguishes null, destroyed, and live windows
✔ getWindowState does not throw for a destroyed window (regression #38468)
✔ getWindowState does not throw for a missing window
✔ getWindowState reflects a live window
✔ getWindowButtonPosition returns null off macOS and falls back when not live
✔ getWindowButtonPosition falls back when the live window returns nothing
ℹ pass 6  fail 0

The root-cause analysis is exactly right and the subtle part is handled correctly. A destroyed Electron BrowserWindow is still a non-null JS object, so optional chaining (win?.isFullScreen?.()) does not protect against it — every native accessor throws "Object has been destroyed". Since startHermes() resolves with ...getWindowState() once the backend is ready, a mid-boot window teardown (updater relaunch / gateway reconnect) turned that read into a whole-boot failure. The fix:

  • extracts the geometry reads into window-state.cjs with a real isDestroyed() guard plus try/catch (correctly handling even the pathological case where isDestroyed() itself throws);
  • keeps the module electron-import-free so it's unit-testable with plain fake windows — good design, matches the existing bootstrap-platform.cjs/backend-probes.cjs pattern in this dir;
  • wires both getWindowButtonPosition and getWindowState in main.cjs through the new helpers; behavior is preserved for the live-window path.

Test coverage is thorough (null, destroyed, throws-on-isDestroyed, live, fullscreen, fallback). Registered in test:desktop:platforms.

Cluster note (not a blocker): this is one of several open desktop PRs following the same "extract a testable .cjs module + wire into main.cjs + register the test in package.json" pattern (also #37471/#39554/#38292/#42901/#39522/#40558). They'll have textual conflicts on main.cjs and the test:desktop:platforms line, but no semantic overlap — each adds a distinct module. Whichever lands first, the rest just need a trivial rebase. No action needed here.

Reviewed by Hermes Agent (local node --test run; verified vs origin/main).

@austinpickett austinpickett left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Clean, well-scoped fix for a subtle Electron lifecycle bug. The root cause analysis is correct: optional chaining (?.) only guards null/undefined — it offers zero protection against a destroyed BrowserWindow, which remains a non-null object but throws "Object has been destroyed" on every native accessor.

What the PR gets right

  • Extracts the two unsafe helpers into a standalone window-state.cjs with no electron import, which makes them genuinely unit-testable with plain fake objects.
  • isWindowLive() uses isDestroyed() as the primary gate, and wraps even that call in a try/catch for the case where the window object is so far gone even isDestroyed() throws — an edge case the comment explicitly calls out.
  • All three exported functions have the same defensive pattern: check liveness first, fall back to safe defaults, swallow any residual throws.
  • Six targeted tests cover every path: null, undefined, destroyed, thoroughly-destroyed (even isDestroyed throws), and live (full-screen + traffic-light position). All cases have explicit assertions.
  • package.json wires the new test file into the existing test:desktop:platforms run, so CI covers it automatically.
  • main.cjs changes are minimal — just delegation calls with no behaviour change for a live window.

Minor observation (non-blocking): the getWindowButtonPosition wrapper already exists in main.cjs as a thin shim. Once this PR lands it is purely a pass-through, so a future cleanup pass could inline the call-site directly. Not a reason to block.

Overall: well-diagnosed, well-tested, minimal blast radius. Approved.

@austinpickett

Copy link
Copy Markdown
Collaborator

LGTM — approved.

The key insight that optional chaining does not protect against a destroyed BrowserWindow is well-documented in both the PR body and the new module header, which will help the next person debugging an Electron lifecycle issue.

Two small things that could be done as follow-ups (not blockers):

  1. Inline the thin shimgetWindowButtonPosition() in main.cjs is now a one-liner pass-through. It could be inlined at its two call-sites to remove the intermediate function entirely, but this is cosmetic.
  2. Test script entry – consider adding window-state.test.cjs to a dedicated test:window-state script as well as the combined test:desktop:platforms run, so it can be exercised in isolation without the full platform suite bootstrap.

Neither is a blocker. Merging as-is is fine.

@teknium1

Copy link
Copy Markdown
Collaborator

Thanks for diagnosing a real Electron lifecycle edge case. The premise remains live on current main: apps/desktop/electron/main.ts:4482 and :4491 still use optional chaining for native BrowserWindow reads, and their result is spread into ready responses at :6783 and :6940.

Problems

  • The implementation targets the pre-migration CJS surface. Commit 39d09453f95e8aefc0c97e5d9b30ff341cae9ed8 renamed electron/main.cjs to main.ts and window-state.cjs to window-state.ts; current main has no tracked CJS target files.
  • apps/desktop/electron/window-state.ts is already imported by main.ts:121-127 for persisted geometry helpers. The proposed new module has the same name but incompatible exports, so it cannot be applied unchanged.

Suggested changes

  • Port the destroyed-window guard and its fake-window tests to TypeScript, wiring main.ts:getWindowButtonPosition() / getWindowState() at :4477-4495.
  • Preserve the existing window-state.ts geometry API; use a distinct liveness helper or extend it compatibly. Register the TypeScript test in apps/desktop/package.json:41.

Automated hermes-sweeper review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P3 Low — cosmetic, nice to have sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Hermes Desktop macOS remote gateway crashes after backend ready: Object has been destroyed in getWindowState()

4 participants