diff --git a/.changelog/20251125135803_ck_19407.md b/.changelog/20251125135803_ck_19407.md new file mode 100644 index 00000000000..15e5d9949f7 --- /dev/null +++ b/.changelog/20251125135803_ck_19407.md @@ -0,0 +1,9 @@ +--- +type: Fix +scope: + - ckeditor5-watchdog +closes: + - https://github.com/ckeditor/ckeditor5/issues/19407 +--- + +Improved the error detection mechanism in `EditorWatchdog`. It now better identifies which editor instance is responsible for an error, preventing unnecessary restarts of other editors that might share some configuration or plugins but were not affected by the error. diff --git a/packages/ckeditor5-watchdog/src/editorwatchdog.ts b/packages/ckeditor5-watchdog/src/editorwatchdog.ts index 8979d2fee89..33ff37509f1 100644 --- a/packages/ckeditor5-watchdog/src/editorwatchdog.ts +++ b/packages/ckeditor5-watchdog/src/editorwatchdog.ts @@ -9,6 +9,7 @@ import { throttle, cloneDeepWith, isElement, type DebouncedFunc } from 'es-toolkit/compat'; import { areConnectedThroughProperties } from './utils/areconnectedthroughproperties.js'; +import { getSubNodes } from './utils/getsubnodes.js'; import { Watchdog, type WatchdogConfig } from './watchdog.js'; import type { CKEditorError } from '@ckeditor/ckeditor5-utils'; import type { ModelNode, ModelText, ModelElement, ModelWriter } from '@ckeditor/ckeditor5-engine'; @@ -432,7 +433,51 @@ export class EditorWatchdog extends Watchdog { * @internal */ public _isErrorComingFromThisItem( error: CKEditorError ): boolean { - return areConnectedThroughProperties( this._editor, error.context, this._excludedProps ); + // If here are no matching properties, the error is definitely not coming from this editor (or any other editor at all). + if ( !this._editor || !areConnectedThroughProperties( this._editor, error.context, this._excludedProps ) ) { + return false; + } + + // ... however, even if there are some matching properties, we still cannot be 100% sure that the error is coming from this editor. + // + // For example, two different editors may share some plugins or other global objects in configuration. While these objects + // are connected through properties, the error may still come from a different editor instance. + // + // To be more certain, we can probe subnodes of the error context and see if they contain any editor-specific objects such as + // the model document, UI view, or editing view. If we find any of these objects, we can be more confident that the error is indeed + // coming from this editor, however, this is still not a 100% guarantee. The exception might happen within context plugins + // that have access to multiple editors. + const subNodes = getSubNodes( error.context, this._excludedProps ); + const editorSpecificEntries = getEditorSpecificEntries( this._editor ); + + let isAssociatedWithThisEditor = false; + let isAssociatedWithOtherEditor = false; + + for ( const node of subNodes ) { + for ( const instance of editorSpecificEntries ) { + if ( node && typeof node === 'object' && node instanceof instance.constructor ) { + if ( node === instance ) { + isAssociatedWithThisEditor = true; + } else { + isAssociatedWithOtherEditor = true; + } + } + } + } + + if ( isAssociatedWithThisEditor ) { + return true; + } + + if ( isAssociatedWithOtherEditor ) { + return false; + } + + // If no editor-specific objects were found, we cannot be sure whether the error is coming from this editor or not. + // The problem is, that some matching properties were found in the first step, so there is some connection + // between the editor and the error context. It's safer to restart all associated editors than risk not restarting + // an editor that is actually broken. + return true; } /** @@ -628,3 +673,28 @@ export type EditorWatchdogCreatorFunction = ( elementOrData: HTMLElement | string | Record | Record, config: EditorConfig ) => Promise; + +/** + * Returns editor-specific entries that can be used to identify whether an error context is connected to the editor. + */ +function getEditorSpecificEntries( editor: Editor ) { + return [ + editor.locale, + editor.accessibility, + editor.data, + editor.plugins, + editor.config, + editor.conversion, + editor.commands, + editor.plugins, + editor.keystrokes, + editor.model, + editor.model?.document, + editor.model?.schema, + editor.ui, + editor.ui?.view, + editor.editing, + editor.editing?.view, + editor.editing?.mapper + ].filter( Boolean ); +}; diff --git a/packages/ckeditor5-watchdog/tests/editorwatchdog.js b/packages/ckeditor5-watchdog/tests/editorwatchdog.js index 7822ba8a950..61538f4dd95 100644 --- a/packages/ckeditor5-watchdog/tests/editorwatchdog.js +++ b/packages/ckeditor5-watchdog/tests/editorwatchdog.js @@ -898,6 +898,113 @@ describe( 'EditorWatchdog', () => { } ); } ); + it( 'Watchdog should restart only the relevant editor when the error context contains both shared and editor-specific objects', + async () => { + const watchdog1 = new EditorWatchdog( ClassicTestEditor ); + const watchdog2 = new EditorWatchdog( ClassicTestEditor ); + const element2 = document.createElement( 'div' ); + document.body.appendChild( element2 ); + + // sinon.stub( window, 'onerror' ).value( undefined ); and similar do not work. + const originalErrorHandler = window.onerror; + window.onerror = undefined; + + const sharedObject = { foo: 'bar' }; + + await Promise.all( [ + watchdog1.create( element ), + watchdog2.create( element2 ) + ] ); + + // Attach shared object to both editors so areConnectedThroughProperties returns true for both. + watchdog1.editor.shared = sharedObject; + watchdog2.editor.shared = sharedObject; + + const context = { + shared: sharedObject, + // This makes it specific to watchdog1's editor + specific: watchdog1.editor.model.document + }; + + const watchdog1RestartSpy = sinon.spy(); + const watchdog2RestartSpy = sinon.spy(); + + watchdog1.on( 'restart', watchdog1RestartSpy ); + watchdog2.on( 'restart', watchdog2RestartSpy ); + + setTimeout( () => throwCKEditorError( 'foo', context ) ); + + await new Promise( res => { + watchdog1.on( 'restart', () => { + // Wait a bit to ensure watchdog2 does not restart + setTimeout( () => { + window.onerror = originalErrorHandler; + + sinon.assert.calledOnce( watchdog1RestartSpy ); + sinon.assert.notCalled( watchdog2RestartSpy ); + + Promise.all( [ watchdog1.destroy(), watchdog2.destroy() ] ).then( () => { + element2.remove(); + res(); + } ); + }, 60 ); + } ); + } ); + } + ); + + it( 'Watchdog should restart all editors connected to the error context if no editor-specific object is found', async () => { + const watchdog1 = new EditorWatchdog( ClassicTestEditor ); + const watchdog2 = new EditorWatchdog( ClassicTestEditor ); + const element2 = document.createElement( 'div' ); + document.body.appendChild( element2 ); + + // sinon.stub( window, 'onerror' ).value( undefined ); and similar do not work. + const originalErrorHandler = window.onerror; + window.onerror = undefined; + + const sharedObject = { foo: 'bar' }; + + await Promise.all( [ + watchdog1.create( element ), + watchdog2.create( element2 ) + ] ); + + watchdog1.editor.shared = sharedObject; + watchdog2.editor.shared = sharedObject; + + const context = { + shared: sharedObject + }; + + const watchdog1RestartSpy = sinon.spy(); + const watchdog2RestartSpy = sinon.spy(); + + watchdog1.on( 'restart', watchdog1RestartSpy ); + watchdog2.on( 'restart', watchdog2RestartSpy ); + + setTimeout( () => throwCKEditorError( 'foo', context ) ); + + await new Promise( res => { + let restarts = 0; + const check = () => { + restarts++; + if ( restarts === 2 ) { + window.onerror = originalErrorHandler; + sinon.assert.calledOnce( watchdog1RestartSpy ); + sinon.assert.calledOnce( watchdog2RestartSpy ); + Promise.all( [ watchdog1.destroy(), watchdog2.destroy() ] ).then( () => { + element2.remove(); + res(); + } ); + } + }; + + watchdog1.on( 'restart', check ); + watchdog2.on( 'restart', check ); + } ); + } ); + it( 'Watchdog should crash permanently if the `crashNumberLimit` is reached' + ' and the average time between errors is lower than `minimumNonErrorTimePeriod` (default values)', async () => { const watchdog = new EditorWatchdog( ClassicTestEditor ); diff --git a/packages/ckeditor5-watchdog/tests/manual/contextwatchdog.html b/packages/ckeditor5-watchdog/tests/manual/contextwatchdog.html new file mode 100644 index 00000000000..9686e31ee34 --- /dev/null +++ b/packages/ckeditor5-watchdog/tests/manual/contextwatchdog.html @@ -0,0 +1,24 @@ + + + +
Context state:
+ +
+
+

Heading 1

+

Paragraph

+
+
+ +
+
+

Heading 1

+

Paragraph

+
+
+ + diff --git a/packages/ckeditor5-watchdog/tests/manual/contextwatchdog.js b/packages/ckeditor5-watchdog/tests/manual/contextwatchdog.js new file mode 100644 index 00000000000..7359e360440 --- /dev/null +++ b/packages/ckeditor5-watchdog/tests/manual/contextwatchdog.js @@ -0,0 +1,124 @@ +/** + * @license Copyright (c) 2003-2025, CKSource Holding sp. z o.o. All rights reserved. + * For licensing, see LICENSE.md or https://ckeditor.com/legal/ckeditor-licensing-options + */ + +import { Context, ContextPlugin } from '@ckeditor/ckeditor5-core'; +import { ClassicEditor } from '@ckeditor/ckeditor5-editor-classic'; +import { ArticlePluginSet } from '@ckeditor/ckeditor5-core/tests/_utils/articlepluginset.js'; +import { CKEditorError } from '@ckeditor/ckeditor5-utils'; + +import { ContextWatchdog } from '../../src/contextwatchdog.js'; + +const stateElement = document.getElementById( 'context-state' ); + +class ContextErrorPlugin extends ContextPlugin { + init() { + window.contextErrorPlugin = this; + } + + throwError() { + setTimeout( () => { + throw new CKEditorError( 'context-error', this.context ); + } ); + } +} + +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 watchdog = new ContextWatchdog( Context ); + +window.watchdog = watchdog; + +const contextConfig = { + plugins: [ ContextErrorPlugin ] +}; + +watchdog.create( contextConfig ) + .then( () => { + return Promise.all( [ + watchdog.add( { + id: 'editor1', + type: 'editor', + creator: async ( element, config ) => { + const editor1 = await ClassicEditor.create( element, config ); + + window.editor1 = editor1; + console.log( 'Created editor1!', editor1 ); + + return editor1; + }, + sourceElementOrData: document.getElementById( 'editor-1' ), + config: { + plugins: [ ArticlePluginSet, TypingError ], + image: { toolbar: [ 'toggleImageCaption', 'imageTextAlternative' ] }, + toolbar: [ 'heading', '|', 'bold', 'italic', 'link', 'bulletedList', 'numberedList', 'blockQuote', 'undo', 'redo' ] + } + } ), + watchdog.add( { + id: 'editor2', + type: 'editor', + creator: async ( element, config ) => { + const editor2 = await ClassicEditor.create( element, config ); + + window.editor2 = editor2; + console.log( 'Created editor2!', editor2 ); + + return editor2; + }, + sourceElementOrData: document.getElementById( 'editor-2' ), + config: { + plugins: [ ArticlePluginSet, TypingError ], + image: { toolbar: [ 'toggleImageCaption', 'imageTextAlternative' ] }, + toolbar: [ 'heading', '|', 'bold', 'italic', 'link', 'bulletedList', 'numberedList', 'blockQuote', 'undo', 'redo' ] + } + } ) + ] ); + } ); + +watchdog.on( 'itemError', ( evt, { itemId, error } ) => { + console.log( `Context watchdog item (${ itemId }) error:`, error ); +} ); + +watchdog.on( 'error', ( evt, { error } ) => { + console.log( 'Context watchdog error:', error ); +} ); + +watchdog.on( 'restart', () => { + console.log( 'Context watchdog restarted.' ); +} ); + +watchdog.on( 'stateChange', () => { + console.log( `Context watchdog state changed to '${ watchdog.state }'` ); + + stateElement.innerText = watchdog.state; +} ); + +stateElement.innerText = watchdog.state; + +document.getElementById( 'context-error' ).addEventListener( 'click', () => { + if ( window.contextErrorPlugin ) { + window.contextErrorPlugin.throwError(); + } +} ); + +document.getElementById( 'random-error' ).addEventListener( 'click', () => { + throw new Error( 'foo' ); +} ); diff --git a/packages/ckeditor5-watchdog/tests/manual/contextwatchdog.md b/packages/ckeditor5-watchdog/tests/manual/contextwatchdog.md new file mode 100644 index 00000000000..75abe29e50f --- /dev/null +++ b/packages/ckeditor5-watchdog/tests/manual/contextwatchdog.md @@ -0,0 +1,7 @@ +# ContextWatchdog manual test + +1. Click `Simulate Error in Context Plugin`. The context and both editors should crash and be restarted. The error should be logged in the console. + +2. Click `Simulate a random error`. No editor should be restarted. + +3. Refresh page and quickly click `Simulate Error in Context Plugin` 4 times. After the last error the watchdog should be crashed permanently and it should not restart.