.data leaks when used on a comment node.

[adam.freidin@…]

Example: ​http://jsfiddle.net/mcfJh/

when a comment node is created and data is applied, the jQuery cache is not restored to its original state when the comment node is removed.

My fix is, in jQuery.remove, to replace

if ( !keepData && elem.nodeType === 1 ) {
     jQuery.cleanData( elem.getElementsByTagName("*") );
     jQuery.cleanData( [ elem ] );
}

with:

if ( !keepData ) {
     if ( elem.nodeType === 1 ) {
         jQuery.cleanData( elem.getElementsByTagName("*") );
     }
     jQuery.cleanData( [ elem ] );
}

the jQuery gurus will have to decide if it's better to have if ( !keepData && (nodeType === 1 || nodeType === 8)) at the top, and if there is any other code that need to updated.

[jitter]

jQuery doesn't support comment nodes afaik. So this is invalid.

[anonymous]

Did you see the fiddle? $('<!-- blah -->') makes a comment node. And then you can assign data to it, and then it leaks said data (as in, retains it in the jQuery.cache) when it's removed.

[jitter]

Replying to anonymous:

Did you see the fiddle? $('<!-- blah -->') makes a comment node. And then you can assign data to it, and then it leaks said data (as in, retains it in the jQuery.cache) when it's removed.

Yes I saw the fiddle. Why are you asking? Only because it works to create a comment node and set data on it doesn't mean it's a supported thing to do.

[Adam Freidin]

Alright. I guess my fix can be in the form of

jQuery.remove = function(old_remove) {
  return function() { .... return old_remove.apply(this, arguments); };
}.call(null, jQuery.remove);

and we don't have to add cycles everyone else's remove function to handle my leak. Thanks for explaining that I'm outside the box of supported behaviour.

[dmethvin]

jQuery collections shouldn't have comment or text nodes other than a few special cases like .content(). So we don't support .data() being used on them.

[danilsomsikov@…]

This is actually a critical leak. Docs says that .data() is memory leak safe, but it is so easy to create a leak. .data() method sets data on any kind of node but, .remove(), .empty() and .html() removes data only for elements. This is definitely wrong!

There are million of ways to get non-element node wrapped with jQuery, like .content() function, html string parsing, etc., and you can never be sure if you have non-element nodes in your jQuery object.

.data() method should either disallow setting data on non-element nodes or their data should be clean up properly by .remove(), .empty() and .html().

Pull request: ​https://github.com/jquery/jquery/pull/1122

[danilsomsikov]

Fix #8335: Avoid memory leak by never setting data on non-element non-document nodes. Ref gh-1127. (cherry picked from commit cc324abf739669bd2a4669742c994b86c4ad467b)

Changeset: 8f72967ee2090d4b0d38a59b2e4743e0ce7a1b5e

[Oleg Gaidarenko]

Fixes #8335. Do not allow add data to non-elements (2.x). Closes gh-1232

Changeset: f61314ff5cb5e6b620ace758a6f00c94a3031154