Memory Leak

[markuspaek]

Check '_samples/ajax.html' from your current distribution

Repeat creating and destroying editor instances, then the browser takes more and more memory. Even after some time, the browsers still does not free the memory.

IE 8 takes roughly 5MB of memory for each editor instance even after the instance has been destroyed.

FireFox 3.5 takes about 1.2MB of memory for each instance.

Windows XP

[Garry Yao]

Confirmed with IE at least.
@markuspaek Thanks for the measurement of the memory usage of each instance, while it's out the scope of this ticket. Another question is could you provide any detail profile about the Firefox leak you reported?

[Garry Yao]

Attaching a memory report while creating/destroying instance with 5s interval on IE6.

[Sylvain]

I have the same bug (memory Leak) in all version of IE : 6 , 7 , 8. In FireFox 2.0.0.17 I have not this bug. Already present in CKEDITOR 3.0.

[Sylvain]

Sorry,

but can you explain why do you change the priority and milestone, the severity is really High.

If a patch exist can you refer in this ticket.

[Alfonso Martínez de Lizarrondo]

The question is why are you changing the priority and milestone unless you are going to fix the bug.

There are lots of things that needs work, and of course everyone thinks that their pet bug is most important than everything else, but each day only has 24 hours.

[Sylvain]

Ok,i understand , but , I haven't never changed the milestone, only the severity. Because for me the severity is independant of the milestone.

This bug Crash the browser and for me it's normal to considered that is high . But I have not the fix .. Then i don't change anything.

[Alfonso Martínez de Lizarrondo]

Patch for core

[Alfonso Martínez de Lizarrondo]

