microsoft / microsoft/vscode

`SuggestModel._onNewContext` dereferences `_completionModel` inside the guard added for `_context` (incomplete fix for #170075)

Open
#331,935 0 comments 0 reactions 1 assignee Claimed by @hediet View on GitHub
Dominant language
TypeScript
Stars
193k
Forks
42.4k
PR merge metrics
PR metrics pending

Description

### The bug

`SuggestModel._onNewContext` still throws `TypeError: Cannot read properties of undefined (reading 'getItemsByProvider')`.

https://github.com/microsoft/vscode/blob/main/src/vs/editor/contrib/suggest/browser/suggestModel.ts — in the "started a new word while IntelliSense shows" branch:

```ts
if (ctx.leadingWord.word.length !== 0 && ctx.leadingWord.startColumn > this._context.leadingWord.startColumn) {
// started a new word while IntelliSense shows -> retrigger but reuse all items that we currently have
const shouldAutoTrigger = LineContext.shouldAutoTrigger(this._editor);
if (shouldAutoTrigger && this._context) {
// shouldAutoTrigger forces tokenization, which can cause pending cursor change events to be emitted, which can cause
// suggestions to be cancelled, which causes `this._context` to be undefined
const map = this._completionModel.getItemsByProvider(); // <-- throws
```

The comment describes the hazard exactly, and the guard was added for it — but it only re-checks `this._context`, while the line it protects dereferences `this._completionModel`.

### Why the guard is not sufficient

`cancel()` clears **both** fields:

```ts
this._triggerState = undefined;
this._completionModel = undefined;
this._context = undefined;
```

but `trigger()` re-assigns `_context` **synchronously** and only assigns `_completionModel` when its asynchronous `provideSuggestionItems` promise resolves:

```ts
this.cancel(options.retrigger);
this._triggerState = options;
this._context = ctx; // <-- set immediately
...
Promise.all([completions, wordDistance]).then(async ([completions, wordDistance]) => {
...
this._completionModel = new CompletionModel(...); // <-- set much later
```

So when the tokenization forced by `shouldAutoTrigger` emits a pending cursor change that cancels **and re-triggers** the session, `this._context` is truthy again while `this._completionModel` is still `undefined`, and the guard lets execution through.

The earlier `if (!this._completionModel) return;` does not help: `_completionModel` is non-`undefined` on entry to the branch and is cleared *during* the `shouldAutoTrigger` call.

Note the sibling branch further down in the same method handles this correctly — it checks `!this._context` after `shouldAutoTrigger` and calls `this.cancel()`.

### Suggested fix

Include the field that is actually dereferenced:

```diff
-if (shouldAutoTrigger && this._context) {
+if (shouldAutoTrigger && this._context && this._completionModel) {
```

The branch already ends in `return`, and the in-flight request's own `_onNewContext` call will run when it resolves, so falling through loses nothing.

### History

- #170075 — same error, same line, closed as completed. That fix is what added the `this._context` guard, but the `_completionModel` dereference was left unguarded.
- #170459 — same error, auto-closed as a duplicate.

### Notes on reproducing

This is a narrow synchronous race and I have not been able to reproduce it on demand; it arrives via error telemetry. In our case it is a `monaco-editor-core` embedding (0.52.2, also verified present in 0.56.0 and in `main` today) with asynchronous completion providers that fetch over HTTP, which widens the window in which `_completionModel` is unset. Observed on Chrome 150 / macOS.

Happy to open a PR with the one-line change if that is useful.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.