Skip to content
This repository has been archived by the owner on Jun 26, 2020. It is now read-only.

Commit

Permalink
Merge pull request #20 from ckeditor/t/3
Browse files Browse the repository at this point in the history 
Feature: Added unhandled promise rejection error handling. Fixed objects in the `crashed` array. Closes #3.
  • Loading branch information
Piotr Jasiun authored Aug 8, 2019
2 parents a54db15 + 996455e commit 1a47364
Show file tree
Hide file tree
Showing 2 changed files with 174 additions and 22 deletions.
62 changes: 41 additions & 21 deletions src/watchdog.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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`,
* * `filename`: `String | undefined`,
* * `lineno`: `Number | undefined`,
* * `colno`: `Number | undefined`,
*
* @public
* @readonly
Expand Down Expand Up @@ -86,7 +87,18 @@ 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 {
this._handleError( evt.error, evt );
}
};

/**
* Throttled save method. The `save()` method is called the specified `saveInterval` after `throttledSave()` is called,
Expand Down Expand Up @@ -240,6 +252,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;
Expand Down Expand Up @@ -267,6 +281,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 );

// Save data if there is a remaining editor data change.
Expand Down Expand Up @@ -307,28 +323,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|PromiseRejectionEvent} 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,
source: evt.source,
message: error.message,
stack: error.stack,

// `evt.filename`, `evt.lineno` and `evt.colno` are available only in ErrorEvent events
filename: evt.filename,
lineno: evt.lineno,
colno: evt.colno,
date: this._now()
} );

this.fire( 'error', { error: evt.error } );
this.fire( 'error', { error } );
this.state = 'crashed';

if ( this._shouldRestartEditor() ) {
Expand All @@ -340,25 +360,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
*/
_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 )
);
}

Expand Down
134 changes: 133 additions & 1 deletion tests/watchdog.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -262,6 +262,9 @@ describe( 'Watchdog', () => {
setTimeout( () => {
throw new Error( 'foo' );
} );
setTimeout( () => {
throw 'bar';
} );

return new Promise( res => {
setTimeout( () => {
Expand Down Expand Up @@ -695,6 +698,80 @@ describe( 'Watchdog', () => {
} );
} );

describe( 'async error handling', () => {
let unhandledRejectionEventSupported;

before( () => {
return isUnhandledRejectionEventSupported()
.then( val => {
unhandledRejectionEventSupported = val;
} );
} );

it( 'Watchdog should handle async CKEditorError errors', () => {
if ( !unhandledRejectionEventSupported ) {
return;
}

const watchdog = Watchdog.for( ClassicTestEditor );
const originalErrorHandler = window.onerror;

window.onerror = undefined;

return watchdog.create( element, {
initialData: '<p>foo</p>',
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( '<p>foo</p>' );

watchdog.destroy().then( res );
} );
} );
} );
} );

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();

window.onerror = undefined;

return watchdog.create( element, {
initialData: '<p>foo</p>',
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( '<p>foo</p>' );

return watchdog.destroy();
} );
} );
} );

describe( 'destroy()', () => {
// See #19.
it( 'should clean internal stuff', () => {
Expand All @@ -719,7 +796,7 @@ describe( 'Watchdog', () => {
watchdog.editor.model.change( writer => {
writer.insertText( 'foo', writer.createPositionAt( doc.getRoot(), 1 ) );
} );
} ).then( () => {

return watchdog.destroy();
} ).then( () => {
// Wait to ensure that the throttled save is cleared and won't be executed
Expand Down Expand Up @@ -782,13 +859,52 @@ 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 );
} );
} );
} );
} );

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', () => {
Expand Down Expand Up @@ -878,3 +994,19 @@ describe( 'Watchdog', () => {
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 isUnhandledRejectionEventSupported() {
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 ) );
} );
}

0 comments on commit 1a47364

Please sign in to comment.