fix: don't throw resolving browser view dynamic events after view destroyed (fixes #333783)

[VS Code PR Bot (vscodebot-pr)]

Summary

The main-process BrowserViewMainService throws Browser view <id> not found when a renderer subscribes to a per-view "dynamic" event (onDynamicDidChangePermissions in the reported stack) over IPC after that view has already been destroyed. Event subscription is resolved asynchronously across the process boundary, so the view can disappear in the window between the renderer subscribing and the main process handling the listen request. The throw crosses the IPC boundary as an unhandled error and is reported to telemetry (~177 users/bucket across 5 sibling buckets that differ only by the random view GUID). This is a post-fix recurrence of the same class fixed for #318995 in 1.124.0, now surfacing through the newer permissions event path.

Fixes #333783
Recommended reviewer: @kycutler

Culprit Commit

Field	Value
Commit	67aea9c1
Author	@kycutler
PR	#322639
Message	Support browser permissions (#322639)
Why	This commit added onDynamicDidChangePermissions(id) (and the renderer subscribes to it in BrowserViewModel's constructor), following the existing onDynamic* pattern that calls _getBrowserView(id) — which throws when the view is gone. The permissions event is the specific accessor in the reported stack; all sibling onDynamic* accessors share the same race.

Code Flow

sequenceDiagram
    participant Renderer as BrowserViewModel (renderer)
    participant IPC as IPC channel
    participant Main as BrowserViewMainService
    participant Map as browserViews (DisposableMap)

    Renderer->>IPC: listen onDynamicDidChangePermissions(id)
    Note over Map: ⚠️ Root cause:<br/>view destroyed before<br/>listen request handled
    IPC->>Main: onEventListen -> onDynamicDidChangePermissions(id)
    Main->>Map: _getBrowserView(id) -> get(id) === undefined
    Note over Main: 💥 throw new Error(<br/>`Browser view ${id} not found`)
    Main-->>IPC: error crosses process boundary

Loading

Affected Files

File	Role	Evidence
src/vs/platform/browserView/electron-main/browserViewMainService.ts	crash site	L122 (from stack): throw new Error(\Browser view ${id} not found`)`
src/vs/platform/browserView/electron-main/browserViewMainService.ts	root cause	L224-L226: onDynamicDidChangePermissions(id) returns this._getBrowserView(id).onDidChangePermissions, throwing when the view is already gone
src/vs/workbench/contrib/browserView/common/browserView.ts	consumer	L539-L540: this._register(this.browserViewService.onDynamicDidChangePermissions(this.id)(snapshot => ...)) subscribes over IPC

Repro Steps

Non-deterministic timing race across the renderer↔main IPC boundary:

1. Open an integrated browser view so a BrowserViewModel is created and subscribes to onDynamicDidChangePermissions(id) (and the other onDynamic* events) over IPC.
2. Destroy/close the browser view (e.g. close the editor) so browserViews.deleteAndDispose(id) removes it from the map in the main process.
3. If the renderer's listen request for any onDynamic* event is handled by the main process after the view is removed, _getBrowserView(id) throws and the error is reported to telemetry.
4. Likelihood increases when views are opened and closed rapidly, or during window teardown when many subscriptions and disposals interleave.

How the Fix Works

Chosen approach (browserViewMainService.ts): Added a small producer-side helper _getBrowserViewEvent<T>(id, getEvent) that resolves the requested event only when the view still exists and returns Event.None otherwise. All 20 onDynamic* event accessors now route through it instead of this._getBrowserView(id).<event>. This fixes the problem at the producer of the invalid state (the main-process event accessor), not at an unrelated crash site or by silencing telemetry.

A missing view at event-subscription time is an expected outcome of asynchronous IPC subscription, not a real error: the view is gone, so there are no further events to deliver and an empty event stream is the semantically correct result. This is the external/untrusted-boundary case in the lifecycle-race guidance — the consumer lives in a separate process and cannot synchronously coordinate its subscription with main-process disposal, so the producer must tolerate the race for these event getters. Non-event methods (getState, layout, loadURL, etc.) intentionally keep throwing via _getBrowserView, because calling those against a destroyed view is a genuine caller error rather than a benign subscription race.

Alternatives considered:

• Wrapping the IPC onEventListen in try/catch — hides the error from telemetry for all channels and swallows genuine bugs, rather than fixing the specific event getters that are legitimately racy.
• Guarding at the renderer consumer (BrowserViewModel) — the renderer cannot know the view was destroyed in the main process at subscription time, so it cannot avoid the race; the fix belongs at the producer.
• Making _getBrowserView itself return undefined/Event.None for everything — would weaken the invariant for the many state-mutating methods where a missing view really is a bug.

Lifecycle pattern: external/untrusted boundary (cross-process IPC event subscription).
Producer site: src/vs/platform/browserView/electron-main/browserViewMainService.ts → onDynamicDidChangePermissions() and sibling onDynamic* accessors.
Consumer-side fix justification: the consumer (BrowserViewModel) runs in the renderer process and subscribes over IPC; it has no synchronous view of main-process disposal, so it cannot prevent the race. The fix is applied at the producer (main-process event accessors), which is the correct side and keeps the throwing contract intact for genuine misuse of state-mutating methods.

Recommended Owner

@kycutler — author of the culprit commit (#322639, "Support browser permissions") and of the surrounding onDynamic* browser-view accessors; actively committing to microsoft/vscode within the last 90 days.

Generated by errors-fix · opus48 · 473.9 AIC · ⌖ 19.5 AIC · ⊞ 18.6K · ◷

[vs-code-engineering]

📬 CODENOTIFY

The following users are being notified based on files changed in this PR:

Joaquín Ruales (@jruales)

Matched files:

• src/vs/platform/browserView/electron-main/browserViewMainService.ts

[Copilot]

Pull request overview

Prevents benign IPC subscription races from throwing after an integrated browser view is destroyed.

Changes:

• Adds a safe dynamic-event resolver returning Event.None.
• Applies it to all per-view dynamic events.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[vs-code-engineering]

Driver cycle recorded

_{errors-fix-driver:cycle head:a478e221a08547d22c7477a9abb5271e419f11dc}

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

[vs-code-engineering]

The VS Code PR Check failed instantaneously on head a478e221 (started and completed at the same second), which indicates a transient pipeline/infra failure rather than a real test failure. This external Azure DevOps check can't be re-run by the automation (read-only token, not a GitHub Actions run) — a maintainer rerun of VS Code PR Check is needed to unblock this otherwise-green, in-scope PR.

[Copilot]

🟡 Changes recommended

A missed close subscription can leave a stale browser editor and model alive.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

• Files reviewed: 1/1 changed files
• Comments generated: 1
• Review effort level: Balanced

[vs-code-engineering]

Driver cycle recorded

_{errors-fix-driver:cycle head:19292e21e7033f07ec4671ae9192df762f45f9f2}

[Copilot]

🟢 Approval recommended

The lifecycle race is handled correctly, with only a minor inline-comment cleanup remaining.

Review details

• Files reviewed: 1/1 changed files
• Comments generated: 1
• Review effort level: Balanced

[vs-code-engineering]

Driver cycle recorded

_{errors-fix-driver:cycle head:c0db4de61875a7fd1abe8d735ca676ed9575a405}

[Copilot]

🟢 Approval recommended

The focused lifecycle fix handles late subscriptions correctly without weakening other browser-view operations.

Review details

• Files reviewed: 1/1 changed files
• Comments generated: 0 new
• Review effort level: Balanced

[Kyle Cutler (kycutler)]

Copilot adding these checks just masks the underlying issue: the workbench is trying to subscribe to the browser after it has been disposed. Investigate the actual root cause more, including identifying the path that is triggering the error and the associated disposal race. Treat the race within the workbench side and do not add these blanket checks as they will only mask further issues.