Merge pull request #1800 from ckeditor/t/ckeditor5/1304

Other: Added error handling to the common code execution paths. Part of ckeditor/ckeditor5#1304.

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,18 @@ 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 ) {
+			CKEditorError.rethrowUnexpectedError( err, this );
 		}
 	}
 
@@ -198,17 +203,21 @@ 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 ) {
+			CKEditorError.rethrowUnexpectedError( err, this );
 		}
 	}
 

src/view/view.js

@@ -457,29 +457,33 @@ export default class View {
 			);
 		}
 
-		// 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;
+		try {
+			// Recursive call to view.change() method - execute listener immediately.
+			if ( this._ongoingChange ) {
+				return callback( this._writer );
+			}
 
-		// 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 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.fire( 'render' );
+			return callbackResult;
+		} catch ( err ) {
+			CKEditorError.rethrowUnexpectedError( err, this );
 		}
-
-		return callbackResult;
 	}
 
 	/**

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,58 @@ 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;
+				} );
+			}, /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', () => {
+			expectToThrowCKEditorError( () => {
+				model.change( () => {
+					throw new CKEditorError( 'foo', null, { foo: 1 } );
+				} );
+			}, /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;
+				} );
+			}, /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()', () => {

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,31 @@ 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;
+				} );
+			}, /unexpected-error/, view, {
+				originalError: {
+					message: 'foo',
+					stack: 'bar',
+					name: 'TypeError'
+				}
+			} );
+		} );
+
+		it( 'should rethrow custom CKEditorError errors', () => {
+			expectToThrowCKEditorError( () => {
+				view.change( () => {
+					throw new CKEditorError( 'foo', view );
+				} );
+			}, /foo/, view );
+		} );
 	} );
 
 	describe( 'createPositionAt()', () => {