Merge pull request #2969 from ckeditor/t/589

Fix for: Memory leaks during editor create / destroy cycles

CHANGES.md

@@ -5,6 +5,7 @@
 
 Fixed Issues:
 
+* [#589](https://github.com/ckeditor/ckeditor-dev/issues/589): Fixed: Editor causes memory leaks in create / destroy cycles.
 * [#1397](https://github.com/ckeditor/ckeditor-dev/issues/1397): Fixed: Using dialog to remove headers from the [table](https://ckeditor.com/cke4/addon/table) with one headers row only throws an error.
 * [#1479](https://github.com/ckeditor/ckeditor-dev/issues/1479): Fixed: [Justification](https://ckeditor.com/cke4/addon/justify) for styled content in BR mode is disabled.
 * [#2816](https://github.com/ckeditor/ckeditor-dev/issues/2816): Fixed: [Enhanced Image](https://ckeditor.com/cke4/addon/image2) resize handler is visible in [read-only mode](https://ckeditor.com/docs/ckeditor4/latest/guide/dev_readonly.html).

core/ckeditor.js

@@ -49,14 +49,19 @@ CKEDITOR.add = function( editor ) {
 		}
 	} );
 
-	editor.on( 'blur', function() {
+	editor.on( 'blur', removeInstance );
+
+	// Remove currentInstance if it's destroyed (#589).
+	editor.on( 'destroy', removeInstance );
+
+	CKEDITOR.fire( 'instance', null, editor );
+
+	function removeInstance() {
 		if ( CKEDITOR.currentInstance == editor ) {
 			CKEDITOR.currentInstance = null;
 			CKEDITOR.fire( 'currentInstance' );
 		}
-	} );
-
-	CKEDITOR.fire( 'instance', null, editor );
+	}
 };
 
 /**

core/loader.js

@@ -158,6 +158,8 @@ if ( !CKEDITOR.loader ) {
 						// Some browsers, such as Safari, may call the onLoad function
 						// immediately. Which will break the loading sequence. (https://dev.ckeditor.com/ticket/3661)
 						setTimeout( function() {
+							// Once script loaded remove listener, which might lead to memory leaks (#589).
+							script.onload = null;
 							onScriptLoaded( scriptName );
 						}, 0 );
 					};

core/scriptloader.js

@@ -132,11 +132,13 @@ CKEDITOR.scriptLoader = ( function() {
 								// Some browsers, such as Safari, may call the onLoad function
 								// immediately. Which will break the loading sequence. (https://dev.ckeditor.com/ticket/3661)
 								setTimeout( function() {
+									removeListeners( script );
 									onLoad( url, true );
 								}, 0 );
 							};
 
 							script.$.onerror = function() {
+								removeListeners( script );
 								onLoad( url, false );
 							};
 						}
@@ -146,12 +148,19 @@ CKEDITOR.scriptLoader = ( function() {
 					script.appendTo( CKEDITOR.document.getHead() );
 
 					CKEDITOR.fire( 'download', url ); // %REMOVE_LINE%
+
 				};
 
 			showBusy && CKEDITOR.document.getDocumentElement().setStyle( 'cursor', 'wait' );
 			for ( var i = 0; i < scriptCount; i++ ) {
 				loadScript( scriptUrl[ i ] );
 			}
+
+			function removeListeners( script ) {
+				// Once script loaded or failed remove listeners, which might lead to memory leaks (#589).
+				script.$.onload = null;
+				script.$.onerror = null;
+			}
 		},
 
 		/**

core/skin.js

@@ -269,24 +269,32 @@
 
 	CKEDITOR.on( 'instanceLoaded', function( evt ) {
 		// The chameleon feature is not for IE quirks.
-		if ( CKEDITOR.env.ie && CKEDITOR.env.quirks )
+		if ( CKEDITOR.env.ie && CKEDITOR.env.quirks ) {
 			return;
+		}
 
 		var editor = evt.editor,
 			showCallback = function( event ) {
-				var panel = event.data[ 0 ] || event.data;
-				var iframe = panel.element.getElementsByTag( 'iframe' ).getItem( 0 ).getFrameDocument();
+				var panel = event.data[ 0 ] || event.data,
+					iframe = panel.element.getElementsByTag( 'iframe' ).getItem( 0 ).getFrameDocument();
 
 				// Add stylesheet if missing.
 				if ( !iframe.getById( 'cke_ui_color' ) ) {
 					var node = getStylesheet( iframe );
 					uiColorMenus.push( node );
 
+					// Cleanup after destroying editor (#589).
+					editor.on( 'destroy', function() {
+						uiColorMenus = CKEDITOR.tools.array.filter( uiColorMenus, function( storedNode ) {
+							return node !== storedNode;
+						} );
+					} );
+
 					var color = editor.getUiColor();
 					// Set uiColor for new panel.
-					if ( color )
+					if ( color ) {
 						updateStylesheets( [ node ], CKEDITOR.skin.chameleon( editor, 'panel' ), [ [ uiColorRegexp, color ] ] );
-
+					}
 				}
 			};
 

plugins/autocomplete/plugin.js

@@ -239,6 +239,10 @@
 				this.attach();
 			}, this );
 		}
+
+		editor.on( 'destroy', function() {
+			this.destroy();
+		}, this );
 	}
 
 	Autocomplete.prototype = {
@@ -359,7 +363,7 @@
 
 			this._listeners = [];
 
-			this.view.element.remove();
+			this.view.element && this.view.element.remove();
 		},
 
 		/**

plugins/codesnippet/plugin.js

@@ -46,6 +46,11 @@
 				editor._.codesnippet.langsRegex = new RegExp( '(?:^|\\s)language-(' +
 					CKEDITOR.tools.objectKeys( langs ).join( '|' ) + ')(?:\\s|$)' );
 			};
+
+			editor.once( 'pluginsLoaded', function() {
+				// Remove method once it can't be used, because it leaks editor reference (#589).
+				this.setHighlighter = null;
+			}, this );
 		},
 
 		onLoad: function() {

plugins/dialog/plugin.js

@@ -2228,6 +2228,9 @@ CKEDITOR.DIALOG_STATE_BUSY = 2;
 		editor.focusManager.remove( currentCover );
 		var win = CKEDITOR.document.getWindow();
 		currentCover.hide();
+
+		// Remove current cover reference once it's removed (#589).
+		currentCover = null;
 		win.removeListener( 'resize', resizeCover );
 
 		if ( CKEDITOR.env.ie6Compat ) {

plugins/image2/plugin.js

@@ -1432,7 +1432,7 @@
 		if ( !editor.plugins.link )
 			return;
 
-		CKEDITOR.on( 'dialogDefinition', function( evt ) {
+		var listener = CKEDITOR.on( 'dialogDefinition', function( evt ) {
 			var dialog = evt.data;
 
 			if ( dialog.name == 'link' ) {
@@ -1483,6 +1483,10 @@
 				};
 			}
 		} );
+		// Listener has to be removed due to leaking editor reference (#589).
+		editor.on( 'destroy', function() {
+			listener.removeListener();
+		} );
 
 		// Overwrite default behaviour of unlink command.
 		editor.getCommand( 'unlink' ).on( 'exec', function( evt ) {

plugins/magicline/plugin.js

@@ -340,7 +340,7 @@
 
 			// This one allows testing and debugging. It reveals some
 			// inner methods to the world.
-			this.backdoor = {
+			editor._.magiclineBackdoor = {
 				accessFocusSpace: accessFocusSpace,
 				boxTrigger: boxTrigger,
 				isLine: isLine,

plugins/widget/plugin.js

@@ -3638,7 +3638,8 @@
 
 	function addCustomStyleHandler() {
 		// Styles categorized by group. It is used to prevent applying styles for the same group being used together.
-		var styleGroups = {};
+		var styleGroups = {},
+			styleDefinitions = [];
 
 		/**
 		 * The class representing a widget style. It is an {@link CKEDITOR#STYLE_OBJECT object} like
@@ -3875,19 +3876,64 @@
 		// Save and categorize style by its group.
 		function saveStyleGroup( style ) {
 			var widgetName = style.widget,
-				group;
+				groupName, group;
 
 			if ( !styleGroups[ widgetName ] ) {
 				styleGroups[ widgetName ] = {};
 			}
 
 			for ( var i = 0, l = style.group.length; i < l; i++ ) {
-				group = style.group[ i ];
-				if ( !styleGroups[ widgetName ][ group ] ) {
-					styleGroups[ widgetName ][ group ] = [];
+				groupName = style.group[ i ];
+				if ( !styleGroups[ widgetName ][ groupName ] ) {
+					styleGroups[ widgetName ][ groupName ] = [];
 				}
 
-				styleGroups[ widgetName ][ group ].push( style );
+				group = styleGroups[ widgetName ][ groupName ];
+
+				// Don't push style if it's already stored (#589).
+				if ( !find( group, getCompareFn( style ) ) ) {
+					group.push( style );
+				}
+			}
+
+			// Copied `CKEDITOR.tools.array` from major branch.
+			function find( array, fn, thisArg ) {
+				var length = array.length,
+					i = 0;
+
+				while ( i < length ) {
+					if ( fn.call( thisArg, array[ i ], i, array ) ) {
+						return array[ i ];
+					}
+					i++;
+				}
+
+				return undefined;
+			}
+
+			function getCompareFn( left ) {
+				return function( right ) {
+					return deepCompare( left.getDefinition(), right.getDefinition() );
+				};
+
+				function deepCompare( left, right ) {
+					var leftKeys = CKEDITOR.tools.objectKeys( left ),
+						rightKeys = CKEDITOR.tools.objectKeys( right );
+
+					if ( leftKeys.length !== rightKeys.length ) {
+						return false;
+					}
+
+					for ( var key in left ) {
+						var areSameObjects = typeof left[ key ] === 'object' && typeof right[ key ] === 'object' && deepCompare( left[ key ], right[ key ] );
+
+						if ( !areSameObjects && left[ key ] !== right[ key ] ) {
+							return false;
+						}
+					}
+
+					return true;
+				}
 			}
 		}
 

tests/core/editor/currentInstance.js

@@ -0,0 +1,27 @@
+/* bender-tags: editor */
+/* bender-ckeditor-plugins: wysiwygarea */
+
+( function() {
+	'use strict';
+
+	bender.editor = {};
+
+	bender.test( {
+		// (#589)
+		'test currentInstance after editor destroy': function() {
+			this.editor.fire( 'focus' );
+			this.editor.on( 'destroy', function() {
+				setTimeout( function() {
+					resume( function() {
+						assert.isNull( CKEDITOR.currentInstance );
+					} );
+				} );
+			} );
+
+			assert.areSame( this.editor, CKEDITOR.currentInstance );
+
+			this.editor.destroy();
+			wait();
+		}
+	} );
+} )();

tests/core/loader.js

@@ -0,0 +1,41 @@
+/* bender-tags: editor */
+( function() {
+	'use strict';
+
+	bender.test( {
+		// Script listeners should be removed because it might cause memory leaks (#589).
+		'test script load listeners removed': function() {
+			var loader = CKEDITOR.loader;
+
+			if ( !loader ) {
+				// All core modules are bundled in release version, so we can't and don't need to test it there.
+				assert.ignore();
+			}
+
+			var scriptName = loader.loadedScripts.pop();
+
+			getScript().remove();
+
+			delete loader.loadedScripts[ 's:' + scriptName ];
+
+			loader.load( scriptName, false );
+
+			var script = getScript();
+
+			script.on( script.$.onload ? 'load' : 'readystatechange', function() {
+				setTimeout( function() {
+					resume( function() {
+						assert.isFalse( !!script.$.onreadystatechange );
+						assert.isFalse( !!script.$.onload );
+					} );
+				}, 50 );
+			} );
+
+			wait();
+
+			function getScript() {
+				return CKEDITOR.document.findOne( 'script[src="' + CKEDITOR.getUrl( 'core/' + scriptName + '.js' ) + '"]' );
+			}
+		}
+	} );
+} )();

tests/core/scriptloader.js

@@ -104,6 +104,21 @@ var tests = {
 		] );
 
 		this.wait();
