Ticket #4555 (closed Bug: fixed)

Opened 5 years ago

Last modified 4 years ago

Memory Leak

Reported by: markuspaek Owned by: alfonsoml
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

ie6_memory_leak_snapshot.png (8.4 KB) - added by garry.yao 5 years ago.
4555.patch (3.2 KB) - added by alfonsoml 4 years ago.
Patch for core
leakTest.html (4.7 KB) - added by alfonsoml 4 years ago.
Test page
4555_2.patch (6.3 KB) - added by alfonsoml 4 years ago.
Improved patch
4555_3.patch (13.3 KB) - added by alfonsoml 4 years ago.
Revised patch
4555_4.patch (12.1 KB) - added by alfonsoml 4 years ago.
Proposed patch
4555_5.patch (8.4 KB) - added by garry.yao 4 years ago.
4555_6.patch (11.3 KB) - added by alfonsoml 4 years ago.
Revised patch
4555_7.patch (11.3 KB) - added by alfonsoml 4 years ago.
Revised patch

Change History

comment:1 Changed 5 years ago by garry.yao

  • Keywords IE Confirmed added
  • Milestone set to 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 5 years ago by garry.yao

comment:2 Changed 5 years ago by garry.yao

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

comment:3 Changed 5 years ago by tsylvai

  • Priority changed from Normal to High
  • Cc sthiebaudt@… added
  • Version changed from 3.0.1 to 3.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 5 years ago by fredck

  • Priority changed from High to Normal
  • Milestone changed from CKEditor 3.2 to CKEditor 3.3

comment:5 Changed 4 years ago by tsylvai

  • Priority changed from Normal to High

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 4 years ago by alfonsoml

  • Priority changed from High to Normal

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 4 years ago by tsylvai

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 4 years ago by alfonsoml

Patch for core

comment:8 Changed 4 years ago by alfonsoml

  • Status changed from new to assigned
  • Owner set to alfonsoml
  • Keywords Review? added; IE removed

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 4 years ago by alfonsoml

Test page

Changed 4 years ago by alfonsoml

Improved patch

comment:9 Changed 4 years ago by alfonsoml

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 4 years ago by alfonsoml

Revised patch

comment:10 Changed 4 years ago by alfonsoml

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 4 years ago by alfonsoml

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 4 years ago by alfonsoml

Proposed patch

comment:12 Changed 4 years ago by alfonsoml

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 4 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 4 years ago by garry.yao

Changed 4 years ago by alfonsoml

Revised patch

comment:14 Changed 4 years ago by alfonsoml

  • 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 4 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 4 years ago by alfonsoml

  • 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 4 years ago by alfonsoml

Revised patch

comment:17 Changed 4 years ago by garry.yao

  • Keywords Review+ added; Review? removed

Please give necessary coding styles fixes before commits.
Congratulation!

comment:18 Changed 4 years ago by alfonsoml

  • Status changed from assigned to closed
  • Resolution set to fixed

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 4 years ago by alfonsoml

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

comment:20 Changed 4 years ago by alfonsoml

Another leak fix with [5289].

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