search

BorisMoore / jquery-tmpl

The original official jQuery Templates plugin. This project was maintained by the jQuery team as an official jQuery plugin. It is no longer in active development, and has been superseded by JsRender.
3.23k stars 1.01k forks source link

Memory Leak #83

Open BorisMoore opened 13 years ago

BorisMoore commented 13 years ago

Copied from https://github.com/nje/jquery-tmpl/issues#issue/1:

Created 7 months ago by kghost Create a node then discard it immediately will cause memory leak Confirmed on all common browsers (IE6,Firefox,Chrome).

test page:

<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Transitional//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd">
<html xmlns="http://www.w3.org/1999/xhtml">
<head>
<title>jQuery tmpl leak</title>
<script src="http://code.jquery.com/jquery.js"></script>
<script src="jquery.tmpl.js" type="text/javascript"></script>
<script id="tmpl" type="text/html">
<div>${text}</div>
</script>
<script type="text/javascript">
    $(window).load(function () {
            $("a").click(function () {
                    for (var i = 0; i < 10000; i++) {
                            var node = $("#tmpl").tmpl({ text: "blah" });
                    }
            });
    });
</script>
<body>
<a href="javascript:void(0)">leak me</a>
</body>
</html>

Comments BorisMoore July 23, 2010 Yes, thanks, there is a memory leak if you don't actually use the rendered template by using $("#tmpl").tmpl({ text: "blah" }).appendTo("#target"); or equivalent.

In real scenarios you would not render the template without inserting it into the DOM. But I hope to investigate the memory leak, nevertheless... It may be to do with the way jQuery caches dom fragments, and at any rate this will encourage me to figure out whether our caching approach is optimal...

sagacity commented 13 years ago

We are also running into memory leaks in combination with Knockout. Disabling template rendering causes the memory leak to disappear, so we are assuming it is related to jquery.tmpl somehow.

The fiddle is available here: http://jsfiddle.net/4KSXe/

johnfields commented 13 years ago

I'm also running into memory leaks with knockout/jquery.tmpl. Please let me know if you'd like me to provide more details or demo code.

sagacity commented 13 years ago

To remove knockout from the equation, I have modified the jquery.tmpl "each" sample to continually rerender the same template. In this case, memory usage also continually increases.

The fiddle is here: http://jsfiddle.net/PqSrT/

However, if I use the appendTo provided by jQuery.tmpl, this is what calls the 'complete' callback which clears the DOM cache 'newTmplItems'. Then, the memory usage remains bounded: http://jsfiddle.net/PqSrT/1/

So there probably should be either an external way to clear this cache (exposing a clearCache() function or something) or a way to 'automatically' clean this cache, I suppose?

sagacity commented 13 years ago

I've also identified the cause of the leak in KO: jquery.tmpl adds data with key 'tmplItem' to the rendered elements. This is not cleared by Knockout when it removes DOM nodes, since it does this 'outside' of jquery.tmpl and therefore the data does not get removed.

thelinuxlich commented 13 years ago

+1

BorisMoore commented 13 years ago

The issue here is the one I called out at the top of this issue: You need to use the correct API for inserted content rendered by jQuery Templates: $("#tmpl").tmpl({ text: "blah" }).appendTo("#target"); or equivalent.

Sounds like ko is (incorrectly) inserting $("#tmpl").tmpl({ text: "blah" }) directly in the DOM.

In fact $("#myTemplate").tmpl(data) creates a document fragment, ready for inserting (and potentially cloning as necessary) by a chained appendTo , .prependTo, .insertAfter or .insertBefore. But it also creates an in-memory data structure which is provided to the appendTo (or equivalent) method, and that data is disposed in appendTo. If you repeatedly call .tmpl, without chaining the appendTo or equivalent, then you have a memory leak because the line 'appendToTmplItems = null;' (line 65 in https://github.com/jquery/jquery-tmpl/blob/master/jquery.tmpl.js) is never called.

BorisMoore commented 13 years ago

I have updated the doc here http://api.jquery.com/tmpl/ and here http://api.jquery.com/tmpl/ to call out more strongly the importance of chaining with appendTo etc.

BorisMoore commented 13 years ago

I have updated the doc here http://api.jquery.com/tmpl/ and here http://api.jquery.com/tmpl/ to call out more strongly the importance of chaining with appendTo etc.

sagacity commented 13 years ago

Excellent. KO has also improved support and now uses the proper methods!

BorisMoore commented 13 years ago

Yes. that's good news. I was in touch with Steve Sanderson, who told me about his fix.

sagacity commented 13 years ago

Good :) As far as I'm concerned this issue can then be closed, though others may want to weigh in as well.

BorisMoore commented 13 years ago

Keeping open for now, to track the fact that incorrect usage of the API can lead to memory leaks.

yori-s commented 13 years ago

I also got bit by something similar extracting html strings for use with dataTables, which takes html but not dom nodes or jquery objects as custom column output:

// leaks jQuery.data
return $('<div />').append($("#template").tmpl(someData)).html();
// ok
return $('<div />').append($("#template").tmpl(someData).removeData("tmplItem")).html();

Maybe this is more pertinent to #80, but it would be nice to have appendTo, etc. perform the removal by default

BorisMoore commented 13 years ago

appendTo does remove data by default. - if you use the API as intended, such as: $("#tmpl").tmpl({ text: "blah" }).appendTo("#target")

If you do: append($("#template").tmpl(someData)) - which is incorrect usage - then there will generally be memory leaks. It is after chaining appendTo that the correct .clean() code is called.

yori-s commented 13 years ago

append was just shorthand and (pleasantly) appears to work as appendTo (complete gets called anyway), so ignore for the moment. The jQuery.data issue applies to appendTo as well - the following uses jquery 1.6.2 and templates beta from the cdn: http://jsfiddle.net/XKWXU/1/

BorisMoore commented 13 years ago

$("#item-template").tmpl(mockResponse(20).books).appendTo("#results") will add jQuery data. (This is by design. It is used when you call $.tmplItem, for example).

If you want to empty the #results without a memory leak, you certainly have to call .empty(), .html() or one of the other jQuery methods which are designed to call clean. Setting innerHTMl ='""; will of course not call clean and will lead to a memory leak. This is the regular jQuery usage pattern. Same will happen when you attach a jQuery UI widget to an element, then remove that element by setting innerHTML = "" on the parent...

rdworth commented 13 years ago

Thanks for taking the time to submit this issue. Just wanted to let you know this plugin is no longer being actively developed or maintained by the jQuery team. See README for more info.