Ie leak #2351

Open
wants to merge 4 commits into
from

Conversation

Projects
None yet
5 participants
Member

kentaromiura commented Apr 13, 2012

changed empty, now it uses destroy to clean every children tree
this change need a check on destroy, because childNodes returns
textNodes
on clean i removed the _fireEvent reference breaking the circular
reference and allowing the gc to clean up the memory (tested with
sIEve-0.0.8)
set('html',...) don't leak but set('text',...) does, so added a
Element.Properties.text setter and done empty() before setting the text

I setup a test page that creates 1000 divs, added these divs to the
body and then use empty(), set('html', ...) and set('text', ...) to get
rid of them, causing a 1000 element leak,
with these changes I managed to drop the leaks from 1000 to 0.

kentaromiura added some commits Apr 12, 2012

Fix IE leaks caused by ._fireEvent circular reference
changed empty, now it uses destroy to clean every children tree
this change need a check on destroy, because childNodes returns
textNodes
on dispose i removed the _fireEvent reference breaking the circular
reference and allowing the gc to clean up the memory (tested with
sIEve-0.0.8)
set('html',...) don't leak but set('text',...) does, so added a
Element.Properties.text setter and done empty() before setting the text

I setup a test page that creates 1000 divs, added these divs to the
body and then use empty(), set('html', ...) and set('text', ...) to get
rid of them, causing a 1000 element leak,
with these changes I managed to drop the leaks from 1000 to 0.
Applied the suggestions
revert dispose, destroy now filter nodeType, so Element.properties.text
can use empty without throwing errors, delete properties in clean.
@@ -792,6 +792,11 @@ var clean = function(item){
delete collected[uid];
delete storage[uid];
}
+ var props = ['_fireEvent','$family','$constructor'];
+ props.each(function(property){
+ if(property in item) delete item[property];
@kentaromiura

kentaromiura Apr 13, 2012

Member

had to check for existence because otherwise IE throw some nasty error. (Object doesn't support this action)

@arian

arian Jul 2, 2012

Owner

do you know why clearAttributes doesn't handle this, or is it because those things begin with $ or _?

@kentaromiura

kentaromiura Jul 2, 2012

Member

http://msdn.microsoft.com/en-us/library/ie/ms536350(v=vs.85).aspx
The clearAttributes method clears only persistent HTML attributes. The ID attribute, styles, and script-only properties are not affected.

@arian

arian Aug 7, 2012

Owner

It's still weird though.. because all other methods are added with Object.append as well, which boils down to simply element.set = function.... So why do you have to delete _fireEvent manually, but not any other method..

@kguelzau

kguelzau Mar 12, 2013

"Object doesn't support htis action" is still thrown at "delete item[property]" in IE8.

@ibolmo ibolmo modified the milestones: 1.5.1, 1.5 Mar 3, 2014

@SergioCrisostomo SergioCrisostomo modified the milestones: 1.5.2, 1.5.1 Jul 23, 2014

Owner

ibolmo commented Nov 11, 2014

@kentaromiura it's been a while, but could you rebase this. I'd also be interested in the Spec. I wonder how we could incorporate memory usage (leak) with our test processes.

@SergioCrisostomo SergioCrisostomo removed this from the 1.5.2 milestone Aug 15, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment