Data: avoid using delete on DOM nodes

jbedard · mgol · commit 0e982433eb94 · 2015-09-08T20:22:54.000+02:00
Closes gh-2479
diff --git a/src/data/Data.js b/src/data/Data.js
@@ -20,13 +20,12 @@ Data.prototype = {
 		if ( owner.nodeType ) {
 			owner[ this.expando ] = value;
 
-		// Otherwise secure it in a non-enumerable, non-writable property
-		// configurability must be true to allow the property to be
-		// deleted with the delete operator
+		// Otherwise secure it in a non-enumerable property
+		// configurable must be true to allow the property to be
+		// deleted when data is removed
 		} else {
 			Object.defineProperty( owner, this.expando, {
 				value: value,
-				writable: true,
 				configurable: true
 			} );
 		}
@@ -144,7 +143,16 @@ Data.prototype = {
 
 		// Remove the expando if there's no more data
 		if ( key === undefined || jQuery.isEmptyObject( cache ) ) {
-			delete owner[ this.expando ];
+
+			// Support: Chrome <= 35-45+
+			// Webkit & Blink performance suffers when deleting properties
+			// from DOM nodes, so set to undefined instead
+			// https://code.google.com/p/chromium/issues/detail?id=378607
+			if ( owner.nodeType ) {
+				owner[ this.expando ] = undefined;
+			} else {
+				delete owner[ this.expando ];
+			}
 		}
 	},
 	hasData: function( owner ) {
diff --git a/src/manipulation.js b/src/manipulation.js
@@ -290,10 +290,16 @@ jQuery.extend( {
 							}
 						}
 					}
-					delete elem[ dataPriv.expando ];
+
+					// Support: Chrome <= 35-45+
+					// Assign undefined instead of using delete, see Data#remove
+					elem[ dataPriv.expando ] = undefined;
 				}
 				if ( elem[ dataUser.expando ] ) {
-					delete elem[ dataUser.expando ];
+
+					// Support: Chrome <= 35-45+
+					// Assign undefined instead of using delete, see Data#remove
+					elem[ dataUser.expando ] = undefined;
 				}
 			}
 		}
diff --git a/test/unit/data.js b/test/unit/data.js
@@ -867,7 +867,7 @@ testIframeWithCallback(
 );
 
 QUnit.test( "Check that the expando is removed when there's no more data", function( assert ) {
-	assert.expect( 1 );
+	assert.expect( 2 );
 
 	var key,
 		div = jQuery( "<div/>" );
@@ -877,6 +877,23 @@ QUnit.test( "Check that the expando is removed when there's no more data", funct
 
 	// Make sure the expando is gone
 	for ( key in div[ 0 ] ) {
+		if ( /^jQuery/.test( key ) ) {
+			assert.strictEqual( div[ 0 ][ key ], undefined, "Expando was not removed when there was no more data" );
+		}
+	}
+});
+
+QUnit.test( "Check that the expando is removed when there's no more data on non-nodes", function( assert ) {
+	assert.expect( 1 );
+
+	var key,
+		obj = jQuery( {key: 42} );
+	obj.data( "some", "data" );
+	assert.equal( obj.data( "some" ), "data", "Data is added" );
+	obj.removeData( "some" );
+
+	// Make sure the expando is gone
+	for ( key in obj[ 0 ] ) {
 		if ( /^jQuery/.test( key ) ) {
 			assert.ok( false, "Expando was not removed when there was no more data" );
 		}
diff --git a/test/unit/event.js b/test/unit/event.js
@@ -2702,7 +2702,7 @@ QUnit.test( "Inline event result is returned (#13993)", function( assert ) {
 } );
 
 QUnit.test( ".off() removes the expando when there's no more data", function( assert ) {
-	assert.expect( 1 );
+	assert.expect( 2 );
 
 	var key,
 		div = jQuery( "<div/>" ).appendTo( "#qunit-fixture" );
@@ -2717,7 +2717,10 @@ QUnit.test( ".off() removes the expando when there's no more data", function( as
 	// Make sure the expando is gone
 	for ( key in div[ 0 ] ) {
 		if ( /^jQuery/.test( key ) ) {
-			assert.ok( false, "Expando was not removed when there was no more data" );
+			assert.strictEqual(
+				div[ 0 ][ key ], undefined,
+				"Expando was not removed when there was no more data"
+			);
 		}
 	}
 } );
diff --git a/test/unit/manipulation.js b/test/unit/manipulation.js
@@ -2086,7 +2086,7 @@ QUnit.test( "jQuery.cleanData eliminates all private data (gh-2127)", function(
 } );
 
 QUnit.test( "jQuery.cleanData eliminates all public data", function( assert ) {
-	assert.expect( 2 );
+	assert.expect( 3 );
 
 	var key,
 		div = jQuery( "<div/>" );
@@ -2100,7 +2100,7 @@ QUnit.test( "jQuery.cleanData eliminates all public data", function( assert ) {
 	// Make sure the expando is gone
 	for ( key in div[ 0 ] ) {
 		if ( /^jQuery/.test( key ) ) {
-			assert.ok( false, "Expando was not removed when there was no more data" );
+			assert.strictEqual( div[ 0 ][ key ], undefined, "Expando was not removed when there was no more data" );
 		}
 	}
 } );

Original file line number	Diff line number	Diff line change
`@@ -290,10 +290,16 @@ jQuery.extend( {`
`290`	`290`	`}`
`291`	`291`	`}`
`292`	`292`	`}`
`293`		`- delete elem[ dataPriv.expando ];`
	`293`	`+`
	`294`	`+ // Support: Chrome <= 35-45+`
	`295`	`+ // Assign undefined instead of using delete, see Data#remove`
	`296`	`+ elem[ dataPriv.expando ] = undefined;`
`294`	`297`	`}`
`295`	`298`	`if ( elem[ dataUser.expando ] ) {`
`296`		`- delete elem[ dataUser.expando ];`
	`299`	`+`
	`300`	`+ // Support: Chrome <= 35-45+`
	`301`	`+ // Assign undefined instead of using delete, see Data#remove`
	`302`	`+ elem[ dataUser.expando ] = undefined;`
`297`	`303`	`}`
`298`	`304`	`}`
`299`	`305`	`}`