Tabs: "manipulation" example leaks memory

[rosenfeld]

Try this example: ​http://jqueryui.com/tabs/#manipulation

Closing a tab works by removing the li and respective panel elements and calling 'refresh' on the tabs component.

But by inspecting $('#tabs').data('uiTabs'), you'll notice that there are still references to the removed elements in 'hoverable', 'focusable' and 'binding' properties at least.

Our application heavily depends on dynamic tabs support and fixing this leak would be very helpful. Unfortunately I have no idea on how to properly fix this.

[Scott González]

How big of an issue is this for you? Widgets don't manually track event bindings, the widget factory does it automatically and only does cleanup on destroy.

[rosenfeld]

Here's how my application works. Users search for deals and open the ones they are interested as new tabs. Once they are done with it, they will close the deals.

When we display a deal, it uses lots of memory, and we also bind several events to elements in the tab panel, which should be automatically unbound once we call remove() on the li and panel. But since the li and panel elements are leaking, all of the DOM we have created for each deal remains in the memory even after the user closes the deals.

Since this is an SPA, user's usually don't leave our application for a long period of time. If they open too many tabs, they will get a big increase in their memory usage due to this leak.

I can't destroy the tabs component itself since this is the first thing that is created in the application and holds all of the parts of our application as tab panes (most are fixed, but the deals are dynamic ones).

The leak goes really fast for our application since each deal tab will use a fair amount of memory.

[petersendidit]

Seems like the correct way to solve this is to handle removing elements from hoverable, focusable, and bindings in the _off method in widget. This would require _off to loop over each of the elements being passed in and see if those elements still had a namespaced event for the widget and then remove them from the correct lists. Would make _off far more costly to call.

Some like this would solve the problem: ​https://gist.github.com/petersendidit/4a4cb89654e88433eecd

Don't know if the cost of doing all that work is worth it for all widgets.

[tj.vantoll]

Well, even if _off() cleans up those bindings, you still need a place to call _off(), and it seems like the appropriate place to do that is refresh().

We can build an extension as a POC to see how messy the cleanup would be.

[tj.vantoll]

Here's an example that uses petersendidit's _off(), and defines a removeTab() method that cleans up after itself: ​http://jsfiddle.net/tj_vantoll/Cwttg/.

Here's a thought: could we just unbind all previously attached events in refresh(), then re-bind them? Or would that have side effects that I'm not thinking of? It seems like we should come up with some solution to this, as it is a problem with any of our widgets that have a refresh() method.

[rosenfeld]

I'd just like to point out that even the example in jsfiddle won't fix the detached elements leak.

After clicking on the remove button for the first time, you can still get the LI element through:

$('#tabs').data('uiTabs').bindings.prevObject.prevObject.prevObject.prevObject.prevObject.prevObject.prevObject[1]

[Scott González]

Discussion in ​https://github.com/jquery/jquery-ui/pull/1288.

[Scott González]

PR: ​https://github.com/jquery/jquery-ui/pull/1319

[Scott González]

Widget: Avoid memory leaks when unbinding events with ._off()

Ref #10056 Ref gh-1319

Changeset: b397294d42e783aacd4cc3a52bbe3aacc0f3f725

[Scott González]

Tabs: Avoid memory leak during refresh

Fixes #10056 Ref gh-1319

Changeset: 2e8e52dec1eaa06ed170a0ed9769c7b97129c955

[Scott González]

Accordion: Avoid memory leak during refresh

Ref #10056 Closes gh-1319

Changeset: 849c6fd5376e12c6093c557bd4836ef0b145f145

[rosenfeld]

Thank you!