about memory use (Trac #30)

[sleemanj]

Hi again! I used htmlarea, but when I saw xinha I think this is very cool and faster. I noticed in htmlarea that when I reload a page with the component, the memory was increased in 2mb more, and each refresh that I made add 2mb more to the ie process, I try refreshing the page between six to ten times, and the components hang, maybe when I try two or three times. Well, that was because I use xinha and saw the same problem, I feel Xinha is more faster, but when I refresh the page the memory doesn??t be purged and only increment the ammount of the ie process.

There are some posts in htmltextarea forum (only if you want to see more information) search "memory leak".

thanks for Xinha ;) forgottentowers

Reported by forgottentowers, migrated from http://trac.xinha.org/ticket/30

[sleemanj]

Chris Allison commented:

You aren't going to like this answer, but here is some information about this bug. I believe this bug is actually caused by an IE bug with closures. Closures are some type of programming method that can be used in Javascript. I do not know too much about them. However, I believe that Xinha/htmlArea uses them a lot. If you want to read more about them and IE's problem with them, please read the following:

http://www.bazon.net/mishoo/articles.epl?art_id=824

That was written by Mishoo who is the original developer of htmlArea. He does not refer to htmlArea directly about this problem, but it is my opinion that this is probably the same cause of the memory leak in htmlArea.

In summary, I believe the core of Xinha probably uses closures a lot, and it would not be an easy task to remove them. James could probably add more input here, since he probably knows much more about closures than I.

[sleemanj]

• @sleemanj commented:

I'm changing this to very low priority, simply because it's probably impossible to fix.

Reading Mish's text, I agree that it is an IE problem where by the garbage collector doesn't handle recursive references correctly. Unfortunatly, removing recursive references would be all but impossible (actually, it probably would be impossible).

Mish's workaround would similarly be impossible to implement.

It's still something we need to keep in mind, but I don't think it can be fixed.

• @sleemanj changed keywords to IE memory leak
• @sleemanj changed priority from high to lowest
• @sleemanj changed severity from major to normal

[sleemanj]

ianb@colorstudy.com commented:

Here's a couple more articles on the problem:

http://jgwebber.blogspot.com/2005/01/dhtml-leaks-like-sieve.html http://www.quirksmode.org/blog/archives/2005/02/javascript_memo.html

My still-vague impression is that it really kills you when you put Javascript objects (especially things like closures) into the DOM, like in an event handler. This script claims to fix this problem, and would probably be easy to apply:

http://novemberborn.net/javascript/event-cache

[sleemanj]

• @sleemanj commented:

Hmmm, that solution looks interesting, I don't know if it would solve all the problem with the IE memory leaks, but certainly it could help. Changing the priority so we can investigate this more (if somebody wants to give it a go, please do, it should be a matter of finding where events are being set and tweaking appropriatly - I can have a look, but might not be soon).

• @sleemanj changed description from:

Hi again! I used htmlarea, but when I saw xinha I think this is very cool and faster. I noticed in htmlarea that when I reload a page with the component, the memory was increased in 2mb more, and each refresh that I made add 2mb more to the ie process, I try refreshing the page between six to ten times, and the components hang, maybe when I try two or three times. Well, that was because I use xinha and saw the same problem, I feel Xinha is more faster, but when I refresh the page the memory doesn??t be purged and only increment the ammount of the ie process.

There are some posts in htmltextarea forum (only if you want to see more information) search "memory leak".

thanks for Xinha ;) forgottentowers

to:

Hi again! I used htmlarea, but when I saw xinha I think this is very cool and faster. I noticed in htmlarea that when I reload a page with the component, the memory was increased in 2mb more, and each refresh that I made add 2mb more to the ie process, I try refreshing the page between six to ten times, and the components hang, maybe when I try two or three times. Well, that was because I use xinha and saw the same problem, I feel Xinha is more faster, but when I refresh the page the memory doesn??t be purged and only increment the ammount of the ie process.

There are some posts in htmltextarea forum (only if you want to see more information) search "memory leak".

thanks for Xinha ;) forgottentowers

• @sleemanj changed priority from lowest to highest

[sleemanj]

anonymous commented:

Internet Explorer being the most widely used browser, and memory being wasted in huge amounts, this problem should have highest priority AND severity.

It effectively renders the component ABSOLUTELY useless for production purposes, and it will never leave beta state if this is not fixed.

Perhaps too much 'recursion' and 'closure' and stuff is used and depended upon, and these kind of problems occur by depending too much on 'garbage collectors' and so on. In C++, this would not happen.. simply because then you are forced to be careful about your memory usage, and terminating objects appropriately.

Closures are not such a nice thing, then. Through a quick scan I conclude it is similar to 'induction of messy programming logic'. A reflection of this is the problem temporarily set to 'low priority' - how could a showstopper be set to low priority? It might result in lots of code being written that would later need re-engineering (as seems to be the case).

Luck is not an issue. Agreeing that the problem is located is way different from analysis of the actual problem.

[sleemanj]

niko commented:

ABSOLUTELY useless for production purposes is imho not true.

Although you are completely right - we must work on this high priority. I read the articles and got an idea of what is happening and what is the problem - but i'm not that a good JS-coder that i might find a solution on this...

[sleemanj]

mike.codeismylife.com commented:

Perhaps this might help a bit. There is spoken of 'IE memory leaks due to a bug in closure - garbage collection'.

At least the first part is not the case - it is not an IE bug!. Mozilla has it too! Only, in IE, the memory-pile-up per page is about 2-3mb, and in mozilla about 500kb. So it would take 6 times more page refreshes before the critical stage is reached in mozilla.

Implicated by this is that looking into 'closing objects' properly (IF it is a closure bug!) is probably more worthwile than orienting to Internet Explorer specific bugs.

