Reputation: 7091
I just cannot for the life of me figure out this memory leak in Internet Explorer.
insertTags
simple takes string str
and places each word within start and end tags for HTML (usually anchor tags). transliterate
is for arabic numbers, and replaces normal numbers 0-9 with a &#..n; XML identity for their arabic counterparts.
fragment = document.createDocumentFragment();
for (i = 0, e = response.verses.length; i < e; i++)
{
fragment.appendChild((function(){
p = document.createElement('p');
p.setAttribute('lang', (response.unicode) ? 'ar' : 'en');
p.innerHTML = ((response.unicode) ? (response.surah + ':' + (i+1)).transliterate() : response.surah + ':' + (i+1)) + ' ' + insertTags(response.verses[i], '<a href="#" onclick="window.popup(this);return false;" class="match">', '</a>');
try { return p } finally { p = null; }
})());
}
params[0].appendChild( fragment );
fragment = null;
I would love some links other than MSDN and about.com, because neither of them have sufficiently explained to me why my script leaks memory. I am sure this is the problem, because without it everything runs fast (but nothing displays).
I've read that doing a lot of DOM manipulations can be dangerous, but the for loops a max of 286 times (# of verses in surah 2, the longest surah in the Qur'an).
* Memory leaks in IE7 and IE8, not sure about 6, but works perfectly fine in Safari 4, FF 3.6, Opera 10.5, Chrome 5... *
Upvotes: 2
Views: 1031
Reputation: 8069
Although the answer has been accepted, I think this could could also have done the job:
var fragment = document.createDocumentFragment();
for (var i = 0, e = response.verses.length; i < e; i++) {
fragment.appendChild((function(p){ // Create a scope here itself :)
p = document.createElement('p'); // ?? without above, it was a global scope
p.setAttribute('lang', (response.unicode) ? 'ar' : 'en');
p.innerHTML = ((response.unicode) ? (response.surah + ':' + (i+1)).transliterate() : response.surah + ':' + (i+1)) + ' ' + insertTags(response.verses[i], '<a href="#" onclick="window.popup(this);return false;" class="match">', '</a>');
try { return p } finally { p = null; }
})());
}
params[0].appendChild( fragment );
fragment = null;
Reason for memory leak: A closure being created and returned in the anonymous function, and then kept alive but not garbage collected since fragment
is using it
So the solution can be as simple as providing a lexical scope, shown above
Upvotes: 2
Reputation: 39057
Variables are scoped to functions, not if/else/for/while/etc. blocks. Every time you call
fragment.appendChild((function() { ...
you are creating a new function (new scope). This new function references the i
and response
variables. So now, i
and response
are scoped to BOTH the outer function and the new function.
That's not enough to leak memory. (i
and response
are normal variables that will go out-of-scope after the new function is done)
BUT, you create a p
DOM element in the new function, and reference it in the outer function (you return it to the fragment.appendChild
call as an argument). Now think about it: you have the outer scope fragment
referencing a p
DOM created from the inner scope, which needed to use the i
and response
variables from the outer scope to create the DOM element in the first place.
The fragment
and p
DOM objects each have a reference to each other. Despite your attempts to zero-out the reference count by nulling the variable pointers, p=null
and fragment = null
will not get rid of all references. The fragment
still has a reference to the inner p
, which still has a reference to the outer response
variable. The two "scopes" will never be garbage collected because of this remaining circular dependency.
Anyone, please correct me if I've made any mistakes.
As for a solution, just don't use an inner function!
fragment = document.createDocumentFragment();
for (var i = 0, var e = response.verses.length; i < e; i++)
{
var p = document.createElement('p');
p.setAttribute('lang', (response.unicode) ? 'ar' : 'en');
p.innerHTML = ((response.unicode) ? (response.surah + ':' + (i+1)).transliterate() : response.surah + ':' + (i+1)) + ' ' + insertTags(response.verses[i], '<a href="#" onclick="window.popup(this);return false;" class="match">', '</a>');
fragment.appendChild(p);
}
params[0].appendChild( fragment );
Upvotes: 6
Reputation: 522523
I can't really tell you why IE supposedly leaks memory, but this code is pretty darn complicated for what it does. And this line seems highly suspicious and superfluous: try { return p } finally { p = null; }
.
How about simplifying it a bit and scoping the variables:
var fragment = document.createDocumentFragment();
var p, t;
for (var i = 0; i < response.verses.length; i++)
{
p = document.createElement('p');
if (response.unicode) {
p.setAttribute('lang', 'ar');
t = (response.surah + ':' + (i+1)).transliterate();
} else {
p.setAttribute('lang', 'en');
t = response.surah + ':' + (i+1);
}
p.innerHTML = t + ' ' + insertTags(response.verses[i], '<a href="#" onclick="window.popup(this);return false;" class="match">', '</a>');
fragment.appendChild(p);
}
params[0].appendChild(fragment);
fragment = p = t = null; // likely unnecessary if they go out of scope anyway
That's still a lot of DOM operations though, which may take a while on slow Javascript engines.
Upvotes: -2
Reputation: 8376
Does it leak if you remove the onclick attribute from the link?
You could try removing the repeated onclick and replacing it with event delegation.
Also all of your variables appear to be in global scope -- that shouldn't get as bad as to cause the issues you're seeing, but you should fix that up regardless.
Upvotes: 0