Skip to content

fix(desktop): guard broadcasts by renderer frame - #5286

Open
Dante-dan wants to merge 2 commits into
apache:mainfrom
Dante-dan:fix/5135-renderer-generation-sendability
Open

fix(desktop): guard broadcasts by renderer frame#5286
Dante-dan wants to merge 2 commits into
apache:mainfrom
Dante-dan:fix/5135-renderer-generation-sendability

Conversation

@Dante-dan

@Dante-dan Dante-dan commented Sep 14, 2026

Copy link
Copy Markdown
Contributor

Summary

  • gate shared Desktop broadcasts on the lifetime of the current main WebFrameMain
  • send through the captured frame so the liveness check and delivery address the same Renderer generation
  • resume normal broadcasts when Electron installs a live replacement frame, leaving the existing recovery and resynchronization paths unchanged

WebContents.isDestroyed() is insufficient during renderer-process loss and navigation because the WebContents can outlive its current main frame. Capturing the frame before checking isDestroyed() avoids asking WebContents.send() to resolve a disposed generation internally.

Fixes #5135

Verification

  • npm run build
  • node --test --test-force-exit apps/desktop/dist/main/__tests__/workhub-presentation.test.js apps/desktop/dist/main/__tests__/main-startup-lifetime.test.js (36 passed)
  • npm run lint
  • npm run format:check
  • npm run typecheck
  • npx knip --workspace apps/desktop
  • npx knip --workspace packages/ui
  • npx biome check apps/desktop/src/main/main-window.ts apps/desktop/src/main/__tests__/workhub-presentation.test.ts
  • git diff --check

AI use

Select exactly one:

  • No generative tool made a substantive contribution
  • Generative tooling made a substantive contribution

Tool(s) and scope: Codex traced the Renderer recovery path and implemented the frame-generation broadcast guard.

Checklist

  • Tests cover the change and fail without it
  • Lint, format, typecheck and the affected suites pass locally

Does this PR entail a change in behavior?

  • Yes — described under Summary above
  • No

@Dante-dan
fix(desktop): guard broadcasts by renderer frame
9913561 
Generated-by: Codex
Signed-off-by: Dante <duanjl.china@gmail.com>
@github-actions github-actions Bot added the effort/S Under 100 readable lines label Sep 14, 2026
hqhq1025
hqhq1025 reviewed Sep 14, 2026
View reviewed changes

@hqhq1025 hqhq1025 left a comment

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.

Technical GO on exact head 99135613c7c9ee00dcbd56ab3865bda4231fc707; I found no P0-P2 issues. One P3 test-coverage gap is noted inline.

The implementation fixes the actual ownership boundary: each broadcast captures the current WebFrameMain, checks that exact renderer generation, and sends through the same object. A real Electron 43.4.1/Xvfb stress probe completed 14,100 broadcasts across forcefullyCrashRenderer() and reload without the prior Error sending from webFrameMain, while broadcasts resumed after a live replacement frame appeared.

Review conclusions:

  1. The solution is optimal for the reported problem and stays at the shared broadcast boundary.
  2. Production code to delete: none identified.
  3. Tests to delete: none; the existing broadcast test should be strengthened as described inline.
  4. No deeper refactor is required.
  5. The revision is technically merge-ready; the P3 is non-blocking, and the final decision remains with a maintainer.
  6. Residual gaps: the exact crash/reload behavior was exercised on Linux/Xvfb, not native Windows or macOS.

This changes renderer-lifecycle IPC delivery behavior, so independent human review is required.

Verification passed build:test, full typecheck/lint/format, ASF headers, changed-file Biome, git diff --check, focused Desktop lifecycle tests 36/36, the real Electron crash/reload probe, all hosted checks, and a clean merge with current main bf6e9422960401da6bba01d03a8ea9979722bb5a. The full Desktop run was 2470 passed / 8 cancelled; those cancellations are in unchanged current-main MCP OAuth deadline tests and are not attributable to this two-file change.

Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.

Comment thread apps/desktop/src/main/__tests__/workhub-presentation.test.ts Outdated
} as unknown as Electron.WebFrameMain;
const renderer = Object.assign(new EventEmitter(), {
isDestroyed: () => false,
mainFrame,

@hqhq1025 hqhq1025 Sep 14, 2026

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.

[P3] Exercise the unavailable-frame branch instead of only adapting the mock

This test now supplies mainFrame.send, but both WebContents and the frame stay permanently live. It therefore does not verify the defect being fixed: a non-destroyed WebContents whose mainFrame is destroyed or throws during renderer replacement, followed by a live replacement frame. For example, removing the frame.isDestroyed() check while keeping mainFrame.send() would leave this test green. Please drive the same registered renderer through destroyed-frame, accessor-throw, and replacement-live states, asserting no delivery/no throw during the gap and delivery after recovery.

@Dante-dan Dante-dan Sep 14, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Automated Codex follow-up: I added the requested regression coverage in 4982416. The existing auxiliary-renderer test now drives the same registered renderer through a destroyed main frame, a mainFrame accessor that throws during replacement, and a live replacement. It verifies that both unavailable phases neither throw nor deliver, then confirms delivery resumes after replacement.

Validation:

  • npm run build
  • node --test --test-force-exit apps/desktop/dist/main/__tests__/workhub-presentation.test.js (29 passed)
  • npm run lint, npm run format:check, npm run typecheck
  • npx knip --workspace apps/desktop, npx knip --workspace packages/ui

I also temporarily bypassed the frame-destruction guard: the new no-delivery assertion failed as intended; restoring the guard returned the suite to 29/29.

@Dante-dan
test(desktop): cover renderer frame replacement
4982416 
Generated-by: Codex
Signed-off-by: Dante <duanjl.china@gmail.com>
@Dante-dan
Dante-dan force-pushed the fix/5135-renderer-generation-sendability branch from c266eef to 4982416 Compare September 14, 2026 09:44
hqhq1025
hqhq1025 reviewed Sep 14, 2026
View reviewed changes

@hqhq1025 hqhq1025 left a comment

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.

Technical GO on exact head 49824162834d0dc8b4d0af2bd0b4d25c4c1bf83c; I found no remaining P0-P3 issues.

The new test closes the previous coverage gap. It now verifies all three relevant renderer-generation states at the shared broadcast boundary: a destroyed WebFrameMain is skipped, a transient webContents.mainFrame access failure is contained, and delivery resumes exactly once after a live replacement frame appears. The production implementation is byte-identical to the previously reviewed head, so the earlier Electron 43.4.1/Xvfb stress result still applies: 14,100 broadcasts across forcefullyCrashRenderer() and reload completed without Error sending from webFrameMain, and delivery resumed after recovery. The current exact head also passed the real WorkHub crash/recovery Electron journey.

Review conclusions:

  1. The solution is optimal for the reported lifecycle race and remains localized at the shared broadcast boundary.
  2. Production code to delete: none identified.
  3. Tests to delete or replace: none; the added regression directly exercises the formerly missing states.
  4. No deeper refactor is required.
  5. The revision is technically merge-ready; GitHub remains REVIEW_REQUIRED, so the final decision belongs to an independent maintainer.
  6. Residual gaps: the crash/recovery path was exercised on Linux/Xvfb, not native Windows or macOS.

This changes renderer-lifecycle IPC delivery behavior, so independent human review is required.

Verification passed build:test, full typecheck/lint/format, ASF headers, changed-file Biome, renderer architecture 112/112, E2E budget 38, focused WorkHub presentation 29/29, the real Electron WorkHub crash/recovery E2E 1/1, hosted test/package, git diff --check, and a clean merge with current main f32cf2b482a17f235b92e2775ed2672c2775e219. The full Desktop run was 2470 passed / 8 cancelled; the same eight cancellations reproduce in the unchanged MCP OAuth deadline suite when run alone and are not attributable to this two-file change.

Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.

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

Reviewers

@hqhq1025 hqhq1025 hqhq1025 left review comments

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

effort/S Under 100 readable lines

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Desktop broadcasts flood stderr while the main Renderer frame is unavailable

2 participants

@Dante-dan @hqhq1025