Perhaps there is simply a static variable/object somewhere (like, 'window.opener.staticobject..') that has an array that piles up.. and not a 'closure' bug at all. Some instances are kept even after a page unloads.

[sleemanj]

ianb@colorstudy.com commented:

Here's a related bug for FCKEditor, which also proposes some solutions in that context. Many of them could apply to Xinha as well:

http://sourceforge.net/tracker/index.php?func=detail&aid=1162200&group_id=75348&atid=543653

[sleemanj]

• @sleemanj commented:

mike: It's almost definatly a closure bug. I can't think of anything that would be assigned in such a way as you suggest. Even assigning to window.something should be destroyed on a reload/exit. It's simply that recursive references are not GCd correctly by IE (you also suggest moz also has a problem, but I have not seen it).

anon: What a load of baloney, I have HTMLArea and Xinha both in production use by many people using IE.

Javascript is DESIGNED such that garbage collection is an integral part of the language, we are SUPPOSED to rely on garbage collection, that's how the language works, infact there isn't any way of "freeing" memory all you can do is unset a reference - GC has to do the rest.

Same goes for closures, the idea of a closure is central to Javascript as a language. Even more so for recursion, which is a basic programmming paradigm of any high level language. Your implication that GC, closures and recursion is indicative of "messy programming" is simply silly.

As for the priority being low, it's because this is ultimatly a browser bug, at least as far as anybody can figure out, as we don't work for Microsoft we don't have a lot of influence, and as it generally works OK it was put at lower priority until more serious problems were corrected. It's now at a higher priority because a possible workaround has been discovered and will be looked at as soon as possible.

As far as your implication of "if you'd written this in C++ this wouldm't be a problem", do yourself a favor and shut up now because you don't have a clue what you are talking about.

• @sleemanj changed milestone to Version 1.0
• @sleemanj changed version to 0.1

[sleemanj]

http://www.dynarch.com/ commented:

Mike is almost right, though clueless. :-)

It's not a closure bug--but it's a bug that's most commonly triggered by closures. The bug is simply that if you assign to some element (A) an object (B) that holds a reference to (A), then we have a circular reference. IE doesn't properly destroy them when at least one of the objects is a DOM node or an ActiveX object.

Why closures trigger it?--because if you have:

  A = document.getElementById("object_a"); // A is a DOM node
  A.action = function() { do something; };

then you already (most probably unwillingly) created a circular reference, because the anonymous function can access the A object, and the A object already keeps a reference to the anonymous function.

As simple as it seems, fixing it in HTMLArea/Xinha/WhatEver requires huge amounts of work--practically all the code needs to be refactored, not to mention the code size would probably double.

Whoever says that closures or recursion is an example of bad programming can burn in hell. A "closure" is a mathematical term older than programming itself and it makes an extremely convenient way to program, that worked well in the first Lisp machines 30 years ago or so. Same goes for recursion. Don't make me feel that I can't use them now, just because IE can't get it right. I believe that people that become conscious about increased RAM usage and are unhappy with this, can click 3 times to install Firefox to be happier. :-p It's really much easier for someone to install a new browser than it is for me to "fix" HTMLArea, or for Gogo to "fix" Xinha.

Cheers.

[sleemanj]

mike.codeismylife.com commented:

gogo->mike

(1) 'Even assigning to window.something should be destroyed on a reload/exit. '

Should sometimes not.

Although this might certainly not be the source of the problems regarding this topic, try this code. In a normal window, nothing happens. BUT open this code in a popup window, and you see objects (and thus, memory) piling up. This is true even when the window that opens the popup is closed! So even when a window has been closed, it can still remain an object!

reload.

to: http://www.dynarch.com/ Note that the problem is also existent in Firefox, so asking people to install or use another browser is not a real solution...

[sleemanj]

ianb@colorstudy.com commented:

This is a good example of the problem:

  A = document.getElementById("object_a"); // A is a DOM node
  A.action = function() { do something; };

But it's not a hard problem to fix. Use the above mentioned Event Cache and replace every A.action = function ... with add(A, 'action', function ...); the changes are localized and easy. Event Cache removes all event handlers on page unload, breaking the circular references. This is what Mozilla already does (which is why the memory leak there isn't bad), and what Event Cache brings to IE.

[sleemanj]

@sleemanj commented:

mike: [code] if (window.opener) { if (!window.opener.myList) {

    window.opener.myList = new Array();

} window.opener.myList[window.opener.myList.length] = new Object(); alert("The window.opener object container " + window.opener.myList.length + " objects."); } </script> <a href="javascript:window.open('?', '_self');"> reload.</a>

[/code]

Well duh. That's not a problem at all. window.opener is a reference to the opening window, which of course doesn't get destroyed when you reload the opened window, so anything you put in the opening window stays when you reload (or destroy!) the opened window. Reload the opening window though and it should be GC'd correctly, provided the opened window doesn't hold a reference to the object locally of course.

ian: yes, as I said, EventCache could be a partial workaround, it would not solve all the problems but would probably help a bit.

[sleemanj]

@sleemanj commented:

PS: mike, I don't think you understand closures, it's not about "closing objects" as you refer to it above.

[sleemanj]

ianb@colorstudy.com commented:

What problems wouldn't Event Cache fix? My understanding is that the DOM objects live in a separate memory space from Javascript objects. GC across that boundary works, but only with reference counting. There's no way for the GC to find a cycle where a JS object refers to a DOM object that refers back to that JS object. But cycles that don't involved objects outside those spaces (i.e., cycles of completely JS objects, or completely DOM objects) are discovered. (They better, since especially DOM objects have references to both their parent and children, thus every DOM tree is full of cycles -- cycles aren't as common in Javascript, I guess, but I think IE still uses a proper GC.)

