Clarification about markers

[jerch]

Just tried to use markers for the image addon to get informed, when it is safe to free resources again. I am not quite sure how markers are supposed to be used via API. Is there a reason why onDispose is not exported? Is it unsafe to rely on it?

Due to this I did a quick hack with a marker collector, which checks for isDisposed and line and found that the line is still decrementing after the marker is meant to be disposed:

(The 'got disposed!' message is from my type overloading with a onDispose callback.)

This somewhat looks like it was not really disposed, as some code from xterm.js internals updates the now illegal line index (thus still holds a ref somehow)? Imho related code would be here:

xterm.js/src/common/buffer/Buffer.ts

Lines 582 to 614 in a5e269a

	public addMarker(y: number): Marker {
	const marker = new Marker(y);
	this.markers.push(marker);
	marker.register(this.lines.onTrim(amount => {
	marker.line -= amount;
	// The marker should be disposed when the line is trimmed from the buffer
	if (marker.line < 0) {
	marker.dispose();
	}
	}));
	marker.register(this.lines.onInsert(event => {
	if (marker.line >= event.index) {
	marker.line += event.amount;
	}
	}));
	marker.register(this.lines.onDelete(event => {
	// Delete the marker if it's within the range
	if (marker.line >= event.index && marker.line < event.index + event.amount) {
	marker.dispose();
	}
	// Shift the marker if it's after the deleted range
	if (marker.line > event.index) {
	marker.line -= event.amount;
	}
	}));
	marker.register(marker.onDispose(() => this._removeMarker(marker)));
	return marker;
	}
	private _removeMarker(marker: Marker): void {
	this.markers.splice(this.markers.indexOf(marker), 1);
	}

@Tyriar Does this work as intended (bear with me, there is a high chance I simply dont get it 😸)

[Tyriar]

I don't think there's any reason why onDisposed isn't exposed. The continuing to decrement indeed looks like a leak, not sure why that is happening as register should ensure they're getting disposed? 🤔

You can see where I've used markers in vscode here: https://github.com/microsoft/vscode/blob/master/src/vs/workbench/contrib/terminal/browser/addons/commandTrackerAddon.ts#L25-L26, after a quick look this might be an issue that I'm not checking if the marker is disposed before using.

[jerch]

I don't think there's any reason why onDisposed isn't exposed.

Imho worth to be added to the API, as getting informed is much easier to deal with than to keep inspecting actively.

The continuing to decrement indeed looks like a leak, not sure why that is happening as register should ensure they're getting disposed?

Did not yet investigate further, but the line 608 looks somewhat fishy - though onDispose returns a Disposable itself, it looks like it will hold this forever (like a recursive descent)?

Thx for the pointer, will check that out.

[jerch]

@Tyriar Guess I found the bug:

xterm.js/src/common/buffer/Marker.ts

Lines 27 to 36 in a5e269a

	public dispose(): void {
	if (this.isDisposed) {
	return;
	}
	this.isDisposed = true;
	this.line = -1;
	// Emit before super.dispose such that dispose listeners get a change to react
	this._onDispose.fire();
	}
	}

This overloads Dispoable.dispose, but never calls the super method, thus the registered disposables on the marker never get freed. It works for me, if I add a super.dispose() after line 34.

Output is now:

Also note the _isDisposed property, which correctly is now set to true.

[Tyriar]

That would do it, thanks for checking it out!