View Full Version : [OPEN-985] Minor memory leak in Ext.form.Field msgTarget="side"

20 May 2010, 5:28 PM
This code snippet from Ext.form.Field / Ext.form.MessageTargets creates a (minor) memory leak:

if (field.ownerCt) {
field.ownerCt.on('afterlayout', field.alignErrorIcon, field);
field.ownerCt.on('expand', field.alignErrorIcon, field);

If you have a container where you create and remove a bunch of fields dynamically, destroying the field does not remove the event listeners on the container if they have ever been marked invalid.

Probably not a huge problem since 'side' is not a default message target and most fields aren't added and removed dynamically, and if they are, probably not in large numbers, but...still a potential leak problem for the right application.

This test case illustrates:

var f = new Ext.form.FormPanel({title: "Test", height: 100, items: {xtype: "textfield", msgTarget: "side"}});
var ff = f.items.get(0);
if(f.events.afterlayout.listeners[0] && f.events.afterlayout.listeners[0].scope == ff) {

This patch fixes it:

Ext.override(Ext.form.Field, {
beforeDestroy: function() {
if(this.ownerCt) {
this.ownerCt.un("afterlayout", this.alignErrorIcon, this);
this.ownerCt.un("expand", this.alignErrorIcon, this);

--Chad Eberle
Web Application Architect

23 May 2010, 7:13 AM
@ceberle --
An alternative might be to use the mon function to handle listener removal once the field is destroyed:

if (field.ownerCt) {
field.mon(field.ownerCt, 'afterlayout', field.alignErrorIcon, field);
field.mon(field.ownerCt, 'expand', field.alignErrorIcon, field);

24 May 2010, 8:20 AM

That would be a lot cleaner, wouldn't it? :) That was what I was going to suggest at first, until I looked at the code for mon() (see this post (http://www.extjs.com/forum/showthread.php?99890-DEFER-3.2.0-Potential-memory-leak-in-Component-mon()-amp-mun())).

In a situation where a Field stays loaded in an application for a long time, each time the field is marked invalid, it would cause memory usage to go up. I guess a best-of-both-worlds approach would be to call mun() before mon() each time.

But it'd probably be fine to just use mon() alone with the expectation that it will be fixed in a future release.