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 [email protected]: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.