Skip to main content

Bug Tracker

Side navigation

#7840 closed bug (fixed)

Opened December 24, 2010 10:39PM UTC

Closed January 17, 2011 09:39PM UTC

Last modified March 13, 2012 11:25PM UTC

Element data cache is not cleared on removeData() when the method is called without arguments

Reported by: algorkov@gmail.com Owned by: snover
Priority: blocker Milestone: 1.5
Component: data Version: 1.4.4
Keywords: memory leak Cc:
Blocked by: Blocking:
Description

jQuery 1.4.4

IE8, FF3.6, Chrome 8

Windows 7

According to the documentation and comments in jQuery code for removeData() method "...when called with no arguments, all values are removed".

But it is obvious from code of the data.js module that if browser either suppors deleting expandos or DOM element has removeAttribute method then only this expando attribute will be removed but jQuery.cache item for the respective element data states in place. That is the link to the data is broken but the data itself is kept. I do not see any obvious reason for such behavior unless someone explains me the reasoning.

Why keep the data in memory when it was explicitly removed and can't be accessed?

Ok, the code in jQuery data.js module is:

// Otherwise, we want to remove all of the element's data
} else {
	if ( isNode && jQuery.support.deleteExpando ) {
		delete elem[ jQuery.expando ];

	} else if ( elem.removeAttribute ) {
		elem.removeAttribute( jQuery.expando );

	// Completely remove the data cache
	} else if ( isNode ) {
		delete cache[ id ];

	// Remove all fields from the object
	} else {
		for ( var n in elem ) {
			delete elem[ n ];
		}
	}
}

Hope my point is clear. And here is the link to jsFiddle script

to reproduce the behavior: http://jsfiddle.net/algor/6vjtM/1/

Please enable Firebug/Chrome/IE console because the script logs to it.

Also, maybe I am wrong but the aforementioned piece of code in data.js looks poorly written to me. For example, why there is no check for isNode when elem.removeAttribute existence is verified. And why the case for object properties deletion is in another "else" while it is most probably intended for plain objects and hence it would be better to logically separate it from this incomprehensible sequence of "else's".

On a separate note. Could someone name a person/place where/to whom it is appropriate to ask questions regarding jQuery data module implementation and jQuery core in general? I mean it would be great to know that the person who answers actually possesses sacred knowledge and not just speculates and makes assumptions regarding reasons why something is implemented the way it is.

Thanks

Attachments (0)
Change History (12)

Changed December 25, 2010 05:20PM UTC by dmethvin comment:1

I agree, it looks like a bug. } else if ( isNode ) { should be } if ( isNode ) { don't you think? Although I agree this whole sequence could probably be written better.

Many different people contribute to the jQuery code base. All of them are human as far as I know (although this *is* the Internet and some of them might be dogs). So sometimes we make mistakes. The great thing about open source is that anyone can review the code and find the bugs.

For the philosophical questions it is probably better to post questions in the Developing jQuery Core section of forum.jquery.com.

Changed December 25, 2010 07:26PM UTC by algorkov@gmail.com comment:2

Replying to [comment:1 dmethvin]:

I agree, it looks like a bug. } else if ( isNode ) { should be } if ( isNode ) { don't you think? Although I agree this whole sequence could probably be written better.

Yes, if the intention was to also clear the cache then it is bug. I do think that if (isNode) should be without else, and yes I believe some parts of the data.js module could be rewritten.

Many different people contribute to the jQuery code base. All of them are human as far as I know (although this *is* the Internet and some of them might be dogs). So sometimes we make mistakes. The great thing about open source is that anyone can review the code and find the bugs.

Not sure I understand the joke about dogs :). I absolutely understand inevitability of errors and did not mean to offend or hurt anybody's feelings. Sorry if it is the case.

For the philosophical questions it is probably better to post questions in the Developing jQuery Core section of forum.jquery.com.

No, I am not good at any kind of philisophy. My aspiration is just to clarify some parts of code which look obscure to me (completely due to my lack of understanding). On the other hand, I do not think there is something wrong in the desire to make some part of open source code clear if the does not look this way and is not well documented either.

Thank you very much for pointing to Developing jQuery Core section of forum.query.com.

Thanks

Changed December 27, 2010 01:24AM UTC by snover comment:3

_comment0: Yikes. Looks like a bad typo.1293417115451085
_comment1: Yikes. Looks like a bad typo. Of course, shouldn’t we also be running `data.events` through `jQuery.event.remove` before we nuke it?1293417140259911
component: unfileddata
keywords: → memory leak
milestone: 1.next1.4.5
priority: undecidedblocker
status: newopen

Yikes. Looks like a bad typo. Of course, shouldn’t we also be running data.events through jQuery.event.remove before we nuke it? Or just, like, persist events if they exist?

Changed December 27, 2010 10:41PM UTC by rwaldron comment:4

milestone: 1.4.51.5
owner: → rwaldron
status: openassigned

If no one is taking this, I'm up for it.

Changed December 28, 2010 03:17AM UTC by dmethvin comment:5

...shouldn’t we also be running data.events through jQuery.event.remove before we nuke it...?

jQuery.cleanData should be able to do the job for that, although it may need to be modified to handle plain JS objects.

Changed December 28, 2010 05:26PM UTC by rwaldron comment:6

_comment0: Moved the if ( isNode ) condition to ensure it doesn't get skipped. \ \ https://github.com/jquery/jquery/pull/1521293558880015641

Moved the if ( isNode ) condition to ensure it doesn't get skipped.

https://github.com/jquery/jquery/pull/152

INVALID - DO NOT PULL.

Changed December 28, 2010 05:55PM UTC by rwaldron comment:7

blockedby: → 6968

Changed December 28, 2010 07:00PM UTC by dmethvin comment:8

keywords: memory leakmemory leak needsdocs

Just a note on some further investigation from rwaldron and myself:

The .removeData() feature to remove all data was added in 1.4, but has not been taking into account any internal jQuery data stored in the data object. In particular, this causes the event and handle objects to leak as snover mentioned above. So the fix is more complex than it might first appear. Most likely .removeData() should not remove jQuery-internal data and events, and it would be much simpler to do that once we've named all the internal data objects consistently.

Changed January 09, 2011 10:53PM UTC by snover comment:9

owner: rwaldronsnover

Changed January 17, 2011 09:39PM UTC by snover comment:10

resolution: → fixed
status: assignedclosed

This should be fixed by commit [8e59a99e0ade75dec434f246f52e8b3f7393f359].

Changed February 17, 2011 11:24PM UTC by timmywil comment:11

I think with the fix, this now does what the docs say. You can probably remove the needsdocs.

Changed February 18, 2011 12:10PM UTC by jitter comment:12

keywords: memory leak needsdocsmemory leak