From what I understand only DOM objects are like this -- they are implemented with COM (XPCOM for Gecko), and it's a well-known problem that cycles can't be detected across the COM barrier (at least that's the impression I get -- I don't do Windows programming so this is the only place I notice this stuff). Well, XMLHttpRequest objects are also COM objects, and maybe there's some other similar objects that Xinha uses. But again, as long as you break all those references on page unload you'll be okay. You could probably use Event Cache for those too -- I think all it really does is set an attribute and remember that object/attribute so it can unset it later; that the attributes are "events" isn't really important.

[sleemanj]

@sleemanj commented:

ian: '''What problems wouldn't Event Cache fix? '''

I don't know off the top of my head without looking at the problem further. but I suspect there would be some. Event handlers are not the only place where circular references would be recreated.

But EventCache would probably help with much of it.

[sleemanj]

• @sleemanj commented:

I have essentially put in place an Event flushing system like EventCache into the Xinha core. The bad news, it makes no discernable difference. IE still eats about 10 meg extra each load.

I have checked in the changes anyway (changeset:177 , but use changeset:178 because I checked in some extra bits in 177 by mistake) as they won't hurt any (and include some other minor fixes here and there, and of course, maybe I made a mistake causing it to not work) but put simply event flushing does not solve the problem, as I expected. IT IS NOT ENABLED BY DEFAULT, to "enable" it you need to add window.onunload = HTMLArea.flushEvents() somewhere, IT IS CURRENTLY ENABLED IN testbed.html so you can play with that.

There is likely other recursive (not sure if they even need to be recursive) references from DOM elements to javascript functions that do not involve event handling. But frankly, I don't like our chances of tracking those down, even if we did I don't know if they could be worked around (it's NOT fixing it, it IS a bug in IE, IE's GC is broken). Besides, if IE doesn't even know where the references are, how are we supposed to :(

If people would like to experiment with what I have done, use the examples/testbed.html file which loads no plugins except FullScreen and runs the event flushing onunload (and alerts to tell you how many events it flushed).

So people, where I stand is, flushing events doesn't help one bit. Further more, it's a (serious) bug in IE, and I don't think we can reasonably "fix" it (perhaps neither can Microsoft), it would probably require a rewrite from the ground up, and even then I don't think we can "fix" it. I'd be interested to know if the competition has similar problems though.

I'm going to leave the ticket open and reassign back to low priority, because there's not much we can do about it, if anything.

• @sleemanj changed priority from highest to lowest
• @sleemanj changed severity from normal to major
• @sleemanj changed version from 0.1 to ``

[sleemanj]

ianb@colorstudy.com commented:

Incidentally, here's a tool for measuring leaking in IE:

http://jgwebber.blogspot.com/2005/05/drip-ie-leak-detector.html

It seems to be able to give fairly specific lists of leaking elements, but I haven't tried it on Xinha.

[sleemanj]

anonymous commented:

FCKEditor 2.0 FC says it cleared all memory-leaks.

http://sourceforge.net/tracker/index.php?func=detail&aid=1162200&group_id=75348&atid=543653

[sleemanj]

Stuart_T commented:

I was hoping to use XINHA as a replacement for HTMLAREA in our CMS system but this memory leakage problem makes it quite unusable. After 6-7 page saves IE memory is over 100MB and after that the system just hangs. This problem does not happen with HTMLAREA. I'm using IE 6.0 .

This is a big pity because there are some really excellent features in XINHA but it is effectively unusable in IE . I really feel that you should UP the priority on this as otherwise it's going to be a White Elephant.

[sleemanj]

mokhet commented:

Justin Rogers, a microsoft developper, has released an article about memory leaks. http://blogs.msdn.com/ie/archive/2005/06/21/431376.aspx http://msdn.microsoft.com/library/default.asp?url=/library/en-us/IETechCol/dnwebgen/ie_leak_patterns.asp

[sleemanj]

anonymous commented:

After working with Xinha for several hours attempting to address some of the memory leak issues, I finally came to the conclusions that:

1) The severity of this issue cannot be overstressed.

2) The problem seems endemic within the code, going back to previous iterations of HTMLarea (possibly a contributing reason that HTMLarea no longer is in circulation),

3) Blaming Internet Explorer's poor garbage collection won't solve the problem.

4) The latest release of fckeditor has no serious memory issues.

I've evaluated software for a good deal of my professional life. I'm afriad this one was a no brainer. If it wasn't for my stubbornness, I probably would have given up on it before investing as much of my times as I did. Good luck on solving this one, but if you don't, then ''sayonara''

[sleemanj]

@sleemanj commented:

2) The problem seems endemic within the code, going back to previous iterations of

Well, it is endemic, but it's not a problem with the code, it's simply that the code is written in a perfectly normal way for Javascript to be written in (infact, just in the way Javascript is supposed to be used), but IE just doesn't handle it well.

HTMLarea (possibly a contributing reason that HTMLarea no longer is in circulation),

The only contributing reason for that is that HTMLArea was left to rot.

3) Blaming Internet Explorer's poor garbage collection won't solve the problem.

No. But that is where the blame squarely lays. IE has a broken garbage collector, hideously broken, it makes a mockery of what Javascript is supposed to be. There comes a point where you have to decide, do we do it right, or do we faff about and write kludgy code so that we don't tread on IE's toes.

4) The latest release of fckeditor has no serious memory issues.

Last I read FCK still leaks about 2m an instance (admittedly, we can leak 10m). I havn't looked at FCK's code, but I imagine you'll find it wasn't written in such a closure-dependant way, which is fine, but it's not using Javascript to it's full. HTMLArea, from which Xinha was derived, is written in a closure dependant, perhaps even closure intense manner, it makes for nice clean code, and it's how Javascript is supposed to be used.

