#8335 closed bug (fixed)

Opened February 21, 2011 05:55AM UTC

Closed April 01, 2011 03:11AM UTC

Last modified April 08, 2013 07:10PM UTC

.data leaks when used on a comment node.

Reported by:	adam.freidin@gmail.com	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.

Attachments (0)

Change History (10)

Changed February 21, 2011 03:41PM UTC by rwaldron comment:1

component:	unfiled → data
keywords:	→ needsreview
priority:	undecided → low

Changed February 23, 2011 12:41AM UTC by jitter comment:2

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

Changed February 23, 2011 01:56AM UTC by anonymous comment:3

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.

Changed February 23, 2011 10:43AM UTC by jitter comment:4

Replying to [comment:3 anonymous]:

Did you see the fiddle?
$('')
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.

Changed February 23, 2011 03:56PM UTC by Adam Freidin comment:5

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.

Changed April 01, 2011 03:11AM UTC by dmethvin comment:6

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.

Changed April 01, 2011 03:11AM UTC by dmethvin comment:7

keywords:	needsreview

Changed January 08, 2013 01:59PM UTC by danilsomsikov@google.com comment:8

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

Changed January 16, 2013 07:43PM UTC by danilsomsikov comment:9

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

Changed April 08, 2013 07:10PM UTC by Oleg Gaidarenko comment:10

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

Changeset: f61314ff5cb5e6b620ace758a6f00c94a3031154

Side navigation