fix: guard multi-diff item clearBinding against re-entrant disposal (fixes #339312)

[VS Code PR Bot (vscodebot-pr)]

Summary

The multi-diff editor throws BugIndicatingError: Cannot unbind a diff editor template from a different item when a ManagedVirtualizedItem disposes a binding. Clearing a binding synchronously disposes the diff editor, which fires editor model-change events that drive the item's own autorun; that autorun re-enters _clearBinding() and disposes the same binding a second time, so unbind() finds _viewModel already cleared (undefined !== item) and throws. Impact: unhandled error surfaced to telemetry when closing/switching multi-diff editors (e.g. the Agent Sessions diff view) on all platforms.

Fixes #339312
Recommended reviewer: @hediet

Culprit Commit

Field	Value
Commit	10de1b93436
Author	@hediet
PR	n/a
Message	Refactor multi-diff virtualized scrolling
Why	This change introduced the ManagedVirtualizedItem lifecycle where _clearBinding() disposes a binding whose dispose() synchronously re-enters the same item's autorun via editor events; the clear path has no re-entrancy guard, so the nested dispose runs against already-cleared state. Subsequent commits (98e228b47bc, 519f5fa46f0) refined the same lifecycle but kept the re-entrant clear path.

Code Flow

sequenceDiagram
    participant Autorun as ManagedVirtualizedItem.autorun
    participant Clear as _clearBinding()
    participant Binding as DiffEditorItemBinding.dispose()
    participant Unbind as DiffEditorItemTemplate.unbind()/setItem()
    participant Editor as DiffEditor events

    Autorun->>Clear: item hidden, not kept alive
    Note over Clear: ⚠️ Root cause:<br/>no re-entrancy guard
    Clear->>Binding: binding.dispose()
    Binding->>Unbind: _template.unbind(item) (before super.dispose)
    Note over Unbind: setItem(undefined)<br/>_viewModel = undefined<br/>editor.setDiffModel(null)
    Unbind->>Editor: synchronous model-change event
    Editor->>Autorun: re-enters autorun
    Autorun->>Clear: _clearBinding() again
    Clear->>Binding: binding.dispose() again
    Binding->>Unbind: unbind(item)
    Note over Unbind: 💥 _viewModel (undefined) !== item<br/>Cannot unbind a diff editor template from a different item

Loading

Affected Files

File	Role	Evidence
src/vs/editor/browser/widget/multiDiffEditor/diffEditorItemTemplate.ts	crash site	L539 (from stack): throw new BugIndicatingError('Cannot unbind a diff editor template from a different item')
src/vs/editor/browser/widget/multiDiffEditor/diffEditorItemTemplate.ts	re-entrant trigger	L592-L593: dispose() calls this._template.unbind(this.item) before super.dispose(), so currentBinding/_store are still live; unbind → setItem(undefined) fires editor events synchronously
src/vs/editor/browser/widget/multiDiffEditor/virtualizedItemManager.ts	root cause (producer)	L228-L247: _clearBinding() disposes the binding with no guard; the autorun at L149-L155 re-enters it during that synchronous dispose

Repro Steps

This is a timing-dependent re-entrancy, reproduced by closing/hiding a multi-diff item whose diff editor fires a synchronous model-change event during disposal.

1. Open a multi-diff editor (e.g. a source-control / Agent Sessions diff view) with at least one visible diff item that has loaded its diff model.
2. Trigger disposal of a bound, visible item — close the editor, switch the active editor, or scroll the item out while it is being cleared.
3. During _clearBinding(), binding.dispose() → unbind() → setItem(undefined) → editor.setDiffModel(null) fires an editor event that re-enters the item's autorun, calling _clearBinding() a second time on the same binding.
4. The nested unbind() sees _viewModel === undefined, undefined !== item, and throws BugIndicatingError.

How the Fix Works

Chosen approach — src/vs/editor/browser/widget/multiDiffEditor/virtualizedItemManager.ts, ManagedVirtualizedItem._clearBinding(): add an _isClearingBinding guard flag. On entry, if a clear is already in progress the method returns immediately; otherwise it sets the flag, performs the unbind/dispose/reset sequence inside a try, and resets the flag in finally. This fixes the bug at the data producer — the clear path that initiates the re-entrant disposal — rather than at the crash site. The outer _clearBinding() invocation still completes the full clear atomically (it sets _templateReference to undefined and disposes the reference after the binding dispose returns), so the nested call being a no-op does not leave the item half-cleared. The throw inside unbind() is preserved as a genuine invariant check for other callers; it is simply no longer reached on this re-entrant path because the duplicate dispose never starts.

Why this is correct: the guard makes the unsupported "dispose during dispose" ordering unrepresentable at the producer, which is the lifecycle-fix priority (reorder/serialize the producer's state mutations), instead of teaching the consumer unbind() to tolerate inconsistent state. No logService.error or telemetry path is removed, and no try/catch swallows the error.

Alternatives considered:

• Guard inside unbind() / DiffEditorItemBinding.dispose() by comparing _viewModel and returning early — rejected: that is a crash-site guard that silently tolerates double-dispose and would mask the same re-entrancy for every other binding type, hiding the producer bug.
• Defer editor.setDiffModel(null) to a microtask so events do not fire synchronously — rejected: broader behavioral change to editor teardown timing with higher regression risk than serializing the clear.

Recommended Owner

@hediet — author of the multi-diff virtualization refactor (10de1b93436) and the follow-up lifecycle fixes; active microsoft/vscode core team member (write access, multiple commits in the last 90 days).

[Copilot]

Copilot review overview

🟡 Changes recommended

The timing-sensitive disposal fix needs a focused regression test.

Review effort: Balanced
Findings: 1

Open (1)

• Add regression test for synchronous re-entry during binding disposal · New

What changed in this PR

Prevents re-entrant disposal from unbinding the same multi-diff editor template twice.

Changes:

• Adds a guarded binding-clear lifecycle with try/finally.
• Preserves existing template invariant checks and cleanup.

File	Description
virtualizedItemManager.ts	Guards _clearBinding() against synchronous re-entry.

---

💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.