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.

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

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

Closed
mgol opened this Issue Oct 21, 2014 · 12 comments

Comments

Projects
None yet
4 participants
@mgol
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

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Oct 21, 2014

Member

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);
}
Member

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

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Oct 21, 2014

Member

Comment author: dmethvin

@rwaldron could you take a look?

Member

mgol commented Oct 21, 2014

Comment author: dmethvin

@rwaldron could you take a look?

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Oct 21, 2014

Member

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.

Member

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

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Oct 21, 2014

Member

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

Member

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

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Oct 21, 2014

Member

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.

Member

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

This comment has been minimized.

Show comment
Hide comment
@dmethvin

dmethvin Jan 26, 2015

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.

Member

dmethvin commented Jan 26, 2015

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

This comment has been minimized.

Show comment
Hide comment
@timmywil

timmywil May 5, 2015

Member

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

Member

timmywil commented May 5, 2015

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

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol May 5, 2015

Member

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.

Member

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

This comment has been minimized.

Show comment
Hide comment
@dmethvin

dmethvin May 6, 2015

Member

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

if ( jQuery.isEmptyObject( events ) ) {

Member

dmethvin commented May 6, 2015

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

if ( jQuery.isEmptyObject( events ) ) {

@timmywil timmywil removed the Needs review label May 6, 2015

@timmywil timmywil self-assigned this May 6, 2015

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol May 6, 2015

Member

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

Member

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

This comment has been minimized.

Show comment
Hide comment
@timmywil

timmywil May 6, 2015

Member

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

Member

timmywil commented May 6, 2015

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

@timmywil

This comment has been minimized.

Show comment
Hide comment
@timmywil

timmywil May 6, 2015

Member

PR submitted.

Member

timmywil commented May 6, 2015

PR submitted.

timmywil added a commit to timmywil/jquery that referenced this issue May 7, 2015

@timmywil timmywil closed this in 56bb677 May 12, 2015

@dmethvin dmethvin modified the milestones: 1.12/2.2, 3.0.0 Jan 7, 2016

@cssmagic cssmagic referenced this issue May 18, 2016

Open

jQuery #5

@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.