From bd8a1165fb201feb4042a5ef1743f38dd711a57b Mon Sep 17 00:00:00 2001 From: DawidKossowskii Date: Tue, 5 Mar 2024 10:51:35 +0100 Subject: [PATCH 1/4] Fixed watchdog crash after immediately refreshing application. --- .../ckeditor5-watchdog/src/editorwatchdog.ts | 26 ++++++++++++- .../tests/editorwatchdog.js | 37 +++++++++++++++++++ 2 files changed, 61 insertions(+), 2 deletions(-) diff --git a/packages/ckeditor5-watchdog/src/editorwatchdog.ts b/packages/ckeditor5-watchdog/src/editorwatchdog.ts index 6c509d9598a..c95d7ee494f 100644 --- a/packages/ckeditor5-watchdog/src/editorwatchdog.ts +++ b/packages/ckeditor5-watchdog/src/editorwatchdog.ts @@ -38,6 +38,14 @@ export default class EditorWatchdog extends Wat */ private _editor: TEditor | null = null; + /** + * Representing the processing of the editor (creation or destruction). + * + * It is used to prevent the initialization of the editor if the previous instance has not been destroyed yet, + * and conversely, to prevent the destruction of the editor if it has not been initialized. + */ + private _editorProcessing: Promise | null = null; + /** * Throttled save method. The `save()` method is called the specified `saveInterval` after `throttledSave()` is called, * unless a new action happens in the meantime. @@ -249,7 +257,10 @@ export default class EditorWatchdog extends Wat config: EditorConfig = this._config!, context?: Context ): Promise { - return Promise.resolve() + const currentProcessing = this._editorProcessing; + + this._editorProcessing = Promise.resolve() + .then( () => currentProcessing ) .then( () => { super._startErrorHandling(); @@ -282,7 +293,11 @@ export default class EditorWatchdog extends Wat this.state = 'ready'; this._fire( 'stateChange' ); + } ).finally( () => { + this._editorProcessing = null; } ); + + return this._editorProcessing; } /** @@ -291,7 +306,10 @@ export default class EditorWatchdog extends Wat * It also sets the state to `destroyed`. */ public override destroy(): Promise { - return Promise.resolve() + const currentProcessing = this._editorProcessing; + + this._editorProcessing = Promise.resolve() + .then( () => currentProcessing ) .then( () => { this.state = 'destroyed'; this._fire( 'stateChange' ); @@ -299,7 +317,11 @@ export default class EditorWatchdog extends Wat super.destroy(); return this._destroy(); + } ).finally( () => { + this._editorProcessing = null; } ); + + return this._editorProcessing; } private _destroy(): Promise { diff --git a/packages/ckeditor5-watchdog/tests/editorwatchdog.js b/packages/ckeditor5-watchdog/tests/editorwatchdog.js index 30d89523202..a29c71f8ac1 100644 --- a/packages/ckeditor5-watchdog/tests/editorwatchdog.js +++ b/packages/ckeditor5-watchdog/tests/editorwatchdog.js @@ -211,6 +211,28 @@ describe( 'EditorWatchdog', () => { await watchdog.destroy(); } ); + + it( 'should create an editor instance after destroying the previous one', async () => { + const watchdog = new EditorWatchdog( ClassicTestEditor ); + + await watchdog.create( element, { + initialData: '

foo

', + plugins: [ Paragraph ] + } ); + + // Do not wait for the destruction process to finish. + watchdog.destroy(); + + await watchdog.create( element, { + initialData: '

foo

', + plugins: [ Paragraph ] + } ); + + expect( watchdog.editor ).to.not.be.null; + expect( watchdog.state ).to.equal( 'ready' ); + + await watchdog.destroy(); + } ); } ); describe( 'editor', () => { @@ -1247,6 +1269,21 @@ describe( 'EditorWatchdog', () => { sinon.assert.calledTwice( spy ); } ); + + it( 'should destroy the editor after finishing initialization', async () => { + const watchdog = new EditorWatchdog( ClassicTestEditor ); + + // Do not wait for the creation process to finish. + watchdog.create( element, { + initialData: '

foo

', + plugins: [ Paragraph ] + } ); + + await watchdog.destroy(); + + expect( watchdog.editor ).to.equal( null ); + expect( watchdog.state ).to.equal( 'destroyed' ); + } ); } ); describe( 'crashes', () => { From 45a1780047a0e8a86d334f2cd6f0096e35b864a3 Mon Sep 17 00:00:00 2001 From: Aleksander Nowodzinski Date: Thu, 14 Mar 2024 16:07:25 +0100 Subject: [PATCH 2/4] Code refactoring. --- .../ckeditor5-watchdog/src/editorwatchdog.ts | 22 +++++++------------ .../tests/editorwatchdog.js | 4 ++-- 2 files changed, 10 insertions(+), 16 deletions(-) diff --git a/packages/ckeditor5-watchdog/src/editorwatchdog.ts b/packages/ckeditor5-watchdog/src/editorwatchdog.ts index c95d7ee494f..0919c38968c 100644 --- a/packages/ckeditor5-watchdog/src/editorwatchdog.ts +++ b/packages/ckeditor5-watchdog/src/editorwatchdog.ts @@ -39,12 +39,12 @@ export default class EditorWatchdog extends Wat private _editor: TEditor | null = null; /** - * Representing the processing of the editor (creation or destruction). + * A promise associated with the life cycle of the editor (creation or destruction processes). * * It is used to prevent the initialization of the editor if the previous instance has not been destroyed yet, * and conversely, to prevent the destruction of the editor if it has not been initialized. */ - private _editorProcessing: Promise | null = null; + private _lifecyclePromise: Promise | null = null; /** * Throttled save method. The `save()` method is called the specified `saveInterval` after `throttledSave()` is called, @@ -257,10 +257,7 @@ export default class EditorWatchdog extends Wat config: EditorConfig = this._config!, context?: Context ): Promise { - const currentProcessing = this._editorProcessing; - - this._editorProcessing = Promise.resolve() - .then( () => currentProcessing ) + this._lifecyclePromise = Promise.resolve( this._lifecyclePromise ) .then( () => { super._startErrorHandling(); @@ -294,10 +291,10 @@ export default class EditorWatchdog extends Wat this.state = 'ready'; this._fire( 'stateChange' ); } ).finally( () => { - this._editorProcessing = null; + this._lifecyclePromise = null; } ); - return this._editorProcessing; + return this._lifecyclePromise; } /** @@ -306,10 +303,7 @@ export default class EditorWatchdog extends Wat * It also sets the state to `destroyed`. */ public override destroy(): Promise { - const currentProcessing = this._editorProcessing; - - this._editorProcessing = Promise.resolve() - .then( () => currentProcessing ) + this._lifecyclePromise = Promise.resolve( this._lifecyclePromise ) .then( () => { this.state = 'destroyed'; this._fire( 'stateChange' ); @@ -318,10 +312,10 @@ export default class EditorWatchdog extends Wat return this._destroy(); } ).finally( () => { - this._editorProcessing = null; + this._lifecyclePromise = null; } ); - return this._editorProcessing; + return this._lifecyclePromise; } private _destroy(): Promise { diff --git a/packages/ckeditor5-watchdog/tests/editorwatchdog.js b/packages/ckeditor5-watchdog/tests/editorwatchdog.js index a29c71f8ac1..c7bba59d7f4 100644 --- a/packages/ckeditor5-watchdog/tests/editorwatchdog.js +++ b/packages/ckeditor5-watchdog/tests/editorwatchdog.js @@ -212,7 +212,7 @@ describe( 'EditorWatchdog', () => { await watchdog.destroy(); } ); - it( 'should create an editor instance after destroying the previous one', async () => { + it( 'should create an editor instance after the ongoing destruction process has been finished', async () => { const watchdog = new EditorWatchdog( ClassicTestEditor ); await watchdog.create( element, { @@ -1270,7 +1270,7 @@ describe( 'EditorWatchdog', () => { sinon.assert.calledTwice( spy ); } ); - it( 'should destroy the editor after finishing initialization', async () => { + it( 'should destroy the editor after finishing the ongoing creation process', async () => { const watchdog = new EditorWatchdog( ClassicTestEditor ); // Do not wait for the creation process to finish. From 045a8bab1095f00995795f33efc91ac30959b027 Mon Sep 17 00:00:00 2001 From: DawidKossowskii Date: Mon, 18 Mar 2024 15:50:03 +0100 Subject: [PATCH 3/4] Refactoring the editor watchdog functions to async/await syntax. --- .../ckeditor5-watchdog/src/editorwatchdog.ts | 90 +++++++++++-------- .../tests/editorwatchdog.js | 18 ++-- 2 files changed, 63 insertions(+), 45 deletions(-) diff --git a/packages/ckeditor5-watchdog/src/editorwatchdog.ts b/packages/ckeditor5-watchdog/src/editorwatchdog.ts index 0919c38968c..8d97bc2d15a 100644 --- a/packages/ckeditor5-watchdog/src/editorwatchdog.ts +++ b/packages/ckeditor5-watchdog/src/editorwatchdog.ts @@ -252,49 +252,60 @@ export default class EditorWatchdog extends Wat * @param config The editor configuration. * @param context A context for the editor. */ - public create( + public async create( elementOrData: HTMLElement | string | Record | Record = this._elementOrData!, config: EditorConfig = this._config!, context?: Context ): Promise { - this._lifecyclePromise = Promise.resolve( this._lifecyclePromise ) - .then( () => { - super._startErrorHandling(); + super._startErrorHandling(); - this._elementOrData = elementOrData; + this._lifecyclePromise = ( async () => { + await this._lifecyclePromise; + await this._createEditor( elementOrData, config, context ); - // Use document data in the first parameter of the editor `.create()` call only if it was used like this originally. - // Use document data if a string or object with strings was passed. - this._initUsingData = typeof elementOrData == 'string' || - ( Object.keys( elementOrData ).length > 0 && typeof Object.values( elementOrData )[ 0 ] == 'string' ); + this._lifecyclePromise = null; + } )(); - // Clone configuration because it might be shared within multiple watchdog instances. Otherwise, - // when an error occurs in one of these editors, the watchdog will restart all of them. - this._config = this._cloneEditorConfiguration( config ) || {}; + return this._lifecyclePromise; + } - this._config!.context = context; + private async _createEditor( + elementOrData: HTMLElement | string | Record | Record, + config: EditorConfig = this._config!, + context?: Context + ): Promise { + this._elementOrData = elementOrData; - return this._creator( elementOrData, this._config! ); - } ) - .then( editor => { - this._editor = editor; + // Use document data in the first parameter of the editor `.create()` call only if it was used like this originally. + // Use document data if a string or object with strings was passed. + this._initUsingData = typeof elementOrData == 'string' || + ( Object.keys( elementOrData ).length > 0 && typeof Object.values( elementOrData )[ 0 ] == 'string' ); - editor.model.document.on( 'change:data', this._throttledSave ); + // Clone configuration because it might be shared within multiple watchdog instances. Otherwise, + // when an error occurs in one of these editors, the watchdog will restart all of them. + this._config = this._cloneEditorConfiguration( config ) || {}; - this._lastDocumentVersion = editor.model.document.version; - this._data = this._getData(); + this._config!.context = context; - if ( !this._initUsingData ) { - this._editables = this._getEditables(); - } + const editor = await this._creator( elementOrData, this._config! ); - this.state = 'ready'; - this._fire( 'stateChange' ); - } ).finally( () => { - this._lifecyclePromise = null; - } ); + this._setupEditor( editor ); + } - return this._lifecyclePromise; + private _setupEditor( editor ): void { + this._editor = editor; + + editor.model.document.on( 'change:data', this._throttledSave ); + + this._lastDocumentVersion = editor.model.document.version; + this._data = this._getData(); + + if ( !this._initUsingData ) { + this._editables = this._getEditables(); + } + + this.state = 'ready'; + this._fire( 'stateChange' ); } /** @@ -302,18 +313,19 @@ export default class EditorWatchdog extends Wat * registered in {@link #setDestructor `setDestructor()`} and uses it to destroy the editor instance. * It also sets the state to `destroyed`. */ - public override destroy(): Promise { - this._lifecyclePromise = Promise.resolve( this._lifecyclePromise ) - .then( () => { - this.state = 'destroyed'; - this._fire( 'stateChange' ); + public override async destroy(): Promise { + this._lifecyclePromise = ( async () => { + await this._lifecyclePromise; - super.destroy(); + this.state = 'destroyed'; + this._fire( 'stateChange' ); - return this._destroy(); - } ).finally( () => { - this._lifecyclePromise = null; - } ); + super.destroy(); + + this._lifecyclePromise = null; + + return this._destroy(); + } )(); return this._lifecyclePromise; } diff --git a/packages/ckeditor5-watchdog/tests/editorwatchdog.js b/packages/ckeditor5-watchdog/tests/editorwatchdog.js index c7bba59d7f4..372034a1775 100644 --- a/packages/ckeditor5-watchdog/tests/editorwatchdog.js +++ b/packages/ckeditor5-watchdog/tests/editorwatchdog.js @@ -215,13 +215,19 @@ describe( 'EditorWatchdog', () => { it( 'should create an editor instance after the ongoing destruction process has been finished', async () => { const watchdog = new EditorWatchdog( ClassicTestEditor ); - await watchdog.create( element, { - initialData: '

foo

', - plugins: [ Paragraph ] - } ); + // It simulates the process of creating a new instance of the editor and immediately destroying it. + const simulateRefreshApp = async () => { + watchdog.create( element, { + initialData: '

foo

', + plugins: [ Paragraph ] + } ); + + watchdog.destroy(); + }; - // Do not wait for the destruction process to finish. - watchdog.destroy(); + // We do not wait for the completion of this process, which simulates the real case when the app is immediately + // restarted and the initial process of the new app is called without waiting for the previous process to finish. + simulateRefreshApp(); await watchdog.create( element, { initialData: '

foo

', From ff9b0fd871cc042a880227fa465c4cec5a98aca5 Mon Sep 17 00:00:00 2001 From: DawidKossowskii Date: Tue, 19 Mar 2024 11:36:15 +0100 Subject: [PATCH 4/4] Restoring the Promise approach in EditorWatchdog. --- .../ckeditor5-watchdog/src/editorwatchdog.ts | 90 ++++++++----------- 1 file changed, 39 insertions(+), 51 deletions(-) diff --git a/packages/ckeditor5-watchdog/src/editorwatchdog.ts b/packages/ckeditor5-watchdog/src/editorwatchdog.ts index 8d97bc2d15a..0919c38968c 100644 --- a/packages/ckeditor5-watchdog/src/editorwatchdog.ts +++ b/packages/ckeditor5-watchdog/src/editorwatchdog.ts @@ -252,60 +252,49 @@ export default class EditorWatchdog extends Wat * @param config The editor configuration. * @param context A context for the editor. */ - public async create( + public create( elementOrData: HTMLElement | string | Record | Record = this._elementOrData!, config: EditorConfig = this._config!, context?: Context ): Promise { - super._startErrorHandling(); - - this._lifecyclePromise = ( async () => { - await this._lifecyclePromise; - await this._createEditor( elementOrData, config, context ); - - this._lifecyclePromise = null; - } )(); - - return this._lifecyclePromise; - } - - private async _createEditor( - elementOrData: HTMLElement | string | Record | Record, - config: EditorConfig = this._config!, - context?: Context - ): Promise { - this._elementOrData = elementOrData; + this._lifecyclePromise = Promise.resolve( this._lifecyclePromise ) + .then( () => { + super._startErrorHandling(); - // Use document data in the first parameter of the editor `.create()` call only if it was used like this originally. - // Use document data if a string or object with strings was passed. - this._initUsingData = typeof elementOrData == 'string' || - ( Object.keys( elementOrData ).length > 0 && typeof Object.values( elementOrData )[ 0 ] == 'string' ); + this._elementOrData = elementOrData; - // Clone configuration because it might be shared within multiple watchdog instances. Otherwise, - // when an error occurs in one of these editors, the watchdog will restart all of them. - this._config = this._cloneEditorConfiguration( config ) || {}; + // Use document data in the first parameter of the editor `.create()` call only if it was used like this originally. + // Use document data if a string or object with strings was passed. + this._initUsingData = typeof elementOrData == 'string' || + ( Object.keys( elementOrData ).length > 0 && typeof Object.values( elementOrData )[ 0 ] == 'string' ); - this._config!.context = context; + // Clone configuration because it might be shared within multiple watchdog instances. Otherwise, + // when an error occurs in one of these editors, the watchdog will restart all of them. + this._config = this._cloneEditorConfiguration( config ) || {}; - const editor = await this._creator( elementOrData, this._config! ); + this._config!.context = context; - this._setupEditor( editor ); - } + return this._creator( elementOrData, this._config! ); + } ) + .then( editor => { + this._editor = editor; - private _setupEditor( editor ): void { - this._editor = editor; + editor.model.document.on( 'change:data', this._throttledSave ); - editor.model.document.on( 'change:data', this._throttledSave ); + this._lastDocumentVersion = editor.model.document.version; + this._data = this._getData(); - this._lastDocumentVersion = editor.model.document.version; - this._data = this._getData(); + if ( !this._initUsingData ) { + this._editables = this._getEditables(); + } - if ( !this._initUsingData ) { - this._editables = this._getEditables(); - } + this.state = 'ready'; + this._fire( 'stateChange' ); + } ).finally( () => { + this._lifecyclePromise = null; + } ); - this.state = 'ready'; - this._fire( 'stateChange' ); + return this._lifecyclePromise; } /** @@ -313,19 +302,18 @@ export default class EditorWatchdog extends Wat * registered in {@link #setDestructor `setDestructor()`} and uses it to destroy the editor instance. * It also sets the state to `destroyed`. */ - public override async destroy(): Promise { - this._lifecyclePromise = ( async () => { - await this._lifecyclePromise; - - this.state = 'destroyed'; - this._fire( 'stateChange' ); - - super.destroy(); + public override destroy(): Promise { + this._lifecyclePromise = Promise.resolve( this._lifecyclePromise ) + .then( () => { + this.state = 'destroyed'; + this._fire( 'stateChange' ); - this._lifecyclePromise = null; + super.destroy(); - return this._destroy(); - } )(); + return this._destroy(); + } ).finally( () => { + this._lifecyclePromise = null; + } ); return this._lifecyclePromise; }