Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Ie leak #2351

Open
wants to merge 4 commits into from

5 participants

@kentaromiura
Collaborator

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
@kentaromiura kentaromiura 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.
4c68d39
@kentaromiura kentaromiura Revert "Fix IE leaks caused by ._fireEvent circular reference"
This reverts commit 4c68d39.
14f5deb
@kentaromiura kentaromiura Revert "Revert "Fix IE leaks caused by ._fireEvent circular reference""
This reverts commit 14f5deb.
86611be
@kentaromiura kentaromiura Applied the suggestions
revert dispose, destroy now filter nodeType, so Element.properties.text
can use empty without throwing errors, delete properties in clean.
f59f069
@kentaromiura kentaromiura commented on the diff
Source/Element/Element.js
@@ -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 Collaborator

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

@arian Owner
arian added a note

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

@kentaromiura Collaborator

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 Owner
arian added a note

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

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@ibolmo ibolmo modified the milestone: 1.5.1, 1.5
@SergioCrisostomo SergioCrisostomo modified the milestone: 1.5.2, 1.5.1
@ibolmo
Owner

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Apr 12, 2012
  1. @kentaromiura

    Fix IE leaks caused by ._fireEvent circular reference

    kentaromiura authored
    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.
  2. @kentaromiura
Commits on Apr 13, 2012
  1. @kentaromiura
  2. @kentaromiura

    Applied the suggestions

    kentaromiura authored
    revert dispose, destroy now filter nodeType, so Element.properties.text
    can use empty without throwing errors, delete properties in clean.
This page is out of date. Refresh to see the latest.
Showing with 18 additions and 4 deletions.
  1. +18 −4 Source/Element/Element.js
View
22 Source/Element/Element.js
@@ -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 Collaborator

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

@arian Owner
arian added a note

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

@kentaromiura Collaborator

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 Owner
arian added a note

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

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ });
+
return item;
};
@@ -800,14 +805,16 @@ var formProps = {input: 'checked', option: 'selected', textarea: 'value'};
Element.implement({
destroy: function(){
- var children = clean(this).getElementsByTagName('*');
- Array.each(children, clean);
- Element.dispose(this);
+ if(this.nodeType == 1){
+ var children = clean(this).getElementsByTagName('*');
+ Array.each(children, clean);
+ }
+ Element.dispose(this);
return null;
},
empty: function(){
- Array.from(this.childNodes).each(Element.dispose);
+ Array.from(this.childNodes).each(Element.destroy);
return this;
},
@@ -949,6 +956,13 @@ Element.Properties.html = {
};
+// fix for IE leak on Element.set('text','')
+Element.Properties.text = {
+ set: function(text){
+ this.empty().setProperty('text',text);
+ }
+}
+
var supportsHTML5Elements, supportsTableInnerHTML, supportsTRInnerHTML;
/*<ltIE9>*/
Something went wrong with that request. Please try again.