Skip to content

fix(desktop): pin preview CDP debugger across webview teardown - #223

Open
leoisadev1 wants to merge 1 commit into
mainfrom
fix/desktop-preview-debugger-pin
Open

fix(desktop): pin preview CDP debugger across webview teardown#223
leoisadev1 wants to merge 1 commit into
mainfrom
fix/desktop-preview-debugger-pin

Conversation

@leoisadev1

Copy link
Copy Markdown
Member

Problem

Heavy preview automation can SIGSEGV the desktop browser process. Electron's webContents.debugger wrapper is GC-managed but registered with Chromium as a raw pointer. Cleanup also re-read wc.debugger, which throws once the webview is destroyed, so detach was skipped.

Changes

Pin the debugger reference on each preview control session and route attach, detach, sendCommand, and color-scheme emulation through it. A new test destroys the webview getter and still expects detach.

Adapted from pingdotgg#9068. Preview artifact transfer remains with the remote-media worker.

Scope

This PR is debugger lifetime only.

Covered here:

Still assigned to this handoff, in later PRs:

Verification

  • vp test run apps/desktop/src/preview/Manager.test.ts: 73 passed, including the destroyed-webview detach case.
  • vp lint on the two files: no new errors.

Native Electron crash reproduction was not run on this Linux host.

Implemented and verified by Grok 4.6 High in Grok Build via Orca.

Keep the Electron Debugger wrapper alive for each preview control
session and detach through that reference. Re-reading wc.debugger after
destruction throws and can crash the browser process.

Adapted from pingdotgg#9068.
@vercel

vercel Bot commented Sep 10, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Actions Updated
akeru-bot-landing Skipped Skipped Sep 10, 2026 5:51pm UTC

Request Review

@github-actions github-actions Bot added vouch:trusted PR author is trusted by repo permissions or the VOUCHED list. size:M labels Sep 10, 2026
@greptile-apps

greptile-apps Bot commented Sep 10, 2026

Copy link
Copy Markdown

Greptile Summary

This PR pins each preview control session's Electron debugger wrapper and consistently uses that retained reference for CDP attachment, commands, color-scheme emulation, listener cleanup, and detachment. This prevents cleanup from re-reading webContents.debugger after the webview has been destroyed.

  • Adds the pinned debugger to BrowserControlSession.
  • Routes control-session CDP operations through the pinned reference.
  • Adds a regression test proving destroyed-webview cleanup still removes the listener and detaches.

Confidence Score: 5/5

The PR appears safe to merge, with the debugger lifetime fix consistently applied and covered by a focused teardown regression test.

No actionable failures remain; the retained debugger is used for attachment, command dispatch, listener cleanup, and detachment, while existing replacement cleanup prevents an old session from detaching its successor.

Important Files Changed

Filename Overview
apps/desktop/src/preview/Manager.ts Pins the Electron debugger wrapper for each control session and uses it consistently throughout the session lifecycle.
apps/desktop/src/preview/Manager.test.ts Adds focused coverage showing that cleanup detaches through the retained debugger after WebContents access becomes invalid.

Sequence Diagram

sequenceDiagram
  participant M as Preview Manager
  participant W as WebContents
  participant D as Pinned Debugger
  participant C as Chromium CDP
  M->>W: Read debugger during session creation
  W-->>M: Debugger wrapper
  M->>D: Retain wrapper for session lifetime
  D->>C: Attach and send commands
  W--xW: Webview destroyed
  M->>D: Remove listener and detach
  D->>C: Detach without re-reading WebContents
Loading

Reviews (1): Last reviewed commit: "fix(desktop): pin preview CDP debugger a..." | Re-trigger Greptile

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

Labels

size:M vouch:trusted PR author is trusted by repo permissions or the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant