Bug Tracker

Opened 9 years ago

Closed 9 years ago

Last modified 7 years ago

#8335 closed bug (fixed)

.data leaks when used on a comment node.

Reported by: adam.freidin@… Owned by:
Priority: low Milestone: 1.next
Component: data Version: 1.5
Keywords: Cc:
Blocked by: Blocking:

Description

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.

Change History (10)

comment:1 Changed 9 years ago by Rick Waldron

Component: unfileddata
Keywords: needsreview added
Priority: undecidedlow

comment:2 Changed 9 years ago by jitter

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

comment:3 Changed 9 years ago by 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.

comment:4 in reply to:  3 Changed 9 years ago by 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.

comment:5 Changed 9 years ago by 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.

comment:6 Changed 9 years ago by dmethvin

Resolution: invalid
Status: newclosed

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.

comment:7 Changed 9 years ago by dmethvin

Keywords: needsreview removed

comment:8 Changed 7 years ago by 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

comment:9 Changed 7 years ago by danilsomsikov

Resolution: invalidfixed

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

comment:10 Changed 7 years ago by Oleg Gaidarenko

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

Changeset: f61314ff5cb5e6b620ace758a6f00c94a3031154

Note: See TracTickets for help on using tickets.