Opened 4 years ago

Closed 3 years ago

Last modified 3 years ago

#10056 closed bug (fixed)

Tabs: "manipulation" example leaks memory

Reported by: rosenfeld Owned by: rosenfeld
Priority: minor Milestone: 1.11.2
Component: ui.tabs Version: 1.10.4
Keywords: Cc:
Blocked by: Blocking:

Description

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.

Change History (13)

comment:1 Changed 4 years ago by Scott González

Component: ui.coreui.tabs
Owner: set to rosenfeld
Status: newpending

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.

comment:2 Changed 4 years ago by rosenfeld

Status: pendingnew

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.

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

comment:4 Changed 3 years ago by tj.vantoll

Summary: jQuery UI tabs "manipulation" example leaks memoryTabs: "manipulation" example leaks memory

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.

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

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

comment:7 Changed 3 years ago by Scott González

Status: newopen

comment:9 Changed 3 years ago by Scott González

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

Ref #10056 Ref gh-1319

Changeset: b397294d42e783aacd4cc3a52bbe3aacc0f3f725

comment:10 Changed 3 years ago by Scott González

Resolution: fixed
Status: openclosed

Tabs: Avoid memory leak during refresh

Fixes #10056 Ref gh-1319

Changeset: 2e8e52dec1eaa06ed170a0ed9769c7b97129c955

comment:11 Changed 3 years ago by Scott González

Accordion: Avoid memory leak during refresh

Ref #10056 Closes gh-1319

Changeset: 849c6fd5376e12c6093c557bd4836ef0b145f145

comment:12 Changed 3 years ago by Scott González

Milestone: none1.11.2

comment:13 Changed 3 years ago by rosenfeld

Thank you!

Note: See TracTickets for help on using tickets.