#8335 closed bug (fixed)
.data leaks when used on a comment node.
Reported by: | 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 12 years ago by
Component: | unfiled → data |
---|---|
Keywords: | needsreview added |
Priority: | undecided → low |
comment:2 Changed 12 years ago by
comment:3 follow-up: 4 Changed 12 years ago by
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 Changed 12 years ago by
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 12 years ago by
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 12 years ago by
Resolution: | → invalid |
---|---|
Status: | new → closed |
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 12 years ago by
Keywords: | needsreview removed |
---|
comment:8 Changed 10 years ago by
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 10 years ago by
Resolution: | invalid → fixed |
---|
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 10 years ago by
Fixes #8335. Do not allow add data to non-elements (2.x). Closes gh-1232
Changeset: f61314ff5cb5e6b620ace758a6f00c94a3031154
jQuery doesn't support comment nodes afaik. So this is invalid.