fix: drop pending webview view revival on cancellation (fixes #320623) - #327583
fix: drop pending webview view revival on cancellation (fixes #320623)#327583vs-code-engineering[bot] wants to merge 2 commits into
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Note
Copilot could not run the full agentic suite for this review because it was automatically requested on a bot-authored pull request. Request a review from Copilot under Reviewers to retry with the full agentic suite. Improved support for bot-authored pull requests is coming soon.
This PR improves cancellation handling for webview view revival so pending “awaiting revival” state is cleaned up when a view is torn down, preventing stale entries and related errors.
Changes:
- Remove pending
_awaitingRevivalentries when the caller’s cancellation token is triggered. - Trigger cancellation for in-flight revival when the webview is torn down.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/vs/workbench/contrib/webviewView/browser/webviewViewService.ts | Cleans up _awaitingRevival on cancellation to prevent stale map entries and “already awaiting revival” errors. |
| src/vs/workbench/contrib/webviewView/browser/webviewViewPane.ts | Cancels the revival token source during webview teardown to drive cleanup in the service. |
| cancellation.onCancellationRequested(() => { | ||
| if (this._awaitingRevival.get(viewType)?.resolve === resolve) { | ||
| this._awaitingRevival.delete(viewType); | ||
| resolve(); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Fixed in bdaa1397: the onCancellationRequested subscription is now captured and disposed on both settle paths — in the cancellation handler itself, and in register() when a resolver revives the pending entry — so the listener no longer retains the closures or accumulates on the token.
Generated by errors-fix-driver · 1.1K AIC · ⌖ 34.7 AIC · ⊞ 72.7K
|
|
||
| // Cancel in-flight revival when the webview is torn down (dispose or re-activation) | ||
| // so the webview view service drops any pending `_awaitingRevival` entry for this view. | ||
| this._webviewDisposables.add(toDisposable(() => source.cancel())); |
There was a problem hiding this comment.
Leaving the cancel hook in _webviewDisposables intentionally. Correctness here does not depend on disposal order: the only requirement is that source.cancel() runs at some point during teardown so the service drops the pending _awaitingRevival entry. The cancel hook has no ordering dependency on the other webview disposables (drag listeners, state listener, etc.), so LIFO vs. FIFO makes no observable difference — the token fires and the service cleans up regardless. Keeping it co-located in the same store also ensures it can never be leaked if teardown paths change. If a future change adds a disposable that must observe the not-yet-cancelled token, we can revisit, but that dependency does not exist today.
Generated by errors-fix-driver · 1.1K AIC · ⌖ 34.7 AIC · ⊞ 72.7K
| cancellation.onCancellationRequested(() => { | ||
| if (this._awaitingRevival.get(viewType)?.resolve === resolve) { | ||
| this._awaitingRevival.delete(viewType); | ||
| resolve(); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Fixed in bdaa1397: the subscription returned by cancellation.onCancellationRequested(...) is now stored alongside the _awaitingRevival entry and disposed whenever the entry settles (either cancellation here or revival in register()), avoiding the retained-closure / listener-accumulation concern.
Generated by errors-fix-driver · 1.1K AIC · ⌖ 34.7 AIC · ⊞ 72.7K
| // If the caller is cancelled (e.g. the view pane is torn down or re-activated) | ||
| // before a resolver registers, drop the pending entry so a later resolve for the | ||
| // same view type does not throw "View already awaiting revival". |
There was a problem hiding this comment.
Reworded in bdaa1397: the comment now says the cleanup runs "at any point before the pending entry is revived by a resolver" rather than implying it only applies before a resolver registers.
Generated by errors-fix-driver · 1.1K AIC · ⌖ 34.7 AIC · ⊞ 72.7K
…ubscription on settle Dispose the onCancellationRequested subscription on both settle paths (cancellation and revival) so it does not retain closures or accumulate listeners. Also clarify the comment to reflect that cleanup runs any time before the pending entry is revived, not only before a resolver registers. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Commit pushed:
|
| const subscription = cancellation.onCancellationRequested(() => { | ||
| if (this._awaitingRevival.get(viewType)?.resolve === resolve) { | ||
| this._awaitingRevival.delete(viewType); | ||
| subscription.dispose(); | ||
| resolve(); | ||
| } | ||
| }); |
| resolver.resolve(pending.webview, CancellationToken.None).then(() => { | ||
| this._awaitingRevival.delete(viewType); | ||
| pending.subscription.dispose(); | ||
| pending.resolve(); | ||
| }); |
| // Cancel in-flight revival when the webview is torn down (dispose or re-activation) | ||
| // so the webview view service drops any pending `_awaitingRevival` entry for this view. | ||
| this._webviewDisposables.add(toDisposable(() => source.cancel())); |
|
assigned to @benibenj as this is to do with feature area: Workbench → Views. |
Summary
WebviewViewService.resolve()throwsError: View already awaiting revivalwhen a webview view pane callsresolve()for a view type that already has a pending_awaitingRevivalentry and no resolver has registered yet. This happens because a pending revival entry is never removed when the pane that requested it is torn down or re-activated — the secondresolve()for the same view type then hits the guard atwebviewViewService.ts:131and throws. With 1,848 affected users on 1.123.0 across all platforms, this is a benign-but-noisy lifecycle race: thecancellationtoken thatresolve()receives was previously ignored entirely.The fix makes the pending-revival entry honor cancellation (producer/service side) and makes the pane actually cancel its token when the webview is torn down.
Fixes #320623
Recommended reviewer:
@mjbvzCulprit Commit
Not a single-commit regression.
git blameshows the_awaitingRevivalguard (webviewViewService.ts:130-131) dates to61f799f53bc1(Matt Bierner, 2020-08-20) and thepromiseWithResolverspending entry toab5dd705b046(2024-01-30). The bucket is flagged as a stable anomaly first appearing in 1.123.0 (6a44c352) with a possible prior occurrence (#227624, closed as not_planned), and the commit range8761a556...6a44c352could not be narrowed (not reproduced in insider builds). This is best classified as a pre-existing latent race that surfaced at stable scale rather than a specific culprit commit; thecancellationparameter has been unused since the pending-revival path was introduced.Code Flow
sequenceDiagram participant Pane as WebviewViewPane.activate participant Svc as WebviewViewService Pane->>Svc: resolve(id, webviewView, token) [no resolver yet] Svc->>Svc: _awaitingRevival.set(id, {webview, resolve}) Note over Pane: pane hidden / disposed / re-activated Pane-->>Pane: _webviewDisposables.clear() (token NOT cancelled before fix) Pane->>Svc: resolve(id, webviewView2, token2) [still no resolver] Svc->>Svc: _awaitingRevival.has(id) === true Svc-->>Pane: throw Error('View already awaiting revival')Affected Files
src/vs/workbench/contrib/webviewView/browser/webviewViewService.ts—resolve()now registers acancellation.onCancellationRequestedhandler that removes the pending_awaitingRevivalentry (guarded by identity check onresolve) and settles the promise.src/vs/workbench/contrib/webviewView/browser/webviewViewPane.ts—activate()now cancels theCancellationTokenSourcewhen_webviewDisposablesis cleared (dispose or re-activation), so the service is notified to drop the pending entry.Repro Steps
_awaitingRevivalentry is created._webviewDisposableswhile the resolver still hasn't registered.activate()callsresolve()again for the same view id →View already awaiting revivalis thrown. Frequent at stable scale during startup/extension-activation windows.How the Fix Works
Chosen approach — fix at the producer of the stale state, not the crash site (the guard at
webviewViewService.ts:131is a symptom):webviewViewService.ts: the_awaitingRevivalentry is the state that goes stale when its requesting pane disappears. The service now subscribes to the caller'scancellationtoken and, on cancellation, deletes the pending entry (only if it is still the same entry — identity check onresolve) and settles its promise. This makes the previously-unusedcancellationparameter meaningful and ensures a laterresolve()for the same view type takes the fresh path instead of throwing.webviewViewPane.ts: the pane owns theCancellationTokenSourcein_webviewDisposables.CancellationTokenSource.dispose()does not cancel, so atoDisposable(() => source.cancel())is added to the same store; when the store is cleared (on pane dispose or re-activation via thedisposecallback / teardown), the token is cancelled and the service drops the pending entry. This closes the lifecycle gap rather than tolerating the inconsistent state.No
try/catchwas added and the throw path is preserved — it simply becomes unreachable in the torn-down-pane scenario because the pending entry is cleaned up at its producer.Alternatives considered
resolve()with atry/catchor replacingthrowwith an early return: hides the lifecycle bug instead of fixing the producer, and would leave the stale_awaitingRevivalentry in place, breaking future genuine revivals.register(): does not address teardown that happens before any resolver registers, which is the actual failing sequence.Recommended Owner
@mjbvz— original author and long-time owner of the webview view service and pane (webviewViewService.ts,webviewViewPane.ts).errors-fix-driver — cycle 1
Trigger: cron_review_comments · Head:
bdaa13976ca524313af9618d9610d9af69082543(bdaa1397)webviewViewService.ts:145(in scope, leak)bdaa1397— dispose cancellation subscription on settle (replied + resolved)webviewViewService.ts:145(in scope, duplicate leak)bdaa1397(replied + resolved)webviewViewService.ts:139(in scope, comment wording)bdaa1397(replied + resolved)webviewViewPane.ts:205(subjective, LIFO order)Push: yes —
bdaa1397· Copilot rerequested: okReady gate: CI in progress on the new head (re-triggered by this push) and Copilot has not re-reviewed the new SHA yet → not marking ready this cycle