fix: memory leak in notebook baseCellViewModel

[SimonSiefke]

Helps with #186361

Disposable output

In the scrolling up and down notebook test I noticed lots of CodeCellViewModel._register disposables:

"CodeCellViewModel._register (vscode/out/vs/base/common/lifecycle.js:400:32)\nCodeCellViewModel.resolveTextModel (vscode/out/vs/workbench/contrib/notebook/browser/viewModel/baseCellViewModel.js:519:22)",
"CodeCellViewModel._register (vscode/out/vs/base/common/lifecycle.js:400:32)\nCodeCellViewModel.resolveTextModel (vscode/out/vs/workbench/contrib/notebook/browser/viewModel/baseCellViewModel.js:519:22)",
"CodeCellViewModel._register (vscode/out/vs/base/common/lifecycle.js:400:32)\nCodeCellViewModel.resolveTextModel (vscode/out/vs/workbench/contrib/notebook/browser/viewModel/baseCellViewModel.js:519:22)",
"CodeCellViewModel._register (vscode/out/vs/base/common/lifecycle.js:400:32)\nCodeCellViewModel.resolveTextModel (vscode/out/vs/workbench/contrib/notebook/browser/viewModel/baseCellViewModel.js:519:22)",
"CodeCellViewModel._register (vscode/out/vs/base/common/lifecycle.js:400:32)\nCodeCellViewModel.resolveTextModel (vscode/out/vs/workbench/contrib/notebook/browser/viewModel/baseCellViewModel.js:519:22)",
"CodeCellViewModel._register (vscode/out/vs/base/common/lifecycle.js:400:32)\nCodeCellViewModel.resolveTextModel (vscode/out/vs/workbench/contrib/notebook/browser/viewModel/baseCellViewModel.js:519:22)",
"CodeCellViewModel._register (vscode/out/vs/base/common/lifecycle.js:400:32)\nCodeCellViewModel.resolveTextModel (vscode/out/vs/workbench/contrib/notebook/browser/viewModel/baseCellViewModel.js:519:22)",
"CodeCellViewModel._register (vscode/out/vs/base/common/lifecycle.js:400:32)\nCodeCellViewModel.resolveTextModel (vscode/out/vs/workbench/contrib/notebook/browser/viewModel/baseCellViewModel.js:519:22)",
"CodeCellViewModel._register (vscode/out/vs/base/common/lifecycle.js:400:32)\nCodeCellViewModel.resolveTextModel (vscode/out/vs/workbench/contrib/notebook/browser/viewModel/baseCellViewModel.js:519:22)",
"CodeCellViewModel._register (vscode/out/vs/base/common/lifecycle.js:400:32)\nCodeCellViewModel.resolveTextModel (vscode/out/vs/workbench/contrib/notebook/browser/viewModel/baseCellViewModel.js:519:22)",
"CodeCellViewModel._register (vscode/out/vs/base/common/lifecycle.js:400:32)\nCodeCellViewModel.resolveTextModel (vscode/out/vs/workbench/contrib/notebook/browser/viewModel/baseCellViewModel.js:519:22)",
"CodeCellViewModel._register (vscode/out/vs/base/common/lifecycle.js:400:32)\nCodeCellViewModel.resolveTextModel (vscode/out/vs/workbench/contrib/notebook/browser/viewModel/baseCellViewModel.js:519:22)",
"CodeCellViewModel._register (vscode/out/vs/base/common/lifecycle.js:400:32)\nCodeCellViewModel.resolveTextModel (vscode/out/vs/workbench/contrib/notebook/browser/viewModel/baseCellViewModel.js:519:22)",
"CodeCellViewModel._register (vscode/out/vs/base/common/lifecycle.js:400:32)\nCodeCellViewModel.resolveTextModel (vscode/out/vs/workbench/contrib/notebook/browser/viewModel/baseCellViewModel.js:519:22)",
"CodeCellViewModel._register (vscode/out/vs/base/common/lifecycle.js:400:32)\nCodeCellViewModel.resolveTextModel (vscode/out/vs/workbench/contrib/notebook/browser/viewModel/baseCellViewModel.js:519:22)",

More details

// baseCellViewModel.ts
async resolveTextModel(): Promise<model.ITextModel> {
	if (!this._textModelRef || !this.textModel) {
		this._textModelRef = await this._modelService.createModelReference(this.uri);
		if (this._isDisposed) {
			return this.textModel!;
		}

		if (!this._textModelRef) {
			throw new Error(`Cannot resolve text model for ${this.uri}`);
		}

		this._register(this.textModel!.onDidChangeContent(() => this.onDidChangeTextModelContent()));
	}

	return this.textModel!;
}

In baseCellViewModel, a text model change listener is registered when resolving a text model. The listener is only disposed when the viewModel is disposed, but not when the text model is detached. Because baseCellViewModel is reused, attaching and detaching many different text editors when scrolling up and down, more change listeners are registered over time.

It seems the listeners would need to be unregistered when detaching a text editor, similar to how textModelRef is disposed when detaching a text editor:

detachTextEditor() {
	if (this._textModelRef) {
		this._textModelRef.dispose();
		this._textModelRef = undefined;
	}
	this._textModelRefChangeDisposable?.dispose();
	this._textModelRefChangeDisposable = undefined;
}

Test script

For testing, I ran the following test script (replace VSCODE_PATH with path to local vscode):

git clone git@github.com:SimonSiefke/vscode-memory-leak-finder.git &&
cd vscode-memory-leak-finder &&
git checkout v5.42.0 &&
npm ci &&
VSCODE_PATH="/home/simon/.cache/repos/vscode/scripts/code.sh"  node packages/cli/bin/test.js --cwd packages/e2e  --check-leaks --measure-after --measure growing-disposable-stores --only notebook.code-scrolling --runs 97 &&
cat .vscode-memory-leak-finder-results/growing-disposable-stores/notebook.code-scrolling.json

Results for disposable counts

In the test output, the number of CodeCellViewModel._register disposables when scrolling up and down 97 times was reduced from 1773 to 0.

[bpasero]

Fyi for this I find MutableDisposable to be a good fit because it allows you to register the overall disposable and make sure it gets disposed properly but then also allows to set the value each time it changes, disposing the previous one.

[SimonSiefke]

Thank you! I updated the implementation to use MutableDisposable and it's indeed simpler now! :)

[rebornix]

Thank you!

[aiday-mar]

I am moving this PR to the March 2024 milestone, feel free to change it back if you would like to make the corresponding issue a candidate.