Skip to content

Commit

Permalink
Merge pull request #10962 from ckeditor/cke/10643
Browse files Browse the repository at this point in the history
Fix (watchdog): Prevented `EditorWatchdog` from crashing during the editor destruction process when one of plugins tries to change data at that moment. Closes #10643.
  • Loading branch information
scofalik authored Dec 6, 2021
2 parents 5066d1e + da5c3f7 commit 9151ef7
Show file tree
Hide file tree
Showing 2 changed files with 46 additions and 3 deletions.
5 changes: 5 additions & 0 deletions packages/ckeditor5-watchdog/src/editorwatchdog.js
Original file line number Diff line number Diff line change
Expand Up @@ -235,6 +235,11 @@ export default class EditorWatchdog extends Watchdog {

this._editor = null;

// Remove the `change:data` listener before destroying the editor.
// Incorrectly written plugins may trigger firing `change:data` events during the editor destruction phase
// causing the watchdog to call `editor.getData()` when some parts of editor are already destroyed.
editor.model.document.off( 'change:data', this._throttledSave );

return this._destructor( editor );
} );
}
Expand Down
44 changes: 41 additions & 3 deletions packages/ckeditor5-watchdog/tests/editorwatchdog.js
Original file line number Diff line number Diff line change
Expand Up @@ -755,9 +755,9 @@ describe( 'EditorWatchdog', () => {
} );
} );

describe( 'destroy()', () => {
// See #19.
it( 'should clean internal stuff', () => {
describe( 'destroying', () => {
// See https://github.com/ckeditor/ckeditor5/issues/4706.
it( 'destroy() should clean internal stuff', () => {
// 30ms should be enough to make the two data changes split into two data save actions.
// This will ensure that the second data save action will be put off in time.
const SAVE_INTERVAL = 30;
Expand Down Expand Up @@ -791,6 +791,44 @@ describe( 'EditorWatchdog', () => {
expect( watchdog.crashes ).to.deep.equal( [] );
} );
} );

// See https://github.com/ckeditor/ckeditor5/issues/10643.
it( 'watchdog should remove the listener for `change:data` event before destroying the editor', async () => {
const watchdog = new EditorWatchdog( ClassicTestEditor );

const spy = sinon.spy();

// A plugin that modifies the editor data during the destruction phase.
class InvalidPlugin {
constructor( editor ) {
this.editor = editor;
}

destroy() {
const doc = this.editor.model.document;

this.editor.model.change( writer => {
writer.insertText( 'bar', writer.createPositionAt( doc.getRoot(), 1 ) );
spy();
} );
}
}

await watchdog.create( element, {
initialData: '<p>foo</p>',
plugins: [ InvalidPlugin, Paragraph ]
} );

await watchdog._restart();

// The watchdog during destroying the editor should not listen to the data changes.
sinon.assert.calledOnce( spy );
expect( watchdog.editor.getData() ).to.equal( '<p>foo</p>' );

await watchdog.destroy();

sinon.assert.calledTwice( spy );
} );
} );

describe( 'crashes', () => {
Expand Down

0 comments on commit 9151ef7

Please sign in to comment.