Prevent overriding of Disposable.dispose

A common mistake made was overriding dispose and not calling super.dispose,
so a TS trick is now used to move it to a readonly property instead of a
member method which prevents it being overridden.

addons/xterm-addon-webgl/src/renderLayer/CursorRenderLayer.ts

@@ -12,6 +12,7 @@ import { IRenderDimensions, IRequestRedrawEvent } from 'browser/renderer/shared/
 import { IEventEmitter } from 'common/EventEmitter';
 import { ICoreBrowserService } from 'browser/services/Services';
 import { ICoreService } from 'common/services/Services';
+import { toDisposable } from 'common/Lifecycle';
 
 interface ICursorState {
   x: number;
@@ -55,12 +56,10 @@ export class CursorRenderLayer extends BaseRenderLayer {
       'underline': this._renderUnderlineCursor.bind(this)
     };
     this.onOptionsChanged(terminal);
-  }
-
-  public override dispose(): void {
-    this._cursorBlinkStateManager?.dispose();
-    this._cursorBlinkStateManager = undefined;
-    super.dispose();
+    this.register(toDisposable(() => {
+      this._cursorBlinkStateManager?.dispose();
+      this._cursorBlinkStateManager = undefined;
+    }));
   }
 
   public resize(terminal: Terminal, dim: IRenderDimensions): void {

src/browser/AccessibilityManager.ts

@@ -9,7 +9,7 @@ import { IBuffer } from 'common/buffer/Types';
 import { isMac } from 'common/Platform';
 import { TimeBasedDebouncer } from 'browser/TimeBasedDebouncer';
 import { addDisposableDomListener } from 'browser/Lifecycle';
-import { Disposable } from 'common/Lifecycle';
+import { Disposable, toDisposable } from 'common/Lifecycle';
 import { ScreenDprMonitor } from 'browser/ScreenDprMonitor';
 import { IRenderService } from 'browser/services/Services';
 import { removeElementFromParent } from 'browser/Dom';
@@ -104,12 +104,10 @@ export class AccessibilityManager extends Disposable {
     // This shouldn't be needed on modern browsers but is present in case the
     // media query that drives the ScreenDprMonitor isn't supported
     this.register(addDisposableDomListener(window, 'resize', () => this._refreshRowsDimensions()));
-  }
-
-  public dispose(): void {
-    super.dispose();
-    removeElementFromParent(this._accessibilityTreeRoot);
-    this._rowElements.length = 0;
+    this.register(toDisposable(() => {
+      removeElementFromParent(this._accessibilityTreeRoot);
+      this._rowElements.length = 0;
+    }));
   }
 
   private _onBoundaryFocus(e: FocusEvent, position: BoundaryPosition): void {

src/browser/Linkifier2.ts

@@ -8,7 +8,7 @@ import { IDisposable } from 'common/Types';
 import { IMouseService, IRenderService } from './services/Services';
 import { IBufferService } from 'common/services/Services';
 import { EventEmitter, IEvent } from 'common/EventEmitter';
-import { Disposable, getDisposeArrayDisposable, disposeArray } from 'common/Lifecycle';
+import { Disposable, getDisposeArrayDisposable, disposeArray, toDisposable } from 'common/Lifecycle';
 import { addDisposableDomListener } from 'browser/Lifecycle';
 
 export class Linkifier2 extends Disposable implements ILinkifier2 {
@@ -36,11 +36,9 @@ export class Linkifier2 extends Disposable implements ILinkifier2 {
   ) {
     super();
     this.register(getDisposeArrayDisposable(this._linkCacheDisposables));
-  }
-
-  public dispose(): void {
-    super.dispose();
-    this._lastMouseEvent = undefined;
+    this.register(toDisposable(() => {
+      this._lastMouseEvent = undefined;
+    }));
   }
 
   public registerLinkProvider(linkProvider: ILinkProvider): IDisposable {

src/browser/ScreenDprMonitor.ts

@@ -3,7 +3,7 @@
  * @license MIT
  */
 
-import { Disposable } from 'common/Lifecycle';
+import { Disposable, toDisposable } from 'common/Lifecycle';
 
 export type ScreenDprListener = (newDevicePixelRatio?: number, oldDevicePixelRatio?: number) => void;
 
@@ -26,6 +26,9 @@ export class ScreenDprMonitor extends Disposable {
   constructor(private _parentWindow: Window) {
     super();
     this._currentDevicePixelRatio = this._parentWindow.devicePixelRatio;
+    this.register(toDisposable(() => {
+      this.clearListener();
+    }));
   }
 
   public setListener(listener: ScreenDprListener): void {
@@ -43,11 +46,6 @@ export class ScreenDprMonitor extends Disposable {
     this._updateDpr();
   }
 
-  public dispose(): void {
-    super.dispose();
-    this.clearListener();
-  }
-
   private _updateDpr(): void {
     if (!this._outerListener) {
       return;

src/browser/Terminal.ts

@@ -56,6 +56,7 @@ import { OverviewRulerRenderer } from 'browser/decorations/OverviewRulerRenderer
 import { DecorationService } from 'common/services/DecorationService';
 import { IDecorationService } from 'common/services/Services';
 import { OscLinkProvider } from 'browser/OscLinkProvider';
+import { toDisposable } from 'common/Lifecycle';
 
 // Let it work inside Node.js for automated testing purposes.
 const document: Document = (typeof window !== 'undefined') ? window.document : null as any;
@@ -184,6 +185,11 @@ export class Terminal extends CoreTerminal implements ITerminal {
 
     // Setup listeners
     this.register(this._bufferService.onResize(e => this._afterResize(e.cols, e.rows)));
+
+    this.register(toDisposable(() => {
+      this._customKeyEventHandler = undefined;
+      this.element?.parentNode?.removeChild(this.element);
+    }));
   }
 
   /**
@@ -235,17 +241,6 @@ export class Terminal extends CoreTerminal implements ITerminal {
     this.viewport?.onThemeChange(this._colorManager.colors);
   }
 
-  public dispose(): void {
-    if (this._isDisposed) {
-      return;
-    }
-    super.dispose();
-    this._renderService?.dispose();
-    this._customKeyEventHandler = undefined;
-    this.write = () => { };
-    this.element?.parentNode?.removeChild(this.element);
-  }
-
   protected _setup(): void {
     super._setup();
 

src/browser/decorations/BufferDecorationRenderer.ts

@@ -5,7 +5,7 @@
 
 import { addDisposableDomListener } from 'browser/Lifecycle';
 import { IRenderService } from 'browser/services/Services';
-import { Disposable } from 'common/Lifecycle';
+import { Disposable, toDisposable } from 'common/Lifecycle';
 import { IBufferService, IDecorationService, IInternalDecoration } from 'common/services/Services';
 
 export class BufferDecorationRenderer extends Disposable {
@@ -39,12 +39,10 @@ export class BufferDecorationRenderer extends Disposable {
     }));
     this.register(this._decorationService.onDecorationRegistered(() => this._queueRefresh()));
     this.register(this._decorationService.onDecorationRemoved(decoration => this._removeDecoration(decoration)));
-  }
-
-  public override dispose(): void {
-    this._container.remove();
-    this._decorationElements.clear();
-    super.dispose();
+    this.register(toDisposable(() => {
+      this._container.remove();
+      this._decorationElements.clear();
+    }));
   }
 
   private _queueRefresh(): void {

src/browser/decorations/OverviewRulerRenderer.ts

@@ -6,7 +6,7 @@
 import { ColorZoneStore, IColorZone, IColorZoneStore } from 'browser/decorations/ColorZoneStore';
 import { addDisposableDomListener } from 'browser/Lifecycle';
 import { ICoreBrowserService, IRenderService } from 'browser/services/Services';
-import { Disposable } from 'common/Lifecycle';
+import { Disposable, toDisposable } from 'common/Lifecycle';
 import { IBufferService, IDecorationService, IOptionsService } from 'common/services/Services';
 
 // Helper objects to avoid excessive calculation and garbage collection during rendering. These are
@@ -68,6 +68,9 @@ export class OverviewRulerRenderer extends Disposable {
     this._registerDecorationListeners();
     this._registerBufferChangeListeners();
     this._registerDimensionChangeListeners();
+    this.register(toDisposable(() => {
+      this._canvas?.remove();
+    }));
   }
 
   /**
@@ -120,11 +123,6 @@ export class OverviewRulerRenderer extends Disposable {
     this._queueRefresh(true);
   }
 
-  public override dispose(): void {
-    this._canvas?.remove();
-    super.dispose();
-  }
-
   private _refreshDrawConstants(): void {
     // width
     const outerWidth = Math.floor(this._canvas.width / 3);

src/browser/renderer/dom/DomRenderer.ts

@@ -6,7 +6,7 @@
 import { IRenderer, IRenderDimensions, IRequestRedrawEvent } from 'browser/renderer/shared/Types';
 import { BOLD_CLASS, ITALIC_CLASS, CURSOR_CLASS, CURSOR_STYLE_BLOCK_CLASS, CURSOR_BLINK_CLASS, CURSOR_STYLE_BAR_CLASS, CURSOR_STYLE_UNDERLINE_CLASS, DomRendererRowFactory } from 'browser/renderer/dom/DomRendererRowFactory';
 import { INVERTED_DEFAULT_COLOR } from 'browser/renderer/shared/Constants';
-import { Disposable } from 'common/Lifecycle';
+import { Disposable, toDisposable } from 'common/Lifecycle';
 import { IColorSet, ILinkifierEvent, ILinkifier2 } from 'browser/Types';
 import { ICharSizeService, ICoreBrowserService } from 'browser/services/Services';
 import { IOptionsService, IBufferService, IInstantiationService } from 'common/services/Services';
@@ -89,16 +89,14 @@ export class DomRenderer extends Disposable implements IRenderer {
 
     this.register(this._linkifier2.onShowLinkUnderline(e => this._onLinkHover(e)));
     this.register(this._linkifier2.onHideLinkUnderline(e => this._onLinkLeave(e)));
-  }
 
-  public dispose(): void {
-    this._element.classList.remove(TERMINAL_CLASS_PREFIX + this._terminalClass);
+    this.register(toDisposable(() => {
+      this._element.classList.remove(TERMINAL_CLASS_PREFIX + this._terminalClass);
 
-    // Outside influences such as React unmounts may manipulate the DOM before our disposal.
-    // https://github.com/xtermjs/xterm.js/issues/2960
-    removeElementFromParent(this._rowContainer, this._selectionContainer, this._themeStyleElement, this._dimensionsStyleElement);
-
-    super.dispose();
+      // Outside influences such as React unmounts may manipulate the DOM before our disposal.
+      // https://github.com/xtermjs/xterm.js/issues/2960
+      removeElementFromParent(this._rowContainer, this._selectionContainer, this._themeStyleElement, this._dimensionsStyleElement);
+    }));
   }
 
   private _updateDimensions(): void {

src/browser/services/RenderService.ts

@@ -169,10 +169,6 @@ export class RenderService extends Disposable implements IRenderService {
     this._onDimensionsChange.fire(this._renderer.dimensions);
   }
 
-  public dispose(): void {
-    super.dispose();
-  }
-
   public hasRenderer(): boolean {
     return !!this._renderer;
   }

src/browser/services/SelectionService.ts

@@ -15,7 +15,7 @@ import { IBufferRange, ILinkifier2 } from 'browser/Types';
 import { IBufferService, IOptionsService, ICoreService } from 'common/services/Services';
 import { getCoordsRelativeToElement } from 'browser/input/Mouse';
 import { moveToCellSequence } from 'browser/input/MoveToCell';
-import { Disposable } from 'common/Lifecycle';
+import { Disposable, toDisposable } from 'common/Lifecycle';
 import { getRangeLength } from 'common/buffer/BufferRange';
 
 /**
@@ -148,10 +148,10 @@ export class SelectionService extends Disposable implements ISelectionService {
 
     this._model = new SelectionModel(this._bufferService);
     this._activeSelectionMode = SelectionMode.NORMAL;
-  }
 
-  public dispose(): void {
-    this._removeMouseDownListeners();
+    this.register(toDisposable(() => {
+      this._removeMouseDownListeners();
+    }));
   }
 
   public reset(): void {

src/common/CoreTerminal.ts

@@ -21,7 +21,7 @@
  *   http://linux.die.net/man/7/urxvt
  */
 
-import { Disposable } from 'common/Lifecycle';
+import { Disposable, toDisposable } from 'common/Lifecycle';
 import { IInstantiationService, IOptionsService, IBufferService, ILogService, ICharsetService, ICoreService, ICoreMouseService, IUnicodeService, LogLevelEnum, ITerminalOptions, IOscLinkService } from 'common/services/Services';
 import { InstantiationService } from 'common/services/InstantiationService';
 import { LogService } from 'common/services/LogService';
@@ -143,15 +143,11 @@ export abstract class CoreTerminal extends Disposable implements ICoreTerminal {
     // Setup WriteBuffer
     this._writeBuffer = new WriteBuffer((data, promiseResult) => this._inputHandler.parse(data, promiseResult));
     this.register(forwardEvent(this._writeBuffer.onWriteParsed, this._onWriteParsed));
-  }
 
-  public dispose(): void {
-    if (this._isDisposed) {
-      return;
-    }
-    super.dispose();
-    this._windowsMode?.dispose();
-    this._windowsMode = undefined;
+    this.register(toDisposable(() => {
+      this._windowsMode?.dispose();
+      this._windowsMode = undefined;
+    }));
   }
 
   public write(data: string | Uint8Array, callback?: () => void): void {

src/common/InputHandler.ts

@@ -382,10 +382,6 @@ export class InputHandler extends Disposable implements IInputHandler {
     this._parser.registerDcsHandler({ intermediates: '$', final: 'q' }, new DcsHandler((data, params) => this.requestStatusString(data, params)));
   }
 
-  public dispose(): void {
-    super.dispose();
-  }
-
   /**
    * Async parse support.
    */

src/common/Lifecycle.ts

@@ -17,15 +17,18 @@ export abstract class Disposable implements IDisposable {
   }
 
   /**
-   * Disposes the object, triggering the `dispose` method on all registered IDisposables.
+   * Disposes the object, triggering the `dispose` method on all registered IDisposables. This is a
+   * readonly property instead of a method to prevent subclasses overriding it which is an easy
+   * mistake that can introduce memory leaks. If a class extends Disposable, all dispose calls
+   * should be done via {@link register}.
    */
-  public dispose(): void {
+  public readonly dispose = (): void => {
     this._isDisposed = true;
     for (const d of this._disposables) {
       d.dispose();
     }
     this._disposables.length = 0;
-  }
+  };
 
   /**
    * Registers a disposable object.

src/common/buffer/Marker.ts

@@ -3,35 +3,28 @@
  * @license MIT
  */
 
-import { EventEmitter, IEvent } from 'common/EventEmitter';
-import { Disposable } from 'common/Lifecycle';
+import { EventEmitter } from 'common/EventEmitter';
+import { Disposable, toDisposable } from 'common/Lifecycle';
 import { IMarker } from 'common/Types';
 
 export class Marker extends Disposable implements IMarker {
   private static _nextId = 1;
 
   private _id: number = Marker._nextId++;
-  public isDisposed: boolean = false;
-
   public get id(): number { return this._id; }
 
   private readonly _onDispose = new EventEmitter<void>();
   public readonly onDispose = this._onDispose.event;
 
+  public get isDisposed(): boolean { return this._isDisposed; };
+
   constructor(
     public line: number
   ) {
     super();
-  }
-
-  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();
-    super.dispose();
+    this.register(toDisposable(() => {
+      this.line = -1;
+      this._onDispose.fire();
+    }));
   }
 }

src/common/parser/EscapeSequenceParser.test.ts

@@ -87,7 +87,7 @@ class TestEscapeSequenceParser extends EscapeSequenceParser {
     }
   }
   public mockOscParser(): void {
-    this._oscParser = oscPutParser;
+    (this as any)._oscParser = oscPutParser;
   }
   public identifier(id: IFunctionIdentifier): number {
     return this._identifier(id);

src/common/parser/EscapeSequenceParser.ts

@@ -5,7 +5,7 @@
 
 import { IParsingState, IDcsHandler, IEscapeSequenceParser, IParams, IOscHandler, IHandlerCollection, CsiHandlerType, OscFallbackHandlerType, IOscParser, EscHandlerType, IDcsParser, DcsFallbackHandlerType, IFunctionIdentifier, ExecuteFallbackHandlerType, CsiFallbackHandlerType, EscFallbackHandlerType, PrintHandlerType, PrintFallbackHandlerType, ExecuteHandlerType, IParserStackState, ParserStackType, ResumableHandlersType } from 'common/parser/Types';
 import { ParserState, ParserAction } from 'common/parser/Constants';
-import { Disposable } from 'common/Lifecycle';
+import { Disposable, toDisposable } from 'common/Lifecycle';
 import { IDisposable } from 'common/Types';
 import { fill } from 'common/TypedArrayUtils';
 import { Params } from 'common/parser/Params';
@@ -242,8 +242,8 @@ export class EscapeSequenceParser extends Disposable implements IEscapeSequenceP
   protected _executeHandlers: { [flag: number]: ExecuteHandlerType };
   protected _csiHandlers: IHandlerCollection<CsiHandlerType>;
   protected _escHandlers: IHandlerCollection<EscHandlerType>;
-  protected _oscParser: IOscParser;
-  protected _dcsParser: IDcsParser;
+  protected readonly _oscParser: IOscParser;
+  protected readonly _dcsParser: IDcsParser;
   protected _errorHandler: (state: IParsingState) => IParsingState;
 
   // fallback handlers
@@ -284,8 +284,13 @@ export class EscapeSequenceParser extends Disposable implements IEscapeSequenceP
     this._executeHandlers = Object.create(null);
     this._csiHandlers = Object.create(null);
     this._escHandlers = Object.create(null);
-    this._oscParser = new OscParser();
-    this._dcsParser = new DcsParser();
+    this.register(toDisposable(() => {
+      this._csiHandlers = Object.create(null);
+      this._executeHandlers = Object.create(null);
+      this._escHandlers = Object.create(null);
+    }));
+    this._oscParser = this.register(new OscParser());
+    this._dcsParser = this.register(new DcsParser());
     this._errorHandler = this._errorHandlerFb;
 
     // swallow 7bit ST (ESC+\)
@@ -338,14 +343,6 @@ export class EscapeSequenceParser extends Disposable implements IEscapeSequenceP
     return res.reverse().join('');
   }
 
-  public dispose(): void {
-    this._csiHandlers = Object.create(null);
-    this._executeHandlers = Object.create(null);
-    this._escHandlers = Object.create(null);
-    this._oscParser.dispose();
-    this._dcsParser.dispose();
-  }
-
   public setPrintHandler(handler: PrintHandlerType): void {
     this._printHandler = handler;
   }

src/common/services/BufferService.ts

@@ -36,12 +36,7 @@ export class BufferService extends Disposable implements IBufferService {
     super();
     this.cols = Math.max(optionsService.rawOptions.cols || 0, MINIMUM_COLS);
     this.rows = Math.max(optionsService.rawOptions.rows || 0, MINIMUM_ROWS);
-    this.buffers = new BufferSet(optionsService, this);
-  }
-
-  public dispose(): void {
-    super.dispose();
-    this.buffers.dispose();
+    this.buffers = this.register(new BufferSet(optionsService, this));
   }
 
   public resize(cols: number, rows: number): void {

src/common/services/DecorationService.ts

@@ -5,7 +5,7 @@
 
 import { css } from 'common/Color';
 import { EventEmitter } from 'common/EventEmitter';
-import { Disposable } from 'common/Lifecycle';
+import { Disposable, toDisposable } from 'common/Lifecycle';
 import { IDecorationService, IInternalDecoration } from 'common/services/Services';
 import { SortedList } from 'common/SortedList';
 import { IColor } from 'common/Types';
@@ -32,6 +32,16 @@ export class DecorationService extends Disposable implements IDecorationService
 
   public get decorations(): IterableIterator<IInternalDecoration> { return this._decorations.values(); }
 
+  constructor() {
+    super();
+
+    this.register(toDisposable(() => {
+      for (const d of this._decorations.values()) {
+        this._onDecorationRemoved.fire(d);
+      }
+      this.reset();
+    }));
+  }
   public registerDecoration(options: IDecorationOptions): IDecoration | undefined {
     if (options.marker.isDisposed) {
       return undefined;
@@ -81,25 +91,19 @@ export class DecorationService extends Disposable implements IDecorationService
       }
     });
   }
-
-  public dispose(): void {
-    for (const d of this._decorations.values()) {
-      this._onDecorationRemoved.fire(d);
-    }
-    this.reset();
-  }
 }
 
 class Decoration extends Disposable implements IInternalDecoration {
   public readonly marker: IMarker;
   public element: HTMLElement | undefined;
-  public isDisposed: boolean = false;
 
   public readonly onRenderEmitter = this.register(new EventEmitter<HTMLElement>());
   public readonly onRender = this.onRenderEmitter.event;
   private readonly _onDispose = this.register(new EventEmitter<void>());
   public readonly onDispose = this._onDispose.event;
 
+  public get isDisposed(): boolean { return this._isDisposed; }
+
   private _cachedBg: IColor | undefined | null = null;
   public get backgroundColorRGB(): IColor | undefined {
     if (this._cachedBg === null) {
@@ -132,14 +136,12 @@ class Decoration extends Disposable implements IInternalDecoration {
     if (this.options.overviewRulerOptions && !this.options.overviewRulerOptions.position) {
       this.options.overviewRulerOptions.position = 'full';
     }
-  }
 
-  public override dispose(): void {
-    if (this._isDisposed) {
-      return;
-    }
-    this._isDisposed = true;
-    this._onDispose.fire();
-    super.dispose();
+    this.register(toDisposable(() => {
+      if (this._isDisposed) {
+        return;
+      }
+      this._onDispose.fire();
+    }));
   }
 }

src/headless/Terminal.ts

@@ -71,14 +71,6 @@ export class Terminal extends CoreTerminal {
     this.register(forwardEvent(this._inputHandler.onA11yTab, this._onA11yTabEmitter));
   }
 
-  public dispose(): void {
-    if (this._isDisposed) {
-      return;
-    }
-    super.dispose();
-    this.write = () => { };
-  }
-
   /**
    * Convenience property to active buffer.
    */