diff --git a/CHANGES.md b/CHANGES.md index 6fad64477b1..2add62eb448 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -32,6 +32,7 @@ Fixed Issues: * [#421](https://github.com/ckeditor/ckeditor-dev/issues/421) Fixed: Expandable [Button](https://ckeditor.com/cke4/addon/button) puts `(Selected)` text at the end of the label when clicked. * [#1454](https://github.com/ckeditor/ckeditor-dev/issues/1454): Fixed: [Upload Widget](https://ckeditor.com/cke4/addon/uploadwidget) [`onAbort`](https://docs.ckeditor.com/ckeditor4/docs/#!/api/CKEDITOR.fileTools.uploadWidgetDefinition-property-onAbort) method is not called when loader is aborted. * [#1451](https://github.com/ckeditor/ckeditor-dev/issues/1451): Fixed: Context menu is incorrectly positioned when opened with `Shift-F10`. +* [#1722](https://github.com/ckeditor/ckeditor-dev/issues/1722): [`CKEDITOR.filter.instances`](https://docs.ckeditor.com/ckeditor4/latest/api/CKEDITOR_filter.html#static-property-instances) is causing memory leaks. API Changes: @@ -40,6 +41,7 @@ API Changes: * [#2045](https://github.com/ckeditor/ckeditor-dev/issues/2045): Extracted [`tools.eventsBuffer`](https://docs.ckeditor.com/ckeditor4/latest/api/CKEDITOR_tools.html#method-eventsBuffer) and [`tools.throttle`](https://docs.ckeditor.com/ckeditor4/latest/api/CKEDITOR_tools.html#method-throttle) functions logic into separate namespace. * [`tools.eventsBuffer`](https://docs.ckeditor.com/ckeditor4/latest/api/CKEDITOR_tools.html#method-eventsBuffer) has been extracted into [`tools.buffers.event`](https://docs.ckeditor.com/ckeditor4/latest/api/CKEDITOR_tools_buffers_event.html) * [`tools.throttle`](https://docs.ckeditor.com/ckeditor4/latest/api/CKEDITOR_tools.html#method-throttle) has been extracted into [`tools.buffers.throttle`](https://docs.ckeditor.com/ckeditor4/latest/api/CKEDITOR_tools_buffers_throttle.html) +* [#2466](https://github.com/ckeditor/ckeditor-dev/issues/2466): The [`CKEDITOR.filter`](https://docs.ckeditor.com/ckeditor4/latest/api/CKEDITOR_tools.html#method-constructor) constructor accepts additional `rules` parameter allowing to bind editor and filter together. ## CKEditor 4.10.1 diff --git a/core/editor.js b/core/editor.js index 9bac76833cd..a0d58f2bfc4 100644 --- a/core/editor.js +++ b/core/editor.js @@ -886,6 +886,9 @@ * element with the instance content. */ destroy: function( noUpdate ) { + var filters = CKEDITOR.filter.instances, + self = this; + this.fire( 'beforeDestroy' ); !noUpdate && updateEditorElement.call( this ); @@ -893,10 +896,17 @@ this.editable( null ); if ( this.filter ) { - this.filter.destroy(); delete this.filter; } + // Destroy filters attached to the editor (#1722). + CKEDITOR.tools.array.forEach( CKEDITOR.tools.objectKeys( filters ), function( id ) { + var filter = filters[ id ]; + if ( self === filter.editor ) { + filter.destroy(); + } + } ); + delete this.activeFilter; this.status = 'destroyed'; diff --git a/core/filter.js b/core/filter.js index fcbd5e4ecc5..31603c126b1 100644 --- a/core/filter.js +++ b/core/filter.js @@ -59,19 +59,32 @@ * {@link CKEDITOR.filter.allowedContentRules} instead of {@link CKEDITOR.editor} * to the constructor: * - * var filter = new CKEDITOR.filter( 'b' ); + * ```javascript + * var filter = new CKEDITOR.filter( 'b' ); * - * filter.check( 'b' ); // -> true - * filter.check( 'i' ); // -> false - * filter.allow( 'i' ); - * filter.check( 'i' ); // -> true + * filter.check( 'b' ); // -> true + * filter.check( 'i' ); // -> false + * filter.allow( 'i' ); + * filter.check( 'i' ); // -> true + * ``` + * + * If filter is only used by a single editor instance, you should pass editor instance alongside with the rules. + * Passing editor as a first parameter binds it with the filter so the filter can be removed + * with {@link CKEDITOR.editor#destroy} method to prevent memory leaks. + * + * ```javascript + * // In both cases filter will be removed during {@link CKEDITOR.editor#destroy} function execution. + * var filter1 = new CKEDITOR.filter( editor ); + * var filter2 = new CKEDITOR.filter( editor, 'b' ); + * ``` * * @since 4.1 * @class * @constructor Creates a filter class instance. * @param {CKEDITOR.editor/CKEDITOR.filter.allowedContentRules} editorOrRules + * @param {CKEDITOR.filter.allowedContentRules} [rules] This parameter is available since 4.11.0. */ - CKEDITOR.filter = function( editorOrRules ) { + CKEDITOR.filter = function( editorOrRules, rules ) { /** * Whether custom {@link CKEDITOR.config#allowedContent} was set. * @@ -165,8 +178,9 @@ // Register filter instance. CKEDITOR.filter.instances[ this.id ] = this; - if ( editorOrRules instanceof CKEDITOR.editor ) { - var editor = this.editor = editorOrRules; + var editor = this.editor = editorOrRules instanceof CKEDITOR.editor ? editorOrRules : null; + + if ( editor && !rules ) { this.customConfig = true; var allowedContent = editor.config.allowedContent; @@ -191,7 +205,7 @@ // Rules object passed in editorOrRules argument - initialize standalone filter. else { this.customConfig = false; - this.allow( editorOrRules, 'default', 1 ); + this.allow( rules || editorOrRules, 'default', 1 ); } }; diff --git a/plugins/clipboard/plugin.js b/plugins/clipboard/plugin.js index 740635fa60e..b6fbc93ec51 100644 --- a/plugins/clipboard/plugin.js +++ b/plugins/clipboard/plugin.js @@ -126,7 +126,7 @@ hidpi: true, // %REMOVE_LINE_CORE% init: function( editor ) { var filterType, - filtersFactory = filtersFactoryFactory(); + filtersFactory = filtersFactoryFactory( editor ); if ( editor.config.forcePasteAsPlainText ) { filterType = 'plain-text'; @@ -1330,7 +1330,7 @@ return switchEnterMode( config, data ); } - function filtersFactoryFactory() { + function filtersFactoryFactory( editor ) { var filters = {}; function setUpTags() { @@ -1346,7 +1346,7 @@ } function createSemanticContentFilter() { - var filter = new CKEDITOR.filter(); + var filter = new CKEDITOR.filter( editor, {} ); filter.allow( { $1: { @@ -1374,12 +1374,12 @@ // so it tries to replace it with an element created based on the active enter mode, eventually doing nothing. // // Now you can sleep well. - return filters.plainText || ( filters.plainText = new CKEDITOR.filter( 'br' ) ); + return filters.plainText || ( filters.plainText = new CKEDITOR.filter( editor, 'br' ) ); } else if ( type == 'semantic-content' ) { return filters.semanticContent || ( filters.semanticContent = createSemanticContentFilter() ); } else if ( type ) { // Create filter based on rules (string or object). - return new CKEDITOR.filter( type ); + return new CKEDITOR.filter( editor, type ); } return null; diff --git a/plugins/copyformatting/plugin.js b/plugins/copyformatting/plugin.js index 73e53ac23af..e9f8474f156 100644 --- a/plugins/copyformatting/plugin.js +++ b/plugins/copyformatting/plugin.js @@ -260,7 +260,7 @@ * @member CKEDITOR.plugins.copyformatting.state * @property {CKEDITOR.filter} */ - this.filter = new CKEDITOR.filter( editor.config.copyFormatting_allowRules ); + this.filter = new CKEDITOR.filter( editor, editor.config.copyFormatting_allowRules ); if ( editor.config.copyFormatting_allowRules === true ) { this.filter.disabled = true; diff --git a/plugins/widget/plugin.js b/plugins/widget/plugin.js index f61313ae472..d49728355c0 100644 --- a/plugins/widget/plugin.js +++ b/plugins/widget/plugin.js @@ -1115,12 +1115,43 @@ * @param {Boolean} [offline] See {@link #method-destroy} method. */ destroyEditable: function( editableName, offline ) { - var editable = this.editables[ editableName ]; + var editable = this.editables[ editableName ], + canDestroyFilter = true; editable.removeListener( 'focus', onEditableFocus ); editable.removeListener( 'blur', onEditableBlur ); this.editor.focusManager.remove( editable ); + // Destroy filter if it's no longer used by other editables (#1722). + if ( editable.filter ) { + for ( var widgetName in this.repository.instances ) { + var widget = this.repository.instances[ widgetName ]; + + if ( !widget.editables ) { + continue; + } + + var widgetEditable = widget.editables[ editableName ]; + + if ( !widgetEditable || widgetEditable === editable ) { + continue; + } + + if ( editable.filter === widgetEditable.filter ) { + canDestroyFilter = false; + } + } + + if ( canDestroyFilter ) { + editable.filter.destroy(); + + var filters = this.repository._.filters[ this.name ]; + if ( filters ) { + delete filters[ editableName ]; + } + } + } + if ( !offline ) { this.repository.destroyAll( false, editable ); editable.removeClass( 'cke_widget_editable' ); diff --git a/tests/core/editor/destroy.js b/tests/core/editor/destroy.js index 968be6d5e4d..e2520ebe1ff 100644 --- a/tests/core/editor/destroy.js +++ b/tests/core/editor/destroy.js @@ -1,84 +1,112 @@ /* bender-tags: editor */ /* bender-ckeditor-plugins: toolbar,button,stylescombo,wysiwygarea */ -bender.editor = { - config: { - startupFocus: true - } -}; +( function() { -bender.test( { - 'test destroy editor with rich combo panel opened': function() { - var bot = this.editorBot, editor = this.editor; - bot.combo( 'Styles', function( combo ) { - var panelEl = combo._.panel.element; - editor.destroy(); - assert.isFalse( CKEDITOR.document.getBody().contains( panelEl ) ); + bender.editor = { + config: { + startupFocus: true + } + }; + + bender.test( { + 'test destroy editor with rich combo panel opened': function() { + var bot = this.editorBot, editor = this.editor; + bot.combo( 'Styles', function( combo ) { + var panelEl = combo._.panel.element; + editor.destroy(); + assert.isFalse( CKEDITOR.document.getBody().contains( panelEl ) ); - // https://dev.ckeditor.com/ticket/4552: Do that one more time. + // https://dev.ckeditor.com/ticket/4552: Do that one more time. + bender.editorBot.create( {}, function( bot ) { + this.wait( function() { + bot.combo( 'Styles', function( combo ) { + var panelEl = combo._.panel.element; + + bot.editor.destroy(); + assert.isFalse( CKEDITOR.document.getBody().contains( panelEl ) ); + } ); + }, 0 ); + } ); + + } ); + }, + + // https://dev.ckeditor.com/ticket/13385. + 'test getSnapshot returns empty string after editor destroyed': function() { bender.editorBot.create( {}, function( bot ) { this.wait( function() { - bot.combo( 'Styles', function( combo ) { - var panelEl = combo._.panel.element; - - bot.editor.destroy(); - assert.isFalse( CKEDITOR.document.getBody().contains( panelEl ) ); - } ); + var editor = bot.editor; + editor.destroy(); + assert.areSame( '', editor.getSnapshot() ); }, 0 ); } ); + }, - } ); - }, + 'test destroy editor before it is fully initialized': function() { + var name = 'test_editor', + element, + editor, + warnStub = sinon.stub( CKEDITOR, 'warn' ); - // https://dev.ckeditor.com/ticket/13385. - 'test getSnapshot returns empty string after editor destroyed': function() { - bender.editorBot.create( {}, function( bot ) { - this.wait( function() { - var editor = bot.editor; - editor.destroy(); - assert.areSame( '', editor.getSnapshot() ); + element = CKEDITOR.document.getById( name ); + this.editor.destroy(); + + editor = CKEDITOR.replace( element ); + editor.destroy(); + + // initConfig is called asynchronously. + wait( function() { + warnStub.restore(); + assert.isTrue( warnStub.calledOnce, 'CKEDITOR.warn should be called once.' ); + assert.areEqual( 'editor-incorrect-destroy', warnStub.firstCall.args[ 0 ], + 'CKEDITOR.warn error code should be "editor-incorrect-destroy".' ); }, 0 ); - } ); - }, - - 'test destroy editor before it is fully initialized': function() { - var name = 'test_editor', - element, - editor, - warnStub = sinon.stub( CKEDITOR, 'warn' ); - - element = CKEDITOR.document.getById( name ); - this.editor.destroy(); - - editor = CKEDITOR.replace( element ); - editor.destroy(); - - // initConfig is called asynchronously. - wait( function() { - warnStub.restore(); - assert.isTrue( warnStub.calledOnce, 'CKEDITOR.warn should be called once.' ); - assert.areEqual( 'editor-incorrect-destroy', warnStub.firstCall.args[ 0 ], - 'CKEDITOR.warn error code should be "editor-incorrect-destroy".' ); - }, 0 ); - - }, - - 'test check editable existence on blur': function() { - CKEDITOR.replace( 'focused', { - on: { - instanceReady: function( evt ) { - resume( function() { - var editor = evt.sender; - // Simulate the circumstances instead of creating them. - editor.focusManager.hasFocus = true; - sinon.stub( editor, 'editable' ).returns( null ); - editor.focusManager.blur( 1 ); - assert.pass(); - } ); + + }, + + 'test check editable existence on blur': function() { + CKEDITOR.replace( 'focused', { + on: { + instanceReady: function( evt ) { + resume( function() { + var editor = evt.sender; + // Simulate the circumstances instead of creating them. + editor.focusManager.hasFocus = true; + sinon.stub( editor, 'editable' ).returns( null ); + editor.focusManager.blur( 1 ); + assert.pass(); + } ); + } } - } - } ); + } ); + + wait(); + }, + + // (#1722) + 'test destroy attached filters': function() { + var filters = countFilters(); + bender.editorBot.create( { name: 'editor_filter_destroy' }, function( bot ) { + var editor = bot.editor; - wait(); + new CKEDITOR.filter( editor, 'br' ); + new CKEDITOR.filter( editor ); + new CKEDITOR.filter( 'br' ); + + editor.destroy(); + + assert.areEqual( 0, countFilters( editor ) ); + assert.areEqual( filters + 1, countFilters() ); + } ); + } + } ); + + function countFilters( editor ) { + var filters = bender.tools.objToArray( CKEDITOR.filter.instances ); + return editor ? CKEDITOR.tools.array.filter( filters, function( filter ) { + return filter.editor === editor; + } ).length : filters.length; } -} ); + +} )(); diff --git a/tests/core/editor/manual/filterdestroy.html b/tests/core/editor/manual/filterdestroy.html new file mode 100644 index 00000000000..93310178ab3 --- /dev/null +++ b/tests/core/editor/manual/filterdestroy.html @@ -0,0 +1,27 @@ + + + + diff --git a/tests/core/editor/manual/filterdestroy.md b/tests/core/editor/manual/filterdestroy.md new file mode 100644 index 00000000000..fb5a78c9b91 --- /dev/null +++ b/tests/core/editor/manual/filterdestroy.md @@ -0,0 +1,14 @@ +@bender-tags: bug, 4.11.0, 1722 +@bender-ui: collapsed +@bender-ckeditor-plugins: toolbar, wysiwygarea, basicstyles + +1. Memrise `Filters count` value. +2. Click `Rebuild editor` button. + +## Expected + +Every time when you click `Rebuild editor` button `Filters count` shouldn't change. + +## Unexpected + +`Filters count` changes. diff --git a/tests/plugins/widget/manual/nestededitablefilters.html b/tests/plugins/widget/manual/nestededitablefilters.html new file mode 100644 index 00000000000..a467d6a3723 --- /dev/null +++ b/tests/plugins/widget/manual/nestededitablefilters.html @@ -0,0 +1,60 @@ + +Filter: +active + +
+ + diff --git a/tests/plugins/widget/manual/nestededitablefilters.md b/tests/plugins/widget/manual/nestededitablefilters.md new file mode 100644 index 00000000000..e68362d1681 --- /dev/null +++ b/tests/plugins/widget/manual/nestededitablefilters.md @@ -0,0 +1,14 @@ +@bender-tags: widget, bug, 4.11.0, 1722 +@bender-ui: collapsed +@bender-ckeditor-plugins: widget, wysiwygarea + +1. Click `Destroy` button first time. +1. Click `Destroy` button second time. + +## Expected + +After first click `Filter` label should stay `active`. Second click should change `Filter` label into `removed`. + +## Unexpected + +`Filter` label stays `active` regardless of button clicks or changes status into `removed` after first click. diff --git a/tests/plugins/widget/nestededitables.js b/tests/plugins/widget/nestededitables.js index 9cc664bb384..9ee4f52e6aa 100644 --- a/tests/plugins/widget/nestededitables.js +++ b/tests/plugins/widget/nestededitables.js @@ -219,6 +219,52 @@ } ); }, + // (#1722) + 'test #destroyEditable destroys unused editable filters': function() { + var editor = this.editor; + + editor.widgets.add( 'testmethod5', { + editables: { + foo: '#foo' + } + } ); + + var widget1Html = '

A

B

', + widget2Html = '

A

B

'; + + this.editorBot.setData( widget1Html + widget2Html, function() { + var widget1 = getWidgetById( editor, 'w1' ), + widget2 = getWidgetById( editor, 'w2' ); + + widget1.initEditable( 'foo', { selector: '.foo', allowedContent: 'p br' } ); + widget2.initEditable( 'foo', { selector: '.foo', allowedContent: 'p br' } ); + + var removedListeners = [], + filters = editor.widgets._.filters.testmethod5, + filterSpy = sinon.spy( filters.foo, 'destroy' ); + + widget1.editables.foo.removeListener = function( evtName ) { + removedListeners.push( evtName ); + }; + + widget2.editables.foo.removeListener = function( evtName ) { + removedListeners.push( evtName ); + }; + + widget1.destroyEditable( 'foo' ); + + assert.isNotUndefined( filters.foo ); + + widget2.destroyEditable( 'foo' ); + + assert.isUndefined( filters.foo ); + + assert.isTrue( filterSpy.calledOnce ); + + filterSpy.restore(); + } ); + }, + 'test nestedEditable enter modes are limited by ACF': function() { var editor = this.editor;