PDA

View Full Version : Component Cleanup (Orphan Control)



hendricd
11 Jan 2008, 11:30 AM
I know you Ext.core.dev's (Brian and I have discussed some of these related issues) are working on DOM cleanup for a future release. I just wanted to share a behavior you should consider.

IMO: It's good practice for any Library to gracefully "destroy what it creates". So, I tried it:



Ext.EventManager.on(window, "beforeunload", function(){
Ext.destroy( aViewport );
},window,{single:true});
The dream here, of coarse, is allow the Viewport to cascade-destroy all its child items, and so on; hopefully ending with no leaks or orphans.

But, on good ol' IE, we (seemingly) need a custom removeNode implementation to pull this off (clear circular references, release event-listeners to name a few).

For those who haven't seen this approach to "IE Leak prevention", the idea is to simply move the node (marked for death) into another DIV (the death-trap) that is not part of the document flow, and then purge it by setting deathTrap.innerHTML = "". (a cryin' shame we even have to consider this, but /:)):


(Ext.)removeNode : isIE ? function(){
var d;
return function(n){
if(n){
d = d || document.createElement('div');
d.appendChild(n);
d.innerHTML = '';
}
}
}() : function(n){
if(n && n.parentNode){
n.parentNode.removeChild(n);
}
}
Next, let's examine Ext.destroy:


destroy : function(){
for(var i = 0, a = arguments, len = a.length; i < len; i++) {
var as = a[i];
if(as){
if(as.dom){
as.removeAllListeners();
as.remove(); //Eventually passed to Ext.removeNode
continue;
}
if(typeof as.destroy == 'function'){
as.destroy();
}
}
}
},Pretty simple. Any argument passed (ultimately) gets removed from the DOM. If it's not an Element and it's derived from Component, the Components destroy method is called.

And, there-in lies some trouble. The Container (el) for Ext.Viewport (the parent container for most Ext UIs) is document.body.

So, if you (like all good boys and girls) want to cleanup after ourselves with:


myViewPort.destroy()On IE, what happens -- is that the BODY (yourViewPort.el) gets moved into our 'removeNode-death-trap', and IE errors out with 'Unknown Runtime Error !' :">

No doubt!

A proposed solution:



Ext.destroy = function(){
for(var i = 0, a = arguments, len = a.length; i < len; i++) {
var as = a[i];
if(as){
if(typeof as.destroy == 'function'){
as.destroy();
}else if(as.dom){
as.removeAllListeners();
as.remove();
}
}
}
};Allow the Component OR Element to decide how (and what in the DOM) to cleanup.

And this, (albeit selfishly ;)), permits an Ext.Element used in a [psuedo-]Decorator pattern (such as ux.ManagedIframe) to benefit from its own destroy() method.

Then, let's either fix Viewport, or removeNode itself:


Ext.removeNode = Ext.isIE ? function(){
var d;
return function(n){
if(n && n.tagName != 'BODY'){
d = d || document.createElement('div');
d.appendChild(n);
d.innerHTML = '';
}
}
}() : function(n){
if(n && n.parentNode){
n.parentNode.removeChild(n);
}
};Then, we're well-on-our-way towards no-Orphans.

hendricd
23 Jan 2008, 12:50 PM
Anyone from Dev squad had a chance to examine this ?

aconran
24 Jan 2008, 8:00 AM
Hey Doug -

Just came across this thread, nice bump :D I'll put this on the list of things to take a look at.

Thanks again,

SeaSharp2
25 Jan 2008, 3:00 AM
I'll put this on the list of things to take a look at.
Is there a plan to host this list on a publicly viewable todo list system where tasks and progress can be checked?

I ask because in Nov/Dec two other forum users posted useful code enhancements to fix memory leaks but at the time no one from the Core.Ext team commented. In January I re-posted links to those threads and within hours Brian checked in a code fix for one item.

Those of us who follow the details of the Ext development can never be sure what issues the Ext Team has catalogued on a formal todo list and what has been overlooked in error.

brian.moeskau
25 Jan 2008, 3:09 AM
Anyone from Dev squad had a chance to examine this ?

I'm familiar with the IE "garbage can" node trick and had actually played around with it back when you and I discussed these issues several months ago, but only with limited success. However, it looks like you've dug a little more deeply into it than I had time to at the time. Thanks for the post, and we'll definitely check out your suggestions.

brian.moeskau
25 Jan 2008, 3:12 AM
Is there a plan to host this list on a publicly viewable todo list system where tasks and progress can be checked?

At this time we're using an internal tracking system. We've considered moving to something publicly visible for the reasons you've mentioned, but so far we haven't decided to make that change.

brian.moeskau
22 Feb 2008, 10:40 PM
I assume the goal is not just to avoid the IE error, but also to not actually remove the document's body, correct? Seems like a good thing to avoid doing. ;)


Ext.removeNode = Ext.isIE ? function(){
var d;
return function(n){
if(n && n.tagName != 'BODY'){
d = d || document.createElement('div');
d.appendChild(n);
d.innerHTML = '';
}
}
}() : function(n){
if(n && n.parentNode && n.tagName != 'BODY'){
n.parentNode.removeChild(n);
}
};

brian.moeskau
22 Feb 2008, 10:46 PM
Fixed in SVN rev. 1678