Bug Tracker

Opened 6 years ago

Closed 5 years ago

#14674 closed bug (migrated)

.off() does not clean up empty cache entries

Reported by: psquared Owned by:
Priority: low Milestone: 1.12/2.2
Component: data Version: 2.0.3
Keywords: Cc: Rick Waldron
Blocked by: Blocking:

Description

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" );

    // Clean up empty cache entries:
    delete data_priv.cache[elem[data_priv.expando]];
}

Change History (6)

comment:1 Changed 6 years ago by 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);
}

comment:2 Changed 6 years ago by dmethvin

Cc: Rick Waldron added
Version: 1.10.22.0.3

@rwaldron could you take a look?

comment:3 Changed 6 years ago by 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.

comment:4 Changed 6 years ago by 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

comment:5 Changed 6 years ago by dmethvin

Component: unfileddata
Milestone: None1.12/2.2
Priority: undecidedlow
Status: newopen

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.

comment:6 Changed 5 years ago by m_gol

Resolution: migrated
Status: openclosed
Note: See TracTickets for help on using tickets.