Skip to content
Permalink
Browse files

Change the way jQuery.data works so that there is no longer a chance …

…of collision between user data and internal data. Fixes #6968.
  • Loading branch information...
csnover committed Jan 9, 2011
1 parent 1d1d4fe commit 8e59a99e0ade75dec434f246f52e8b3f7393f359
Showing with 341 additions and 192 deletions.
  1. +2 −2 src/attributes.js
  2. +107 −55 src/data.js
  3. +5 −5 src/effects.js
  4. +25 −21 src/event.js
  5. +21 −14 src/manipulation.js
  6. +2 −2 src/queue.js
  7. +4 −6 test/unit/attributes.js
  8. +162 −76 test/unit/data.js
  9. +2 −2 test/unit/effects.js
  10. +11 −9 test/unit/event.js
@@ -133,11 +133,11 @@ jQuery.fn.extend({
} else if ( type === "undefined" || type === "boolean" ) {
if ( this.className ) {
// store className if set
jQuery.data( this, "__className__", this.className );
jQuery._data( this, "__className__", this.className );
}

// toggle whole className
this.className = this.className || value === false ? "" : jQuery.data( this, "__className__" ) || "";
this.className = this.className || value === false ? "" : jQuery._data( this, "__className__" ) || "";
}
});
},
@@ -1,7 +1,6 @@
(function( jQuery ) {

var windowData = {},
rbrace = /^(?:\{.*\}|\[.*\])$/;
var rbrace = /^(?:\{.*\}|\[.*\])$/;

jQuery.extend({
cache: {},
@@ -23,110 +22,163 @@ jQuery.extend({
},

hasData: function( elem ) {
if ( elem.nodeType ) {
elem = jQuery.cache[ elem[jQuery.expando] ];
}
elem = elem.nodeType ? jQuery.cache[ elem[jQuery.expando] ] : elem[ jQuery.expando ];

return !!elem && !jQuery.isEmptyObject(elem);
},

data: function( elem, name, data ) {
data: function( elem, name, data, pvt /* Internal Use Only */ ) {
if ( !jQuery.acceptData( elem ) ) {
return;
}

elem = elem == window ?
windowData :
elem;
var internalKey = jQuery.expando, getByName = typeof name === "string", thisCache,

// We have to handle DOM nodes and JS objects differently because IE6-7
// can't GC object references properly across the DOM-JS boundary
isNode = elem.nodeType,

// Only DOM nodes need the global jQuery cache; JS object data is
// attached directly to the object so GC can occur automatically
cache = isNode ? jQuery.cache : elem,

var isNode = elem.nodeType,
id = isNode ? elem[ jQuery.expando ] : null,
cache = jQuery.cache, thisCache;
// Only defining an ID for JS objects if its cache already exists allows
// the code to shortcut on the same path as a DOM node with no cache
id = isNode ? elem[ jQuery.expando ] : elem[ jQuery.expando ] && jQuery.expando;

if ( isNode && !id && typeof name === "string" && data === undefined ) {
// Avoid doing any more work than we need to when trying to get data on an
// object that has no data at all
if ( (!id || (pvt && id && !cache[ id ][ internalKey ])) && getByName && data === undefined ) {

This comment has been minimized.

Copy link
@ndp

ndp May 5, 2011

We are having problems with this line on IE7. I have an empty DOM node that I've inserted into the page with a jQuery template. I call empty() on it, and-- on IE7-- it croaks. Actually, it seems like it works once, but then not the second time, but I don't have a good Javascript tracing setup working, and using alerts() doesn't work, since this method appears to be called 100s of times.

I'm getting the IE equivalent of a NPE. In our case, it appears that cache[id] is not set, even though pvt and id are, and therefore we are getting an error.

Anyway, I am able to "fix" the problem by substituting "(!cache[id] || !cache[id][internalKey])"

I have no idea if this is right.

Would appreciate some guidance. I'm not sure what is actually necessary to reproduce and test, since I'm not familiar with the intent of this code.

This comment has been minimized.

Copy link
@dmethvin

dmethvin May 5, 2011

Member

This isn't a good place to ask for support. Since the problem surfaces with .template() you should ask about it there. Regardless, whoever tries to track it down will need a reduced test case in order to debug it.

return;
}

// Get the data from the object directly
if ( !isNode ) {
cache = elem;
if ( !id ) {
// Only DOM nodes need a new unique ID for each element since their data
// ends up in the global cache
if ( isNode ) {
elem[ jQuery.expando ] = id = ++jQuery.uuid;
} else {
id = jQuery.expando;
}
}

// Compute a unique ID for the element
} else if ( !id ) {
elem[ jQuery.expando ] = id = ++jQuery.uuid;
if ( !cache[ id ] ) {
cache[ id ] = {};

This comment has been minimized.

Copy link
@BorisMoore

BorisMoore Jan 31, 2011

This means that currently the event binding code, for example, is adding jQuery expandos to plain objects that are not of type function, and therefore show up on JSON serialized objects. It is inconsistent with this comment in the event.add method:

if ( typeof events === "function" ) {
// On plain objects events is a fn that holds the the data
// which prevents this data from being JSON serialized
// the function does not need to be called, it just contains the data

Suggested fix:
if ( !cache[ id ] ) {
// cache[ id ] = {};
cache[ id ] = isNode ? {} : function(){};
}

}

// Avoid generating a new cache unless none exists and we
// want to manipulate it.
// An object can be passed to jQuery.data instead of a key/value pair; this gets
// shallow copied over onto the existing cache
if ( typeof name === "object" ) {
if ( isNode ) {
if ( pvt ) {
cache[ id ][ internalKey ] = jQuery.extend(cache[ id ][ internalKey ], name);
} else {
cache[ id ] = jQuery.extend(cache[ id ], name);
}
}

} else {
jQuery.extend( cache, name );
thisCache = cache[ id ];

// Internal jQuery data is stored in a separate object inside the object's data
// cache in order to avoid key collisions between internal data and user-defined
// data
if ( pvt ) {
if ( !thisCache[ internalKey ] ) {
thisCache[ internalKey ] = {};
}

} else if ( isNode && !cache[ id ] ) {
cache[ id ] = {};
thisCache = thisCache[ internalKey ];
}

thisCache = isNode ? cache[ id ] : cache;

// Prevent overriding the named cache with undefined values
if ( data !== undefined ) {
thisCache[ name ] = data;
}

return typeof name === "string" ? thisCache[ name ] : thisCache;
return getByName ? thisCache[ name ] : thisCache;
},

removeData: function( elem, name ) {
removeData: function( elem, name, pvt /* Internal Use Only */ ) {
if ( !jQuery.acceptData( elem ) ) {
return;
}

elem = elem == window ?
windowData :
elem;
var internalKey = jQuery.expando, isNode = elem.nodeType,

// See jQuery.data for more information
cache = isNode ? jQuery.cache : elem,

// See jQuery.data for more information
id = isNode ? elem[ jQuery.expando ] : jQuery.expando;

var isNode = elem.nodeType,
id = isNode ? elem[ jQuery.expando ] : elem,
cache = jQuery.cache,
thisCache = isNode ? cache[ id ] : id;
// If there is already no cache entry for this object, there is no
// purpose in continuing
if ( !cache[ id ] ) {
return;
}

// If we want to remove a specific section of the element's data
if ( name ) {
var thisCache = pvt ? cache[ id ][ internalKey ] : cache[ id ];

if ( thisCache ) {
// Remove the section of cache data
delete thisCache[ name ];

// If we've removed all the data, remove the element's cache
if ( isNode && jQuery.isEmptyObject(thisCache) ) {
jQuery.removeData( elem );
// If there is no data left in the cache, we want to continue
// and let the cache object itself get destroyed
if ( !jQuery.isEmptyObject(thisCache) ) {
return;
}
}
}

// See jQuery.data for more information
if ( pvt ) {
delete cache[ id ][ internalKey ];

// Don't destroy the parent cache unless the internal data object
// had been the only thing left in it
if ( !jQuery.isEmptyObject(cache[ id ]) ) {
return;
}
}

var internalCache = cache[ id ][ internalKey ];

// Otherwise, we want to remove all of the element's data
// Browsers that fail expando deletion also refuse to delete expandos on
// the window, but it will allow it on all other JS objects; other browsers
// don't care
if ( jQuery.support.deleteExpando || cache != window ) {
delete cache[ id ];
} else {
if ( isNode && jQuery.support.deleteExpando ) {
delete elem[ jQuery.expando ];
cache[ id ] = null;
}

// We destroyed the entire user cache at once because it's faster than
// iterating through each key, but we need to continue to persist internal
// data if it existed
if ( internalCache ) {
cache[ id ] = {};
cache[ id ][ internalKey ] = internalCache;

// Otherwise, we need to eliminate the expando on the node to avoid
// false lookups in the cache for entries that no longer exist
} else if ( isNode ) {
// IE does not allow us to delete expando properties from nodes,
// nor does it have a removeAttribute function on Document nodes;
// we must handle all of these cases
if ( 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 ];
}
elem[ jQuery.expando ] = null;
}
}
},

// For internal use only.
_data: function( elem, name, data ) {
return jQuery.data( elem, name, data, true );
},

// A method for determining if a DOM node can handle the data expando
acceptData: function( elem ) {
if ( elem.nodeName ) {
@@ -27,15 +27,15 @@ jQuery.fn.extend({

// Reset the inline display of this element to learn if it is
// being hidden by cascaded rules or not
if ( !jQuery.data(elem, "olddisplay") && display === "none" ) {
if ( !jQuery._data(elem, "olddisplay") && display === "none" ) {
display = elem.style.display = "";
}

// Set elements which have been overridden with display: none
// in a stylesheet to whatever the default browser style is
// for such an element
if ( display === "" && jQuery.css( elem, "display" ) === "none" ) {
jQuery.data(elem, "olddisplay", defaultDisplay(elem.nodeName));
jQuery._data(elem, "olddisplay", defaultDisplay(elem.nodeName));
}
}

@@ -46,7 +46,7 @@ jQuery.fn.extend({
display = elem.style.display;

if ( display === "" || display === "none" ) {
elem.style.display = jQuery.data(elem, "olddisplay") || "";
elem.style.display = jQuery._data(elem, "olddisplay") || "";
}
}

@@ -62,8 +62,8 @@ jQuery.fn.extend({
for ( var i = 0, j = this.length; i < j; i++ ) {
var display = jQuery.css( this[i], "display" );

if ( display !== "none" && !jQuery.data( this[i], "olddisplay" ) ) {
jQuery.data( this[i], "olddisplay", display );
if ( display !== "none" && !jQuery._data( this[i], "olddisplay" ) ) {
jQuery._data( this[i], "olddisplay", display );
}
}

5 comments on commit 8e59a99

@louisremi

This comment has been minimized.

Copy link
Contributor

louisremi replied Feb 8, 2011

Have you tested the impact on performance of such change? You are adding one more function call when using jQuery._data() instead of jQuery.data().
In places were performance is critical, we should probably use the old jQuery.data with true as the last argument, shouldn't we?

@louisremi

This comment has been minimized.

Copy link
Contributor

louisremi replied Feb 8, 2011

And what does "Internal Use Only" really means? Should plugin developers use it?
Has it been documented somewhere?

@dmethvin

This comment has been minimized.

Copy link
Member

dmethvin replied Feb 8, 2011

@lrbabe, our intent with jQuery._data() is to prevent name collisions between internal jQuery data and external data attached by user code or plugins. Plugin developers should continue to use jQuery.data() or $().data() but of course they can use their own conventions to avoid collisions there, such as prepending "myPlugin" to the name of each data item or creating a master "myPlugin" object that contains all the data.

@louisremi

This comment has been minimized.

Copy link
Contributor

louisremi replied Feb 8, 2011

Thank you for these explanations @dmethvin.
What about always passing true (or 1 if we're concerned about filesize) as a fourth param instead of using _data and adding unnecessary function calls?

@dmethvin

This comment has been minimized.

Copy link
Member

dmethvin replied Feb 9, 2011

@lrbabe, we discussed and didn't think an extra function call would make too much difference. If you'd like to create some benchmarks to show a performance hit, we could take a look.

Please sign in to comment.
You can’t perform that action at this time.