I'm all for helping IE out where we can, but personally, I'm more concerned with fixing the real bugs, THIS IS NOT A BUG, not in Xinha, and therefore can take a back seat (especially since, even if it does leak, Xinha can still be used in IE, in my experience at least).

If somebody else wan'ts to give it a go, by all means, go for your life.

[sleemanj]

@sleemanj commented:

I have some good news, and some bad news.

== The Good ==

The good news is that I have come up with something of a solution, and implemented it, and tested, and checked it in (changeset:277) and it works!

== The Bad ==

The bad news is that I have only patched up the majority of the leaks IN THE CORE, I havn't looked at patching them in plugins, and I would like some help in doing that.

What I did was...

1. Added the function HTMLArea.freeLater(obj, prop) this function adds the given object and property name to an array of items that we need to free (well, set to null in javascript terms) "later" (ie, onunload). The property is optional, if not specified all properties of the object are freed. The actual freeing is done onunload by HTMLArea.free() which is called by HTMLArea.garbageCollectForIE()

2. I painstakingly worked through htmlarea.js, everywhere I saw that we assign a DOM element to a n object property I added a call to HTMLArea.freeLater() to cause that object property to be freed (set null) onunload.

3. At the same time I found the places where we assign some javascript to a property of a DOM element, and used HTMLArea.freeLater() to clear those too.

Of course previosuly I had implemented an EventCache system into the existing functions for adding events, and these are a necessary part of leak clearance also (i.e. make sure there is no setting of element.onwhatever = ..... use the correct HTMLArea.addDom0Event or whatever which will clear the leak for you).

== The Ugly ==

That's it, that's "all" that's required to eliminate a LOT of leaks (mostly the ones eliminated were around the toolbar, not surprisingly the images there caused a lot of leakage). But I think there will be a LOT of work involved in finding and clearing these leaks, so people, I've done the leg work in figuring out how to fix it, now you guys pick a plugin and get fixing.

FYI without these fixes Xinha eats 7 to 10 meg per load for a BARE editor (no plugins), with the fixes this is reduced to 2 at the most, infact using drip the usage grew for a few loads, then actually DROPPED several meg, so I think roughly it's pretty steady in memory usage. The core that is, no plugins.

[sleemanj]

david commented:

I'd like to clarify what situations lead to memory leaks. From reading the comments above, I've come to the conclusion that especially the term "closures" is misunderstood in this context. Before I begin, I would like to stress that this is not an IE-only problem. It's not even a problem. Leaking is a necessary evil. I'll explain why. There are three major potential types of memory leaks: == (XP)COM<->DOM references == GCs have a hard time freeing those up. Whenever you do something like

myobject.element = document.getElementById('foo');
myobject.element.obj = myobj.element;

then you have a reference between DOM elements and JavaScript objects. Those two are implemented at different levels. The GCs cannot remove these circular references. It's easy to work around them, tho, because you may use a global array or something similar to map elements to objects and vice versa. == Inline event handlers ==

element.addEventListener('click', function(e) { alert('clicked!'); }, false);

will create a memory leak. The easiest approach is to unregister these constructs using a simple event caching functionality which does removeEventListener for all items on window.onunload. == Closures == Closures per se are not evil. Closures themselves don't leak either. But if you access an object inside the closure that is not in the closures scope, a reference to that object and thus to the outer construct has to be maintained even if the outer construct has already been left, e.g.:

function foo()
{
  var inner = 5;
  function bar()
  {
    return inner + 5;
  }
}

voila, a memory leak == Leaking by accident == Leaks may also often happens when using temporary variables that are just there to improve readability or whatever. Consider the following:

function eventHandlerFactory()
{
  // I'm returning a function. Nevermind me, it doesn't matter much what exactly I do
}
function setEverythingUp()
{
  // blah
  var myeventhandler = eventHandlerFactory('arg', 'gu', ments);
  document.getElementById('foo').addEventListener('mouseover', myeventhandler, false);
}

The author of the above had good intentions, because he didn't use inline event handlers. However, this will also leak, because of the ''myeventhandler'' variable. The right way to register the event handler, without leaking memory, is this:

function setEverythingUp()
{
  // blah
  document.getElementById('foo').addEventListener('mouseover', eventHandlerFactory('arg', 'gu', ments), false);
}

I hope I was able to shed some light on this issue. Please don't say "it's impossible". It's far from impossible. And by looking at some of the numbers here and in the forums, I get the impression that Xinha really needs some leaks fixed ;) I'm afraid I don't have any free time right now to jump in and help you guys out with this, but I'll do so as soon as I can. Keep it up, folks, you're doing an awesome job with this editor.

Ah yeah and... here are some links worth reading:

http://jgwebber.blogspot.com/2005/01/dhtml-leaks-like-sieve.html http://jgwebber.blogspot.com/2005/05/drip-ie-leak-detector.html http://jgwebber.blogspot.com/2005/06/drip-redux.html http://jgwebber.blogspot.com/2005/06/drip-02.html http://jgwebber.blogspot.com/2005/06/another-word-or-two-on-memory-leaks.html

http://msdn.microsoft.com/library/default.asp?url=/library/en-us/IETechCol/dnwebgen/ie_leak_patterns.asp

http://jibbering.com/faq/faq_notes/closures.html

[sleemanj]

@sleemanj commented:

david writes: ''I would like to stress that this is not an IE-only problem. It's not even a problem. Leaking is a necessary evil.''

