From 2c29136d9c14e504370f63c6dbbeff60e96901bb Mon Sep 17 00:00:00 2001 From: Maciej Bukowski Date: Mon, 7 Oct 2019 17:10:36 +0200 Subject: [PATCH 1/3] Introduced `CKEditorError.rethrowUnexpectedError()`. --- src/ckeditorerror.js | 32 ++++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/src/ckeditorerror.js b/src/ckeditorerror.js index b51000f..3cbde13 100644 --- a/src/ckeditorerror.js +++ b/src/ckeditorerror.js @@ -91,6 +91,38 @@ export default class CKEditorError extends Error { is( type ) { return type === 'CKEditorError'; } + + /** + * A utility that ensures the the thrown error is a {@link utils/ckeditorerror~CKEditorError} one. + * It is uesful when combined with the {@link watchdog/watchdog~Watchdog} feature, which can restart the editor in case + * of a {@link utils/ckeditorerror~CKEditorError} error. + * + * @param {Error} err An error. + * @param {Object} context An object conected through properties with the editor instance. This context will be used + * by the watchdog to verify which editor should be restarted. + */ + static rethrowUnexpectedError( err, context ) { + if ( err.is && err.is( 'CKEditorError' ) ) { + throw err; + } + + /** + * An unexpected error occurred inside the CKEditor 5 codebase. The `error.data.originalError` property + * shows the original error properties. + * + * This error is only useful when the editor is initialized using the {@link watchdog/watchdog~Watchdog} feature. + * In case of such error (or any {@link utils/ckeditorerror~CKEditorError} error) the wathcdog should restart the editor. + * + * @error unexpected-error + */ + throw new CKEditorError( 'unexpected-error', context, { + originalError: { + message: err.message, + stack: err.stack, + name: err.name + } + } ); + } } /** From 438819a3e7b852ac732a7ac0e0b5d777ed3682df Mon Sep 17 00:00:00 2001 From: Maciej Bukowski Date: Tue, 8 Oct 2019 11:10:53 +0200 Subject: [PATCH 2/3] Introduced error rethrowing from `EmitterMixin#fire()`. --- src/emittermixin.js | 91 +++++++++++++++++++++++-------------------- tests/emittermixin.js | 31 +++++++++++++++ 2 files changed, 79 insertions(+), 43 deletions(-) diff --git a/src/emittermixin.js b/src/emittermixin.js index 5970b7d..a20a38f 100644 --- a/src/emittermixin.js +++ b/src/emittermixin.js @@ -13,6 +13,7 @@ import priorities from './priorities'; // To check if component is loaded more than once. import './version'; +import CKEditorError from './ckeditorerror'; const _listeningTo = Symbol( 'listeningTo' ); const _emitterId = Symbol( 'emitterId' ); @@ -184,58 +185,62 @@ const EmitterMixin = { * @inheritDoc */ fire( eventOrInfo, ...args ) { - const eventInfo = eventOrInfo instanceof EventInfo ? eventOrInfo : new EventInfo( this, eventOrInfo ); - const event = eventInfo.name; - let callbacks = getCallbacksForEvent( this, event ); - - // Record that the event passed this emitter on its path. - eventInfo.path.push( this ); - - // Handle event listener callbacks first. - if ( callbacks ) { - // Arguments passed to each callback. - const callbackArgs = [ eventInfo, ...args ]; - - // Copying callbacks array is the easiest and most secure way of preventing infinite loops, when event callbacks - // are added while processing other callbacks. Previous solution involved adding counters (unique ids) but - // failed if callbacks were added to the queue before currently processed callback. - // If this proves to be too inefficient, another method is to change `.on()` so callbacks are stored if same - // event is currently processed. Then, `.fire()` at the end, would have to add all stored events. - callbacks = Array.from( callbacks ); + try { + const eventInfo = eventOrInfo instanceof EventInfo ? eventOrInfo : new EventInfo( this, eventOrInfo ); + const event = eventInfo.name; + let callbacks = getCallbacksForEvent( this, event ); + + // Record that the event passed this emitter on its path. + eventInfo.path.push( this ); + + // Handle event listener callbacks first. + if ( callbacks ) { + // Arguments passed to each callback. + const callbackArgs = [ eventInfo, ...args ]; + + // Copying callbacks array is the easiest and most secure way of preventing infinite loops, when event callbacks + // are added while processing other callbacks. Previous solution involved adding counters (unique ids) but + // failed if callbacks were added to the queue before currently processed callback. + // If this proves to be too inefficient, another method is to change `.on()` so callbacks are stored if same + // event is currently processed. Then, `.fire()` at the end, would have to add all stored events. + callbacks = Array.from( callbacks ); + + for ( let i = 0; i < callbacks.length; i++ ) { + callbacks[ i ].callback.apply( this, callbackArgs ); + + // Remove the callback from future requests if off() has been called. + if ( eventInfo.off.called ) { + // Remove the called mark for the next calls. + delete eventInfo.off.called; + + removeCallback( this, event, callbacks[ i ].callback ); + } - for ( let i = 0; i < callbacks.length; i++ ) { - callbacks[ i ].callback.apply( this, callbackArgs ); + // Do not execute next callbacks if stop() was called. + if ( eventInfo.stop.called ) { + break; + } + } + } - // Remove the callback from future requests if off() has been called. - if ( eventInfo.off.called ) { - // Remove the called mark for the next calls. - delete eventInfo.off.called; + // Delegate event to other emitters if needed. + if ( this._delegations ) { + const destinations = this._delegations.get( event ); + const passAllDestinations = this._delegations.get( '*' ); - removeCallback( this, event, callbacks[ i ].callback ); + if ( destinations ) { + fireDelegatedEvents( destinations, eventInfo, args ); } - // Do not execute next callbacks if stop() was called. - if ( eventInfo.stop.called ) { - break; + if ( passAllDestinations ) { + fireDelegatedEvents( passAllDestinations, eventInfo, args ); } } - } - - // Delegate event to other emitters if needed. - if ( this._delegations ) { - const destinations = this._delegations.get( event ); - const passAllDestinations = this._delegations.get( '*' ); - - if ( destinations ) { - fireDelegatedEvents( destinations, eventInfo, args ); - } - if ( passAllDestinations ) { - fireDelegatedEvents( passAllDestinations, eventInfo, args ); - } + return eventInfo.return; + } catch ( err ) { + CKEditorError.rethrowUnexpectedError( err, this ); } - - return eventInfo.return; }, /** diff --git a/tests/emittermixin.js b/tests/emittermixin.js index 580c897..9ad96ac 100644 --- a/tests/emittermixin.js +++ b/tests/emittermixin.js @@ -5,6 +5,8 @@ import { default as EmitterMixin, _getEmitterListenedTo, _getEmitterId, _setEmitterId } from '../src/emittermixin'; import EventInfo from '../src/eventinfo'; +import { expectToThrowCKEditorError } from './_utils/utils'; +import CKEditorError from '../src/ckeditorerror'; describe( 'EmitterMixin', () => { let emitter, listener; @@ -154,6 +156,35 @@ describe( 'EmitterMixin', () => { sinon.assert.calledThrice( spyFoo2 ); } ); + it( 'should rethrow the CKEditorError error', () => { + emitter.on( 'test', () => { + throw new CKEditorError( 'Foo', null ); + } ); + + expectToThrowCKEditorError( () => { + emitter.fire( 'test' ); + }, /Foo/, null ); + } ); + + it( 'should wrap an error into the CKEditorError if a native error was thrown', () => { + const error = new Error( 'foo' ); + error.stack = 'bar'; + + emitter.on( 'test', () => { + throw error; + } ); + + expectToThrowCKEditorError( () => { + emitter.fire( 'test' ); + }, /unexpected-error/, emitter, { + originalError: { + message: 'foo', + stack: 'bar', + name: 'Error' + } + } ); + } ); + describe( 'return value', () => { it( 'is undefined by default', () => { expect( emitter.fire( 'foo' ) ).to.be.undefined; From bf36157bf0d63a243dabc263a12b21f047ad91fd Mon Sep 17 00:00:00 2001 From: Maciej Bukowski Date: Tue, 8 Oct 2019 12:02:12 +0200 Subject: [PATCH 3/3] Added tests for the `CKEditorError.rethrowUnexpectedError()`. --- tests/ckeditorerror.js | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/tests/ckeditorerror.js b/tests/ckeditorerror.js index 9d56410..66e6df9 100644 --- a/tests/ckeditorerror.js +++ b/tests/ckeditorerror.js @@ -4,6 +4,7 @@ */ import { default as CKEditorError, DOCUMENTATION_URL } from '../src/ckeditorerror'; +import { expectToThrowCKEditorError } from './_utils/utils'; describe( 'CKEditorError', () => { it( 'inherits from Error', () => { @@ -88,4 +89,30 @@ describe( 'CKEditorError', () => { expect( ( !!regularError.is && regularError.is( 'CKEditorError' ) ) ).to.be.false; } ); } ); + + describe( 'static rethrowUnexpectedError()', () => { + it( 'should rethrow the original CKEditorError as it is', () => { + const ckeditorError = new CKEditorError( 'foo', null ); + + expectToThrowCKEditorError( () => { + CKEditorError.rethrowUnexpectedError( ckeditorError, {} ); + }, /foo/, null ); + } ); + + it( 'should rethrow an unexpected error wrapped in CKEditorError', () => { + const error = new Error( 'foo' ); + error.stack = 'bar'; + const context = {}; + + expectToThrowCKEditorError( () => { + CKEditorError.rethrowUnexpectedError( error, context ); + }, /unexpected-error/, context, { + originalError: { + message: 'foo', + stack: 'bar', + name: 'Error' + } + } ); + } ); + } ); } );