Ensure all references are released when Terminal.dispose is called

[Tyriar]

This PR adds the following:

• A new abstract class Disposable that is extended by a number of classes and provides a register function to register disposables and a dispose function which disposes them
• Moves util/Dom.ts to ui/Lifecycle.ts
• Registered disposables for all DOM event and EventEmitter listeners (excluding some mouse ones which need some more work)
• Added a dispose terminal button to the bottom of the demo for easier testing of leaked references

After these changes, when a heap snapshot is taken it will no longer show Terminal in the heap. The following was taken after filling the buffer and disposing the terminal:

This is what it looked like before (with demo and internal Terminal references holding onto the Terminal):

Fixes #1518

[parisk]

LGTM in general, but I would suggest someone else reviewing it as well, since it deals with a crucial part of the project.

[parisk]

Just an idea; would it make sense to replace this.register(... with TypeScript decorators? This way we could keep disposable properties declared outside the constructor.

[Tyriar]

You mean something like this? (if it works)

function register(target: any, key: string) {
    let _val = this[key];
	
    const getter = () => _val;
    const setter = (newVal) => {
        if (_val) {
            _val.dispose();
        }
        target.register(newVal);
        _val = newVal;
    };
    Object.defineProperty(target, key, {
        get: getter,
        set: setter,
        enumerable: true,
        configurable: true 
    });
}

[Tyriar]

I looked into this and I don't think we can do it. For starters decorators are experimental, I switched it on and tried to get it working but it seemed to be acting not as I expected. Maybe we can do this in the future?

[mofux]

term._core,register looks like a typo to me

[Tyriar]

Fixed

[mofux]

I think registerDisposable or addDisposable would be better, especially because classes that inherit from Disposable might want to use register with a totally different meaning. Say we have an AddonRegistry class that extends Disposable, it would certainly get very confusing what AddonRegistry.register would do 😅

[mofux]

What do you think about register taking a list of arguments ...disposables, so we could simply register multiple disposables at once, e.g.:

this._terminal.core.register(
  this._terminal.addDisposableListener('blur', () => this._clearLiveRegion()),
  this._terminal.addDisposableListener('linefeed', () => this._lineFeed()),
  this._terminal.addDisposableListener('focus', () => this._focus())
)

[Tyriar]

I think registerDisposable or addDisposable would be better, especially because classes that inherit from Disposable might want to use register with a totally different meaning.

I was trying to make it not so verbose as the lines can already get quite long. This also matches the naming scheme we use in vscode https://github.com/Microsoft/vscode/blob/26af5f5392012067294755ef91cf66fc782b4d48/src/vs/base/common/lifecycle.ts#L70

What do you think about register taking a list of arguments ...disposables, so we could simply register multiple disposables at once, e.g.:

You can already do this with:

this._terminal.core._disposables.push(a, b, c);

I prefer the look of the other way personally

[mofux]

Fine with me, your call 👍

[mofux]

@Tyriar Should I give this a final test / review?

[Tyriar]

@mofux that would be good, also I think tests are passing it's just that #1531 broke everything

[Tyriar]

@mofux I think this is ready for final review, CI failure was unrelated.

[mofux]

Works like a charm, ready to merge from my side 👍

[Tyriar]

🎉