And I'd like to stress that's a load of crap. IE leaks because of a poor design, particularly when you create circular references between DOM objects (COM) and javascript. This IS A BUG IN IE. How many times do I have to say it. A proper garbage collector WOULD NOT LEAK even with circular references. IE's garbage collector does not properly account for the circular references IN THIS CASE where the reference circulates through both COM (DOM Elements are implemented as COM objects in IE) and javascript. It is of course possible to work around this, as I have posted above, but the fact remains that IE's garbage collector does not collect the garbage in this case, and thus, IT is what is broken.

The closure you write is not a leak. That's by design. That's how closures work. That's the entire point of closures. It's when your closure creates circular reference through a DOM element in IE that you start to leak. In IE. Because IE's garbage collector doesn't handle this case properly. BECAUSE IE IS BROKEN. Unfortunatly these circularities can be hard to spot.

= Please. Everybody. For the LAST TIME. =

The bug is a bug in IE. IE leaks memory severely because it's garbage collector does not handle the case when a circular reference passes through a COM object, unfortunatly, IE's html element objects as implemented as COM objects (which creates other problems as well, but lets not go there), ergo, if you create a circular reference from the javascript scope, to the dom, and back again, you get a leak unless you manually go and break the circularity. My post directly above david's provides a means of doing this reasonably pain-free.

In Xinha we create lots of circular references, and that means that we get lots of leaks, more so, if just one of those circularities involves the Xinha core, the entire Xinha setup could be un-garbage-collectable in IE. So it's important that we err on the side of caution and use the methods I have written above and in the Xinha source to nullify anything that looks like it could create a circular reference.

'''But the fact remains that this is a bug in IE, it's because of the design of IE and no matter what anybody else says, at the end of the day it does mean that IE's GC is broken because IE's ''Garbage Collector'' doesn't actually ''collect the garbage'' in this particular instance. It doesn't do what it is supposed to do. What we do to fix it is working around a big problem in IE. You cannot deny that. Even the IE developers admit it.'''

Jeez. Sometimes I feel like I'm smacking my head against a brick wall when it comes to explaining this stuff.

[sleemanj]

david commented:

gogo... I'm sorry, but you're not correct here. Gecko will leak memory, too, because the garbage collector cannot destroy circular references between DOM and XPCOM objects. It's not an IE-only problem. Gecko doesn't leak as much memory as IE, but it '''does''' leak. And the closure there ''will'' leak, because '''no''' GC could clean up the "inner" var.

[sleemanj]

@sleemanj commented:

The closure you wrote (technically bar is the closure):

function foo()
{
  var inner = 5;
  function bar()
  {
    return inner + 5;
  }
}

will most certainly not leak once references to foo have been destroyed. Mark. And. Sweep.

[sleemanj]

@sleemanj commented:

Just to clarify here david, because I think you may be confused, we are NOT talking about general memory usage while Xinha is running - that's not important (well, it is, but not very), it's memory usage after the page has been torn down by the browser, after you've navigated AWAY from Xinha, where there is no need for ANY of the memory Xinha allocated to remain allocated on the heap, and yet IE leaves it allocated and unusable - i.e. IE leaks.

Certainly Gecko leaks, but it really just leaks in a general sense, not in the hopeless and specific "x meg per page load" manner that IE does.

[sleemanj]

mwvdlee commented:

What is the current status of this bug and the fixes?

Last release of Xinha I downloaded (over half a year ago) had MSIE crash in just a few page loads. I understand that current versions MSIE will never really work without leakage but if Xinha can be loaded a few hundred (thousand?) times, then the problem is fixed for most practical purposes.

I know of several CMS systems which would like to switch to Xinha but are unable to because of these MSIE-related problems.

As far as the "blame game" goes; Xinha is not to blame, MSIE is. But since MSIE is not going to be fixed any time soon, and certainly not retroactively (current situation is that at best it will be fixed for WinXP-based MSIE's), Xinha is the only one who can fix it. It's a sad state, but it's reality.

Personally I'd put severity to "blocker", because for a very large portion of the people, Xinha simply does not work. But since there's FCKEditor which apparently has fixed these problems, there's a workaround.

[sleemanj]

mokhet commented:

http://laurens.vd.oever.nl/weblog/items2005/closures/

Looks like an interesting way to fix closure issue.

[sleemanj]

mleiv commented:

I am not familiar with the protocol for submitting updates and I do not even have the most current Xinha (mine in highly customized and a pain to synch up). And this closures thing went right over my head. But I did read the official MSDN doc on these leaks (http://msdn.microsoft.com/library/default.asp?url=/library/en-us/IETechCol/dnwebgen/ie_leak_patterns.asp it also maintain that this is correct behavior and we should just all be doing our own garbage collection like good little programmers, yay IE). And I did install the leak checker Drip. And then I just hacked my way around and tested what did and didn't work. Admittedly, my approach is inelegant and overkill. But the results were AMAZING. My IE works almost like my Firefox now. I can even leave it running for hours now. And there was only one closures issue I addressed (in _loadback).

What I did:

• Every instance of document.createElement() in htmlarea.js I followed with a HTMLArea.freeLater() so the dom element itself would be set to null on page unload (as instructed by msdn). I had to modify freeLater() a bit to accept whole objects, not just object/property pairs. Also, inline-dialog.js also needed a freeLater() added.

• I changed every .appendChild() to append from top down as instructed by msdn. This required extensive changes to createToolbar1() and generate() particularly.

• special cases : in _loadback() I changed the script insert to use HTMLArea._addEvent() for evt so it would free it up onunload. And in loadStyle() I did not use freeLater (caused errors).

• (from a previous fix, the only legit design flaw IMO) I changed the a._updateAnchTimeout stuff to use a global intervals array instead of the perpetually repeating timeouts, and then onunload I cleared those intervals, and all the _timerX timeouts.

And for anyone who says this is how good code is written - ARGH! No one should have to implement DOM this way. The insertion order leak alone was enough to make me scream for days.

[sleemanj]

mleiv commented:

I spoke too soon - IE still freezes up after too many page reloads (it just takes a little longer). dispirited sigh

[sleemanj]

bobesch commented:

Seems that the memory leakage problem with IE reappears heavily in the current SVN version. Using the full example without any plugins, I get a leak of about 10 MB on each reload. So I had a look at it. Using drip, I isolated three areas:

• _loadback()
• the toolbar button links, their containers and images
• the panels

The following patch reduces the leak to about 1,2 MB.

Index: htmlarea.js
===================================================================
--- htmlarea.js   (revision 528)
+++ htmlarea.js   (working copy)
@@ -1283,6 +1283,7 @@
           tb_cell.className = 'toolbarElement';
           tb_row.appendChild(tb_cell);
           tb_cell.appendChild(tb_element);
+          HTMLArea.freeLater(tb_element);
         }
         else if ( tb_element === null )
         {
@@ -1516,6 +1517,10 @@
     'sb_cell': document.createElement('td')  // status bar

   };
+
+  HTMLArea.freeLater(this._framework.tp_row);
+  HTMLArea.freeLater(this._framework.ler_row);
+  HTMLArea.freeLater(this._framework.bp_row);
   HTMLArea.freeLater(this._framework);

   var fw = this._framework;
@@ -6455,15 +6460,16 @@
   S.src = U;
   if ( C )
   {
-    S[HTMLArea.is_ie ? "onreadystatechange" : "onload"] = function()
+    var onLoad = function()
     {
       if ( HTMLArea.is_ie && ! ( /loaded|complete/.test(window.event.srcElement.readyState) ) )
       {
-        return;
+ return;
       }
       C.call(O ? O : this, B);
     };
   }
+  HTMLArea.addDom0Event(S, HTMLArea.is_ie ? "readystatechange" : "load", onLoad);
   document.getElementsByTagName("head")[0](../commit/0).appendChild(S);
 };

