Skip to content

.off() does not clean up empty cache entries #1760

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

Jump to bottom
Closed
mgol opened this issue Oct 21, 2014 · 12 comments
Closed

.off() does not clean up empty cache entries #1760

mgol opened this issue Oct 21, 2014 · 12 comments
Assignees
@timmywil
Labels
2.x-only Data Event
Milestone
1.12.0/2.2.0

Comments

@mgol
Copy link
Member

mgol commented Oct 21, 2014

Originally reported by psquared at: http://bugs.jquery.com/ticket/14674

Even if all event handlers are removed with .off(), an empty cache entry is left in data_priv. Unfortunately, those empty cache entries remain if elements are removed using some method other than .remove(), causing a memory leak.

Test Environment:
Chrome 31.0.1650.63, jQuery 2.0.3

Steps to Reproduce:

  1. Attach an event handler to an element (e.g. using .on())
  2. Remove the event handler with .off()
  3. Remove the element with Node.removeChild()

Expected Results:
No memory leaks.

Actual Results:
An orphaned cache entry remains in data_priv.

Example:
 http://jsfiddle.net/Rx3v3/

Additional info:
Modifying the last block of jQuery.event.remove() as below seems to alleviate the problem. 

// Remove the expando if it's no longer used
if ( jQuery.isEmptyObject( events ) ) {
    delete elemData.handle;
    data_priv.remove( elem, "events" );

<span class="c1">// Clean up empty cache entries:


    
      
      
    
  


    delete data_priv.cache[elem[data_priv.expando]];

}

Issue reported for jQuery 2.0.3
@mgol mgol added this to the 3.0.0 milestone Oct 21, 2014
@mgol
Copy link
Member Author

mgol commented Oct 21, 2014

Comment author: psquared

Perhaps this is a better suggestion than the one I made above--adding something like this to the end of Data.remove() seems to help: 

if (jQuery.isEmptyObject(cache)) {
    this.discard(owner);
}

@mgol
Copy link
Member Author

mgol commented Oct 21, 2014

Comment author: dmethvin

@rwaldron could you take a look?

@mgol
Copy link
Member Author

mgol commented Oct 21, 2014

Comment author: gibson042

In general, we do not support mixing native DOM manipulation with jQuery. This is close to $( el ).data({ leaks: true })[0].parentNode.removeChild( el ), but distinct enough that it may be worth looking into a small fix.

@mgol
Copy link
Member Author

mgol commented Oct 21, 2014

Comment author: dmethvin

I agree with that, there are plenty of ways you can get into trouble if you use native methods to remove elements behind jQuery's back.

But in the 1.x branch if you remove all events and there is no other data in the object, it removes the data object preemptively. e.g.,

 https://github.com/jquery/jquery/blob/8b88ca28874cd990d25854a6fea0565b716bb482/src/event.js#L223

 https://github.com/jquery/jquery/blob/8b88ca28874cd990d25854a6fea0565b716bb482/src/data.js#L205

@mgol
Copy link
Member Author

mgol commented Oct 21, 2014

Comment author: dmethvin

I'll mark this valid pending investigation. Since events are often the only thing in the data cache it would be helpful to remove them when all events are gone.

@markelog markelog added the Event label Dec 18, 2014
@dmethvin
Copy link
Member

Once the changes to .data() land, that would be a good time to circle back and see if we need to change anything here.

@timmywil
Copy link
Member

timmywil commented May 5, 2015

Now that the data changes have landed, what needs to be done here?

@mgol
Copy link
Member Author

mgol commented May 5, 2015

If you attach a couple of handlers to elements & then .off() them without removing elements then you still have a leak. This is way less serious now as removing elements makes the data go away but I'd say part of the issue is still applicable.

@dmethvin
Copy link
Member

dmethvin commented May 6, 2015

It looks like we take care of the no-more-events case now?

jquery/src/event.js

Line 220 in 62d5579

if ( jQuery.isEmptyObject( events ) ) {

@timmywil timmywil self-assigned this May 6, 2015
@mgol
Copy link
Member Author

mgol commented May 6, 2015

@dmethvin Indeed. It seems everything was done in d702b76. It would be good to have a test for that behavior, though, wouldn't it?

@timmywil
Copy link
Member

timmywil commented May 6, 2015

@dmethvin @mzgol Not quite. compat also clears the expando.

timmywil added a commit to timmywil/jquery that referenced this issue May 6, 2015
@timmywil
Data: remove the expando when there's no more data
f9f3988 
Fixes jquerygh-1760
@timmywil timmywil mentioned this issue May 6, 2015
Closed
@timmywil
Copy link
Member

timmywil commented May 6, 2015

PR submitted.

timmywil added a commit to timmywil/jquery that referenced this issue May 7, 2015
@timmywil
Data: remove the expando when there's no more data
d9dd4fb 
Fixes jquerygh-1760
timmywil added a commit that referenced this issue Nov 10, 2015
@timmywil @markelog
Data: remove the expando when there's no more data
996f80d 
Fixes gh-1760
Close gh-2271
@dmethvin dmethvin modified the milestones: 1.12/2.2, 3.0.0 Jan 7, 2016
@cssmagic cssmagic mentioned this issue May 18, 2016
Open
@lock lock bot locked as resolved and limited conversation to collaborators Jun 19, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@timmywil timmywil

Labels
2.x-only Data Event
Milestone
1.12.0/2.2.0
Development

Successfully merging a pull request may close this issue.

Data: remove the expando when there's no more data; minor cleanup timmywil/jquery
4 participants
@dmethvin @timmywil @markelog @mgol