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 comment:1
Changed December 25, 2010 07:26PM UTC by 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 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: | unfiled → data |
| keywords: | → memory leak |
| milestone: | 1.next → 1.4.5 |
| priority: | undecided → blocker |
| status: | new → open |
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 comment:4
| milestone: | 1.4.5 → 1.5 |
|---|---|
| owner: | → rwaldron |
| status: | open → assigned |
If no one is taking this, I'm up for it.
Changed December 28, 2010 03:17AM UTC by 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 comment:6
| _comment0: | Moved the if ( isNode ) condition to ensure it doesn't get skipped. \ \ https://github.com/jquery/jquery/pull/152 → 1293558880015641 |
|---|
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 comment:7
| blockedby: | → 6968 |
|---|
Changed December 28, 2010 07:00PM UTC by comment:8
| keywords: | memory leak → memory 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 comment:9
| owner: | rwaldron → snover |
|---|
This is fixed in https://github.com/jquery/jquery/pull/170
Changed January 17, 2011 09:39PM UTC by comment:10
| resolution: | → fixed |
|---|---|
| status: | assigned → closed |
This should be fixed by commit [8e59a99e0ade75dec434f246f52e8b3f7393f359].
Changed February 17, 2011 11:24PM UTC by 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 comment:12
| keywords: | memory leak needsdocs → memory leak |
|---|
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.