@@ -6742,7 +6748,8 @@
     }
     HTMLArea.free(HTMLArea.toFree[x].o, HTMLArea.toFree[x].p);
   }
+  HTMLArea.toFree = null;
 };

 HTMLArea.init();
-HTMLArea.addDom0Event(window,'unload',HTMLArea.collectGarbageForIE);
\ No newline at end of file
+HTMLArea.addDom0Event(window,'unload',HTMLArea.collectGarbageForIE);

[sleemanj]

mokhet commented:

Concerning the _loadback() function, can you check if nullyfing the variables (and especially the application scope) is reducing the leak ? I dont think the C (Callback) and B (Bonus) variables are guilty for the leak. I heavily suspect the O (scOpe) variable to be the one since it is creating a reference to the Xinha variable in a HTMLElement (the famous IE circular mess)

Please try this _loadback() version and tell us if it is reducing the leak or not at all ?

I would have do it myself, but unfortunatly, drip is not working here. Thanks.

HTMLArea._loadback = function(U, C, O, B)
{
  var S = document.createElement("script");
  S.type = "text/javascript";
  S.src = U;
  if ( C )
  {
    S[HTMLArea.is_ie ? "onreadystatechange" : "onload"] = function()
    {
      if ( HTMLArea.is_ie && ! ( /loaded|complete/.test(window.event.srcElement.readyState) ) )
      {
        return;
      }
      C.call(O ? O : this, B);
      C = null;
      O = null;
      B = null;
    };
  }
  document.getElementsByTagName("head")[0](../commit/0).appendChild(S);
};

[sleemanj]

mokhet commented:

Bahh me again, after looking more heavily at your patch, I have seen you have changed a bit the _loadback() by introducing the use of addDom0Event. I personnaly dont like addDom0Event, it allows us to have more than one type of event set on DOM0 but we loose too much of the DOM0 advantages imo (simplicity and application scope). Anyway, that's another ticket :)

Back to the leak, if you could confirm or not confirm that the O = null; is enough to break the leak, i would prefer a lot before thinking about the use of addDom0Event.

At least, if the O = null; doesnt breake the leak (C = null; and B = null; are imo totaly useless here), here is another version to test. thanks.

HTMLArea._loadback = function(U, C, O, B)
{
  var S = document.createElement("script");
  var T = HTMLArea.is_ie ? "onreadystatechange" : "onload";
  S.type = "text/javascript";
  S.src = U;
  if ( C )
  {
    S[T] = function()
    {
      if ( HTMLArea.is_ie && ! ( /loaded|complete/.test(window.event.srcElement.readyState) ) )
      {
        return;
      }
      C.call(O ? O : this, B);
      // LV: I think thoses 2 lines are useless
      C = null; 
      B = null;
      // LV: thoses 2 lines should break the IE leak
      O = null;
      S.[T] = null;
    };
  }
  document.getElementsByTagName("head")[0](../commit/0).appendChild(S);
};

[sleemanj]

mokhet commented:

damn it, i swear i had read and read again before submitting, but i'm seeing the mistake only now. it is obviously not S.[T] = null; but :

S[T] = null;

sorry

[sleemanj]

djeeg commented:

hi there.

using the latest nightly build i have managed to reduce memory usage between page refreshes in the xinha core to basically nothing. there are a only about 5 places that leak memory, but when they are fixed Drip 0.4 can refresh for ages without any problem.

this is though with no toolbar items and only the gethtml plugin. when i run testbed.html in examples, the memory usgae still spikes but not as much. my feeling is that if i apply the same logic to where the toolbar is displayed things will begin to work out.

[sleemanj]

guest commented:

my mistake, i have just tried adding the toolbar + plugins from the testbed.html file and the memory usage is stable. must have been the bad script loading from cache. where can i submit htmlarea.js for review?

