-
Notifications
You must be signed in to change notification settings - Fork 37.6k
Fix disposed store error in DetachedTerminalSnapshotMirror #289154
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Add disposal checks in async methods to prevent accessing disposed terminal after component disposal: - Check _store.isDisposed after async terminal retrieval - Check after terminal.xterm.write() completes - Handle terminal disposal gracefully in constructor promise Co-authored-by: meganrogge <29464607+meganrogge@users.noreply.github.com>
Co-authored-by: meganrogge <29464607+meganrogge@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR fixes a race condition where DetachedTerminalSnapshotMirror can be disposed while async operations are in flight, causing "Trying to add a disposable to a DisposableStore that has already been disposed of" errors when accessing terminal.xterm.buffer.
Changes:
- Added disposal checks after async operations in constructor,
attach(), andrender()methods - Added
test-output.jsonto .gitignore to exclude test artifacts
Reviewed changes
Copilot reviewed 1 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/vs/workbench/contrib/terminal/browser/chatTerminalCommandMirror.ts | Added disposal checks in DetachedTerminalSnapshotMirror after awaiting terminal to prevent accessing disposed terminal buffers |
| .gitignore | Excluded test-output.json test artifact from version control |
| if (this._store.isDisposed) { | ||
| return; | ||
| } |
Copilot
AI
Jan 21, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The disposal check guards added to prevent the "Trying to add a disposable to a DisposableStore that has already been disposed of" error are not covered by tests. Consider adding a test case that simulates the disposal race condition by disposing the DetachedTerminalSnapshotMirror while async operations (attach() or render()) are in flight, to ensure the disposal checks work as intended and prevent the error.
DetachedTerminalSnapshotMirrorcan be disposed while async operations (attach(),render()) are in flight. Subsequent access toterminal.xterm.buffertriggers xterm's internal lazy registration, causing "Trying to add a disposable to a DisposableStore that has already been disposed of" errors.Changes
attach(): Early return after awaiting terminal if disposedrender(): Early return after_getTerminal()and afterterminal.xterm.write()if disposedWarning
Firewall rules blocked me from connecting to one or more addresses (expand for details)
I tried to connect to the following addresses, but was blocked by firewall rules:
redirector.gvt1.com/proc/self/exe /proc/self/exe --type=utility --utility-sub-type=network.mojom.NetworkService --lang=en-US --service-sandbox-type=none --no-sandbox --crashpad-handler-pid=6781 --enable-crash-reporter=7406e0f2-c70e-45b3-a85a-795a433530b7,no_channel --user-data-dir=/tmp/vscode-tests-1768928301321 --shared-files=v8_context_snapshot_data:100 --field-trial-handle=3,i,12506016518474805750,15792222962786304686,262144 --enable-features=PdfUseShowSaveFilePicker --disable-features=LocalNetworkAccessChecks,ScreenAIOCREnabled,SpareRendererForSitePerProcess,TraceSiteInstanceGetProcessCreation --variations-seed-version --trace-process-track-uuid=3190708989122997041(dns block).build/electron/code-oss .build/electron/code-oss test/unit/electron/index.js --crash-reporter-directory=/home/REDACTED/work/vscode/vscode/.build/crashes --runGlob **/chatTerminalCommandMirror.test.js(dns block)/proc/self/exe /proc/self/exe --type=utility --utility-sub-type=network.mojom.NetworkService --lang=en-US --service-sandbox-type=none --no-sandbox --crashpad-handler-pid=8480 --enable-crash-reporter=7406e0f2-c70e-45b3-a85a-795a433530b7,no_channel --user-data-dir=/tmp/vscode-tests-1768928654352 --shared-files=v8_context_snapshot_data:100 --field-trial-handle=3,i,14289129642060378853,1945214153318072413,262144 --enable-features=PdfUseShowSaveFilePicker --disable-features=LocalNetworkAccessChecks,ScreenAIOCREnabled,SpareRendererForSitePerProcess,TraceSiteInstanceGetProcessCreation --variations-seed-version --trace-process-track-uuid=3190708989122997041 d tsgo --project .ldd ommandMirror.tes/home/REDACTED/.cache/ms-playwright/webkit-2215/minibrowser-wpe/sys/lib/libsoup-3.0.so.0.7.4 odules/npm/node_modules/@npmcli/run-script/lib/node-gyp-bin/ldd ldd(dns block)If you need me to access, download, or install something from one of these locations, you can either:
Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.