Bug Tracker

Modify

Ticket #7840 (closed bug: fixed)

Opened 3 years ago

Last modified 2 years ago

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

Reported by: algorkov@… Owned by: snover
Priority: blocker Milestone: 1.5
Component: data Version: 1.4.4
Keywords: memory leak Cc:
Blocking: Blocked by: #6968

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

Change History

comment:1 follow-up: ↓ 2 Changed 3 years ago by 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.

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.

comment:2 in reply to: ↑ 1 Changed 3 years ago by algorkov@…

Replying to 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

comment:3 Changed 3 years ago by snover

  • Keywords memory leak added
  • Priority changed from undecided to blocker
  • Status changed from new to open
  • Component changed from unfiled to data
  • Milestone changed from 1.next to 1.4.5

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

Version 1, edited 3 years ago by snover (previous) (next) (diff)

comment:4 Changed 3 years ago by rwaldron

  • Owner set to rwaldron
  • Status changed from open to assigned
  • Milestone changed from 1.4.5 to 1.5

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

comment:5 Changed 3 years ago by dmethvin

...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.

comment:6 Changed 3 years ago by rwaldron

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

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

INVALID - DO NOT PULL.

Last edited 3 years ago by rwaldron (previous) (diff)

comment:7 Changed 3 years ago by rwaldron

  • Blocked by 6968 added

comment:8 Changed 3 years ago by dmethvin

  • Keywords needsdocs added

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.

comment:9 Changed 3 years ago by snover

  • Owner changed from rwaldron to snover

comment:10 Changed 3 years ago by snover

  • Status changed from assigned to closed
  • Resolution set to fixed

This should be fixed by commit [8e59a99e0ade75dec434f246f52e8b3f7393f359].

comment:11 Changed 3 years ago by timmywil

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

comment:12 Changed 3 years ago by jitter

  • Keywords needsdocs removed

Please follow the  bug reporting guidlines and use  jsFiddle when providing test cases and demonstrations instead of pasting the code in the ticket.

View

Add a comment

Modify Ticket

Action
as closed
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.