Opened 15 years ago

Closed 21 months ago

#30 closed defect (inactive)

about memory use

Reported by: forgottentowers Owned by: gogo
Priority: lowest Milestone: Version 1.0
Component: Xinha Core Version:
Severity: major Keywords: IE memory leak
Cc: bobesch@…

Description (last modified by gogo)

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

Change History (67)

comment:1 Changed 15 years ago by Chris Allison

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.

comment:2 Changed 15 years ago by gogo

  • Keywords IE memory leak added
  • Priority changed from high to lowest
  • Severity changed from major to normal

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.

comment:3 Changed 14 years ago by ianb@…

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

comment:4 Changed 14 years ago by gogo

  • Description modified (diff)
  • Priority changed from lowest to highest

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).

comment:5 Changed 14 years ago by anonymous

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.

comment:6 Changed 14 years ago by niko

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...

comment:7 Changed 14 years ago by mike.codeismylife.com

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.

comment:8 Changed 14 years ago by ianb@…

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

comment:9 Changed 14 years ago by gogo

  • Milestone set to Version 1.0
  • Version set to 0.1

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.

comment:10 Changed 14 years ago by http://www.dynarch.com/

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.

comment:11 Changed 14 years ago by mike.codeismylife.com

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!

<script>

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>

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...

comment:12 Changed 14 years ago by ianb@…

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.

comment:13 Changed 14 years ago by gogo

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.

comment:14 Changed 14 years ago by gogo

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

comment:15 Changed 14 years ago by ianb@…

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.

comment:16 Changed 14 years ago by gogo

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.

comment:17 Changed 14 years ago by gogo

  • Priority changed from highest to lowest
  • Severity changed from normal to major
  • Version 0.1 deleted

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.

comment:18 Changed 14 years ago by ianb@…

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.

comment:20 Changed 14 years ago by Stuart_T

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.

comment:22 Changed 14 years ago by anonymous

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

comment:23 Changed 14 years ago by gogo

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.

comment:24 Changed 14 years ago by gogo

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()
  1. 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.
  1. 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.

comment:25 Changed 14 years ago by david

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

comment:26 Changed 14 years ago by gogo

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.

comment:27 Changed 14 years ago by david

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.

comment:28 Changed 14 years ago by gogo

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.

comment:29 Changed 14 years ago by gogo

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.

comment:30 Changed 14 years ago by mwvdlee

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.

comment:31 Changed 14 years ago by mokhet

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

Looks like an interesting way to fix closure issue.

comment:32 Changed 14 years ago by mleiv

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.

comment:33 Changed 14 years ago by mleiv

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

comment:34 Changed 13 years ago by bobesch

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].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);


    

comment:35 Changed 13 years ago by mokhet

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].appendChild(S);
};

comment:36 Changed 13 years ago by mokhet

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].appendChild(S);
};

comment:37 Changed 13 years ago by mokhet

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

comment:38 Changed 13 years ago by djeeg

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.

comment:39 Changed 13 years ago by guest

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.

comment:40 Changed 13 years ago by bobesch

  • Cc bobesch@… added; forgottentowers@… removed

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].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

comment:41 Changed 13 years ago by guest

  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];
      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], // enabled in text mode?
        cmd	: btn[3], // the command ID
        state	: setButtonStatus, // for changing state
        context : btn[4] || null // enabled in a certain context?
      };
      HTMLArea.freeLater(el); <--  RELEASE THE <A>
      HTMLArea.freeLater(obj);

comment:42 Changed 13 years ago by guest

  1. 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);  

comment:43 Changed 13 years ago by guest

  1. 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;
	};

comment:44 Changed 13 years ago by guest

  1. 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].appendChild(S);
};

comment:45 Changed 13 years ago by guest

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;
};

comment:46 Changed 13 years ago by guest

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();
    }
  );
};

comment:47 Changed 13 years ago by djeeg

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.

comment:48 Changed 13 years ago by djeeg

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

comment:49 Changed 13 years ago by mokhet

_loadback() leak fixed in changeset:531

comment:50 Changed 13 years ago by mokhet

  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.

comment:51 Changed 13 years ago by mokhet

I tried to apply bobesch's patch to panels, but it does not break the 4 leaks of the TD. It is because those TD are hidden (display:none), and so are leaking in IE (another of the kind of possible leaks in this browser).

So i've changed a bit HTMLArea.free() to take care of this situation in changeset:533, from here I think the leaks are greatly reduced. Can someone confirm drip is happy now ?

I'm not convinced by the proposal to setup a global tracking of editors with the variable HTMLArea.EDITORS created in makeEditors(). makeEditors() is NOT the only way to create the editor so we can't rely on this method to set such variable which, if this proposal is implemented, become needed to have Xinha work. And i dont understand the idea behind adding another variable. We already have plenty (too much) of variables to rely on (htmareas, this.htmlarea_id_num, this._htmlArea, this._textArea, this.editor)

I dont like either the reordering of the framework. It doesnt seems correct to me to do it this way, despite what MSDN crap can suggest.

comment:52 Changed 13 years ago by djeeg

i think the reason behind the global array of editors is so that inner anonyomous functions can have access to the current editor (this) by a string variable in the call stack/state rather than storing the editor itself. this way the memory usage is reduced and can be free'd easier by the GC since its just a string. i'm just grasping at straws with this explaination though, just tried it and it worked.

sorry i didnt read the code enough to see the htmlarea_id_num property, so the better way to do it would be this.

HTMLArea.prototype.setEditorEvents = function()
{
  var editorid = this.__htmlarea_id_num;
  
	var nested_event = function(event) { 
		return __htmlareas[editorid]._editorEvent(HTMLArea.is_ie ? __htmlareas[editorid]._iframe.contentWindow.event : event);
	}
  
  __htmlareas[editorid].whenDocReady(
    function()
    {
      // if we have multiple editors some bug in Mozilla makes some lose editing ability
      HTMLArea._addEvents(
        __htmlareas[editorid]._doc,
        ["mousedown"],
        function()
        {
          __htmlareas[editorid].activateEditor();
          return true;
        }
      );
      
      // intercept some events; for updating the toolbar & keyboard handlers
      HTMLArea._addEvents(
      	__htmlareas[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 __htmlareas[editorid].plugins )
      {
        var plugin = __htmlareas[editorid].plugins[i].instance;
        HTMLArea.refreshPlugin(plugin);
      }

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

comment:53 Changed 13 years ago by mokhet

Clever. So basically, because i need to understand :) , this has nothing to do with leaks but it's about amount of memory used over the different call stack, right ? I wasnt able to see the valuable behind this update, now I think I understand it loud and clear, thanks for the explanation (please correct me if i'm wrong). I think this patch should do the trick, i'll look more at it (and test) later probably sunday, if nobody else jump on it. But for now it's about time to go bed :) despite the valuable informations i've learned tonight, thank to you.

Index: htmlarea.js
===================================================================
--- htmlarea.js	(revision 536)
+++ htmlarea.js	(working copy)
@@ -2340,18 +2340,19 @@
 
 HTMLArea.prototype.setEditorEvents = function()
 {
-  var editor=this;
-  var doc=this._doc;
-  editor.whenDocReady(
+  var id = this.__htmlarea_id_num;
+  this.whenDocReady(
     function()
     {
+      var editor = __htmlareas[id];
+      var doc = editor._doc;
       // if we have multiple editors some bug in Mozilla makes some lose editing ability
       HTMLArea._addEvents(
         doc,
         ["mousedown"],
         function()
         {
-          editor.activateEditor();
+          __htmlareas[id].activateEditor();
           return true;
         }
       );
@@ -2362,7 +2363,7 @@
         ["keydown", "keypress", "mousedown", "mouseup", "drag"],
         function (event)
         {
-          return editor._editorEvent(HTMLArea.is_ie ? editor._iframe.contentWindow.event : event);
+          return __htmlareas[id]._editorEvent(HTMLArea.is_ie ? __htmlareas[id]._iframe.contentWindow.event : event);
         }
       );
 
@@ -2379,7 +2380,7 @@
         editor._onGenerate();
       }
 
-      HTMLArea.addDom0Event(window, 'resize', function(e) { editor.sizeEditor(); });
+      HTMLArea.addDom0Event(window, 'resize', function(e) { __htmlareas[id].sizeEditor(); });
       editor.removeLoadingMessage();
     }
   );

Why is removing the "drag" event should change anything in the leak situation ?

Is the "nested_event" declaration a must have ? I dont see the point to do it (yeah, again :( sorry to ask explanation but i dont understand why to do it), despite perhaps to have the event handler more readable, ... hummm but after making the patch, i think i start to see the point behind. Is it again a trick to reduce the memory usage ? is it really doing something to the memory usage ? damn it, i need to bench this new info. Basically, it seems to means we could save a lot of usefull memory (especially for IE) all over the code and in plugins. Interesting...

comment:54 Changed 13 years ago by djeeg

when you declare a function inline like this

       HTMLArea._addEvents(
         doc,
         ["mousedown"],
         function()
         {
           editor.activateEditor();
           return true;
         }
       );

it is called a closure apparently. the issue is IE doesnt do a good job of garbage collecting the objects when the page refreshes due to some referencing problems. a way around this in this instance is to try to make the call stack memory as small as possible. that is, instead of having a reference to the editor, which is an object, which propably has a reference to every other object. have instead only the id of the editor on the call stack. this way the memory used is only very small. it also means that the garbage collection for the closure can complete because there is only a primative.

comment:55 Changed 13 years ago by guest

Are you really sure, djeeg, that a string occupies less memory on a stack frame than a reference to an object? When you pass a 'var editor' to myFunction(), you pass in a reference, a sort of pointer (although javascript references aren't C pointers, see the Definitive Guide 4th edition, p. 166), not the the object itself. I don't know how big javascript references can get, but I guess there's no common mesure between the size of an object reference and the size of the object itsself. So I heavily doubt that not passing around 'editor' will save us a noticeable amount of memory on the stack. Otherwise all the Ajax library stuff out there would be heavily flawed in this respect, since they pass objects around all the time.

The real problem, however, isn't about the size of memory occupied by strings vs. references, the real problem in this context is about closures. But sure, I admit, your proposal is a smart way to work around the problem, because leaking a string is a small problem, while leaking an object can be a huge one. So I finish to contradict myself? No, the memory occupied by the object reference is small, but the memory not freed because the JS engine does not release the reference is big. Because on unload it's not only the memory occupied by the reference that is important, but the memory of the whole object it references. *This* amount of memory has to be freed, and failing to do so adds the size of the whole object to the memory footprint of the application, on each reload.

comment:56 Changed 13 years ago by mokhet

Yes, i think like "guest" that there's no benefice to use a string or an object all over the calling stack. I am not able to find any significant enhancement in the amount of memory used by one or another method. But i'm not an expert.

I'm thinking we should give up on the HTMLArea.free and HTMLArea.freeLater and use instead some kind of disposer which could be execute for every Xinha generated and every plugins. Would be easier to know what can leaks and what is not, since everything would be tied up on the same place instead of having a lot of HMTLArea.freeLater all over the code.

comment:57 Changed 13 years ago by bobesch

Just for the record: The 'guest' commenting on djeeg and the closure issue was me (sorry, forgot to change the email/username field).

Yes, a kind of disposer would be great, but I don't see how we can implement it ad hoc, given the complexity of Xinha. The best thing I see for now is to really evince this leakage problem. This also helps us to understand the problem points and -- who knows -- figure out how to implement the desirable disposer.

As for the panel/stylist issue, to prevent the panel TDs from leaking, the following is still necessairy (according to drip 0.4)

Index: htmlarea.js
===================================================================
--- htmlarea.js	(revision 536)
+++ htmlarea.js	(working copy)
@@ -200,6 +200,7 @@
       panels[i].container.className = 'panels ' + i;
       HTMLArea.freeLater(panels[i], 'container');
       HTMLArea.freeLater(panels[i], 'div');
+      HTMLArea.freeLater(panels[i], 'panels');
     }
     // finally store the variable
     this._panels = panels;

comment:58 Changed 13 years ago by djeeg

this is absolutely correct

No, the memory occupied by the object reference is small, but the memory not freed because the JS engine 
does not release the reference is big. Because on unload it's not only the memory occupied by the reference that is important, 
but the memory of the whole object it references. *This* amount of memory has to be freed, and failing to do so adds the size of 
the whole object to the memory footprint of the 
application, on each reload. 

there are two ways to fix this problem: either fix the closure issue, which seems that it will never be 100% good, or change the amount of memory that isn't garbage collected in the closure. That is the whole editor ~2mb, to just an int variable for the editor id. ~4 bytes

the current logic for the attach and detach of events is flawed too and needs to be changed to my recommentdation on "Fri Jul 21 21:31:46 2006"

any ideas about what to do with the drag events?

comment:59 Changed 13 years ago by bobesch

Djeeg, I don't understand why drag events are special. Could you please explain in some detail? As for the other events, I don't understand (yet) whether the detach logic is flawed or not (drip doesn't show any leakages, but memory consumption goes up by 1-2MB on each reload).

For the moment I'm looking into the event handlers themselves. For instance, at each keystroke memory consumption goes up by 20-30KB. Is this normal?

And yes, your workaround does the right thing in practical terms. Still it's sort of a kludge. On the other hand you could argue, that IE does not merit better than kludges, and you're probably right on this one. Perhaps I'm simply too ambitious when wishing to get it done the "clean way".

Warmest regards, Bodo

comment:60 Changed 13 years ago by bobesch

Just wanted to confirm the bench by mokhet:

In _addEvents, the original

return editor._editorEvent(HTMLArea.is_ie ? editor._iframe.contentWindow.event : event);

does not leak more (if at all) than djeeg's proposed

return __htmlareas[id]._editorEvent(HTMLArea.is_ie ?__htmlareas[id]._iframe.contentWindow.event : event);

With drip showing no more leaks, fixing the remaining 1MB or so leak is getting difficult. Any ideas?

comment:61 Changed 13 years ago by bobesch

Another freeLater() patch for the SuperClean? plugin (courtesy of drip 0.4)

Index: super-clean.js
===================================================================
--- super-clean.js	(revision 528)
+++ super-clean.js	(working copy)
@@ -267,6 +267,8 @@
   // Now we have everything we need, so we can build the dialog.
   var dialog = this.dialog = new HTMLArea.Dialog(SuperClean.editor, this.html, 'SuperClean');
 
+  HTMLArea.freeLater(this, 'dialog');
+
   this.ready = true;
 };

comment:62 Changed 13 years ago by bobesch

In an earlier post in this thread, I pointed to the fact that memory consumption in IE goes up by 20-30 KB on each key stroke. The culprit seams to be the first part of updateToolbar() where the breadcrumbs representing the DOM tree in the status bar bet assigned event handlers which backreference the javascript object. These handlers get (re-)created along with the A-Tag nodes on each and every keystroke but only get nullified when flushEvents() is called upon unload. To prevent IE from leaking we have to nullify the old ones before creating new ones, right? The following patch attempts to do this with quite noticeable results.

Index: htmlarea.js
===================================================================
--- htmlarea.js	(revision 536)
+++ htmlarea.js	(working copy)
@@ -1407,6 +1408,8 @@
     statusbar.style.display = "none";
   }
 
+  this._statusBarItems = [];
+
   return statusbar;
 };
 
