From 87d7de0574561272c423bf41ba66e1b9bb972c12 Mon Sep 17 00:00:00 2001 From: Maciej Bukowski Date: Thu, 3 Oct 2019 14:30:00 +0200 Subject: [PATCH 1/3] Wrapped `model.change()` and `model.enqueueChange()` into try/catch blocks. --- src/model/model.js | 75 ++++++++++++++++++++++++++++++++++---------- tests/model/model.js | 56 +++++++++++++++++++++++++++++++++ 2 files changed, 114 insertions(+), 17 deletions(-) diff --git a/src/model/model.js b/src/model/model.js index 61a6b73f0..5d6197492 100644 --- a/src/model/model.js +++ b/src/model/model.js @@ -24,6 +24,7 @@ import deleteContent from './utils/deletecontent'; import modifySelection from './utils/modifyselection'; import getSelectedContent from './utils/getselectedcontent'; import { injectSelectionPostFixer } from './utils/selection-post-fixer'; +import CKEditorError from '@ckeditor/ckeditor5-utils/src/ckeditorerror'; /** * Editor's data model. Read about the model in the @@ -153,14 +154,34 @@ export default class Model { * @returns {*} Value returned by the callback. */ change( callback ) { - if ( this._pendingChanges.length === 0 ) { - // If this is the outermost block, create a new batch and start `_runPendingChanges` execution flow. - this._pendingChanges.push( { batch: new Batch(), callback } ); - - return this._runPendingChanges()[ 0 ]; - } else { - // If this is not the outermost block, just execute the callback. - return callback( this._currentWriter ); + try { + if ( this._pendingChanges.length === 0 ) { + // If this is the outermost block, create a new batch and start `_runPendingChanges` execution flow. + this._pendingChanges.push( { batch: new Batch(), callback } ); + + return this._runPendingChanges()[ 0 ]; + } else { + // If this is not the outermost block, just execute the callback. + return callback( this._currentWriter ); + } + } catch ( err ) { + if ( err.is && err.is( 'CKEditorError' ) ) { + throw err; + } + + /** + * An unexpected error occurred inside the `model.change()` block. The `error.data.originalError` property + * shows the original error properties. + * + * @error model-change-unexpected-error + */ + throw new CKEditorError( 'model-change-unexpected-error', this, { + originalError: { + message: err.message, + stack: err.stack, + name: err.name + } + } ); } } @@ -198,17 +219,37 @@ export default class Model { * @param {Function} callback Callback function which may modify the model. */ enqueueChange( batchOrType, callback ) { - if ( typeof batchOrType === 'string' ) { - batchOrType = new Batch( batchOrType ); - } else if ( typeof batchOrType == 'function' ) { - callback = batchOrType; - batchOrType = new Batch(); - } + try { + if ( typeof batchOrType === 'string' ) { + batchOrType = new Batch( batchOrType ); + } else if ( typeof batchOrType == 'function' ) { + callback = batchOrType; + batchOrType = new Batch(); + } - this._pendingChanges.push( { batch: batchOrType, callback } ); + this._pendingChanges.push( { batch: batchOrType, callback } ); - if ( this._pendingChanges.length == 1 ) { - this._runPendingChanges(); + if ( this._pendingChanges.length == 1 ) { + this._runPendingChanges(); + } + } catch ( err ) { + if ( err.is && err.is( 'CKEditorError' ) ) { + throw err; + } + + /** + * An unexpected error occurred inside the `model.enqueueChange()` block. The `error.data.originalError` property + * shows the original error properties. + * + * @error model-enqueueChange-unexpected-error + */ + throw new CKEditorError( 'model-enqueueChange-unexpected-error', this, { + originalError: { + message: err.message, + stack: err.stack, + name: err.name + } + } ); } } diff --git a/tests/model/model.js b/tests/model/model.js index 2762e8163..1fd6d0be5 100644 --- a/tests/model/model.js +++ b/tests/model/model.js @@ -12,6 +12,8 @@ import ModelSelection from '../../src/model/selection'; import ModelDocumentFragment from '../../src/model/documentfragment'; import Batch from '../../src/model/batch'; import { getData, setData, stringify } from '../../src/dev-utils/model'; +import { expectToThrowCKEditorError } from '@ckeditor/ckeditor5-utils/tests/_utils/utils'; +import CKEditorError from '@ckeditor/ckeditor5-utils/src/ckeditorerror'; describe( 'Model', () => { let model, schema, changes; @@ -321,6 +323,60 @@ describe( 'Model', () => { expect( writer.batch.type ).to.equal( 'transparent' ); } ); } ); + + it( 'should catch a non-ckeditor error inside the `change()` block and throw the CKEditorError error outside of it', () => { + const error = new TypeError( 'foo' ); + error.stack = 'bar'; + + expectToThrowCKEditorError( () => { + model.change( () => { + throw error; + } ); + }, /model-change-unexpected-error/, model, { + originalError: { + message: 'foo', + stack: 'bar', + name: 'TypeError' + } + } ); + } ); + + it( 'should throw the original CKEditorError error if it was thrown inside the `change()` block', () => { + const err = new CKEditorError( 'foo', null, { foo: 1 } ); + + expectToThrowCKEditorError( () => { + model.change( () => { + throw err; + } ); + }, /foo/, null, { foo: 1 } ); + } ); + + it( 'should catch a non-ckeditor error inside the `enqueueChange()` block and throw the CKEditorError error outside of it', () => { + const error = new TypeError( 'foo' ); + error.stack = 'bar'; + + expectToThrowCKEditorError( () => { + model.enqueueChange( () => { + throw error; + } ); + }, /model-enqueueChange-unexpected-error/, model, { + originalError: { + message: 'foo', + stack: 'bar', + name: 'TypeError' + } + } ); + } ); + + it( 'should throw the original CKEditorError error if it was thrown inside the `enqueueChange()` block', () => { + const err = new CKEditorError( 'foo', null, { foo: 1 } ); + + expectToThrowCKEditorError( () => { + model.enqueueChange( () => { + throw err; + } ); + }, /foo/, null, { foo: 1 } ); + } ); } ); describe( 'applyOperation()', () => { From 9a563c5c1ee67be6009d829e10b2f44a1c39b6b4 Mon Sep 17 00:00:00 2001 From: Maciej Bukowski Date: Thu, 3 Oct 2019 15:38:32 +0200 Subject: [PATCH 2/3] Wrapped the `view.change()` into the try/catch block. --- src/view/view.js | 59 ++++++++++++++++++++++++++++------------- tests/view/view/view.js | 29 ++++++++++++++++++++ 2 files changed, 69 insertions(+), 19 deletions(-) diff --git a/src/view/view.js b/src/view/view.js index f7f999b34..63fc52317 100644 --- a/src/view/view.js +++ b/src/view/view.js @@ -457,29 +457,50 @@ export default class View { ); } - // Recursive call to view.change() method - execute listener immediately. - if ( this._ongoingChange ) { - return callback( this._writer ); - } + // Use try-catch to catch the + try { + // Recursive call to view.change() method - execute listener immediately. + if ( this._ongoingChange ) { + return callback( this._writer ); + } - // This lock will assure that all recursive calls to view.change() will end up in same block - one "render" - // event for all nested calls. - this._ongoingChange = true; - const callbackResult = callback( this._writer ); - this._ongoingChange = false; + // This lock will assure that all recursive calls to view.change() will end up in same block - one "render" + // event for all nested calls. + this._ongoingChange = true; + const callbackResult = callback( this._writer ); + this._ongoingChange = false; + + // This lock is used by editing controller to render changes from outer most model.change() once. As plugins might call + // view.change() inside model.change() block - this will ensures that postfixers and rendering are called once after all + // changes. Also, we don't need to render anything if there're no changes since last rendering. + if ( !this._renderingDisabled && this._hasChangedSinceTheLastRendering ) { + this._postFixersInProgress = true; + this.document._callPostFixers( this._writer ); + this._postFixersInProgress = false; + + this.fire( 'render' ); + } - // This lock is used by editing controller to render changes from outer most model.change() once. As plugins might call - // view.change() inside model.change() block - this will ensures that postfixers and rendering are called once after all changes. - // Also, we don't need to render anything if there're no changes since last rendering. - if ( !this._renderingDisabled && this._hasChangedSinceTheLastRendering ) { - this._postFixersInProgress = true; - this.document._callPostFixers( this._writer ); - this._postFixersInProgress = false; + return callbackResult; + } catch ( err ) { + if ( err.is && err.is( 'CKEditorError' ) ) { + throw err; + } - this.fire( 'render' ); + /** + * An unexpected error occurred inside the `view.change()` block. The `error.data.originalError` property + * shows the original error properties. + * + * @error view-change-unexpected-error + */ + throw new CKEditorError( 'view-change-unexpected-error', this, { + originalError: { + message: err.message, + stack: err.stack, + name: err.name + } + } ); } - - return callbackResult; } /** diff --git a/tests/view/view/view.js b/tests/view/view/view.js index 3615aab65..95fb83fb6 100644 --- a/tests/view/view/view.js +++ b/tests/view/view/view.js @@ -25,6 +25,7 @@ import createViewRoot from '../_utils/createroot'; import createElement from '@ckeditor/ckeditor5-utils/src/dom/createelement'; import { expectToThrowCKEditorError } from '@ckeditor/ckeditor5-utils/tests/_utils/utils'; import env from '@ckeditor/ckeditor5-utils/src/env'; +import CKEditorError from '@ckeditor/ckeditor5-utils/src/ckeditorerror'; describe( 'view', () => { const DEFAULT_OBSERVERS_COUNT = 6; @@ -802,6 +803,34 @@ describe( 'view', () => { expect( result2 ).to.equal( true ); expect( result3 ).to.undefined; } ); + + it( 'should catch native errors and wrap them into the CKEditorError errors', () => { + const error = new TypeError( 'foo' ); + error.stack = 'bar'; + + expectToThrowCKEditorError( () => { + view.change( () => { + throw error; + } ); + }, /view-change-unexpected-error/, view, { + originalError: { + message: 'foo', + stack: 'bar', + name: 'TypeError' + } + } ); + } ); + + it( 'should re-throw custom CKEditorError errors', () => { + const error = new TypeError( 'foo' ); + error.stack = 'bar'; + + expectToThrowCKEditorError( () => { + view.change( () => { + throw new CKEditorError( 'foo', view ); + } ); + }, /foo/, view ); + } ); } ); describe( 'createPositionAt()', () => { From ae2d70620f061bd7bafdb7b30909b23a4f9f33c1 Mon Sep 17 00:00:00 2001 From: Maciej Bukowski Date: Mon, 7 Oct 2019 17:08:00 +0200 Subject: [PATCH 3/3] Moved common error handling part to the `CKEditorError` class. --- src/model/model.js | 36 ++---------------------------------- src/view/view.js | 19 +------------------ tests/model/model.js | 8 +++----- tests/view/view/view.js | 7 ++----- 4 files changed, 8 insertions(+), 62 deletions(-) diff --git a/src/model/model.js b/src/model/model.js index 5d6197492..b8fcb071f 100644 --- a/src/model/model.js +++ b/src/model/model.js @@ -165,23 +165,7 @@ export default class Model { return callback( this._currentWriter ); } } catch ( err ) { - if ( err.is && err.is( 'CKEditorError' ) ) { - throw err; - } - - /** - * An unexpected error occurred inside the `model.change()` block. The `error.data.originalError` property - * shows the original error properties. - * - * @error model-change-unexpected-error - */ - throw new CKEditorError( 'model-change-unexpected-error', this, { - originalError: { - message: err.message, - stack: err.stack, - name: err.name - } - } ); + CKEditorError.rethrowUnexpectedError( err, this ); } } @@ -233,23 +217,7 @@ export default class Model { this._runPendingChanges(); } } catch ( err ) { - if ( err.is && err.is( 'CKEditorError' ) ) { - throw err; - } - - /** - * An unexpected error occurred inside the `model.enqueueChange()` block. The `error.data.originalError` property - * shows the original error properties. - * - * @error model-enqueueChange-unexpected-error - */ - throw new CKEditorError( 'model-enqueueChange-unexpected-error', this, { - originalError: { - message: err.message, - stack: err.stack, - name: err.name - } - } ); + CKEditorError.rethrowUnexpectedError( err, this ); } } diff --git a/src/view/view.js b/src/view/view.js index 63fc52317..31735865d 100644 --- a/src/view/view.js +++ b/src/view/view.js @@ -457,7 +457,6 @@ export default class View { ); } - // Use try-catch to catch the try { // Recursive call to view.change() method - execute listener immediately. if ( this._ongoingChange ) { @@ -483,23 +482,7 @@ export default class View { return callbackResult; } catch ( err ) { - if ( err.is && err.is( 'CKEditorError' ) ) { - throw err; - } - - /** - * An unexpected error occurred inside the `view.change()` block. The `error.data.originalError` property - * shows the original error properties. - * - * @error view-change-unexpected-error - */ - throw new CKEditorError( 'view-change-unexpected-error', this, { - originalError: { - message: err.message, - stack: err.stack, - name: err.name - } - } ); + CKEditorError.rethrowUnexpectedError( err, this ); } } diff --git a/tests/model/model.js b/tests/model/model.js index 1fd6d0be5..fe657c4f4 100644 --- a/tests/model/model.js +++ b/tests/model/model.js @@ -332,7 +332,7 @@ describe( 'Model', () => { model.change( () => { throw error; } ); - }, /model-change-unexpected-error/, model, { + }, /unexpected-error/, model, { originalError: { message: 'foo', stack: 'bar', @@ -342,11 +342,9 @@ describe( 'Model', () => { } ); it( 'should throw the original CKEditorError error if it was thrown inside the `change()` block', () => { - const err = new CKEditorError( 'foo', null, { foo: 1 } ); - expectToThrowCKEditorError( () => { model.change( () => { - throw err; + throw new CKEditorError( 'foo', null, { foo: 1 } ); } ); }, /foo/, null, { foo: 1 } ); } ); @@ -359,7 +357,7 @@ describe( 'Model', () => { model.enqueueChange( () => { throw error; } ); - }, /model-enqueueChange-unexpected-error/, model, { + }, /unexpected-error/, model, { originalError: { message: 'foo', stack: 'bar', diff --git a/tests/view/view/view.js b/tests/view/view/view.js index 95fb83fb6..aa5a2c3f4 100644 --- a/tests/view/view/view.js +++ b/tests/view/view/view.js @@ -812,7 +812,7 @@ describe( 'view', () => { view.change( () => { throw error; } ); - }, /view-change-unexpected-error/, view, { + }, /unexpected-error/, view, { originalError: { message: 'foo', stack: 'bar', @@ -821,10 +821,7 @@ describe( 'view', () => { } ); } ); - it( 'should re-throw custom CKEditorError errors', () => { - const error = new TypeError( 'foo' ); - error.stack = 'bar'; - + it( 'should rethrow custom CKEditorError errors', () => { expectToThrowCKEditorError( () => { view.change( () => { throw new CKEditorError( 'foo', view );