Memory leak risk in JavaScript closures

Question

Solved

There's a lot of contradictory information on the web, regarding this subject. Thanks to @John, I managed to work out that the closures (as used below) aren't the cause of memory leaks, and that -even in IE8- they're not that common as people claim. In fact there was only 1 leak that occurred in my code, which proved not that difficult to fix.

From now on, my answer to this question will be:
AFAIK, the only time IE8 leaks, is when events are attached/handlers are set on the global object. (window.onload,window.onbeforeunload,...). To get around this, see my answer below.

---

HUGE UPDATE:

I'm completly lost now... After some time digging through articles and tuts both old and new, I'm left with at least one humongous contradiction. While one of THE JavaScript Guru's (Douglas Crockford) says:

Since IE is unable to do its job and reclaim the cycles, it falls on us to do it. If we explicitly break the cycles, then IE will be able to reclaim the memory. According to Microsoft, closures are the cause of memory leaks. This is of course deeply wrong, but it leads to Microsoft giving very bad advice to programmers on how to cope with Microsoft's bugs. It turns out that it is easy to break the cycles on the DOM side. It is virtually impossible to break them on the JScript side.

And as @freakish pointed out that my snippets below are similar to jQuery's internal workings I felt pretty secure about my solution not causing memory leaks. At the same time I found this MSDN page, where the section Circular References with Closures was of particular interest to me. The figure below is pretty much a schematic representation of how my code works, isn't it:

The only difference being that I have the common sense of not attaching my event listeners to the elements themselves.
All the same Douggie is quite unequivocal: closures are not the source of mem-leaks in IE. This contradiction leaves me clueless as to who's right.

