From d01efdda270f0d9c2a0c55ec78bc40e7213e1d11 Mon Sep 17 00:00:00 2001 From: Daniel Imms Date: Wed, 26 Dec 2018 14:08:53 -0800 Subject: [PATCH 1/5] Use array instead of linkedlist, add typings --- src/EscapeSequenceParser.ts | 110 ++++++++++++++++-------------------- 1 file changed, 49 insertions(+), 61 deletions(-) diff --git a/src/EscapeSequenceParser.ts b/src/EscapeSequenceParser.ts index 3215513065..52007244d6 100644 --- a/src/EscapeSequenceParser.ts +++ b/src/EscapeSequenceParser.ts @@ -11,6 +11,13 @@ interface IHandlerLink extends IDisposable { nextHandler: IHandlerLink | null; } +interface IHandlerCollection { + [key: string]: T[]; +} + +type CsiHandler = (params: number[], collect: string) => boolean | void; +type OscHandler = (data: string) => boolean | void; + /** * Returns an array filled with numbers between the low and high parameters (right exclusive). * @param low The low number. @@ -46,7 +53,7 @@ export class TransitionTable { * @param action parser action to be done * @param next next parser state */ - add(code: number, state: number, action: number | null, next: number | null): void { + add(code: number, state: number, action: number | null, next: number | null): void { this.table[state << 8 | code] = ((action | 0) << 4) | ((next === undefined) ? state : next); } @@ -227,9 +234,9 @@ export class EscapeSequenceParser extends Disposable implements IEscapeSequenceP // handler lookup containers protected _printHandler: (data: string, start: number, end: number) => void; protected _executeHandlers: any; - protected _csiHandlers: any; + protected _csiHandlers: IHandlerCollection; protected _escHandlers: any; - protected _oscHandlers: any; + protected _oscHandlers: IHandlerCollection; protected _dcsHandlers: any; protected _activeDcsHandler: IDcsHandler | null; protected _errorHandler: (state: IParsingState) => IParsingState; @@ -284,16 +291,7 @@ export class EscapeSequenceParser extends Disposable implements IEscapeSequenceP this._printHandler = null; this._executeHandlers = null; this._escHandlers = null; - let handlers; - while ((handlers = this._csiHandlers) && handlers.dispose !== undefined) { - handlers.dispose(); - if (handlers === this._csiHandlers) { break; } // sanity check - } this._csiHandlers = null; - while ((handlers = this._oscHandlers) && handlers.dispose !== undefined) { - handlers.dispose(); - if (handlers === this._oscHandlers) { break; } // sanity check - } this._oscHandlers = null; this._dcsHandlers = null; this._activeDcsHandler = null; @@ -317,42 +315,18 @@ export class EscapeSequenceParser extends Disposable implements IEscapeSequenceP this._executeHandlerFb = callback; } - private _linkHandler(handlers: object[], index: number, newCallback: object): IDisposable { - const newHead: any = newCallback; - newHead.nextHandler = handlers[index] as IHandlerLink; - newHead.dispose = function (): void { - let previous = null; - let cur = handlers[index] as IHandlerLink; - for (; cur && cur.nextHandler; - previous = cur, cur = cur.nextHandler) { - if (cur === newHead) { - if (previous) { previous.nextHandler = cur.nextHandler; } - else { handlers[index] = cur.nextHandler; } - cur.nextHandler = null; - handlers = null; newCallback = null; // just in case - break; - } - } - }; - handlers[index] = newHead; - return newHead; - } - - addCsiHandler(flag: string, callback: (params: number[], collect: string) => boolean): IDisposable { + addCsiHandler(flag: string, callback: CsiHandler): IDisposable { const index = flag.charCodeAt(0); - const newHead = - (params: number[], collect: string): void => { - if (! callback(params, collect)) { - const next = (newHead as unknown as IHandlerLink).nextHandler; - if (next) { (next as any)(params, collect); } - else { this._csiHandlerFb(collect, params, index); } - } - }; - return this._linkHandler(this._csiHandlers, index, newHead); + if (this._csiHandlers[index] === undefined) { + this._csiHandlers[index] = []; + } + this._csiHandlers[index].push(callback); + return { + dispose: () => this._csiHandlers[index].splice(this._csiHandlers[index].indexOf(callback)) + }; } - setCsiHandler(flag: string, callback: (params: number[], collect: string) => void): void { - this._csiHandlers[flag.charCodeAt(0)] = callback; + this._csiHandlers[flag.charCodeAt(0)] = [callback]; } clearCsiHandler(flag: string): void { if (this._csiHandlers[flag.charCodeAt(0)]) delete this._csiHandlers[flag.charCodeAt(0)]; @@ -372,18 +346,16 @@ export class EscapeSequenceParser extends Disposable implements IEscapeSequenceP } addOscHandler(ident: number, callback: (data: string) => boolean): IDisposable { - const newHead = - (data: string): void => { - if (! callback(data)) { - const next = (newHead as unknown as IHandlerLink).nextHandler; - if (next) { (next as any)(data); } - else { this._oscHandlerFb(ident, data); } - } - }; - return this._linkHandler(this._oscHandlers, ident, newHead); + if (this._oscHandlers[ident] === undefined) { + this._oscHandlers[ident] = []; + } + this._oscHandlers[ident].push(callback); + return { + dispose: () => this._oscHandlers[ident].splice(this._oscHandlers[ident].indexOf(callback)) + }; } setOscHandler(ident: number, callback: (data: string) => void): void { - this._oscHandlers[ident] = callback; + this._oscHandlers[ident] = [callback]; } clearOscHandler(ident: number): void { if (this._oscHandlers[ident]) delete this._oscHandlers[ident]; @@ -520,9 +492,17 @@ export class EscapeSequenceParser extends Disposable implements IEscapeSequenceP } break; case ParserAction.CSI_DISPATCH: - callback = this._csiHandlers[code]; - if (callback) callback(params, collect); - else this._csiHandlerFb(collect, params, code); + // Trigger CSI Handler + const handlers = this._csiHandlers[code]; + let j: number; + for (j = handlers.length - 1; j >= 0; j--) { + if (handlers[j](params, collect)) { + break; + } + } + if (j < 0) { + this._csiHandlerFb(collect, params, code); + } break; case ParserAction.PARAM: if (code === 0x3b) params.push(0); @@ -597,9 +577,17 @@ export class EscapeSequenceParser extends Disposable implements IEscapeSequenceP // or with an explicit NaN OSC handler const identifier = parseInt(osc.substring(0, idx)); const content = osc.substring(idx + 1); - callback = this._oscHandlers[identifier]; - if (callback) callback(content); - else this._oscHandlerFb(identifier, content); + // Trigger OSC Handler + const handlers = this._oscHandlers[identifier]; + let j: number; + for (j = handlers.length - 1; j >= 0; j--) { + if (handlers[j](content)) { + break; + } + } + if (j < 0) { + this._oscHandlerFb(identifier, content); + } } } if (code === 0x1b) transition |= ParserState.ESCAPE; From 38796a0f748340044f875580870764f80b5535f6 Mon Sep 17 00:00:00 2001 From: Daniel Imms Date: Wed, 26 Dec 2018 14:40:23 -0800 Subject: [PATCH 2/5] Add tests, fix NPE --- src/EscapeSequenceParser.test.ts | 96 ++++++++++++++++++++++++++++++++ src/EscapeSequenceParser.ts | 12 ++-- 2 files changed, 100 insertions(+), 8 deletions(-) diff --git a/src/EscapeSequenceParser.test.ts b/src/EscapeSequenceParser.test.ts index e92f412574..3b01cb8a17 100644 --- a/src/EscapeSequenceParser.test.ts +++ b/src/EscapeSequenceParser.test.ts @@ -1169,6 +1169,54 @@ describe('EscapeSequenceParser', function (): void { parser2.parse(INPUT); chai.expect(csi).eql([]); }); + describe('CSI custom handlers', () => { + it('Prevent fallback', () => { + const csiCustom: [string, number[], string][] = []; + parser2.setCsiHandler('m', (params, collect) => csi.push(['m', params, collect])); + parser2.addCsiHandler('m', (params, collect) => { csiCustom.push(['m', params, collect]); return true; }); + parser2.parse(INPUT); + chai.expect(csi).eql([], 'Should not fallback to original handler'); + chai.expect(csiCustom).eql([['m', [1, 31], ''], ['m', [0], '']]); + }); + it('Allow fallback', () => { + const csiCustom: [string, number[], string][] = []; + parser2.setCsiHandler('m', (params, collect) => csi.push(['m', params, collect])); + parser2.addCsiHandler('m', (params, collect) => { csiCustom.push(['m', params, collect]); return false; }); + parser2.parse(INPUT); + chai.expect(csi).eql([['m', [1, 31], ''], ['m', [0], '']], 'Should fallback to original handler'); + chai.expect(csiCustom).eql([['m', [1, 31], ''], ['m', [0], '']]); + }); + it('Multiple custom handlers fallback once', () => { + const csiCustom: [string, number[], string][] = []; + const csiCustom2: [string, number[], string][] = []; + parser2.setCsiHandler('m', (params, collect) => csi.push(['m', params, collect])); + parser2.addCsiHandler('m', (params, collect) => { csiCustom.push(['m', params, collect]); return true; }); + parser2.addCsiHandler('m', (params, collect) => { csiCustom2.push(['m', params, collect]); return false; }); + parser2.parse(INPUT); + chai.expect(csi).eql([], 'Should not fallback to original handler'); + chai.expect(csiCustom).eql([['m', [1, 31], ''], ['m', [0], '']]); + chai.expect(csiCustom2).eql([['m', [1, 31], ''], ['m', [0], '']]); + }); + it('Multiple custom handlers no fallback', () => { + const csiCustom: [string, number[], string][] = []; + const csiCustom2: [string, number[], string][] = []; + parser2.setCsiHandler('m', (params, collect) => csi.push(['m', params, collect])); + parser2.addCsiHandler('m', (params, collect) => { csiCustom.push(['m', params, collect]); return true; }); + parser2.addCsiHandler('m', (params, collect) => { csiCustom2.push(['m', params, collect]); return true; }); + parser2.parse(INPUT); + chai.expect(csi).eql([], 'Should not fallback to original handler'); + chai.expect(csiCustom).eql([], 'Should not fallback once'); + chai.expect(csiCustom2).eql([['m', [1, 31], ''], ['m', [0], '']]); + }); + it('Execution order should go from latest handler down to the original', () => { + const order: number[] = []; + parser2.setCsiHandler('m', () => order.push(1)); + parser2.addCsiHandler('m', () => { order.push(2); return false; }); + parser2.addCsiHandler('m', () => { order.push(3); return false; }); + parser2.parse('\x1b[0m'); + chai.expect(order).eql([3, 2, 1]); + }); + }); it('EXECUTE handler', function (): void { parser2.setExecuteHandler('\n', function (): void { exe.push('\n'); @@ -1196,6 +1244,54 @@ describe('EscapeSequenceParser', function (): void { parser2.parse(INPUT); chai.expect(osc).eql([]); }); + describe('OSC custom handlers', () => { + it('Prevent fallback', () => { + const oscCustom: [number, string][] = []; + parser2.setOscHandler(1, data => osc.push([1, data])); + parser2.addOscHandler(1, data => { oscCustom.push([1, data]); return true; }); + parser2.parse(INPUT); + chai.expect(osc).eql([], 'Should not fallback to original handler'); + chai.expect(oscCustom).eql([[1, 'foo=bar']]); + }); + it('Allow fallback', () => { + const oscCustom: [number, string][] = []; + parser2.setOscHandler(1, data => osc.push([1, data])); + parser2.addOscHandler(1, data => { oscCustom.push([1, data]); return false; }); + parser2.parse(INPUT); + chai.expect(osc).eql([[1, 'foo=bar']], 'Should fallback to original handler'); + chai.expect(oscCustom).eql([[1, 'foo=bar']]); + }); + it('Multiple custom handlers fallback once', () => { + const oscCustom: [number, string][] = []; + const oscCustom2: [number, string][] = []; + parser2.setOscHandler(1, data => osc.push([1, data])); + parser2.addOscHandler(1, data => { oscCustom.push([1, data]); return true; }); + parser2.addOscHandler(1, data => { oscCustom2.push([1, data]); return false; }); + parser2.parse(INPUT); + chai.expect(osc).eql([], 'Should not fallback to original handler'); + chai.expect(oscCustom).eql([[1, 'foo=bar']]); + chai.expect(oscCustom2).eql([[1, 'foo=bar']]); + }); + it('Multiple custom handlers no fallback', () => { + const oscCustom: [number, string][] = []; + const oscCustom2: [number, string][] = []; + parser2.setOscHandler(1, data => osc.push([1, data])); + parser2.addOscHandler(1, data => { oscCustom.push([1, data]); return true; }); + parser2.addOscHandler(1, data => { oscCustom2.push([1, data]); return true; }); + parser2.parse(INPUT); + chai.expect(osc).eql([], 'Should not fallback to original handler'); + chai.expect(oscCustom).eql([], 'Should not fallback once'); + chai.expect(oscCustom2).eql([[1, 'foo=bar']]); + }); + it('Execution order should go from latest handler down to the original', () => { + const order: number[] = []; + parser2.setOscHandler(1, () => order.push(1)); + parser2.addOscHandler(1, () => { order.push(2); return false; }); + parser2.addOscHandler(1, () => { order.push(3); return false; }); + parser2.parse('\x1b]1;foo=bar\x1b\\'); + chai.expect(order).eql([3, 2, 1]); + }); + }); it('DCS handler', function (): void { parser2.setDcsHandler('+p', { hook: function (collect: string, params: number[], flag: number): void { diff --git a/src/EscapeSequenceParser.ts b/src/EscapeSequenceParser.ts index 52007244d6..acb5a42d29 100644 --- a/src/EscapeSequenceParser.ts +++ b/src/EscapeSequenceParser.ts @@ -7,10 +7,6 @@ import { ParserState, ParserAction, IParsingState, IDcsHandler, IEscapeSequenceP import { IDisposable } from 'xterm'; import { Disposable } from './common/Lifecycle'; -interface IHandlerLink extends IDisposable { - nextHandler: IHandlerLink | null; -} - interface IHandlerCollection { [key: string]: T[]; } @@ -494,8 +490,8 @@ export class EscapeSequenceParser extends Disposable implements IEscapeSequenceP case ParserAction.CSI_DISPATCH: // Trigger CSI Handler const handlers = this._csiHandlers[code]; - let j: number; - for (j = handlers.length - 1; j >= 0; j--) { + let j = handlers ? handlers.length - 1 : -1; + for (; j >= 0; j--) { if (handlers[j](params, collect)) { break; } @@ -579,8 +575,8 @@ export class EscapeSequenceParser extends Disposable implements IEscapeSequenceP const content = osc.substring(idx + 1); // Trigger OSC Handler const handlers = this._oscHandlers[identifier]; - let j: number; - for (j = handlers.length - 1; j >= 0; j--) { + let j = handlers ? handlers.length - 1 : -1; + for (; j >= 0; j--) { if (handlers[j](content)) { break; } From c045c803d60dc569baab8efd53b6722faebd3e81 Mon Sep 17 00:00:00 2001 From: Daniel Imms Date: Wed, 26 Dec 2018 14:48:35 -0800 Subject: [PATCH 3/5] Add tests for dispose --- src/EscapeSequenceParser.test.ts | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/src/EscapeSequenceParser.test.ts b/src/EscapeSequenceParser.test.ts index 3b01cb8a17..9216dd4cc2 100644 --- a/src/EscapeSequenceParser.test.ts +++ b/src/EscapeSequenceParser.test.ts @@ -1216,6 +1216,15 @@ describe('EscapeSequenceParser', function (): void { parser2.parse('\x1b[0m'); chai.expect(order).eql([3, 2, 1]); }); + it('Dispose', () => { + const csiCustom: [string, number[], string][] = []; + parser2.setCsiHandler('m', (params, collect) => csi.push(['m', params, collect])); + const customHandler = parser2.addCsiHandler('m', (params, collect) => { csiCustom.push(['m', params, collect]); return true; }); + customHandler.dispose(); + parser2.parse(INPUT); + chai.expect(csi).eql([['m', [1, 31], ''], ['m', [0], '']]); + chai.expect(csiCustom).eql([], 'Should not use custom handler as it was disposed'); + }); }); it('EXECUTE handler', function (): void { parser2.setExecuteHandler('\n', function (): void { @@ -1291,6 +1300,15 @@ describe('EscapeSequenceParser', function (): void { parser2.parse('\x1b]1;foo=bar\x1b\\'); chai.expect(order).eql([3, 2, 1]); }); + it('Dispose', () => { + const oscCustom: [number, string][] = []; + parser2.setOscHandler(1, data => osc.push([1, data])); + const customHandler = parser2.addOscHandler(1, data => { oscCustom.push([1, data]); return true; }); + customHandler.dispose(); + parser2.parse(INPUT); + chai.expect(osc).eql([[1, 'foo=bar']]); + chai.expect(oscCustom).eql([], 'Should not use custom handler as it was disposed'); + }); }); it('DCS handler', function (): void { parser2.setDcsHandler('+p', { From adbb929f7881d6f678cdde8b8736bfd113565204 Mon Sep 17 00:00:00 2001 From: Daniel Imms Date: Wed, 26 Dec 2018 14:49:49 -0800 Subject: [PATCH 4/5] Wrap .d.ts comments to 80 chars --- typings/xterm.d.ts | 24 +++++++++++------------- 1 file changed, 11 insertions(+), 13 deletions(-) diff --git a/typings/xterm.d.ts b/typings/xterm.d.ts index cf489a610c..0ceab01dcc 100644 --- a/typings/xterm.d.ts +++ b/typings/xterm.d.ts @@ -483,14 +483,13 @@ declare module 'xterm' { /** * (EXPERIMENTAL) Adds a handler for CSI escape sequences. - * @param flag The flag should be one-character string, which specifies - * the final character (e.g "m" for SGR) of the CSI sequence. - * @param callback The function to handle the escape sequence. - * The callback is called with the numerical params, - * as well as the special characters (e.g. "$" for DECSCPP). - * Return true if the sequence was handled; false if we should - * try a previous handler (set by addCsiHandler or setCsiHandler). - * The most recently-added handler is tried first. + * @param flag The flag should be one-character string, which specifies the + * final character (e.g "m" for SGR) of the CSI sequence. + * @param callback The function to handle the escape sequence. The callback + * is called with the numerical params, as well as the special characters + * (e.g. "$" for DECSCPP). Return true if the sequence was handled; false if + * we should try a previous handler (set by addCsiHandler or setCsiHandler). + * The most recently-added handler is tried first. * @return An IDisposable you can call to remove this handler. */ addCsiHandler(flag: string, callback: (params: number[], collect: string) => boolean): IDisposable; @@ -498,11 +497,10 @@ declare module 'xterm' { /** * (EXPERIMENTAL) Adds a handler for OSC escape sequences. * @param ident The number (first parameter) of the sequence. - * @param callback The function to handle the escape sequence. - * The callback is called with OSC data string. - * Return true if the sequence was handled; false if we should - * try a previous handler (set by addOscHandler or setOscHandler). - * The most recently-added handler is tried first. + * @param callback The function to handle the escape sequence. The callback + * is called with OSC data string. Return true if the sequence was handled; + * false if we should try a previous handler (set by addOscHandler or + * setOscHandler). The most recently-added handler is tried first. * @return An IDisposable you can call to remove this handler. */ addOscHandler(ident: number, callback: (data: string) => boolean): IDisposable; From 51e1f49bf36085e46d58ce98c07e115d74c112a8 Mon Sep 17 00:00:00 2001 From: Daniel Imms Date: Thu, 27 Dec 2018 10:26:40 -0800 Subject: [PATCH 5/5] Make dispose more resilient --- src/EscapeSequenceParser.test.ts | 24 ++++++++++++++++++++++-- src/EscapeSequenceParser.ts | 20 ++++++++++++++++---- 2 files changed, 38 insertions(+), 6 deletions(-) diff --git a/src/EscapeSequenceParser.test.ts b/src/EscapeSequenceParser.test.ts index 9216dd4cc2..0f1d63cc0d 100644 --- a/src/EscapeSequenceParser.test.ts +++ b/src/EscapeSequenceParser.test.ts @@ -1216,7 +1216,7 @@ describe('EscapeSequenceParser', function (): void { parser2.parse('\x1b[0m'); chai.expect(order).eql([3, 2, 1]); }); - it('Dispose', () => { + it('Dispose should work', () => { const csiCustom: [string, number[], string][] = []; parser2.setCsiHandler('m', (params, collect) => csi.push(['m', params, collect])); const customHandler = parser2.addCsiHandler('m', (params, collect) => { csiCustom.push(['m', params, collect]); return true; }); @@ -1225,6 +1225,16 @@ describe('EscapeSequenceParser', function (): void { chai.expect(csi).eql([['m', [1, 31], ''], ['m', [0], '']]); chai.expect(csiCustom).eql([], 'Should not use custom handler as it was disposed'); }); + it('Should not corrupt the parser when dispose is called twice', () => { + const csiCustom: [string, number[], string][] = []; + parser2.setCsiHandler('m', (params, collect) => csi.push(['m', params, collect])); + const customHandler = parser2.addCsiHandler('m', (params, collect) => { csiCustom.push(['m', params, collect]); return true; }); + customHandler.dispose(); + customHandler.dispose(); + parser2.parse(INPUT); + chai.expect(csi).eql([['m', [1, 31], ''], ['m', [0], '']]); + chai.expect(csiCustom).eql([], 'Should not use custom handler as it was disposed'); + }); }); it('EXECUTE handler', function (): void { parser2.setExecuteHandler('\n', function (): void { @@ -1300,7 +1310,7 @@ describe('EscapeSequenceParser', function (): void { parser2.parse('\x1b]1;foo=bar\x1b\\'); chai.expect(order).eql([3, 2, 1]); }); - it('Dispose', () => { + it('Dispose should work', () => { const oscCustom: [number, string][] = []; parser2.setOscHandler(1, data => osc.push([1, data])); const customHandler = parser2.addOscHandler(1, data => { oscCustom.push([1, data]); return true; }); @@ -1309,6 +1319,16 @@ describe('EscapeSequenceParser', function (): void { chai.expect(osc).eql([[1, 'foo=bar']]); chai.expect(oscCustom).eql([], 'Should not use custom handler as it was disposed'); }); + it('Should not corrupt the parser when dispose is called twice', () => { + const oscCustom: [number, string][] = []; + parser2.setOscHandler(1, data => osc.push([1, data])); + const customHandler = parser2.addOscHandler(1, data => { oscCustom.push([1, data]); return true; }); + customHandler.dispose(); + customHandler.dispose(); + parser2.parse(INPUT); + chai.expect(osc).eql([[1, 'foo=bar']]); + chai.expect(oscCustom).eql([], 'Should not use custom handler as it was disposed'); + }); }); it('DCS handler', function (): void { parser2.setDcsHandler('+p', { diff --git a/src/EscapeSequenceParser.ts b/src/EscapeSequenceParser.ts index acb5a42d29..70c1c6c8ef 100644 --- a/src/EscapeSequenceParser.ts +++ b/src/EscapeSequenceParser.ts @@ -316,9 +316,15 @@ export class EscapeSequenceParser extends Disposable implements IEscapeSequenceP if (this._csiHandlers[index] === undefined) { this._csiHandlers[index] = []; } - this._csiHandlers[index].push(callback); + const handlerList = this._csiHandlers[index]; + handlerList.push(callback); return { - dispose: () => this._csiHandlers[index].splice(this._csiHandlers[index].indexOf(callback)) + dispose: () => { + const handlerIndex = handlerList.indexOf(callback); + if (handlerIndex !== -1) { + handlerList.splice(handlerIndex); + } + } }; } setCsiHandler(flag: string, callback: (params: number[], collect: string) => void): void { @@ -345,9 +351,15 @@ export class EscapeSequenceParser extends Disposable implements IEscapeSequenceP if (this._oscHandlers[ident] === undefined) { this._oscHandlers[ident] = []; } - this._oscHandlers[ident].push(callback); + const handlerList = this._oscHandlers[ident]; + handlerList.push(callback); return { - dispose: () => this._oscHandlers[ident].splice(this._oscHandlers[ident].indexOf(callback)) + dispose: () => { + const handlerIndex = handlerList.indexOf(callback); + if (handlerIndex !== -1) { + handlerList.splice(handlerIndex); + } + } }; } setOscHandler(ident: number, callback: (data: string) => void): void {