PDA

View Full Version : [OPEN-1198] EditorGrid Pseudoleak in Chrome/Firefox due to EventManager.js line 97



tungj
12 Aug 2010, 4:11 AM
Ext version tested:


Ext 3.2.1 rev ____



Adapter used:


ext



css used:


only default ext-all.css





Browser versions tested against:


IE8 //doesn't leak in IE since it uses attachEvent
FF3 (firebug 1.5.4 installed)
Chrome



Operating System:


Ubuntu 9.10
WinXP Pro



Description:


The internal addListener in EventManager.js line 97 has the code
Ext.EventManager.addListener(WINDOW, 'unload', function(){
el.removeEventListener.apply(el, args);
})
which causes a pseudoleak (the memory isn't released until the window unloads).
This is problematic since we have a one-page application using property grids with lots of properties, and the grid (and therefore store) stays in memory despite being destroyed due to EditorGrid.js line 154
this.getGridEl().on('mousewheel', this.stopEditing.createDelegate(this, [true]), this);



Test Case:



<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8" />
<title>Property Grid Leak Test</title>
<link rel="stylesheet" type="text/css" href="../../resources/css/ext-all.css" />
<link rel="stylesheet" type="text/css" href="../shared/examples.css" />
<style type="text/css">
</style>
<script type="text/javascript" src="../../adapter/ext/ext-base.js"></script>
<script type="text/javascript" src="../../ext-all-debug.js"></script>
<script type="text/javascript" src="../shared/examples.js"></script>
<script type="text/javascript">
Ext.onReady(function(){
Ext.fly('create').on('click', make, window, {single:true});

// This is just a marker to show in Chrome if the property grid is leaked
var ZZZ_LeakMarker = function(){};

function make(){
var share = {};
share.propertiesGrid = new Ext.grid.PropertyGrid({});
// Set a property on the grid so we know whether the grid is still in memory
// otherwise in Chrome heap debugger it'll just be listed as another 'constructor'
share.propertiesGrid._leakMarker = new ZZZ_LeakMarker();

share.panel = new Ext.Panel({
height: 400,
renderTo: 'prop-grid',
items: share.propertiesGrid,
tbar: [{
text: 'Destroy',
handler: function(){
window.setTimeout(function(){
share.propertiesGrid = null;
share.panel.destroy();
share.panel = null;
// Point the static Ext.fly instance at a div element to avoid false positives about leaks in sIEve
Ext.fly('irrelevant');
}, 100);
}
}]
});
}

var applyFix = false;
if (applyFix) {
// Patch createInterceptor not to leak references to .target and .method, is mentioned as fixed in 3.2.2 changelog
Function.prototype.createInterceptor = function(fcn, scope){
var method = this;
return !Ext.isFunction(fcn) ?
this :
function() {
var me = this,
args = arguments;
fcn.target = me;
fcn.method = method;
var ret = (fcn.apply(scope || me || window, args) !== false);
// If you don't null the method and target, then closures will stay and keep a reference to all
// objects scoped to the closure until the next time the intercepted method is called.
fcn.target = fcn.method = null;
return ret ?
method.apply(me || window, args) :
null;
};
};

// Essentially want to comment out line 97 of Ext 3.2.1 EventManager.js which pseudoleaks. e.g. Ext.grid.EditorGridPanel
// initEvents this.getGridEl().on('mousewheel', this.stopEditing.createDelegate(this, [true]), this);
//
// Seems to be a workaround for jQuery which we don't need since we don't use the jQuery adaptor;
// mousewheel doesn't leak in sIEve in IE7 nor IE8
// Ext.EventManager.addListener(WINDOW, 'unload', function(){
// el.removeEventListener.apply(el, args);
// });
// Only affects Chrome/Firefox, IE doesn't have an addEventListener so doesn't go into that part of the code.
if (document.addEventListener) {
Ext.EventManager.addListener = Ext.EventManager.on = Ext.EventManager.addListener.createInterceptor(function(element, eventName, fn, scope, options){
if (element === window && eventName === 'unload') {
var stringFn = String(fn);
// Testing for both normal and minified version of the line 97 code , e.g. Ext-all-debug and Ext-all
// 'function (){\n el.removeEventListener.apply(el, args);\n}'
// 'function (){\n E.removeEventListener.apply(E,I);\n}'

if (/^\s*function\s*\(\)\s*\{\s*(\w+)\.removeEventListener\.apply\(\1,\s*\w+\);?\s*\}$/.test(stringFn)) {
return false;
}
}
});
}
}
});
</script>

</head>
<body>
<p><a id="create">Create PropertyGrid</a></p>
<div id="prop-grid"></div>
<div id="irrelevant"></div>
</body>
</html>

See this URL : http://


Steps to reproduce the problem:


Copy test code to /ext-3.2.1/examples/grid/property-grid-leak-test.html
View test code in Chrome, press Ctrl-Shift-J to bring up debugger tools, go to Profiles tab, press 'Take Heap Snapshot' (the eye icon).
In page, click 'Create PropertyGrid'. Verify that property grid appears, go back to Profile tab and take another heap snapshot. Verify that an instance of ZZZ_LeakMarker was created and is referred to by the EditorGrid (it appears as J.I.constructor.e.I.constructor in the heap debugger).
In page, press 'Destroy' in tab button. Wait ~30 seconds to ensure the Ext.elCache garbage collection code has run, and go back to Profile tab and take heap snapshot. Verify that the ZZZ_LeakMarker still remains in memory, since the EditorGrid is still in memory (held in the closure scope of
EventManager.js line 97)



The result that was expected:


ZZZ_LeakMarker and the EditorGrid should be freed, the count should go back to zero.



The result that occurs instead:


ZZZ_LeakMarker and the EditorGrid stay in memory , count is one.



Screenshot or Video:


Debugging already done:


Tracing down pseudoleak, trying various fixes for pseudoleak. There's a var applyFix=false; if you set it true it overrides Ext.EventManager.addListener to avoid registering the window.unload callback, which avoids the pseudoleak in Chrome (and presumably Firefox). This doesn't affect IE since it uses attachEvent. If you repeat the test with applyFix=true, the ZZZ_LeakMarker and EditorGrid are freed.



Possible fix:


Comment out the lines EventManager.js line 97. Are they really required when using ext-base; I thought only IE required removing event listeners from DOM elements before removing them from the DOM? (please correct me if I'm wrong) Setting applyFix=true essentially comments out those lines without violating our commercial license with a really hacky override; but would slow down all event registration.
The code mentions it's a workaround for JQuery, so presumably it can't be just commented out. I've attached a version of EventManager that keeps track of the registered window.onunload listeners and deregisters them on event remove/removeAll with addCleanup/doCleanup. It seems to allow the property grid to get freed.

If this does get changed, could you please explain which approach you took? I'm working on memory cleanup for a large (>200MB memory) single-page application at present, so it'd be helpful to know more about events and leaks. Thank you!

Jamie Avins
12 Aug 2010, 10:12 AM
My guess is there is some legacy code that was not well documented and left in instead of being removed.

igorvin
24 Sep 2010, 4:51 PM
We see memory leaks using Ext.grid.EditorGridPanel as well - we tested proposed above solution (the first one - commented out code inside EventManager) and it fixed the issue. In which version of Extjs it's going to be fixed? Thanks.