PDA

View Full Version : Grid Leaks Memory



steen
30 May 2007, 3:03 AM
In IE7 when you replace a grid with a new grid then the old grid is not properly cleaned up and memory is leaked.

Is there a way to to completely remove the old grid from memory?

When I examine the DOM using firebug I notice that ExtJS create div element with names like "ext-gen57" (and classname: "x-dd-drag-proxy x-dd-drop-nodrop"), maybe its those div elements that cause the leak.

Also in IE7 the grid fails to be rendered after the grid has been replace 27 times and the grid area is empty. In Firefox the grid is rendered properly.

Animal
30 May 2007, 4:35 AM
http://extjs.com/deploy/ext/docs/output/Ext.grid.Grid.html#destroy

steen
30 May 2007, 4:59 AM
Thanks for pointing at that method.

If you use the .destroy() on the grid then lost div tags are cleaned up, but memory is still leaked.
And the grid widget fails to draw anything after 27 times (in IE7).

jack.slocum
30 May 2007, 2:21 PM
In IE7 when you replace a grid with a new grid then the old grid is not properly cleaned up and memory is leaked.

Are you actually cleaning it up? You will need to do some work to clean things up (e.g. destroying the grid, removing the container element and creating a new one).

steen
30 May 2007, 11:12 PM
I clean up by calling the destroy method and then I remove div tag from the dom.

How can you clean up more?

BTW in IE7 grid fail to render more than 27 grids on one page. In Firefox and Opera you can render more.

liggett78
13 Jun 2007, 12:27 AM
BTW in IE7 grid fail to render more than 27 grids on one page. In Firefox and Opera you can render more.
The problem is in the createStyleSheet() function, which creates stylesheets on the fly

if (Ext.isIE) {
ss = doc.createStyleSheet();
ss.cssText = cssText;
}
As http://support.microsoft.com/kb/262161/en-us points out, there is a limit of 30 style tags for one page.

robpi
16 Jun 2007, 10:42 AM
Thanks for this Info. I've the same problem with IE after 25 times of grid-reconfigure. Can this problem solved by a workaround or will there be a solution in a later release?

jack.slocum
17 Jun 2007, 2:38 AM
The problem is in the createStyleSheet() function, which creates stylesheets on the fly

if (Ext.isIE) {
ss = doc.createStyleSheet();
ss.cssText = cssText;
}
As http://support.microsoft.com/kb/262161/en-us points out, there is a limit of 30 style tags for one page.

Nice find! In Ext 2.0's GridView3 this isn't an issue as it no longer generates a stylesheet.

vtswingkid
19 Jun 2007, 11:24 AM
Well atleast grid is the only widget using this in 1.1:

widgets\grid\AbstractGridView.js(94): return Ext.util.CSS.createStyleSheet(ruleBuf.join(""));
widgets\grid\GridView.js(609): return Ext.util.CSS.createStyleSheet(ruleBuf.join(""));

Probably should remove this function for 2.0 so nothing else ends up using it.


Is there a work around for us 1.1 users? Is svn 2.0 stable yet?

jack.slocum
19 Jun 2007, 3:11 PM
Not completely stable, but improving daily.

robpi
20 Jun 2007, 6:29 AM
But what about the ext 1.1 grid :-?. Is there any workaround for this problem?

vtswingkid
21 Jun 2007, 5:30 AM
Maybe this old code could be modified for a work around. I am going to spend a little time on it now.

http://extjs.com/forum/showthread.php?t=875

vtswingkid
21 Jun 2007, 5:43 AM
Well, this got rid of the error but ie still only allows 30 styles to be loaded into the head section.

I suppose we have to find a way to delete them.




Ext.util.CSS.createStyleSheet=function(cssText){

varss;

vardoc=document;

if(Ext.isIE){

//ss = doc.createStyleSheet();

//ss.cssText = cssText;

ss=document.createElement("style");

ss.setAttribute("type","text/css");

ss.styleSheet.cssText=cssText;

document.getElementsByTagName("head")[0].appendChild(ss);

}else{

varhead=doc.getElementsByTagName("head")[0];

varrules=doc.createElement("style");

rules.setAttribute("type","text/css");

try{

rules.appendChild(doc.createTextNode(cssText));

}catch(e){

rules.cssText=cssText;
}

head.appendChild(rules);

ss=rules.styleSheet?rules.styleSheet:(rules.sheet||doc.styleSheets[doc.styleSheets.length-1]);

}

this.cacheStyleSheet(ss);

returnss;
}

vtswingkid
21 Jun 2007, 6:16 AM
This is better.




Ext

.util.CSS.createStyleSheet=function(cssText){

var ss;
var doc=document;
var head=doc.getElementsByTagName("head")[0];
var rules=doc.createElement("style");
rules.setAttribute("type","text/css");
if(Ext.isIE){
//ss = doc.createStyleSheet();
//ss.cssText = cssText;
rules.styleSheet.cssText=cssText;
}else{
try{
rules.appendChild(doc.createTextNode(cssText));
}catch(e){
rules.cssText=cssText;
}
}
head.appendChild(rules);
ss=rules.styleSheet?rules.styleSheet:(rules.sheet||doc.styleSheets[doc.styleSheets.length-1]);
this.cacheStyleSheet(ss);
return ss;
}


Is the clean up code running? If so, what do I need to do to ensure that it is called. Currently my grids are inside tabs, so when I destroy the tab they go with it. This probably does not cut it.

In short if we start removing these styles when grids are destroyed, then we can keep creating more grids. However, with the current rules technique IE will limit us to 30 grids, or less if we have other style tags loaded in the header.

That is a start.

Edit: fixed some spacing up above...could easly add rules.setAttribute("id", "grid-css-#"); for removal process. I still do not know enough about what is going on.

vtswingkid
21 Jun 2007, 6:40 AM
Alright, I have two possible solutions:

1) have the grid destroy function remove the styles (limiting IE to 30 grids at any given time). Would probably be good to prevent infinit unused styles from loading in FF too.

2) dig through the code and use tag embedded styles to achieve the same purpose there for removing the need for style tags. (best solution)

I have higher priorities at the moment, so I will revisit this afterwords (hopefully a few days). In the meantime if anyone else wants to do this please, please, please post your work.

Good luck.

mystix
21 Jun 2007, 10:58 PM
think i'll rake up the past here :)

will be moving a little off-topic though... suggested this before in this thread (http://extjs.com/forum/showthread.php?t=5328&highlight=style), that the createStyleSheet() method should assign Ext.id()s to the <style> elements it creates, making them retrievable via Ext.get (and thus removable via the removeStyleSheet() method).

if a cache of all created stylesheets (ala Ext.get()'s cache for example) is created, it would also help greatly in stylesheet location, manipulation(??) & removal.

vtswingkid
22 Jun 2007, 3:51 AM
I like mystix's idea. However, in my experimenting i found that an id could not be added to the element returned by createStyleSheet. Nothing could be. The code above on the other hand could allow an id to be inserted.

robpi
22 Jun 2007, 5:10 AM
Your suggestions seem very interesting. As a javascript-beginner and ext-newbie i've at this time not the knowledge to help at this level.
I think ext is an excellent framework and there should be a solution for this problem, i particular as the grid is one of the most used component in ext and internet explorer users are also not rare.
So the question is, should the problem be solved in the ext-library itself. A workaround-solution would be nice as fast help, but i think in the 1.1 Release (1.1 is still beta)) this problem should also be solved.

genius551v
22 Jun 2007, 11:22 AM
somebody found the solutions to this??? :-?

tnks

timb
22 Jun 2007, 11:55 AM
So the question is, should the problem be solved in the ext-library itself. A workaround-solution would be nice as fast help, but i think in the 1.1 Release (1.1 is still beta)) this problem should also be solved.
I agree completely. It sounds like there are going to be quite a few changes in 2.0, so for larger applications that use 1.x, it would be quite a bit of work to upgrade, change the code and test. I'd be pretty disappointed if we have to upgrade from 1.x to 2.x just for bug fixes. I think basic bug fixes should be included in 1.x at least for a while after 2.x is released.

Put it this way. If Microsoft told everyone that there would be no more updates to Windows XP the minute Windows Vista was released, would you be happy?

I'd like to know what the official stance is from the Ext Team on this subject.

