-
Notifications
You must be signed in to change notification settings - Fork 511
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
base: master
Are you sure you want to change the base?
Ie leak #2351
Conversation
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.
This reverts commit 4c68d39.
This reverts commit 14f5deb.
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]; |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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 _
?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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..
There was a problem hiding this comment.
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.
@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. |
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.