@@ -2972,11 +2975,23 @@
   var doc = this._doc;
   var text = (this._editMode == "textmode");
   var ancestors = null;
+
   if ( !text )
   {
     ancestors = this.getAllAncestors();
     if ( this.config.statusBar && !noStatus )
     {
+      while (this._statusBarItems.length) { // chop circular reference for IE
+        var item = this._statusBarItems.pop();
+	 item.el = null;
+	 item.editor = null;
+	 item.onclick = null;
+	 item.oncontextmenu = null;
+	 item._xinha_dom0Events['click'] = null;
+	 item._xinha_dom0Events['contextmenu'] = null;
+	 item = null;
+      }
+
       this._statusBarTree.innerHTML = HTMLArea._lc("Path") + ": "; // clear
       for ( var i = ancestors.length; --i >= 0; )
       {
@@ -2993,6 +3008,9 @@
         a.href = "javascript:void(0)";
         a.el = el;
         a.editor = this;
+
+        this._statusBarItems.push(a);
+
         HTMLArea.addDom0Event(
           a,
           'click',

comment:63 Changed 13 years ago by ray

applied patch from last comment in rev [780]

comment:64 Changed 13 years ago by ray

I spent Saturday afternoon pressing F5 and doing some statistcs, mainly to find out if calling Xinha.collectGarbageForIE() on unload is any good in Mozilla. Answer is no; it takes really some time to unload the page, to no avail. Disabled it for Mozilla in rev [807]
http://xinha.raimundmeyer.de/attic/XinhaMemoryUsage/XinhaMemoryUsage-Dateien/image001.gif
What you see, is that there is memory leaking in Mozilla, but equally with JavaScript or static HTML pages. In IE, memory usage is almost 100% stable with HTML, but leaking in ridiculous amounts with JS. It's better with our own garbage collecting, but still far worse than Firefox.

http://xinha.raimundmeyer.de/attic/XinhaMemoryUsage/XinhaMemoryUsage-Dateien/image002.gif

comment:65 Changed 12 years ago by ray

Microsoft claims to have improved their memory handling in IE8. I wondered if this meant we can remove the irritating manual garbage collection for IE8. To find out I continued the test from my last post to include IE8. The findigs were that memory leaking is now linear as opposed to the ridiculous roller coaster curves in previous versions, though in no way as flat as in Firefox and Safari AND the curves show that our garbage collection gives a small advantage, but not a significant one.
http://xinha.raimundmeyer.de/attic/XinhaMemoryUsage/XinhaMemoryUsage-Dateien/image002.gif

comment:66 Changed 12 years ago by ray

disabled in rev [980]

comment:67 Changed 21 months ago by gogo

  • Resolution set to inactive
  • Status changed from new to closed
Note: See TracTickets for help on using tickets.