+	},
+
+	// Script listeners should be removed because it might cause memory leaks (#589).
+	'test script listeners removed when loaded': function() {
+		CKEDITOR.scriptLoader.load( '../_assets/sample.js', function() {
+			resume( function() {
+				var script = CKEDITOR.document.findOne( 'script[src="../_assets/sample.js"]' );
+
+				assert.isFalse( !!script.$.onreadystatechange );
+				assert.isFalse( !!script.$.onload );
+				assert.isFalse( !!script.$.onerror );
+			} );
+		} );
+
+		wait();
 	}
 };
 

tests/plugins/autocomplete/autocomplete.html

@@ -1 +1,2 @@
 <div id="init"></div>
+<textarea id="destroy"></textarea>

tests/plugins/autocomplete/autocomplete.js

@@ -562,6 +562,25 @@
 				} );
 			} );
 
+			wait();
+		},
+
+		// (#589)
+		'test autocomplete is destroyed with editor': function() {
+			var editor = CKEDITOR.replace( 'destroy' ),
+				ac = new CKEDITOR.plugins.autocomplete( editor, configDefinition ),
+				spy = sinon.spy( ac, 'destroy' );
+
+			editor.on( 'destroy', function() {
+				setTimeout( function() {
+					resume( function() {
+						assert.isTrue( spy.called );
+					} );
+				} );
+			} );
+
+			editor.destroy();
+
 			wait();
 		}
 	} );