fix: validate lineNumber bounds in hover computeSync (fixes #318008)#318018
Open
vs-code-engineering[bot] wants to merge 1 commit into
Open
fix: validate lineNumber bounds in hover computeSync (fixes #318008)#318018vs-code-engineering[bot] wants to merge 1 commit into
vs-code-engineering[bot] wants to merge 1 commit into
Conversation
) The hover operation uses a debouncer that can deliver a stale anchor after the model content has changed. When lines are deleted, the anchor's lineNumber may exceed the model's line count, causing getLineMaxColumn to throw 'Illegal value for lineNumber'. Add a guard in contentHoverComputer.computeSync to validate the anchor's lineNumber is within bounds before passing it to participants. Also add a redundant guard in markdownHoverParticipant.computeSync for defense. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🔧 Error Fix
Summary
The hover operation uses a
Debouncerthat can deliver a stale anchor after the model content has changed. When lines are deleted between anchor creation andcomputeSyncexecution, the anchor'sstartLineNumberexceeds the model's line count, causinggetLineMaxColumnto throwBugIndicatingError('Illegal value for lineNumber').This error affects all platforms (Windows, Mac, Linux) on version 1.121.0 with 262 hits from 243 users.
Fixes #318008
Recommended reviewer:
@alexdimaCulprit Commit
No single culprit commit identified. This is a latent race condition in the hover debounce architecture that has existed since the
Debouncerwas introduced for hover operations. The spike in telemetry correlates with the 1.121.0 stable release reaching more users rather than a specific code change.Code Flow
sequenceDiagram participant User participant HC as HoverController participant HO as HoverOperation participant DB as Debouncer participant CHC as ContentHoverComputer participant MHP as MarkdownHoverParticipant participant TM as TextModel User->>HC: mouse hover (line 50) HC->>HO: start(Delayed, anchor{line:50}) HO->>DB: schedule(anchor, firstWaitTime) User->>TM: delete lines (model now has 40 lines) HC->>HC: onDidChangeModelContent cancelScheduler Note over HC: Only cancels _reactToEditorMouseMoveRunner Note over DB: Debouncer still pending with stale anchor DB->>HO: _triggerAsyncComputation(anchor{line:50}) HO->>DB: schedule syncComputation(secondWaitTime) DB->>HO: _triggerSyncComputation(anchor{line:50}) HO->>CHC: computeSync(anchor{line:50}) CHC->>MHP: computeSync(anchor{line:50}) MHP->>TM: getLineMaxColumn(50) TM-->>MHP: THROW Illegal value for lineNumberAffected Files
src/vs/editor/contrib/hover/browser/contentHoverComputer.tssrc/vs/editor/contrib/hover/browser/markdownHoverParticipant.tsgetLineMaxColumnwith stale lineNumbersrc/vs/editor/contrib/hover/browser/contentHoverController.tssrc/vs/editor/common/model/textModel.tsRepro Steps
computeSyncwith the now-stale anchor whose lineNumber exceeds the new line countgetLineMaxColumnthrowsIllegal value for lineNumberHow the Fix Works
Chosen approach: Added a guard clause in
contentHoverComputer.ts:computeSyncthat validates the anchor'sstartLineNumberis within bounds (>= 1and<= model.getLineCount()) before passing it to participants. This is a guard at the data consumer boundary where stale data from the debouncer first meets the model API — fixing at the boundary where invalid data enters rather than at the crash site (textModel.ts). A redundant guard is also added inmarkdownHoverParticipant.ts:computeSyncfor defense in depth since it is the direct caller ofgetLineMaxColumn.Alternatives considered:
onDidChangeModelContent: would change visible behavior (hover disappears when typing elsewhere in the document), and is a larger architectural change with broader impact.getLineMaxColumn: would silence the error without fixing the stale data flow, violating the principle of not masking errors from telemetry.Recommended Owner
@alexdima— primary owner of the editor hover infrastructure and most recent contributor tomarkdownHoverParticipant.tscomputeSync logic.