Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ie leak #2351

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Ie leak #2351

wants to merge 4 commits into from

Conversation

kentaromiura
Copy link
Member

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.

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.
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];
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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..

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"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
@ibolmo
Copy link
Member

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants