diff --git a/app/lib/pty.ts b/app/lib/pty.ts index 2ced71c33..cc002cf0a 100644 --- a/app/lib/pty.ts +++ b/app/lib/pty.ts @@ -3,7 +3,7 @@ import { v4 as uuidv4 } from 'uuid' import { ipcMain } from 'electron' import { Application } from './app' import { UTF8Splitter } from './utfSplitter' -import { Subject, debounceTime } from 'rxjs' +import { Observable, Subject, debounceTime } from 'rxjs' class PTYDataQueue { private buffers: Buffer[] = [] @@ -90,6 +90,7 @@ class PTYDataQueue { export class PTY { private pty: nodePTY.IPty private outputQueue: PTYDataQueue + private closedSubject = new Subject() exited = false constructor (private id: string, private app: Application, ...args: any[]) { @@ -112,6 +113,10 @@ export class PTY { return this.pty.pid } + get closed$ (): Observable { + return this.closedSubject.asObservable() + } + resize (columns: number, rows: number): void { if ((this.pty as any)._writable) { this.pty.resize(columns, rows) @@ -134,17 +139,32 @@ export class PTY { private emit (event: string, ...args: any[]) { this.app.broadcast(`pty:${this.id}:${event}`, ...args) + if (event === 'close') { + this.closedSubject.next() + this.closedSubject.complete() + } } } export class PTYManager { - private ptys: Record = {} + private ptys = new Map() init (app: Application): void { ipcMain.on('pty:spawn', (event, ...options) => { const id = uuidv4().toString() + try { - this.ptys[id] = new PTY(id, app, ...options) + const pty = new PTY(id, app, ...options) + this.ptys.set(id, pty) + + // A PTY owns its output queue and native event handlers. Release + // the manager's reference as soon as the child process exits so + // repeatedly opened terminals cannot accumulate in this table. + pty.closed$.subscribe(() => { + if (this.ptys.get(id) === pty) { + this.ptys.delete(id) + } + }) } catch (error) { // Spawning fails for reasons the user can act on - an invalid // working directory being by far the most common one. Reporting @@ -160,27 +180,28 @@ export class PTYManager { }) ipcMain.on('pty:exists', (event, id) => { - event.returnValue = this.ptys[id] && !this.ptys[id].exited + const pty = this.ptys.get(id) + event.returnValue = Boolean(pty && !pty.exited) }) ipcMain.on('pty:get-pid', (event, id) => { - event.returnValue = this.ptys[id]?.getPID() + event.returnValue = this.ptys.get(id)?.getPID() }) ipcMain.on('pty:resize', (_event, id, columns, rows) => { - this.ptys[id]?.resize(columns, rows) + this.ptys.get(id)?.resize(columns, rows) }) ipcMain.on('pty:write', (_event, id, data) => { - this.ptys[id]?.write(Buffer.from(data)) + this.ptys.get(id)?.write(Buffer.from(data)) }) ipcMain.on('pty:kill', (_event, id, signal) => { - this.ptys[id]?.kill(signal) + this.ptys.get(id)?.kill(signal) }) ipcMain.on('pty:ack-data', (_event, id, length) => { - this.ptys[id]?.ackData(length) + this.ptys.get(id)?.ackData(length) }) } } diff --git a/tabby-electron/src/pty.ts b/tabby-electron/src/pty.ts index 390da13e0..41d886e4c 100644 --- a/tabby-electron/src/pty.ts +++ b/tabby-electron/src/pty.ts @@ -87,6 +87,7 @@ export class ElectronPTYProxy extends PTYProxy { for (const k of this.subscriptions.keys()) { ipcRenderer.off(k, this.subscriptions.get(k)) } + this.subscriptions.clear() } async resize (columns: number, rows: number): Promise { diff --git a/tabby-terminal/src/frontends/xtermFrontend.ts b/tabby-terminal/src/frontends/xtermFrontend.ts index 9d2c990e1..1f93c3ea5 100644 --- a/tabby-terminal/src/frontends/xtermFrontend.ts +++ b/tabby-terminal/src/frontends/xtermFrontend.ts @@ -85,6 +85,20 @@ export class XTermFrontend extends Frontend { private canvasAddon?: CanvasAddon private opened = false private resizeObserver?: any + private hostEventHandlers?: { + wheel: (event: WheelEvent) => void + dragOver: (event: Event) => void + drop: (event: Event) => void + mousedown: (event: Event) => void + mouseup: (event: Event) => void + mousewheel: (event: Event) => void + contextmenu: (event: Event) => void + } + + private resizeTimeout?: ReturnType + private resizeAnimationFrame?: number + private resizePending = false + private disposed = false private flowControl: FlowControl private pinnedToBottom = true private pendingRendererRecovery = false @@ -96,6 +110,10 @@ export class XTermFrontend extends Frontend { private hostApp: HostAppService private themes: ThemesService + private isAttachActive (): boolean { + return !this.disposed && this.opened + } + constructor (injector: Injector) { super(injector) this.configService = injector.get(ConfigService) @@ -270,23 +288,29 @@ export class XTermFrontend extends Frontend { // always running a trailing fit keeps the final size correct without // outrunning the renderer. Tune RESIZE_MIN_INTERVAL if needed. const RESIZE_MIN_INTERVAL = 32 - let resizePending = false let lastResize = 0 const runResize = () => { - resizePending = false + this.resizeAnimationFrame = undefined + this.resizePending = false + if (!this.isAttachActive()) { + return + } lastResize = Date.now() doResize() } this.resizeHandler = () => { - if (resizePending) { + if (this.resizePending) { return } - resizePending = true + this.resizePending = true const wait = Math.max(0, RESIZE_MIN_INTERVAL - (Date.now() - lastResize)) if (wait > 0) { - setTimeout(() => requestAnimationFrame(runResize), wait) + this.resizeTimeout = setTimeout(() => { + this.resizeTimeout = undefined + this.resizeAnimationFrame = requestAnimationFrame(runResize) + }, wait) } else { - requestAnimationFrame(runResize) + this.resizeAnimationFrame = requestAnimationFrame(runResize) } } @@ -314,6 +338,9 @@ export class XTermFrontend extends Frontend { } async attach (host: HTMLElement, profile: BaseTerminalProfile): Promise { + if (this.disposed) { + return + } this.element = host this.xterm.open(host) @@ -321,6 +348,9 @@ export class XTermFrontend extends Frontend { // Work around font loading bugs await new Promise(resolve => setTimeout(resolve, this.hostApp.platform === Platform.Web ? 1000 : 0)) + if (!this.isAttachActive()) { + return + } // Just configure the colors to avoid a flash this.configureColors(profile.terminalColorScheme) @@ -344,6 +374,9 @@ export class XTermFrontend extends Frontend { // Allow an animation frame await new Promise(r => setTimeout(r, 100)) + if (!this.isAttachActive()) { + return + } this.ready.next() this.ready.complete() @@ -366,20 +399,23 @@ export class XTermFrontend extends Frontend { // Allow an animation frame await new Promise(r => setTimeout(r, 0)) + if (!this.isAttachActive()) { + return + } // User-initiated scroll detection: only wheel and keyboard events // should unpin. xterm.onScroll is content-driven only and must never // unpin (see constructor comment). Use capture phase — xterm.js // handles wheel/key events on its internal viewport element and may // stop propagation, so bubbling listeners on host would never fire. - host.addEventListener('wheel', (event: WheelEvent) => { + const wheelHandler = (event: WheelEvent) => { // Immediately unpin on scroll-up so that writes arriving before // the next animation frame don't yank the viewport back down. if (event.deltaY < 0) { this.pinnedToBottom = false } requestAnimationFrame(() => this.updatePinnedState()) - }, { capture: true, passive: true }) + } this.hotkeysService.hotkey$ @@ -404,28 +440,67 @@ export class XTermFrontend extends Frontend { requestAnimationFrame(() => this.updatePinnedState()) }) - host.addEventListener('dragOver', (event: any) => this.dragOver.next(event)) - host.addEventListener('drop', event => this.drop.next(event)) + this.hostEventHandlers = { + wheel: wheelHandler, + dragOver: event => this.dragOver.next(event as DragEvent), + drop: event => this.drop.next(event as DragEvent), + mousedown: event => this.mouseEvent.next(event as MouseEvent), + mouseup: event => this.mouseEvent.next(event as MouseEvent), + mousewheel: event => this.mouseEvent.next(event as MouseEvent), + contextmenu: event => { + event.preventDefault() + event.stopPropagation() + }, + } - host.addEventListener('mousedown', event => this.mouseEvent.next(event)) - host.addEventListener('mouseup', event => this.mouseEvent.next(event)) - host.addEventListener('mousewheel', event => this.mouseEvent.next(event as MouseEvent)) - host.addEventListener('contextmenu', event => { - event.preventDefault() - event.stopPropagation() - }) + host.addEventListener('wheel', this.hostEventHandlers.wheel, { capture: true, passive: true }) + host.addEventListener('dragOver', this.hostEventHandlers.dragOver) + host.addEventListener('drop', this.hostEventHandlers.drop) + host.addEventListener('mousedown', this.hostEventHandlers.mousedown) + host.addEventListener('mouseup', this.hostEventHandlers.mouseup) + host.addEventListener('mousewheel', this.hostEventHandlers.mousewheel) + host.addEventListener('contextmenu', this.hostEventHandlers.contextmenu) this.resizeObserver = new window['ResizeObserver'](() => this.resizeHandler()) this.resizeObserver.observe(host) } detach (_host: HTMLElement): void { + const host = this.element window.removeEventListener('resize', this.resizeHandler) + if (this.resizeTimeout !== undefined) { + clearTimeout(this.resizeTimeout) + this.resizeTimeout = undefined + } + if (this.resizeAnimationFrame !== undefined) { + cancelAnimationFrame(this.resizeAnimationFrame) + this.resizeAnimationFrame = undefined + } + this.resizePending = false + if (host && this.hostEventHandlers) { + host.removeEventListener('wheel', this.hostEventHandlers.wheel, true) + host.removeEventListener('dragOver', this.hostEventHandlers.dragOver) + host.removeEventListener('drop', this.hostEventHandlers.drop) + host.removeEventListener('mousedown', this.hostEventHandlers.mousedown) + host.removeEventListener('mouseup', this.hostEventHandlers.mouseup) + host.removeEventListener('mousewheel', this.hostEventHandlers.mousewheel) + host.removeEventListener('contextmenu', this.hostEventHandlers.contextmenu) + this.hostEventHandlers = undefined + } this.resizeObserver?.disconnect() - delete this.resizeObserver + this.resizeObserver = undefined + this.opened = false + this.element = undefined } destroy (): void { + if (this.disposed) { + return + } + this.disposed = true + if (this.element) { + this.detach(this.element) + } super.destroy() this.webGLAddon?.dispose() this.canvasAddon?.dispose()