I've also found out that the leak issue isn't completely solved in IE9 either (can't find the link ATM).

One last thing: I've also come to learn that IE manages the DOM outside the JScript engine, which puts me in a spot of bother when I change the children of a <select> element, based on an ajax request:

function changeSeason(e)
{
    var xhr,sendVal,targetID;
    e = e || window.event;//(IE...
    targetID = this.id.replace(/commonSourceFragment/,'commonTargetFragment');//fooHomeSelect -> barHomeSelect
    sendVal = this.options[this.selectedIndex].innerHTML.trim().substring(0,1);
    xhr = prepareAjax(false,(function(t)
    {
        return function()
        {
            reusableCallback.apply(this,[t]);
        }
    })(document.getElementById(targetID)),'/index/ajax');
    xhr({data:{newSelect:sendVal}});
}

function reusableCallback(elem)
{
    if (this.readyState === 4 && this.status === 200)
    {
        var data = JSON.parse(this.responseText);
        elem.innerHTML = '<option>' + data.theArray.join('</option><option>') + '</option>';
    }
}

If IE really does manage the DOM as though the JScript engine weren't there, what are the odds that the option elements aren't deallocated using this code?
I've deliberately added this snippet as an example, because in this case I'm passing variables that are part of the closure scope as an argument to a global function. I couldn't find any documentation on this practice, but based on the documentation provided by Miscrosoft, it should break any circular references that might occur, doesn't it?

---

---

Warning: lengthy question... (sorry)

I've written a couple of fairly large JavaScripts to make Ajax calls in my web application. in order to avoid tons of callbacks and events, I'm taking full advantage of event delegation and closures. Now I've written a function that has me wondering as to possible memory leaks. Though I know IE > 8 deals with closures a lot better then its predecessors, it is company policy to support IE 8 all the same.

Below I've provided an example of what I'm on about, here you can find a similar example, though it doesn't use ajax, but a setTimeout, the result is pretty much the same. (You can, of course skip the code below, to the question itself)

The code I have in mind is this:

function prepareAjax(callback,method,url)
{
    method = method || 'POST';
    callback = callback || success;//a default CB, just logs/alerts the response
    url = url || getUrl();//makes default url /currentController/ajax
    var xhr = createXHRObject();//try{}catch etc...
    xhr.open(method,url,true);
    xhr.setRequestMethod('X-Requested-with','XMLHttpRequest');
    xhr.setRequestHeader('Content-type','application/x-www-form-urlencoded');
    xhr.setRequestHeader('Accept','*/*');
    xhr.onreadystatechange = function()
    {
        callback.apply(xhr);
    }
    return function(data)
    {
        //do some checks on data before sending: data.hasOwnProperty('user') etc...
        xhr.send(data);
    }
}

All pretty straight-forward stuff, except for the onreadystatechange callback. I noticed some issues with IE when binding the handler directly: xhr.onreadystatechange = callback;, hence the anonymous function. Don't know why, but I found this to be the easiest way to make it work.

As I said, I'm using a lot of event delegation, so you can imagine it may prove useful to have access to the actual element/event that fired the ajax call. So I have some event handlers that look like this:

function handleClick(e)
{
    var target,parent,data,i;
    e = e || window.event;
    target = e.target || e.srcElement;
    if (target.tagName.toLowerCase() !== 'input' && target.className !== 'delegateMe')
    {
        return true;
    }
    parent = target;
    while(parent.tagName.toLowerCase() !== 'tr')
    {
        parent = parent.parentNode;
    }
    data = {};
    for(i=0;i<parent.cells;i++)
    {
        data[parent.cells[i].className] = parent.cells[i].innerHTML;
    }
    //data looks something like {name:'Bar',firstName:'Foo',title:'Mr.'}
    i = prepareAjax((function(t)
    {
        return function()
        {
            if (this.readyState === 4 && this.status === 200)
            {
                //check responseText and, if ok:
                t.setAttribute('disabled','disabled');
            }
        }
    })(target));
    i(data);
}

As you can see, the onreadystatechange callback is the return value of a function, that provides the reference to the target element when the callback is called. Thanks to event delegation, I no longer have to worry about events that might be bound to that element, when I decide to remove it from the DOM (which I do sometimes).
To my mind, however, the call object of the callback function might prove too much for IE's JScript engine and its garbage collector:

Event ==> handler ==> prepareAjax is a pretty normal call sequence, but the callback argument:

[anon. func (argument t = target) returns anon. F (has access to t which in turn refs back to target)]
   ===> passed to a anon callback function, called using .apply method to the xhr object, in turn a private variable to the prepareAjax function

I've tested this "construction" in FF and chrome. It works just fine there, but would this kind of callstack of closure upon closure upon closure, on each occasion passing a reference to a DOM element be an issue in IE (especially versions prior to IE9)?

---

No, I'm not going to use jQuery or other libs. I like pure JS, and want to know as much as I can about this seriously underrated language. The code snippets are not actual copy-paste examples, but provide, IMO, a good representation of how I'm using delegation, closures and callbacks throughout my script. So if some syntax isn't quite right, feel free to correct it, but that's not what this question is about, of course.

Answer 1

I used to work with the ex-program manager for JavaScript within Microsoft, on a non-browser EcmaScript (err.. JScr... JavaScript) project. We had some lengthy discussions about closures. In the end, the point is that they are harder to GC, not impossible. I'd have to read DC's discussion of how MS is 'wrong' that closures cause memory leaks -- for in IE's older implementations, closures were certainly problematic, because they were very hard to garbage collect with the MS implementation. I find it strange that a Yahoo guy would try to tell the MS architects that a known issue with their code was somewhere else. As much as I appreciate his work, I don't see what basis he had there.

Remember, the article you reference above refers to IE6, as IE7 was still under heavy development at the time of its writing.

As an aside -- thank god, IE6 is dead (don't make my dig up the funeral pictures). Although, don't forget its legacy... I've yet to see anyone make a credible argument that it wasn't the finest browser in the world on day 1 of its release -- the problem was that they won the browser war. And so, in what amounts to one of the larger blunders of their history -- they fired the team afterwards and it sat stagnant for nearly 5 years. The IE team was down to 4 or 5 guys doing bug fixes for years, creating a huge brain drain and falling behind the curve dramatically. By the time they rehired the team and realized where they were, they were years behind because of the added cruft of dealing with a monolothic codebase that nobody really understood anymore. That's my perspective as an internal in the company, but not directly tied to that team.

Remember too, IE never optimized for closures because there was no ProtoypeJS (heck, there was no Rails), and jQuery was barely a glimmer in Resig's head.

At the time of the writing, they were also still targeting machines with 256 megs of RAM, which also hadn't been end-of-life'd.

I thought it only fair to hand you this history lesson, after making me read your entire book of a question.

In the end, my point is that you're referencing material which is hugely dated. Yes, avoid closures in IE6, as they cause memory leaks -- but what didn't in IE6?

In the end, it is a problem that MS has addressed and continues to address. You're going to make some level of closures, that was the case, even back then.

I know that they did heavy work in this area around IE8 (as my unmentionable project used the non-at-the-time standard JavaScript engine), and that work has continued into IE9/10. StatCounter (http://gs.statcounter.com/) suggests that IE7 is down to a 1.5% market share, down from 6% a year ago, and in developing 'new' sites, IE7 becomes less and less relevant. You can also develop for NetScape 2.0, which introduced JavaScript support, but that would be only slightly less silly.

Really... Don't try to over-optimize for the sake of an engine which doesn't exist anymore.

Answer 2

Right, after spending some time with that IEJSLeaksDetector tool, I've found out that the things I talked about in my original question DO NOT CAUSE MEM LEAKS. However, there was 1 leak that did pop up. Thankfully, I've managed to find a solution:

I have a main script, and at the bottom, there's an old school:

window.onload = function()
{
    //do some stuff, get URI, and call:
    this['_init' + uri[0].ucFirst()](uri);//calls func like _initController
    //ucFirst is an augmentation of the String.prototype
}

This causes a leak in IE8, That I couldn't fix with a window.onbeforeunload handler. It seems you have to avoid binding the handler to the global object. The solution lies in closures and event listeners, it's a bit of a faff, but here's what I ended up doing:

(function(go)
{//use closure to avoid leaking issue in IE
    function loader()
    {
        var uri = location.href.split(location.host)[1].split('/');
        //do stuff
        if (this['_init' + uri[0].ucFirst()] instanceof Function)
        {
            this['_init' + uri[0].ucFirst()](uri);
        }
        if (!(this.removeEventListener))
        {
            this.detachEvent('onload',loader);//(fix leak?
            return null;
        }
        this.removeEventListener('load',loader,false);
    }
    if (!(go.addEventListener))
    {
        go.attachEvent('onload',loader);//(IE...
    }
    else
    {
        go.addEventListener('load',loader,false);
    }
})(window);

That way, the (on)load event is unbound just before the window.load handler returns, according to the IEJSLeaksDetector tool, there are NO leaks in my application. I'm happy with that. I hope this snippet is of some use to one of you - if someone has suggestions to improve this approach, don't hesitate to do so!

Cheers, and thanks to all of you who went through the trouble of reading and trying my dribble above!

---

PS: in case someone cares, here's the ucFirst String method:

if (!(String.prototype.ucFirst))
{
    String.prototype.ucFirst = function()
    {
        "use strict";
        return this.charAt(0).toUpperCase() + this.slice(1);
    };
}