Removed memory leak caused by CKEDITOR.filter.instances

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
 

core/editor.js

@@ -886,17 +886,27 @@
 		 * element with the instance content.
 		 */
 		destroy: function( noUpdate ) {
+			var filters = CKEDITOR.filter.instances,
+				self = this;
+
 			this.fire( 'beforeDestroy' );
 
 			!noUpdate && updateEditorElement.call( this );
 
 			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';

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 );
 		}
 	};
 

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;

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;

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' );

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;
 	}
-} );
+
+} )();