`SuggestModel._onNewContext` dereferences `_completionModel` inside the guard added for `_context` (incomplete fix for #170075)
- 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
Assessment
This issue has not been assessed yet.