Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#4555 closed Bug (fixed)

Memory Leak

Reported by: markuspaek Owned by: Alfonso Martínez de Lizarrondo
Priority: Normal Milestone: CKEditor 3.3
Component: General Version: 3.0
Keywords: Confirmed Review+ Cc: sthiebaudt@…

Description

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

Attachments (9)

ie6_memory_leak_snapshot.png (8.4 KB) - added by Garry Yao 8 years ago.
4555.patch (3.2 KB) - added by Alfonso Martínez de Lizarrondo 8 years ago.
Patch for core
leakTest.html (4.7 KB) - added by Alfonso Martínez de Lizarrondo 8 years ago.
Test page
4555_2.patch (6.3 KB) - added by Alfonso Martínez de Lizarrondo 8 years ago.
Improved patch
4555_3.patch (13.3 KB) - added by Alfonso Martínez de Lizarrondo 8 years ago.
Revised patch
4555_4.patch (12.1 KB) - added by Alfonso Martínez de Lizarrondo 8 years ago.
Proposed patch
4555_5.patch (8.4 KB) - added by Garry Yao 8 years ago.
4555_6.patch (11.3 KB) - added by Alfonso Martínez de Lizarrondo 8 years ago.
Revised patch
4555_7.patch (11.3 KB) - added by Alfonso Martínez de Lizarrondo 8 years ago.
Revised patch

Download all attachments as: .zip

Change History (29)

comment:1 Changed 8 years ago by Garry Yao

Keywords: IE Confirmed added
Milestone: CKEditor 3.2

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?

Changed 8 years ago by Garry Yao

comment:2 Changed 8 years ago by Garry Yao

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

comment:3 Changed 8 years ago by Sylvain

Cc: sthiebaudt@… added
Priority: NormalHigh
Version: 3.0.13.0

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.

comment:4 Changed 8 years ago by Frederico Caldeira Knabben

Milestone: CKEditor 3.2CKEditor 3.3
Priority: HighNormal

comment:5 Changed 8 years ago by Sylvain

Priority: NormalHigh

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.

comment:6 Changed 8 years ago by Alfonso Martínez de Lizarrondo

Priority: HighNormal

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.

comment:7 Changed 8 years ago by 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.

Changed 8 years ago by Alfonso Martínez de Lizarrondo

Attachment: 4555.patch added

Patch for core

comment:8 Changed 8 years ago by Alfonso Martínez de Lizarrondo

Keywords: Review? added; IE removed
Owner: set to Alfonso Martínez de Lizarrondo
Status: newassigned

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

Changed 8 years ago by Alfonso Martínez de Lizarrondo

Attachment: leakTest.html added

Test page

Changed 8 years ago by Alfonso Martínez de Lizarrondo

Attachment: 4555_2.patch added

Improved patch

comment:9 Changed 8 years ago by 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.

Changed 8 years ago by Alfonso Martínez de Lizarrondo

Attachment: 4555_3.patch added

Revised patch

comment:10 Changed 8 years ago by 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.

comment:11 Changed 8 years ago by 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

Changed 8 years ago by Alfonso Martínez de Lizarrondo

Attachment: 4555_4.patch added

Proposed patch

comment:12 Changed 8 years ago by 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.

comment:13 Changed 8 years ago by Garry Yao

Keywords: Review- added; Review? removed

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.

Changed 8 years ago by Garry Yao

Attachment: 4555_5.patch added

Changed 8 years ago by Alfonso Martínez de Lizarrondo

Attachment: 4555_6.patch added

Revised patch

comment:14 Changed 8 years ago by Alfonso Martínez de Lizarrondo

Keywords: Review? added; Review- removed

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.

comment:15 Changed 8 years ago by Garry Yao

Keywords: Review- added; Review? removed

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.

comment:16 Changed 8 years ago by Alfonso Martínez de Lizarrondo

Keywords: Review? added; Review- removed

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.

Changed 8 years ago by Alfonso Martínez de Lizarrondo

Attachment: 4555_7.patch added

Revised patch

comment:17 Changed 8 years ago by Garry Yao

Keywords: Review+ added; Review? removed

Please give necessary coding styles fixes before commits.
Congratulation!

comment:18 Changed 8 years ago by Alfonso Martínez de Lizarrondo

Resolution: fixed
Status: assignedclosed

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.

comment:19 Changed 8 years ago by Alfonso Martínez de Lizarrondo

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

comment:20 Changed 8 years ago by Alfonso Martínez de Lizarrondo

Another leak fix with [5289].

Note: See TracTickets for help on using tickets.
© 2003 – 2017 CKSource – Frederico Knabben. All rights reserved. | Terms of use | Privacy policy