Skip to content

Prevent potential memory leaks in jQueryUI pseudo selectors#1309

Closed
dcherman wants to merge 2 commits intojquery:masterfrom
dcherman:bug_10538
Closed

Prevent potential memory leaks in jQueryUI pseudo selectors#1309
dcherman wants to merge 2 commits intojquery:masterfrom
dcherman:bug_10538

Conversation

@dcherman
Copy link
Copy Markdown

When these pseudo selectors are used, they have the side effect of creating the data cache for a given element somewhat surprisingly. If you use these pseudo selectors on a set of elements and then null out those references ( or just let them fall out of scope ) without first calling something that invokes $.cleanData, you've just leaked memory.

I could not figure out a way to reasonably unit test this since in jQuery 2.x, there is no way to actually inspect whether or not the cache exists for a given element since it's contained entirely within jQuery's closure.

Fixes #10538

Prevent potential memory leaks in the :data() pseudo selector by first
checking if an element has data in the jQuery cache before actually
inspecting the data itself so that the data object isn't inappropriately
created.
Prevent potential memory leaks in the widget pseudo selectors by first
checking if an element has data in the jQuery cache before actually
inspecting the data itself so that the data object isn't inappropriately
created.
@scottgonzalez
Copy link
Copy Markdown
Member

The widget selectors will never leak since they always provide a key.

@scottgonzalez
Copy link
Copy Markdown
Member

And the :data() selector is only meant to be used with a key as well, so neither of these leak with the documented APIs.

@dcherman
Copy link
Copy Markdown
Author

@scottgonzalez I don't think that's correct, and here's a JSFiddle to demonstrate it.

http://jsfiddle.net/56p4pb7b/

I've modified the version of jQuery 2 in use there to expose the public data on $.cache so that we can inspect it, and you can see that simply using one of these pseudo selectors has the side effect of populating the cache if it did not exist.

@scottgonzalez
Copy link
Copy Markdown
Member

So is this limited to jQuery 2.x?

@dcherman
Copy link
Copy Markdown
Author

Yes, I should have specified that. My bad.

@dcherman
Copy link
Copy Markdown
Author

Just realized that it looks like I improperly modified the <1.8 path; assuming you're interested in this, should I revert that or leave it as-is for consistency with the other code paths?

@scottgonzalez
Copy link
Copy Markdown
Member

I'd like to go down the road of getting jQuery 2.x to not have this behavior before deciding to land this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants