Skip to content

Data: avoid using delete on DOM nodes #2479

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
jbedard wants to merge 1 commit into from
Closed

Data: avoid using delete on DOM nodes #2479

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 12 additions & 5 deletions src/data/Data.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,13 +21,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
});
}
Expand Down Expand Up @@ -147,7 +146,15 @@ 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
Copy link
Member

@markelog markelog Jul 19, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this should be a "Support:" comment, we didn't do it for the compat but perhaps we should.

So we talking about Safari, Chrome. Could you try to identify affected versions? I guess we can omit other Chromium browsers since we don't do it in other cases.

Copy link
Member

@markelog markelog Jul 19, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI jquery/contribute.jquery.org#95 (comment)

Copy link
Contributor Author

@jbedard jbedard Jul 19, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess Support: Chrome <=35 - 45 then? It could have been an issue earlier but it looks like it was originally reported for Chrome35, and it seems to still be an issue last time I tested in 45.

I'm not sure about Safari though. I don't recall ever seeing any webkit/safari bugs for this. I think the "Webkit &" was because it was noticeable in jsperf (#1664 (comment)).

Copy link
Member

@markelog markelog Jul 20, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was is it noticeable specifically on Safari browser? If it was, can you recall version numbers?

Copy link
Contributor Author

@jbedard jbedard Jul 20, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I never tested safari myself so I'm not sure, I was just basing that on the comment: #1664 (comment)

// from DOM nodes, so set to undefined instead
// https://code.google.com/p/chromium/issues/detail?id=378607
Copy link
Member

@markelog markelog Jul 19, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, i get 403 for this link, you don't?

Copy link
Contributor Author

@jbedard jbedard Jul 19, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I get a 403 as well. It looks like google has made the bug private, maybe due to security issues with the bug?

Copy link
Member

@markelog markelog Jul 19, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm weird.

@paulirish could you help us out? Can we add another link or something?

Copy link

@Garbee Garbee Jul 19, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing can really be done here except remove the bug link. From the description of it given here, it either has a known security issue or they are verifying there isn't one before allowing it to be public again.

Copy link
Member

@markelog markelog Jul 20, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right now this is a speculation, there might be a mistake or issues was moved, or basically anything else, i would like to wait day or two for the @paulirish response

Copy link

@Garbee Garbee Jul 20, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Paul is on vacation. Surely won't respond within a few days.

Copy link
Member

@markelog markelog Jul 20, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I think we can add Support comment for the Chrome, if we don't have information on the Safari, we can leave something in manner of "Possibly reproducible on Safari too" and i will open a ticket for it, so we could deal with it later, when jsperf would be up.

Duplicate that comment in manipulation module too and since this matter was previously discussed already we can safely land it.

Sounds good?

Copy link
Member

@arthurvr arthurvr Jul 20, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

if ( owner.nodeType ) {
owner[ this.expando ] = undefined;
} else {
delete owner[ this.expando ];
}
}
},
hasData: function( owner ) {
Expand Down
8 changes: 6 additions & 2 deletions src/manipulation.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -283,10 +283,14 @@ jQuery.extend({
}
}
}
delete elem[ dataPriv.expando ];
// Support: Chrome <=35 - 45
// Assign undefined instead of using delete, see Data#remove
Copy link
Contributor Author

@jbedard jbedard Aug 7, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want the full comment copied here and below? Seemed like a lot to put in 3 places...

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;
}
}
}
Expand Down
19 changes: 18 additions & 1 deletion test/unit/data.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -859,7 +859,7 @@ testIframeWithCallback( "enumerate data attrs on body (#14894)", "data/dataAttrs
});

test( "Check that the expando is removed when there's no more data", function() {
expect( 1 );
expect( 2 );

var key,
div = jQuery( "<div/>" );
Expand All @@ -869,6 +869,23 @@ test( "Check that the expando is removed when there's no more data", function()

// Make sure the expando is gone
for ( key in div[ 0 ] ) {
if ( /^jQuery/.test( key ) ) {
strictEqual( div[ 0 ][ key ], undefined, "Expando was not removed when there was no more data" );
}
}
});

test( "Check that the expando is removed when there's no more data on non-nodes", function() {
expect( 1 );

var key,
obj = jQuery( {key: 42} );
obj.data( "some", "data" );
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 ) ) {
ok( false, "Expando was not removed when there was no more data" );
}
Expand Down
4 changes: 2 additions & 2 deletions test/unit/manipulation.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2089,7 +2089,7 @@ test( "jQuery.cleanData eliminates all private data (gh-2127)", function() {
});

test( "jQuery.cleanData eliminates all public data", function() {
expect( 2 );
expect( 3 );

var key,
div = jQuery( "<div/>" );
Expand All @@ -2103,7 +2103,7 @@ test( "jQuery.cleanData eliminates all public data", function() {
// Make sure the expando is gone
for ( key in div[ 0 ] ) {
if ( /^jQuery/.test( key ) ) {
ok( false, "Expando was not removed when there was no more data" );
strictEqual( div[ 0 ][ key ], undefined, "Expando was not removed when there was no more data" );
}
}
});
Expand Down