The test page creates and destroy CKEditor instances in a loop, but using only the core, so we can start looking at the base code. In IE6 it's clear that it's leaking continuously and the patch almost fixes that (there's still a slight leak that should be tracked in the future for perfect performance).

The main fix is to remove the events applied in the theme constructor to the container, the changes in ckeditor.js are just safe-wards that doesn't seem to improve the usage at this moment, although they might help in the future.

The leaks using the full editor are much bigger, but they will be tracked in another patch (after finding them)

BTW: I didn't seem to notice leaks in Firefox, but I focused my tests in IE6

[Alfonso Martínez de Lizarrondo]

Test page

[Alfonso Martínez de Lizarrondo]

Improved patch

[Alfonso Martínez de Lizarrondo]

The new patch fixes the leaks in the wysiwyg plugin, basic functionality of the toolbar and button plugins. There's still a long way to go though.

And using Process Explorer I've verified that loading full instances creates leaks in Firefox, but way smaller than IE.

[Alfonso Martínez de Lizarrondo]

Revised patch

[Alfonso Martínez de Lizarrondo]

With the new patch it's possible to create and destroy lots of instances with very little leaks even in IE6. I would say that even hundreds of instances.

Future fixes: test execution of commands, and load/unload of a page with an instance.

As a summary: any call to CKEDITOR.tools.addFunction must be matched by a call to remove that function when the instance is destroyed, and every element wrapping a DOM object that has some listener set, must be cleaned up.

In this patch I've removed the listener set on ui.button for editor.on('destroy'), adding the clickFn and the index in the global array of buttons to the instance, and the editor.destroy loops the toolbars cleaning all the buttons. The idea is that storing two numbers is lighter than creating a new listener for each button.

[Alfonso Martínez de Lizarrondo]

Some info for anyone wanting to test this:
Drip or sIEve can't be used correctly to test the leaks due to ​http://sourceforge.net/tracker/?func=detail&aid=1776519&group_id=165799&atid=837173 and ​http://sourceforge.net/tracker/?func=detail&aid=1775443&group_id=165799&atid=837173 They will report and keep in memory nodes that are used in CKEDITOR.dom.element.createFromHtml for example, but testing in the real IE doesn't show any such problems.

For my tests I'm using ​Process Explorer and looking at the Private bytes in the Performance Graph tab of the process that I want to test. It has also the benefit that it can be used with any browser, not just IE.

Currently, running 100 loops of the test page with all the plugins jumps in IE6 from 31 to 38Mb, so it's under 100Kb per instance, IE8 goes from 39 to 43Mb (something like 50Kb)
In Firefox 3.6, it starts at 32Mb and then goes up to 54Mb, then the garbage collector runs and goes back to 42Mb, and it seems to keep that sawtooth graph, no big leaks, although it seems to point a certain increase.
Opera 10.10 runs stable at 50Mb, Safari 4 also runs stable at 44Mb
Chrome 4 starts at 10Mb, does two jumps and ends in 30Mb
Opera 10.5 beta 2 starts at 44Mb, increases in a slope to 88Mb and after that it seems to keep increasing little by little, something like 8Mb for another 100 runs

[Alfonso Martínez de Lizarrondo]

Proposed patch

[Alfonso Martínez de Lizarrondo]

This is the final patch. There are still some code that must be reviewed in several plugins (as I said, any call to tools.addFunction is dangerous) but now I need a rest away from all these leaks.

The results should be similar to those explained in my previous comment, but I've removed some code that didn't seem to provide any extra benefits.

[Garry Yao]

The patch works well to eliminate the memory leak in all IE verions, while my doubts come from the rationale behind this patch, for the two major fixes:

1. Ideally event listener shouldn't become a leak source, which has been well considered from our initial ​design
2. Theoretically inline event handlers that use 'CKEDITOR.tools.addFunction' should be leak free also.
   

It would be great if you can give some clues here to better recognize the leak patterns.

Beside, I'm able to identify certain unnecessary changes in the patch, which is excluded in incoming patch.

[Alfonso Martínez de Lizarrondo]

Revised patch

[Alfonso Martínez de Lizarrondo]

The new patch includes the simplifications in editor.js, as well as removing the removeAllListeners for non-DOM objects, but the changes in the filebrowser and showborders plugin are still needed.

The problem talking here about "memory leak" is that usually that term means a leak in the browser after a page unloads due to a circular reference between JS and DOM, the architecture tries to avoid that problem, but here we are talking about JS objects that are created for each editor instance, and currently even if the editor is destroyed they remain in memory for that page. So creating and destroying tens or hundreds of instances means that the page will keep references to functions and objects that could be cleaned up, but as there is some reference to it the js engine can't do it automatically.

That pattern is very clear with the tools.AddFunction: you pass a function to it and it stores that in an array, if we don't clear the reference in that array the function will remain in memory as long as the page lives, and usually those functions include references to JS that have a DOM object in its .$ so the DOM object will also remain in memory even if that function won't ever be called again.

[Garry Yao]

Originally I thought the problem was a typical ​IE Circular References Leak but alfonsoml's above comment has clarified a JS closure leak due to “persisted closures”(closures that referencing DOM and editor are been improperly stored in a global object), the patch's successfully caught two known leak patterns so far (CKEDITOR.dom.element::on and CKEDITOR.tools.addFunction) and thus eliminates the significant leak when creating/destroying multiple editor instances (with all plugins) in IE, I can also confirm it brought extra memory relief to other browsers as well.

Minors issues found in the patch:

1. IMO the fixings for 'showborders' plugin could be as simple as lifting the dialog related part outside of 'afterInit' method.
2. We should introduce similar fix to 'CKEDITOR.dom.element::removeEventListener' as 'removeAllListeners', it's just to keep consistency, in case there're future usages of the former.

[Alfonso Martínez de Lizarrondo]

Good advice about the 'showborders' plugin, I've implemented it in the new patch, but there's already CKEDITOR.dom.element::removeListener so no change about that.

[Alfonso Martínez de Lizarrondo]

Revised patch

[Garry Yao]

Please give necessary coding styles fixes before commits.
Congratulation!

[Alfonso Martínez de Lizarrondo]

Fixed with [5231]

I've added an extra clean up for the new editor.config.elementsPath_filters because it was creating new leaks.

Filed #5317 as follow-up and the ​docs have this new info.

[Alfonso Martínez de Lizarrondo]

[5249] includes an additional protection if the elementsPath plugin isn't loaded.

[Alfonso Martínez de Lizarrondo]

Another leak fix with [5289].