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

Fix UIControl memory leak #11362

Merged
merged 1 commit into from Mar 25, 2017

Conversation

Projects
None yet
3 participants
@jvilk
Contributor

jvilk commented Feb 19, 2017

Previously, every UIControl ever created would be stored in UIControl._controls. During cleanup, every UIControl would be checked to see if it is still active in the DOM, but would never be removed from the array.

Now, cleanup only retains a reference to active UIControl instances. This change also decouples UIControl IDs from the length of the array.

John Vilk
Fix UIControl memory leak
Previously, every `UIControl` ever created would be stored in `UIControl._controls`. During cleanup, every `UIControl` would be checked to see if it is still active in the DOM, but would never be removed from the array.

Now, cleanup only retains a reference to active `UIControl` instances. This change also decouples `UIControl` IDs from the length of the array.

@mattab mattab modified the milestone: 3.0.3 Feb 21, 2017

@@ -67,6 +75,9 @@
throw new Error("Error: " + control.constructor.name + "'s destroy method does not call " +
"UIControl.destroy. You may have a memory leak.");
}
} else {
// Control is still active / used.
activeControls.push(control);
}
}

This comment has been minimized.

@sgiehl

sgiehl Mar 25, 2017

Member

Isn't here something missing like UIControl._controls = activeControls;, in order to keep track of those controls not removed by the current run of cleanupUnusedControls

This comment has been minimized.

@jvilk

jvilk Mar 25, 2017

Contributor

Nope.

var activeControls = UIControl._controls = []; sets both UIControl._controls and activeControls to the same new array instance.

activeControls.push modifies that same array instance.

This comment has been minimized.

@sgiehl

sgiehl Mar 25, 2017

Member

ah. yes. you're right. thanks for clarifying

@sgiehl sgiehl merged commit 4138e95 into matomo-org:3.x-dev Mar 25, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment