From a177cb46065ffb19d779e799408e95dcfaed4588 Mon Sep 17 00:00:00 2001 From: Dawid Kossowski <20561161+DawidKossowski@users.noreply.github.com> Date: Thu, 20 Jul 2023 16:03:34 +0200 Subject: [PATCH] Improving watchdog performance (#14563) Fix (watchdog): Improved the Watchdog save mechanism performance to prevent editor unresponsiveness ("lags") while editing the document. Closes #13098. --- packages/ckeditor5-watchdog/package.json | 1 + .../ckeditor5-watchdog/src/augmentation.ts | 18 ++ .../ckeditor5-watchdog/src/editorwatchdog.ts | 174 ++++++++++++++++-- packages/ckeditor5-watchdog/src/index.ts | 2 + .../tests/editorwatchdog.js | 142 +++++++++++++- .../tests/manual/watchdog-multi-root.html | 37 ++++ .../tests/manual/watchdog-multi-root.js | 93 ++++++++++ .../tests/manual/watchdog-multi-root.md | 9 + 8 files changed, 453 insertions(+), 23 deletions(-) create mode 100644 packages/ckeditor5-watchdog/src/augmentation.ts create mode 100644 packages/ckeditor5-watchdog/tests/manual/watchdog-multi-root.html create mode 100644 packages/ckeditor5-watchdog/tests/manual/watchdog-multi-root.js create mode 100644 packages/ckeditor5-watchdog/tests/manual/watchdog-multi-root.md diff --git a/packages/ckeditor5-watchdog/package.json b/packages/ckeditor5-watchdog/package.json index 42a05028e32..17c155505c3 100644 --- a/packages/ckeditor5-watchdog/package.json +++ b/packages/ckeditor5-watchdog/package.json @@ -16,6 +16,7 @@ "devDependencies": { "@ckeditor/ckeditor5-core": "38.1.1", "@ckeditor/ckeditor5-editor-classic": "38.1.1", + "@ckeditor/ckeditor5-editor-multi-root": "38.1.1", "@ckeditor/ckeditor5-paragraph": "38.1.1", "@ckeditor/ckeditor5-utils": "38.1.1", "ckeditor5": "38.1.1", diff --git a/packages/ckeditor5-watchdog/src/augmentation.ts b/packages/ckeditor5-watchdog/src/augmentation.ts new file mode 100644 index 00000000000..0aa3ade4c57 --- /dev/null +++ b/packages/ckeditor5-watchdog/src/augmentation.ts @@ -0,0 +1,18 @@ +/** + * @license Copyright (c) 2003-2023, CKSource Holding sp. z o.o. All rights reserved. + * For licensing, see LICENSE.md or https://ckeditor.com/legal/ckeditor-oss-license + */ + +import type { EditorData } from './editorwatchdog'; + +declare module '@ckeditor/ckeditor5-core' { + interface EditorConfig { + + /** + * The temporary property that is used for passing data to the plugin which restores the editor state. + * + * @internal + */ + _watchdogInitialData?: EditorData | null; + } +} diff --git a/packages/ckeditor5-watchdog/src/editorwatchdog.ts b/packages/ckeditor5-watchdog/src/editorwatchdog.ts index 0c049748726..47853813e37 100644 --- a/packages/ckeditor5-watchdog/src/editorwatchdog.ts +++ b/packages/ckeditor5-watchdog/src/editorwatchdog.ts @@ -13,17 +13,16 @@ import type { CKEditorError } from 'ckeditor5/src/utils'; // eslint-disable-next-line ckeditor5-rules/no-cross-package-imports -import type { - Editor, - EditorConfig, - Context -} from 'ckeditor5/src/core'; +import type { Editor, EditorConfig, Context, EditorReadyEvent } from 'ckeditor5/src/core'; import areConnectedThroughProperties from './utils/areconnectedthroughproperties'; import Watchdog, { type WatchdogConfig } from './watchdog'; import { throttle, cloneDeepWith, isElement, type DebouncedFunc } from 'lodash-es'; +// eslint-disable-next-line ckeditor5-rules/no-cross-package-imports +import type { Node, Text, Element, Writer } from 'ckeditor5/src/engine'; + /** * A watchdog for CKEditor 5 editors. * @@ -45,7 +44,7 @@ export default class EditorWatchdog extends Wat /** * The latest saved editor data represented as a root name -> root data object. */ - private _data?: Record; + private _data?: EditorData; /** * The last document version. @@ -165,12 +164,24 @@ export default class EditorWatchdog extends Wat console.error( 'An error happened during the editor destroying.', err ); } ) .then( () => { + const existingRoots = Object.keys( this._data!.roots ).reduce( ( acc, rootName ) => { + acc[ rootName ] = ''; + + return acc; + }, {} as Record ); + + const updatedConfig = { + ...this._config, + extraPlugins: this._config!.extraPlugins || [], + _watchdogInitialData: this._data + }; + + updatedConfig.extraPlugins!.push( EditorWatchdogInitPlugin as any ); + if ( typeof this._elementOrData === 'string' ) { - return this.create( this._data, this._config, this._config!.context ); + return this.create( existingRoots, updatedConfig, updatedConfig!.context ); } else { - const updatedConfig = Object.assign( {}, this._config, { - initialData: this._data - } ); + updatedConfig.initialData = existingRoots; return this.create( this._elementOrData, updatedConfig, updatedConfig.context ); } @@ -284,13 +295,35 @@ export default class EditorWatchdog extends Wat } /** - * Returns the editor data. + * Gets all data that is required to reinitialize editor instance. */ - private _getData(): Record { - const data: Record = {}; + private _getData(): EditorData { + const editor = this.editor!; + const rootNames = editor.model.document.getRootNames(); + const data: EditorData = { + roots: {}, + markers: {} + }; + + rootNames.forEach( rootName => { + const root = editor.model.document.getRoot( rootName )!; + + data.roots[ rootName ] = { + content: JSON.stringify( Array.from( root.getChildren() ) ), + attributes: JSON.stringify( Array.from( root.getAttributes() ) ) + }; + } ); - for ( const rootName of this._editor!.model.document.getRootNames() ) { - data[ rootName ] = this._editor!.data.get( { rootName } ); + for ( const marker of editor.model.markers ) { + if ( !marker._affectsData ) { + continue; + } + + data.markers[ marker.name ] = { + rangeJSON: marker.getRange().toJSON() as any, + usingOperation: marker._managedUsingOperations, + affectsData: marker._affectsData + }; } return data; @@ -323,6 +356,117 @@ export default class EditorWatchdog extends Wat } } +/** + * Internal plugin that is used to stop the default editor initialization and restoring the editor state + * based on the `editor.config._watchdogInitialData` data. + */ +class EditorWatchdogInitPlugin { + public editor: Editor; + private _data: EditorData; + + constructor( editor: Editor ) { + this.editor = editor; + + this._data = editor.config.get( '_watchdogInitialData' )!; + } + + /** + * @inheritDoc + */ + public init(): void { + // Stops the default editor initialization and use the saved data to restore the editor state. + // Some of data could not be initialize as a config properties. It is important to keep the data + // in the same form as it was before the restarting. + this.editor.data.on( 'init', evt => { + evt.stop(); + + this.editor.model.enqueueChange( { isUndoable: true }, writer => { + this._restoreEditorData( writer ); + } ); + + this.editor.data.fire( 'ready' ); + + // Keep priority `'high' - 1` to be sure that RTC initialization will be first. + }, { priority: 1000 - 1 } ); + } + + /** + * Creates a model node (element or text) based on provided JSON. + */ + private _createNode( writer: Writer, jsonNode: any ): Text | Element { + if ( 'name' in jsonNode ) { + // If child has name property, it is an Element. + const element = writer.createElement( jsonNode.name, jsonNode.attributes ); + + if ( jsonNode.children ) { + for ( const child of jsonNode.children ) { + element._appendChild( this._createNode( writer, child ) ); + } + } + + return element; + } else { + // Otherwise, it is a Text node. + return writer.createText( jsonNode.data, jsonNode.attributes ); + } + } + + /** + * Restores the editor by setting the document data, roots attributes and markers. + */ + private _restoreEditorData( writer: Writer ): void { + const editor = this.editor!; + + Object.entries( this._data!.roots ).forEach( ( [ rootName, { content, attributes } ] ) => { + const parsedNodes: Array = JSON.parse( content ); + const parsedAttributes: Array<[ string, unknown ]> = JSON.parse( attributes ); + + const rootElement = editor.model.document.getRoot( rootName )!; + + for ( const [ key, value ] of parsedAttributes ) { + writer.setAttribute( key, value, rootElement ); + } + + for ( const child of parsedNodes ) { + const node = this._createNode( writer, child ); + + writer.insert( node, rootElement, 'end' ); + } + } ); + + Object.entries( this._data!.markers ).forEach( ( [ markerName, markerOptions ] ) => { + const { document } = editor.model; + const { + rangeJSON: { start, end }, + ...options + } = markerOptions; + + const root = document.getRoot( start.root )!; + const startPosition = writer.createPositionFromPath( root, start.path, start.stickiness ); + const endPosition = writer.createPositionFromPath( root, end.path, end.stickiness ); + + const range = writer.createRange( startPosition, endPosition ); + + writer.addMarker( markerName, { + range, + ...options + } ); + } ); + } +} + +export type EditorData = { + roots: Record; + markers: Record; +}; + /** * Fired after the watchdog restarts the error in case of a crash. * diff --git a/packages/ckeditor5-watchdog/src/index.ts b/packages/ckeditor5-watchdog/src/index.ts index 65c9f8cb9d7..f2786dcea30 100644 --- a/packages/ckeditor5-watchdog/src/index.ts +++ b/packages/ckeditor5-watchdog/src/index.ts @@ -10,3 +10,5 @@ export { default as ContextWatchdog } from './contextwatchdog'; export { default as EditorWatchdog } from './editorwatchdog'; export { default as Watchdog } from './watchdog'; + +import './augmentation'; diff --git a/packages/ckeditor5-watchdog/tests/editorwatchdog.js b/packages/ckeditor5-watchdog/tests/editorwatchdog.js index 7b6c7b08aab..3eb5d9d977a 100644 --- a/packages/ckeditor5-watchdog/tests/editorwatchdog.js +++ b/packages/ckeditor5-watchdog/tests/editorwatchdog.js @@ -91,6 +91,93 @@ describe( 'EditorWatchdog', () => { await watchdog.destroy(); } ); + + it( 'should support root attributes', async () => { + const watchdog = new EditorWatchdog(); + + watchdog.setCreator( ( data, config ) => ClassicTestEditor.create( data, config ) ); + + // sinon.stub( window, 'onerror' ).value( undefined ); and similar do not work. + const originalErrorHandler = window.onerror; + const windowErrorSpy = sinon.spy(); + window.onerror = windowErrorSpy; + + await watchdog.create( '

foo

', { plugins: [ Paragraph ] } ); + + const root = watchdog.editor.model.document.getRoot(); + + watchdog.editor.model.change( writer => { + writer.setAttribute( 'test', 1, root ); + } ); + + expect( root.getAttribute( 'test' ) ).to.equal( 1 ); + + await new Promise( res => { + setTimeout( () => throwCKEditorError( 'foo', watchdog.editor ) ); + + watchdog.on( 'restart', () => { + window.onerror = originalErrorHandler; + res(); + } ); + } ); + + expect( root.getAttribute( 'test' ) ).to.equal( 1 ); + + await watchdog.destroy(); + } ); + + it( 'should support markers', async () => { + const watchdog = new EditorWatchdog(); + + watchdog.setCreator( ( data, config ) => ClassicTestEditor.create( data, config ) ); + + // sinon.stub( window, 'onerror' ).value( undefined ); and similar do not work. + const originalErrorHandler = window.onerror; + const windowErrorSpy = sinon.spy(); + window.onerror = windowErrorSpy; + + await watchdog.create( '

foo

', { plugins: [ Paragraph ] } ); + + const root = watchdog.editor.model.document.getRoot(); + + watchdog.editor.model.change( writer => { + writer.addMarker( 'first', { + usingOperation: false, + affectsData: false, + range: writer.createRange( + writer.createPositionAt( root, [ 0 ] ), + writer.createPositionAt( root, [ 1 ] ) + ) + } ); + + writer.addMarker( 'second', { + usingOperation: true, + affectsData: true, + range: writer.createRange( + writer.createPositionAt( root, [ 0 ] ), + writer.createPositionAt( root, [ 1 ] ) + ) + } ); + } ); + + const marker = watchdog.editor.model.markers.get( 'second' ); + + watchdog._save(); + + await new Promise( res => { + setTimeout( () => throwCKEditorError( 'foo', watchdog.editor ) ); + + watchdog.on( 'restart', () => { + window.onerror = originalErrorHandler; + res(); + } ); + } ); + + expect( watchdog.editor.model.markers.get( 'first' ) ).to.be.null; + expect( watchdog.editor.model.markers.get( 'second' ).name ).to.equal( marker.name ); + + await watchdog.destroy(); + } ); } ); describe( 'editor', () => { @@ -547,7 +634,7 @@ describe( 'EditorWatchdog', () => { watchdog.on( 'restart', () => { window.onerror = originalErrorHandler; - expect( watchdog.editor.getData() ).to.equal( '

foo

bar

' ); + expect( watchdog.editor.getData() ).to.equal( '

foo

bar' ); watchdog.destroy().then( res ); } ); @@ -610,8 +697,9 @@ describe( 'EditorWatchdog', () => { } ); const editorGetDataError = new Error( 'Some error' ); - const getDataStub = sinon.stub( watchdog.editor.data, 'get' ) - .throwsException( editorGetDataError ); + const getDataStub = sinon.stub( watchdog, '_getData' ) + .onCall( 0 ).throwsException( editorGetDataError ) + .onCall( 1 ).returns( {} ); // Keep the reference to cleanly destroy it at in the end, as during the TC it // throws an exception during destruction. const firstEditor = watchdog.editor; @@ -640,11 +728,6 @@ describe( 'EditorWatchdog', () => { 'An error happened during restoring editor data. Editor will be restored from the previously saved data.' ); - sinon.assert.calledWith( - console.error, - 'An error happened during the editor destroying.' - ); - await watchdog.destroy(); getDataStub.restore(); @@ -683,6 +766,49 @@ describe( 'EditorWatchdog', () => { } ); } ); } ); + + it( 'should handle the error when the editor destroying failed', async () => { + const watchdog = new EditorWatchdog( ClassicTestEditor ); + + // sinon.stub( window, 'onerror' ).value( undefined ); and similar do not work. + const originalErrorHandler = window.onerror; + window.onerror = undefined; + + sinon.stub( console, 'error' ); + + await watchdog.create( element, { + initialData: '

foo

', + plugins: [ Paragraph ] + } ); + + const editorGetDataError = new Error( 'Some error' ); + const destroyStub = sinon.stub( watchdog, '_destroy' ) + .throwsException( editorGetDataError ); + + // Keep the reference to cleanly destroy it at in the end, as during the TC it + // throws an exception during destruction. + const firstEditor = watchdog.editor; + + await new Promise( res => { + setTimeout( () => throwCKEditorError( 'foo', watchdog.editor ) ); + + watchdog.on( 'restart', async () => { + window.onerror = originalErrorHandler; + + sinon.assert.calledWith( + console.error, + 'An error happened during the editor destroying.' + ); + + destroyStub.restore(); + + await watchdog.destroy(); + await firstEditor.destroy(); + + res(); + } ); + } ); + } ); } ); describe( 'async error handling', () => { diff --git a/packages/ckeditor5-watchdog/tests/manual/watchdog-multi-root.html b/packages/ckeditor5-watchdog/tests/manual/watchdog-multi-root.html new file mode 100644 index 00000000000..feeee9fd418 --- /dev/null +++ b/packages/ckeditor5-watchdog/tests/manual/watchdog-multi-root.html @@ -0,0 +1,37 @@ + + +
Editor state:
+ +
+ +
+
+
+ +
+ +
+
+

Destination of the Month

+ +

Valletta

+ +

The capital city of Malta is the top destination this summer. It’s home to cutting-edge contemporary architecture, baroque masterpieces, delicious local cuisine, and at least 8 months of sun. It’s also a top destination for filmmakers, so you can take a tour through locations familiar to you from Game of Thrones, Gladiator, Troy, and many more.

+
+
+
+
+ + diff --git a/packages/ckeditor5-watchdog/tests/manual/watchdog-multi-root.js b/packages/ckeditor5-watchdog/tests/manual/watchdog-multi-root.js new file mode 100644 index 00000000000..4016ef61b90 --- /dev/null +++ b/packages/ckeditor5-watchdog/tests/manual/watchdog-multi-root.js @@ -0,0 +1,93 @@ +/** + * @license Copyright (c) 2003-2023, CKSource Holding sp. z o.o. All rights reserved. + * For licensing, see LICENSE.md or https://ckeditor.com/legal/ckeditor-oss-license + */ + +/* globals document, console, window */ + +import ArticlePluginSet from '@ckeditor/ckeditor5-core/tests/_utils/articlepluginset'; +import { MultiRootEditor } from '@ckeditor/ckeditor5-editor-multi-root'; + +import EditorWatchdog from '../../src/editorwatchdog'; + +class TypingError { + constructor( editor ) { + this.editor = editor; + } + + init() { + const inputCommand = this.editor.commands.get( 'input' ); + + inputCommand.on( 'execute', ( evt, data ) => { + const commandArgs = data[ 0 ]; + + if ( commandArgs.text === '1' ) { + // Simulate error. + this.editor.foo.bar = 'bom'; + } + } ); + } +} + +const editorConfig = { + image: { toolbar: [ 'toggleImageCaption', 'imageTextAlternative' ] }, + plugins: [ + ArticlePluginSet, TypingError + ], + toolbar: [ 'heading', '|', 'bold', 'italic', 'link', 'bulletedList', 'numberedList', 'blockQuote', + 'insertTable', 'mediaEmbed', 'undo', 'redo' ], + table: { + contentToolbar: [ + 'tableColumn', + 'tableRow', + 'mergeTableCells' + ] + } +}; + +const watchdog = createWatchdog( document.getElementById( 'editor-state' ) ); + +Object.assign( window, { watchdog } ); + +document.getElementById( 'random-error' ).addEventListener( 'click', () => { + throw new Error( 'foo' ); +} ); + +function createWatchdog( stateElement ) { + const watchdog = new EditorWatchdog( MultiRootEditor ); + + watchdog + .create( + { + header: document.querySelector( '#header' ), + content: document.querySelector( '#content' ) + }, + editorConfig + ) + .then( () => { + const toolbarContainer = document.querySelector( '#toolbar' ); + toolbarContainer.appendChild( watchdog.editor.ui.view.toolbar.element ); + } ); + + watchdog.on( 'error', () => { + console.log( 'Editor crashed!' ); + } ); + + watchdog.on( 'restart', () => { + console.log( 'Editor restarted.' ); + } ); + + watchdog.on( 'stateChange', () => { + console.log( `Watchdog state changed to '${ watchdog.state }'` ); + + stateElement.innerText = watchdog.state; + + if ( watchdog.state === 'crashedPermanently' ) { + watchdog.editor.enableReadOnlyMode( 'manual-test' ); + } + } ); + + stateElement.innerText = watchdog.state; + + return watchdog; +} diff --git a/packages/ckeditor5-watchdog/tests/manual/watchdog-multi-root.md b/packages/ckeditor5-watchdog/tests/manual/watchdog-multi-root.md new file mode 100644 index 00000000000..1c59b0ae170 --- /dev/null +++ b/packages/ckeditor5-watchdog/tests/manual/watchdog-multi-root.md @@ -0,0 +1,9 @@ +**Important:** Be sure to run manual test with the `--debug false` flag. Otherwise errors won't be rethrown by the `CKEditorError.rethrowUnexpectedError()` method. + +1. Type `1` in the editor. Only the editor should crash and be restarted. The error should be logged in the console. + +1. Click `Simulate a random error`. Editor should not be restarted. + +1. Refresh page and quickly type `1` in the editor 4 times. After the last error the editor should be crashed and it should not restart. + +1. Refresh page and slowly (slower than 1 per 5 seconds) type `1` in the editor 4 times. After the last error the editor should be restarted and it should still work.