Ticket #5368 (closed Bug: fixed)

Opened 4 years ago

Last modified 4 years ago

Memory leak in setData function

Reported by: oarnault Owned by: alfonsoml
Priority: Normal Milestone: CKEditor 3.3
Component: General Version: 3.0
Keywords: Review+ Cc: oarnault@…

Description

Hi

I think there is a memory leak in the function CKEDITOR.editor.setData().

OS : WIndow 7 Firefox 3.6

I create an editor and a button. When I click the button I put 10 time 1 million 'a' in the editor using the function setData. At the beginning my brower took 69MO , after 10 click on the button it goes 330MO. (I have waiting for the GC to collect).

It is a problem for me because I'm using the ckeditor in an ajax application and so never reloaded the page moreover my customer does big Word paste into the editor.

I join my simple html witch illustate that.

Thanks.

Attachments

memoryLeak.html (1.0 KB) - added by oarnault 4 years ago.
5368.patch (1.4 KB) - added by alfonsoml 4 years ago.
Proposed patch

Change History

Changed 4 years ago by oarnault

comment:1 Changed 4 years ago by oarnault

  • Cc oarnault@… added

comment:2 Changed 4 years ago by alfonsoml

  • Status changed from new to closed
  • Resolution set to duplicate

If you test the current code you'll see that there's almost no increase after fixing #4555. Also, the test is too rude and it should wait for the new data to have been set before trying to change it again.

This avoids to hang the browser with the test:

	var counter=0

	function fillTextArea() {
		if ( ++counter > 10 ) 
			return;
		
		document.title=counter;
		window.CKEDITOR.instances['editor1'].setData(value, fillTextArea);
	}

comment:3 follow-up: ↓ 4 Changed 4 years ago by oarnault

I update to nightly build and use the following code

<html xmlns="http://www.w3.org/1999/xhtml">
<head>
<title>CKEditor memory leak</title>
<meta content="text/html; charset=utf-8" http-equiv="content-type" />
<script type="text/javascript" src="ckeditor/ckeditor.js"></script>

<script type="text/javascript">
	var value = "";
	for ( var i = 1; i < 1000000; i++) {
		value += "a";
		if (i % 100 == 0) {
			value += "<br />";
		}
	};

	
	var counter=0

	function process() {
		counter=0;
		fillTextArea();
	}
	
	function fillTextArea() {
		if ( ++counter > 10 ) 
			return;
		
		document.title=counter;
		window.CKEDITOR.instances['editor1'].setData(value, fillTextArea);
	}

	
</script>

</head>


<body onload="CKEDITOR.replace( 'editor1'); window.CKEDITOR.instances['editor1'].setData(value) ">

<button onclick="process()">Put 10 times 1 million 'a' in textArea</button>
<textarea id="editor1">
</textarea>
</body>
</html>

I loose about 30MO by click on the button.

comment:4 in reply to: ↑ 3 Changed 4 years ago by oarnault

Replying to oarnault:

I update to nightly build and use the following code

<html xmlns="http://www.w3.org/1999/xhtml">
<head>
<title>CKEditor memory leak</title>
<meta content="text/html; charset=utf-8" http-equiv="content-type" />
<script type="text/javascript" src="ckeditor/ckeditor.js"></script>

<script type="text/javascript">
	var value = "";
	for ( var i = 1; i < 1000000; i++) {
		value += "a";
		if (i % 100 == 0) {
			value += "<br />";
		}
	};

	
	var counter=0

	function process() {
		counter=0;
		fillTextArea();
	}
	
	function fillTextArea() {
		if ( ++counter > 10 ) 
			return;
		
		document.title=counter;
		window.CKEDITOR.instances['editor1'].setData(value, fillTextArea);
	}

	
</script>

</head>


<body onload="CKEDITOR.replace( 'editor1'); window.CKEDITOR.instances['editor1'].setData(value) ">

<button onclick="process()">Put 10 times 1 million 'a' in textArea</button>
<textarea id="editor1">
</textarea>
</body>
</html>

I loose about 30MO by click on the button.

I wait the counter reach 10 before click again

comment:5 Changed 4 years ago by alfonsoml

  • Status changed from closed to reopened
  • Resolution duplicate deleted

That's funny. Seems that I tested IE or didn't look clearly at the data. There are still leaks, but fortunately thanks to #5317 it was easier to find them.

Well, find them mostly...
Before the patch I get this usage after each set of runs:

60.9	93.7	113.0	133

And now it's like this:

62.5	75.3	82.2	82.9	81.4	83.0	82.4

There's still an increase on first runs, (but much lower), and then it seems to stabilize a little over the 82Mb, and if it was a real leak then it should keep increasing always.

So for the moment I'm not gonna try to find out if there's another extra leak because I don't think so given the current data.

Changed 4 years ago by alfonsoml

Proposed patch

comment:6 Changed 4 years ago by alfonsoml

  • Status changed from reopened to new
  • Keywords Review? added
  • Owner set to alfonsoml

comment:7 Changed 4 years ago by garry.yao

  • Keywords Review+ added; Review? removed
  • Version set to 3.0
  • Milestone set to CKEditor 3.3

Please rename 'clearCustomData' to 'onDispose' before commit, we would introduce such convention for leak protection in other places.

comment:8 Changed 4 years ago by alfonsoml

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

Fixed with [5289]. The #4555 change log entry is enough for this.

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