jack.slocum
23 Jun 2007, 6:21 AM
We will continue to support and bug fix 1.x. In the license/subscription it has the full details. We probably won't be adding any features to 1.x after 2.x is released.

This particular problem should be corrected in SVN.

vtswingkid
25 Jun 2007, 5:07 AM
Awsome! I took a quick look at svn. It appears he added an id for each style sheet, and then deletes the style sheet when we destroy the grid. I'll give it a try.

EDIT: don't forget, this still limits you to 30 grids (assuming no other styles tags are loaded in the header) at any given time.

mystix
25 Jun 2007, 5:20 AM
Awsome! I took a quick look at svn. It appears he added an id for each style sheet, and then deletes the style sheet when we destroy the grid. I'll give it a try.

EDIT: don't forget, this still limits you to 30 grids (assuming no other styles tags are loaded in the header) at any given time.

hmmm... interesting... how did he add an id for elements returned by createStyleSheet?

jack.slocum
25 Jun 2007, 6:02 AM
hmmm... interesting... how did he add an id for elements returned by createStyleSheet?

I added an id argument to createStylesheet and did it in there. :)

mystix
25 Jun 2007, 6:05 AM
I added an id argument to createStylesheet and did it in there. :)

d'oh! =D>

vtswingkid
25 Jun 2007, 6:37 AM
alright, I am missing something. I rendered a grid and destroyed it and the style is still there. the id did get set.

grid.render();
grid.destroy(true);

is there something else?

vtswingkid
25 Jun 2007, 7:02 AM
adding
var rulesId = this.grid.id + '-cssrules';
Ext.util.CSS.removeStyleSheet(rulesId);
to destroy takes care of it.



Ext.grid.GridView.prototype.destroy=function(){
var rulesId = this.grid.id + '-cssrules';
Ext.util.CSS.removeStyleSheet(rulesId);
if(this.colMenu){
this.colMenu.removeAll();
Ext.menu.MenuMgr.unregister(this.colMenu);
this.colMenu.getEl().remove();
delete this.colMenu;
}
if(this.hmenu){
this.hmenu.removeAll();
Ext.menu.MenuMgr.unregister(this.hmenu);
this.hmenu.getEl().remove();
delete this.hmenu;
}
if(this.grid.enableColumnMove){
var dds = Ext.dd.DDM.ids['gridHeader' + this.grid.getGridEl().id];
if(dds){
for(var dd in dds){
if(!dds[dd].config.isTarget && dds[dd].dragElId){
var elid = dds[dd].dragElId;
dds[dd].unreg();
Ext.get(elid).remove();
} else if(dds[dd].config.isTarget){
dds[dd].proxyTop.remove();
dds[dd].proxyBottom.remove();
dds[dd].unreg();
}
if(Ext.dd.DDM.locationCache[dd]){
delete Ext.dd.DDM.locationCache[dd];
}
}
delete Ext.dd.DDM.ids['gridHeader' + this.grid.getGridEl().id];
}
}
this.bind(null, null);
Ext.EventManager.removeResizeListener(this.onWindowResize, this);
}

jack.slocum
25 Jun 2007, 8:24 AM
Ahhhh I did it for reconfigure, but not for destroy. I will get that in asap.

robpi
25 Jun 2007, 8:39 AM
Any idea when this bugfix will be availible without svn-access?

genius551v
26 Jun 2007, 2:51 PM
vtswingkid, can you explain a little more your solutions?
becouse to me dont work... :((

vtswingkid
27 Jun 2007, 4:50 AM
Jack, I don't want to over step my bounds. Would you post your function changes for them? Or release a bug fix? Or I'd be happy to post the fixes with your permission ofcoarse.

genius551v
27 Jun 2007, 6:26 AM
please, please, please, please, please.......... :((

jack.slocum
27 Jun 2007, 3:26 PM
Jack, I don't want to over step my bounds. Would you post your function changes for them? Or release a bug fix? Or I'd be happy to post the fixes with your permission ofcoarse.

We will be releasing 1.1 Beta 2 on or before Friday. It will have the fix.

robpi
28 Jun 2007, 7:27 AM
We will be releasing 1.1 Beta 2 on or before Friday. It will have the fix.

That's what i call good news! =D>

Thanks Jack. I can't say it often enough, ext is an excellent framwork.