items fixed: 1.free dom elements from buttons. 2.reorder framework dom element creation order. 3.correct closures in editor events. 4.remove drag event from editor events. 5.fix add/detach event logic. 6.fix script includes

number 4 needs a better solution, but its friday afternoon.

[sleemanj]

• bobesch changed cc from **forgottentowers**~~@hotmail.com~~ to **bobesch**~~@letras.net~~
• bobesch commented:

Hi Mokhet,

thanx for your answer. I'm rather new to Javascript hacking and this leak problem, while not being too difficult to understand, isn't too easy to solve in complex environments like Xinha, so the following reflections might be erronuous. I'm trying to learn, to understand and to help in pushing Xinha to version 1. Having read und reread this thread and the articles mentioned therein, I think that the main problem with _loadback() is not the circular reference to O, but the closure created by the anonymous function assigned to S[HANDLER]. Nullifying C,O and B does, err, nullify those objects, but leaves the closure intact. On the other hand, destroying the closure by nullifying just S[T] in your second test code, effectively resolves the problem - if and only if we can rely on drip 0.4. So the following version of _loadback() is fine:

HTMLArea._loadback = function(U, C, O, B)
{
  var S = document.createElement("script");
  var T = HTMLArea.is_ie ? "onreadystatechange" : "onload";
  S.type = "text/javascript";
  S.src = U;
  if ( C )
  {
    S[T] = function()
    {
      if ( HTMLArea.is_ie && ! ( /loaded|complete/.test(window.event.srcElement.readyState) ) )
      {
        return;
      }
      C.call(O ? O : this, B);
      S[T] = null;
    };
  }
  document.getElementsByTagName("head")[0](../commit/0).appendChild(S);
};

Using the addDom0Event() function in this context was certainly overkill. I wonder however why there are two functions of this sort, addDom0Event() and _addEvent(). Could someone of the more knowing people please explain?

-- The Stylist Plugin --

This said, further testing showed that the Stylist plugin also leaks. I'll try to explain why, hoping for some coment to see whether I begin to understand this leak problem the right way.

The Stylist() constructor in stylist.js adds the stylist panel to the right panel by saying:

editor._stylist = addPanel('right');

The DIV node returned by addPanel() serves as the container element for Stylist links aka A-tags which in turn, in their innerHTML(), reference style specifications held by editor.config.css_styles[]. Voilà the circular reference to destroy, right? So the above line must be followed by a freeLater() statement:

editor._stylist = addPanel('right');
HTMLArea.freeLater(editor, '_stylist');

HTMLArea._stylist however is not the only reference to the stylist panel. The other one is stored in editor._panels.right.panels[], as the stylist panel DIV is pushed onto this array in addPanel(). As this reference making concerns all plugins making use of panels, the right place to destroy this reference seems to be around line 204.

Index: htmlarea.js
===================================================================
--- htmlarea.js   (revision 528)
+++ htmlarea.js   (working copy)
@@ -200,10 +200,13 @@
       panels[i].container.className = 'panels ' + i;
       HTMLArea.freeLater(panels[i], 'container');
       HTMLArea.freeLater(panels[i], 'div');      
+      HTMLArea.freeLater(panels[i], 'panels');
+//This one might substitute the three lines above
+//HTMLArea.freeLater(panels);
     }
     // finally store the variable
     this._panels = panels;

Anyway, these two freeLater() statements make sure the stylist panel DIV doesn't leak memory.

Hoping for some coment, have a nice weekend,

Bodo

PS. Hi djeeg, couldn't post a diff of what you've done here. This way we could join efforts and make sure we are not duplicating work.

Cheers, Bodo

[sleemanj]

guest commented:

1. free dom element from buttons

line: 1520

      el = document.createElement("a");
      el.style.display = 'block';
      el.href = 'javascript:void(0)';
      el.style.textDecoration = 'none';
      el.title = btn[0](../commit/0);
      el.className = "button";
      el.style.direction = "ltr";
      // let's just pretend we have a button object, and
      // assign all the needed information to it.
      obj =
      {
        name : txt, // the button name (i.e. 'bold')
        element : el, // the UI element (DIV)
        enabled : true, // is it enabled?
        active : false, // is it pressed?
        text : btn[2](../commit/2), // enabled in text mode?
        cmd   : btn[3](../commit/3), // the command ID
        state : setButtonStatus, // for changing state
        context : btn[4](../commit/4) || null // enabled in a certain context?
      };
      HTMLArea.freeLater(el); <--  RELEASE THE <A>
      HTMLArea.freeLater(obj);

[sleemanj]

guest commented:

2. reorder framework

line 1591

 var htmlarea = this._framework.table;
  this._htmlArea = htmlarea;
  HTMLArea.freeLater(this, '_htmlArea');
  htmlarea.className = "htmlarea";

  var fw = this._framework;
  fw.table.id = "generate_table";
  fw.table.border = "0";
  fw.table.cellPadding = "0";
  fw.table.cellSpacing = "0";

  fw.tb_row.style.verticalAlign = 'top';
  fw.tp_row.style.verticalAlign = 'top';
  fw.ler_row.style.verticalAlign= 'top';
  fw.bp_row.style.verticalAlign = 'top';
  fw.sb_row.style.verticalAlign = 'top';
  fw.ed_cell.style.position     = 'relative';

  var textarea = this._textArea;
  textarea.className = 'xinha_textarea';

  fw.tb_cell.colSpan = 3;
  fw.tp_cell.colSpan = 3;
  fw.bp_cell.colSpan = 3;
  fw.sb_cell.colSpan = 3;

  var iframe = document.createElement("iframe");
  iframe.id = "generate_iframe";
  iframe.src = _editor_url + editor.config.URIs.blank;
  this._iframe = iframe;
  this._iframe.className = 'xinha_iframe';
  HTMLArea.freeLater(this, '_iframe');

  var statusbar = this._createStatusBar();
  var toolbar = this._createToolbar();

  textarea.parentNode.insertBefore(htmlarea, textarea);
  HTMLArea.removeFromParent(textarea);

  fw.table.appendChild(fw.tbody);
  fw.tbody.appendChild(fw.tb_row);  // Toolbar
  fw.tb_row.appendChild(fw.tb_cell);
  this._framework.tb_cell.appendChild( toolbar );
  fw.tbody.appendChild(fw.tp_row); // Left, Top, Right panels
  fw.tp_row.appendChild(fw.tp_cell);
  fw.tbody.appendChild(fw.ler_row);  // Editor/Textarea
  fw.ler_row.appendChild(fw.lp_cell);
  fw.ler_row.appendChild(fw.ed_cell);
  this._framework.ed_cell.appendChild(textarea);
  fw.ler_row.appendChild(fw.rp_cell);
  fw.tbody.appendChild(fw.bp_row);  // Bottom panel
  fw.bp_row.appendChild(fw.bp_cell);
  fw.tbody.appendChild(fw.sb_row);  // Statusbar
  fw.sb_row.appendChild(fw.sb_cell);
  this._framework.sb_cell.appendChild(statusbar);
  this._framework.ed_cell.appendChild(iframe);  

[sleemanj]

guest commented:

5. fix attach detach events

line ~5040

  HTMLArea._addEvent = function addEvent( obj, type, fn ) {
      obj["e"+type+fn] = fn;
      obj[type+fn] = function() { obj["e"+type+fn]( window.event ); }
      obj.attachEvent( "on"+type, obj[type+fn] );
      HTMLArea._eventFlushers.push([obj, type, fn]);
  };

  HTMLArea._removeEvent = function removeEvent( obj, type, fn ) {
      obj.detachEvent( "on"+type, obj[type+fn] );
      obj[type+fn] = null;
      obj["e"+type+fn] = null;
  };

[sleemanj]

guest commented:

6. fix include script (_loadback)

line 6464

HTMLArea._loadback = function(U, C, O, B)
{
  var S = document.createElement("script");
  S.type = "text/javascript";
  S.src = U;
  if ( C )
  {
    S[HTMLArea.is_ie ? "onreadystatechange" : "onload"] = function()
    {
      if ( HTMLArea.is_ie && ! ( /loaded|complete/.test(window.event.srcElement.readyState) ) )
      {
        return;
      }
      C.call(O ? O : this, B);
      S[HTMLArea.is_ie ? "onreadystatechange" : "onload"] = null; <-- RELEASE THE EVENT
    };
  }
  document.getElementsByTagName("head")[0](../commit/0).appendChild(S);
};

[sleemanj]

guest commented:

setup global tracking of editors, this allows getting editor by id rather than having it as a function local variable

HTMLArea.makeEditors = function(editor_names, default_config, plugin_names)
{
  if ( typeof default_config == 'function' )
  {
    default_config = default_config();
  }

  var editors = {};
  for ( var x = 0; x < editor_names.length; x++ )
  {
    var editor = new HTMLArea(editor_names[x], HTMLArea.cloneObject(default_config));
    editor.registerPlugins(plugin_names);
    editors[editor_names[x]] = editor;
  }
  HTMLArea.EDITORS = editors <-- ADD GLOBAL TRACKING
  return editors;
};

[sleemanj]

guest commented:

have editor events use global EDITORS array and remove drag (for now)

HTMLArea.prototype.setEditorEvents = function()
{
  var editorid = this._textArea.id;

  var nested_event = function(event) { 
      return HTMLArea.EDITORS[editorid]._editorEvent(HTMLArea.is_ie ? HTMLArea.EDITORS[editorid]._iframe.contentWindow.event : event);
  }

  HTMLArea.EDITORS[editorid].whenDocReady(
    function()
    {
      // if we have multiple editors some bug in Mozilla makes some lose editing ability
      HTMLArea._addEvents(
        HTMLArea.EDITORS[editorid]._doc,
        ["mousedown"],
        function()
        {
          HTMLArea.EDITORS[editorid].activateEditor();
          return true;
        }
      );

      // intercept some events; for updating the toolbar & keyboard handlers
      HTMLArea._addEvents(
          HTMLArea.EDITORS[editorid]._doc,
        //["keydown", "keypress", "mousedown", "mouseup", "drag"], 
        ["keydown", "keypress", "mousedown", "mouseup"], <-- REMOVED DRAG
      nested_event
      );  

      // check if any plugins have registered refresh handlers
      for ( var i in HTMLArea.EDITORS[editorid].plugins )
      {
        var plugin = HTMLArea.EDITORS[editorid].plugins[i].instance;
        HTMLArea.refreshPlugin(plugin);
      }

      // specific editor initialization
      if ( typeof HTMLArea.EDITORS[editorid]._onGenerate == "function" )
      {
        HTMLArea.EDITORS[editorid]._onGenerate();
      }
      HTMLArea.EDITORS[editorid].removeLoadingMessage();
    }
  );
};

[sleemanj]

djeeg commented:

and thats it, no more memory leaks. i don't need the drag event for my solution but i can imagine that other people do, so it would propably be better to either fix the issue in the drag or have a editor config setting to disable dragging.

[sleemanj]

djeeg commented:

Hey Mokhet/Bobesch ignore my number 6, it looks like you have got this one nailed in a previous post.

[sleemanj]

mokhet commented:

_loadback() leak fixed in changeset:531

[sleemanj]

mokhet commented:

1. free dom element from buttons

update commited in changeset:532

this changeset also improve HTMLArea.collectGarbageForIE() by nullifying the object once its properties are removed.