Skip to content

Avoid memory leaks during refresh - DO NOT MERGE #1288

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

Closed
wants to merge 1 commit into from
Closed

Avoid memory leaks during refresh - DO NOT MERGE #1288

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion ui/tabs.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -372,7 +372,10 @@ return $.widget( "ui.tabs", {
},

_processTabs: function() {
var that = this;
var that = this,
prevTabs = this.tabs,
prevAnchors = this.anchors,
prevPanels = this.panels;

this.tablist = this._getList()
.addClass( "ui-tabs-nav ui-helper-reset ui-helper-clearfix ui-widget-header ui-corner-all" )
Expand Down Expand Up @@ -456,6 +459,12 @@ return $.widget( "ui.tabs", {
this.panels
.addClass( "ui-tabs-panel ui-widget-content ui-corner-bottom" )
.attr( "role", "tabpanel" );

if ( prevTabs ) {
this._off( prevTabs.not( this.tabs ) );
this._off( prevAnchors.not( this.anchors ) );
this._off( prevPanels.not( this.panels ) );
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This actually shouldn't be too hard to manage, since we're already tracking the elements in each widget. We can either have a conditional like this or initialize all the properties to empty jQuery objects to avoid calling .not() on undefined. We'll just need to go through every widget and make sure we're doing this.

},

// allow overriding how to find the list for rare usage scenarios (#7715)
Expand Down
7 changes: 6 additions & 1 deletion ui/widget.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -435,8 +435,13 @@ $.Widget.prototype = {
},

_off: function( element, eventName ) {
eventName = (eventName || "").split( " " ).join( this.eventNamespace + " " ) + this.eventNamespace;
eventName = (eventName || "").split( " " ).join( this.eventNamespace + " " ) +
this.eventNamespace;
element.unbind( eventName ).undelegate( eventName );

this.bindings = $( this.bindings.not( element ).get() );
this.focusable = $( this.focusable.not( element ).get() );
this.hoverable = $( this.hoverable.not( element ).get() );
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm generating new jQuery instances from arrays to avoid the .prevObject history. This history actually gets really deep really quickly. Removing the history is required to prevent the memory leak. We could just delete this.bindings.prevObject, but that feels a little dirty. @dmethvin thoughts?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For 1.12/2.2 we could add a method to clear off the .prevObject and you could polyfill it? The hard part is gonna be coming up with a name. 😸

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we'll continue to be limited to just a handful of these uses (likely just this one place for all of UI), so it's probably not worth adding a new method for it. This should be a pretty fast operation as is, especially with how few objects we're working with.

},

_delay: function( handler, delay ) {
Expand Down