From d1e2a5e2870ecaa93d69b341d1c6f43a2afbe0e3 Mon Sep 17 00:00:00 2001 From: Maciej Bukowski Date: Wed, 7 Aug 2019 18:10:49 +0200 Subject: [PATCH 1/8] Added unhandled promise rejection error handling. --- src/watchdog.js | 61 +++++++++++++++++++++++++++++++---------------- tests/watchdog.js | 60 ++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 101 insertions(+), 20 deletions(-) diff --git a/src/watchdog.js b/src/watchdog.js index f0983c7..2265fd2 100644 --- a/src/watchdog.js +++ b/src/watchdog.js @@ -30,10 +30,11 @@ export default class Watchdog { * An array of crashes saved as an object with the following properties: * * * `message`: `String`, - * * `source`: `String`, - * * `lineno`: `String`, - * * `colno`: `String`, + * * `stack`: `String`, * * `date`: `Number`, + * * `source`: `String | undefined`, + * * `lineno`: `String | undefined`, + * * `colno`: `String` | undefined, * * @public * @readonly @@ -78,7 +79,19 @@ export default class Watchdog { * @private * @type {Function} */ - this._boundErrorHandler = this._handleGlobalErrorEvent.bind( this ); + this._boundErrorHandler = evt => { + // `evt.error` is exposed by EventError while `evt.reason` is available in PromiseRejectionEvent. + + if ( evt.reason ) { + // Note that evt.reason might be everything that is in the promise rejection. + if ( evt.reason instanceof Error ) { + this._handleError( evt.reason, evt ); + } + } else { + // TODO: what throw 'foo' will do? + this._handleError( evt.error, evt ); + } + }; /** * Throttled save method. The `save()` method is called the specified `saveInterval` after `throttledSave()` is called, @@ -229,6 +242,8 @@ export default class Watchdog { this._editor = editor; window.addEventListener( 'error', this._boundErrorHandler ); + window.addEventListener( 'unhandledrejection', this._boundErrorHandler ); + this.listenTo( editor.model.document, 'change:data', this._throttledSave ); this._lastDocumentVersion = editor.model.document.version; @@ -256,6 +271,8 @@ export default class Watchdog { */ _destroy() { window.removeEventListener( 'error', this._boundErrorHandler ); + window.removeEventListener( 'unhandledrejection', this._boundErrorHandler ); + this.stopListening( this._editor.model.document, 'change:data', this._throttledSave ); return Promise.resolve() @@ -293,28 +310,32 @@ export default class Watchdog { } /** - * Checks if the event error comes from the editor that is handled by the watchdog (by checking the error context) and - * restarts the editor. It handles {@link module:utils/ckeditorerror~CKEditorError `CKEditorError` errors} only. + * Checks if the error comes from the editor that is handled by the watchdog (by checking the error context) and + * restarts the editor. It reacts to {@link module:utils/ckeditorerror~CKEditorError `CKEditorError` errors} only. * * @private * @fires error - * @param {Event} evt Error event. + * @param {Error} error Error. + * @param {ErrorEvent} evt Error event. */ - _handleGlobalErrorEvent( evt ) { - if ( evt.error.is && evt.error.is( 'CKEditorError' ) && evt.error.context === undefined ) { + _handleError( error, evt ) { + if ( error.is && error.is( 'CKEditorError' ) && error.context === undefined ) { console.error( 'The error is missing its context and Watchdog cannot restart the proper editor.' ); } - if ( this._shouldReactToErrorEvent( evt ) ) { + if ( this._shouldReactToError( error ) ) { this.crashes.push( { - message: evt.error.message, + message: error.message, + stack: error.stack, + + // evt.source, evt.lineno and evt.colno are available only in ErrorEvent source: evt.source, lineno: evt.lineno, colno: evt.colno, date: Date.now() } ); - this.fire( 'error', { error: evt.error } ); + this.fire( 'error', { error } ); this.state = 'crashed'; if ( this._shouldRestartEditor() ) { @@ -326,25 +347,25 @@ export default class Watchdog { } /** - * Checks whether the error event should be handled. + * Checks whether the error should be handled. * * @private - * @param {Event} evt Error event. + * @param {Error} error Error event. */ - _shouldReactToErrorEvent( evt ) { + _shouldReactToError( error ) { return ( - evt.error.is && - evt.error.is( 'CKEditorError' ) && - evt.error.context !== undefined && + error.is && + error.is( 'CKEditorError' ) && + error.context !== undefined && // In some cases the editor should not be restarted - e.g. in case of the editor initialization. // That's why the `null` was introduced as a correct error context which does cause restarting. - evt.error.context !== null && + error.context !== null && // Do not react to errors if the watchdog is in states other than `ready`. this.state === 'ready' && - this._isErrorComingFromThisEditor( evt.error ) + this._isErrorComingFromThisEditor( error ) ); } diff --git a/tests/watchdog.js b/tests/watchdog.js index c5d80d5..8fe84d0 100644 --- a/tests/watchdog.js +++ b/tests/watchdog.js @@ -262,6 +262,9 @@ describe( 'Watchdog', () => { setTimeout( () => { throw new Error( 'foo' ); } ); + setTimeout( () => { + throw 'bar'; + } ); return new Promise( res => { setTimeout( () => { @@ -695,6 +698,63 @@ describe( 'Watchdog', () => { } ); } ); + describe( 'async error handling', () => { + it( 'Watchdog should handle async CKEditorError errors', () => { + const watchdog = Watchdog.for( ClassicTestEditor ); + const originalErrorHandler = window.onerror; + + window.onerror = undefined; + + return watchdog.create( element, { + initialData: '

foo

', + plugins: [ Paragraph ] + } ).then( () => { + const oldEditor = watchdog.editor; + + Promise.resolve().then( () => throwCKEditorError( 'foo', watchdog.editor ) ); + + return new Promise( res => { + watchdog.on( 'restart', () => { + window.onerror = originalErrorHandler; + + expect( watchdog.editor ).to.not.equal( oldEditor ); + expect( watchdog.editor.getData() ).to.equal( '

foo

' ); + + watchdog.destroy().then( res ); + } ); + } ); + } ); + } ); + + it( 'Watchdog should not react to non-editor async errors', () => { + const watchdog = Watchdog.for( ClassicTestEditor ); + const originalErrorHandler = window.onerror; + const editorErrorSpy = sinon.spy(); + + window.onerror = undefined; + + return watchdog.create( element, { + initialData: '

foo

', + plugins: [ Paragraph ] + } ).then( () => { + watchdog.on( 'error', editorErrorSpy ); + + Promise.resolve().then( () => Promise.reject( 'foo' ) ); + Promise.resolve().then( () => Promise.reject( new Error( 'bar' ) ) ); + + // Wait a cycle. + return new Promise( res => setTimeout( res ) ); + } ).then( () => { + window.onerror = originalErrorHandler; + + sinon.assert.notCalled( editorErrorSpy ); + expect( watchdog.editor.getData() ).to.equal( '

foo

' ); + + return watchdog.destroy(); + } ); + } ); + } ); + describe( 'static for()', () => { it( 'should be a shortcut method for creating the watchdog', () => { const watchdog = Watchdog.for( ClassicTestEditor ); From f493004ff1a337e67ac1f2b8ef626574bdef0213 Mon Sep 17 00:00:00 2001 From: Maciej Bukowski Date: Wed, 7 Aug 2019 18:12:21 +0200 Subject: [PATCH 2/8] Improved docs. --- src/watchdog.js | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/watchdog.js b/src/watchdog.js index 2265fd2..07f60d7 100644 --- a/src/watchdog.js +++ b/src/watchdog.js @@ -88,7 +88,6 @@ export default class Watchdog { this._handleError( evt.reason, evt ); } } else { - // TODO: what throw 'foo' will do? this._handleError( evt.error, evt ); } }; @@ -316,7 +315,7 @@ export default class Watchdog { * @private * @fires error * @param {Error} error Error. - * @param {ErrorEvent} evt Error event. + * @param {ErrorEvent|PromiseRejectionEvent} evt Error event. */ _handleError( error, evt ) { if ( error.is && error.is( 'CKEditorError' ) && error.context === undefined ) { @@ -328,7 +327,7 @@ export default class Watchdog { message: error.message, stack: error.stack, - // evt.source, evt.lineno and evt.colno are available only in ErrorEvent + // `evt.source`, `evt.lineno` and `evt.colno` are available only in ErrorEvent events source: evt.source, lineno: evt.lineno, colno: evt.colno, From abee00a7c4befb5eaf658bd38fabe8d1b4f669da Mon Sep 17 00:00:00 2001 From: Maciej Bukowski Date: Thu, 8 Aug 2019 12:52:50 +0200 Subject: [PATCH 3/8] Added unhandledrejection event detection. --- tests/watchdog.js | 35 +++++++++++++++++++++++++++++++++-- 1 file changed, 33 insertions(+), 2 deletions(-) diff --git a/tests/watchdog.js b/tests/watchdog.js index 8fe84d0..ca3f6e2 100644 --- a/tests/watchdog.js +++ b/tests/watchdog.js @@ -372,7 +372,7 @@ describe( 'Watchdog', () => { } ); it( 'Watchdog should crash permanently if the `crashNumberLimit` is reached' + - ' and the average time between errors is lower than `minimumNonErrorTimePeriod` (default values)', () => { + ' and the average time between errors is lower than `minimumNonErrorTimePeriod` (default values)', () => { const watchdog = new Watchdog(); watchdog.setCreator( ( el, config ) => ClassicTestEditor.create( el, config ) ); @@ -409,7 +409,7 @@ describe( 'Watchdog', () => { } ); it( 'Watchdog should crash permanently if the `crashNumberLimit` is reached' + - ' and the average time between errors is lower than `minimumNonErrorTimePeriod` (custom values)', () => { + ' and the average time between errors is lower than `minimumNonErrorTimePeriod` (custom values)', () => { const watchdog = new Watchdog( { crashNumberLimit: 2, minimumNonErrorTimePeriod: 1000 } ); watchdog.setCreator( ( el, config ) => ClassicTestEditor.create( el, config ) ); @@ -699,7 +699,20 @@ describe( 'Watchdog', () => { } ); describe( 'async error handling', () => { + let unhandledRejectionEventSupported; + + before( () => { + return doesEnvSupportUnhandledRejectionEvent() + .then( val => { + unhandledRejectionEventSupported = val; + } ); + } ); + it( 'Watchdog should handle async CKEditorError errors', () => { + if ( !unhandledRejectionEventSupported ) { + return; + } + const watchdog = Watchdog.for( ClassicTestEditor ); const originalErrorHandler = window.onerror; @@ -727,6 +740,10 @@ describe( 'Watchdog', () => { } ); it( 'Watchdog should not react to non-editor async errors', () => { + if ( !unhandledRejectionEventSupported ) { + return; + } + const watchdog = Watchdog.for( ClassicTestEditor ); const originalErrorHandler = window.onerror; const editorErrorSpy = sinon.spy(); @@ -900,3 +917,17 @@ describe( 'Watchdog', () => { function throwCKEditorError( name, context ) { throw new CKEditorError( name, context ); } + +function doesEnvSupportUnhandledRejectionEvent() { + return new Promise( res => { + window.addEventListener( 'unhandledrejection', function listener() { + res( true ); + + window.removeEventListener( 'unhandledrejection', listener ); + } ); + + Promise.resolve().then( () => Promise.reject( new Error() ) ); + + setTimeout( () => res( false ) ); + } ); +} From 095105a6f0c6a176bad0b8149843202069930186 Mon Sep 17 00:00:00 2001 From: Maciej Bukowski Date: Thu, 8 Aug 2019 12:59:50 +0200 Subject: [PATCH 4/8] Added a comment for the feature detector. --- tests/watchdog.js | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/watchdog.js b/tests/watchdog.js index ca3f6e2..05d6bcc 100644 --- a/tests/watchdog.js +++ b/tests/watchdog.js @@ -918,6 +918,8 @@ function throwCKEditorError( name, context ) { throw new CKEditorError( name, context ); } +// Feature detection works as a race condition - if the `unhandledrejection` event +// is supported then the listener should be called first. Otherwise the timeout will be reached. function doesEnvSupportUnhandledRejectionEvent() { return new Promise( res => { window.addEventListener( 'unhandledrejection', function listener() { From acc7d1d79ac1db51f0abce361f07111f53d6978d Mon Sep 17 00:00:00 2001 From: Maciej Bukowski Date: Thu, 8 Aug 2019 13:00:58 +0200 Subject: [PATCH 5/8] Changed function name. --- tests/watchdog.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/watchdog.js b/tests/watchdog.js index 05d6bcc..00a82b5 100644 --- a/tests/watchdog.js +++ b/tests/watchdog.js @@ -702,7 +702,7 @@ describe( 'Watchdog', () => { let unhandledRejectionEventSupported; before( () => { - return doesEnvSupportUnhandledRejectionEvent() + return isUnhandledRejectionEventSupported() .then( val => { unhandledRejectionEventSupported = val; } ); @@ -920,7 +920,7 @@ function throwCKEditorError( name, context ) { // Feature detection works as a race condition - if the `unhandledrejection` event // is supported then the listener should be called first. Otherwise the timeout will be reached. -function doesEnvSupportUnhandledRejectionEvent() { +function isUnhandledRejectionEventSupported() { return new Promise( res => { window.addEventListener( 'unhandledrejection', function listener() { res( true ); From 412ed6d35f5ab16523f4a3b8c71f12791ec19178 Mon Sep 17 00:00:00 2001 From: Maciej Bukowski Date: Thu, 8 Aug 2019 14:37:07 +0200 Subject: [PATCH 6/8] Fixed `crashes` property, added test for it. --- src/watchdog.js | 10 +++++----- tests/watchdog.js | 39 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 44 insertions(+), 5 deletions(-) diff --git a/src/watchdog.js b/src/watchdog.js index 07f60d7..6672f37 100644 --- a/src/watchdog.js +++ b/src/watchdog.js @@ -32,9 +32,9 @@ export default class Watchdog { * * `message`: `String`, * * `stack`: `String`, * * `date`: `Number`, - * * `source`: `String | undefined`, - * * `lineno`: `String | undefined`, - * * `colno`: `String` | undefined, + * * `filename`: `String | undefined`, + * * `lineno`: `Number | undefined`, + * * `colno`: `Number` | undefined, * * @public * @readonly @@ -327,8 +327,8 @@ export default class Watchdog { message: error.message, stack: error.stack, - // `evt.source`, `evt.lineno` and `evt.colno` are available only in ErrorEvent events - source: evt.source, + // `evt.filename`, `evt.lineno` and `evt.colno` are available only in ErrorEvent events + filename: evt.filename, lineno: evt.lineno, colno: evt.colno, date: Date.now() diff --git a/tests/watchdog.js b/tests/watchdog.js index 00a82b5..96a12e5 100644 --- a/tests/watchdog.js +++ b/tests/watchdog.js @@ -821,6 +821,12 @@ describe( 'Watchdog', () => { window.onerror = originalErrorHandler; expect( watchdog.crashes[ 0 ].message ).to.equal( 'foo' ); + expect( watchdog.crashes[ 0 ].stack ).to.be.a( 'string' ); + expect( watchdog.crashes[ 0 ].date ).to.be.a( 'number' ); + expect( watchdog.crashes[ 0 ].filename ).to.be.a( 'string' ); + expect( watchdog.crashes[ 0 ].lineno ).to.be.a( 'number' ); + expect( watchdog.crashes[ 0 ].colno ).to.be.a( 'number' ); + expect( watchdog.crashes[ 1 ].message ).to.equal( 'bar' ); watchdog.destroy().then( res ); @@ -828,6 +834,39 @@ describe( 'Watchdog', () => { } ); } ); } ); + + it( 'should include async errors', () => { + return isUnhandledRejectionEventSupported().then( isSupported => { + if ( !isSupported ) { + return; + } + + const watchdog = Watchdog.for( ClassicTestEditor ); + + // sinon.stub( window, 'onerror' ).value( undefined ); and similar do not work. + const originalErrorHandler = window.onerror; + window.onerror = undefined; + + return watchdog.create( element ).then( () => { + Promise.resolve().then( () => throwCKEditorError( 'foo', watchdog.editor ) ); + + return new Promise( res => { + setTimeout( () => { + window.onerror = originalErrorHandler; + + expect( watchdog.crashes[ 0 ].message ).to.equal( 'foo' ); + expect( watchdog.crashes[ 0 ].stack ).to.be.a( 'string' ); + expect( watchdog.crashes[ 0 ].date ).to.be.a( 'number' ); + expect( watchdog.crashes[ 0 ].filename ).to.be.an( 'undefined' ); + expect( watchdog.crashes[ 0 ].lineno ).to.be.an( 'undefined' ); + expect( watchdog.crashes[ 0 ].colno ).to.be.an( 'undefined' ); + + watchdog.destroy().then( res ); + } ); + } ); + } ); + } ); + } ); } ); describe( 'state', () => { From 4d47c011695fa43f9a1244b3b8e1fc072aeef566 Mon Sep 17 00:00:00 2001 From: Maciej Bukowski Date: Thu, 8 Aug 2019 15:01:56 +0200 Subject: [PATCH 7/8] Fixed API docs. --- src/watchdog.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/watchdog.js b/src/watchdog.js index ef26a29..13fe76f 100644 --- a/src/watchdog.js +++ b/src/watchdog.js @@ -34,7 +34,7 @@ export default class Watchdog { * * `date`: `Number`, * * `filename`: `String | undefined`, * * `lineno`: `Number | undefined`, - * * `colno`: `Number` | undefined, + * * `colno`: `Number | undefined`, * * @public * @readonly From 996455e6dc6d89083da5eb8a281fc940d333e607 Mon Sep 17 00:00:00 2001 From: Maciej Bukowski Date: Thu, 8 Aug 2019 15:06:36 +0200 Subject: [PATCH 8/8] Fixed API docs. --- src/watchdog.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/watchdog.js b/src/watchdog.js index 13fe76f..634caab 100644 --- a/src/watchdog.js +++ b/src/watchdog.js @@ -363,7 +363,7 @@ export default class Watchdog { * Checks whether the error should be handled. * * @private - * @param {Error} error Error event. + * @param {Error} error Error */ _shouldReactToError( error ) { return (