.adopt() causes memory leak #2127

Closed
pronouncedJerry opened this Issue Nov 16, 2011 · 16 comments

Comments

Projects
None yet
4 participants
@pronouncedJerry

If you use .adopt() versus .inject() or .grab() you'll notice a memory leak in IE7, possibly IE8.

Example:

var newDiv = new Element('div', { id: 'newrDiv' });
$(document.body).adopt(newDiv);

@pronouncedJerry

This comment has been minimized.

Show comment
Hide comment
@pronouncedJerry

pronouncedJerry Nov 17, 2011

After further investigation, if I change the following in adopt:

from:
elements = Array.flatten(arguments)

to:
elements = arguments

The leak disappears.

After further investigation, if I change the following in adopt:

from:
elements = Array.flatten(arguments)

to:
elements = arguments

The leak disappears.

@pronouncedJerry

This comment has been minimized.

Show comment
Hide comment
@pronouncedJerry

pronouncedJerry Nov 18, 2011

My colleague followed up on my findings and stated the following:

After digging a little deeper into the code, I found the culprit: instanceof. Apparently if you call instanceof on a “non-object” in IE, memory gets leaked (http://ajaxian.com/archives/working-aroung-the-instanceof-memory-leak). So, this isn’t a mootools bug, it’s a bug in IE itself.

My colleague followed up on my findings and stated the following:

After digging a little deeper into the code, I found the culprit: instanceof. Apparently if you call instanceof on a “non-object” in IE, memory gets leaked (http://ajaxian.com/archives/working-aroung-the-instanceof-memory-leak). So, this isn’t a mootools bug, it’s a bug in IE itself.

@arian

This comment has been minimized.

Show comment
Hide comment
@arian

arian Nov 21, 2011

Member

Sounds interesting (and fun, memory leaks in IE7)...

Member

arian commented Nov 21, 2011

Sounds interesting (and fun, memory leaks in IE7)...

@arian

This comment has been minimized.

Show comment
Hide comment
@arian

arian Nov 21, 2011

Member

I'll try if I can reproduce this..

Member

arian commented Nov 21, 2011

I'll try if I can reproduce this..

@arian

This comment has been minimized.

Show comment
Hide comment
@arian

arian Nov 21, 2011

Member

I can't really reproduce this with a small testpage. It seems to use more memory after each refresh indeed, however not a lot more than an simple page with only window.addEvent('domready', function(){ ... }); or window.onload.

Now i'm not memory IE leak expert, so maybe I'm doing something different then you? I found this little program called 'Drip' (via http://blogs.msdn.com/b/ie/archive/2007/11/29/tools-for-detecting-memory-leaks.aspx) for viewing the memory usage. I don't know if there are other tools for analyzing memory leaks and where they come from..

Member

arian commented Nov 21, 2011

I can't really reproduce this with a small testpage. It seems to use more memory after each refresh indeed, however not a lot more than an simple page with only window.addEvent('domready', function(){ ... }); or window.onload.

Now i'm not memory IE leak expert, so maybe I'm doing something different then you? I found this little program called 'Drip' (via http://blogs.msdn.com/b/ie/archive/2007/11/29/tools-for-detecting-memory-leaks.aspx) for viewing the memory usage. I don't know if there are other tools for analyzing memory leaks and where they come from..

@ibolmo

This comment has been minimized.

Show comment
Hide comment
@ibolmo

ibolmo Dec 10, 2011

Member

I'm having trouble reproducing this as well. I've gone as far as identifying a few leaks in Element.js that I've cleaned up and will create a PR for, and will continue to try to find the other leaks, but it'd be best if @gd0t provided us with a procedure on how to reproduce.

Member

ibolmo commented Dec 10, 2011

I'm having trouble reproducing this as well. I've gone as far as identifying a few leaks in Element.js that I've cleaned up and will create a PR for, and will continue to try to find the other leaks, but it'd be best if @gd0t provided us with a procedure on how to reproduce.

@ibolmo ibolmo referenced this issue Dec 11, 2011

Merged

Fixes #2127 #2166

@ghost ghost assigned ibolmo Dec 11, 2011

@pronouncedJerry

This comment has been minimized.

Show comment
Hide comment
@pronouncedJerry

pronouncedJerry Dec 14, 2011

Thank you!

I was simply using Process Explorer to track the usage. http://technet.microsoft.com/en-us/sysinternals/bb896653

It appears that the calling of instanceof on 'non-objects' (mootools.js flatten()) causes the memory leaks in IE. http://ajaxian.com/archives/working-aroung-the-instanceof-memory-leak

We ended up implementing the following:

if(Browser.ie) {
        instanceOf = this.instanceOf = function(item, object) {
            if (item == null) return false;
            // Avoid calling instanceof on certain "non-objects" to prevent memory leaks in IE
            if (!item.hasOwnProperty) return false;
            var constructor = item.$constructor || item.constructor;
            while (constructor){
                if (constructor === object) return true;
                constructor = constructor.parent;
            }
            return item instanceof object;
        };
    }

Thank you!

I was simply using Process Explorer to track the usage. http://technet.microsoft.com/en-us/sysinternals/bb896653

It appears that the calling of instanceof on 'non-objects' (mootools.js flatten()) causes the memory leaks in IE. http://ajaxian.com/archives/working-aroung-the-instanceof-memory-leak

We ended up implementing the following:

if(Browser.ie) {
        instanceOf = this.instanceOf = function(item, object) {
            if (item == null) return false;
            // Avoid calling instanceof on certain "non-objects" to prevent memory leaks in IE
            if (!item.hasOwnProperty) return false;
            var constructor = item.$constructor || item.constructor;
            while (constructor){
                if (constructor === object) return true;
                constructor = constructor.parent;
            }
            return item instanceof object;
        };
    }
@ibolmo

This comment has been minimized.

Show comment
Hide comment
@ibolmo

ibolmo Dec 20, 2011

Member

Thanks @gd0t. I'll add another issue to test for instanceOf leaks. In the mean time, you should reconsider using Process Explorer and opt to use sIEve. A couple of reason why:

  • Process Explorer looks at a whole process. Not very granular (e.g. you don't know which nodes are leaked).
  • There are fake leaks that stay in memory until the browser is closed. We should minimize these, but not much we can do.

Otherwise, I think it's fine to use Process Explorer; but I think you'll find sIEve much more detailed tool.

Member

ibolmo commented Dec 20, 2011

Thanks @gd0t. I'll add another issue to test for instanceOf leaks. In the mean time, you should reconsider using Process Explorer and opt to use sIEve. A couple of reason why:

  • Process Explorer looks at a whole process. Not very granular (e.g. you don't know which nodes are leaked).
  • There are fake leaks that stay in memory until the browser is closed. We should minimize these, but not much we can do.

Otherwise, I think it's fine to use Process Explorer; but I think you'll find sIEve much more detailed tool.

@arian arian closed this in a64bcbd Jan 9, 2012

@jdalton

This comment has been minimized.

Show comment
Hide comment
@jdalton

jdalton Jan 9, 2012

Tools like sIEve give false positives and can cause devs to waste time. I find it's best to use use Process Explorer and a simplified test case to locate leaks. I am aware of the instanceof leak and try to avoid it in places like Benchmark.js. That said not every dummy element created has to be nulled.

jdalton commented Jan 9, 2012

Tools like sIEve give false positives and can cause devs to waste time. I find it's best to use use Process Explorer and a simplified test case to locate leaks. I am aware of the instanceof leak and try to avoid it in places like Benchmark.js. That said not every dummy element created has to be nulled.

@ibolmo

This comment has been minimized.

Show comment
Hide comment
@ibolmo

ibolmo Jan 9, 2012

Member

Do you have a blog post or screencast in your leak finding process? I'd be
a good article.

On Mon, Jan 9, 2012 at 2:14 PM, John-David Dalton <
reply@reply.github.com

wrote:

Tools like sIEve give false positives and can cause devs to waste time. I
find it's best to use use Process Explorer and a
simplified test case to locate leaks. I am aware of the instanceof leak
and try to avoid it in places like Benchmark.js.
That said not even dummy element created has to be nulled.


Reply to this email directly or view it on GitHub:
#2127 (comment)

Member

ibolmo commented Jan 9, 2012

Do you have a blog post or screencast in your leak finding process? I'd be
a good article.

On Mon, Jan 9, 2012 at 2:14 PM, John-David Dalton <
reply@reply.github.com

wrote:

Tools like sIEve give false positives and can cause devs to waste time. I
find it's best to use use Process Explorer and a
simplified test case to locate leaks. I am aware of the instanceof leak
and try to avoid it in places like Benchmark.js.
That said not even dummy element created has to be nulled.


Reply to this email directly or view it on GitHub:
#2127 (comment)

@jdalton

This comment has been minimized.

Show comment
Hide comment
@jdalton

jdalton Jan 9, 2012

No blog post at the moment, but for some tricky IE6 memory leaks it may require early versions of XP pre SP2, which can be a pain to get a hold of.

I was able to reproduce leaks without having to scrounge up an old IE6 Win 2K/XP install by using Multiple-IE, but those browsers are really unstable so try to get the real deal if possible.

jdalton commented Jan 9, 2012

No blog post at the moment, but for some tricky IE6 memory leaks it may require early versions of XP pre SP2, which can be a pain to get a hold of.

I was able to reproduce leaks without having to scrounge up an old IE6 Win 2K/XP install by using Multiple-IE, but those browsers are really unstable so try to get the real deal if possible.

@jdalton

This comment has been minimized.

Show comment
Hide comment
@jdalton

jdalton Jan 9, 2012

Here is a blog post I did over a nice single page memory leak in IE associated with removeChild.
http://allyoucanleet.com/2011/02/16/exploring-the-removechild-single-page-memory-leak/

jdalton commented Jan 9, 2012

Here is a blog post I did over a nice single page memory leak in IE associated with removeChild.
http://allyoucanleet.com/2011/02/16/exploring-the-removechild-single-page-memory-leak/

@ibolmo

This comment has been minimized.

Show comment
Hide comment
@ibolmo

ibolmo Jan 9, 2012

Member

Nice screencast. I guess process explorer is really handy to just notice if
there's a leak. I liked that sIEve gave some nodes to help me find the
leak. How did you narrow the leak to replaceChild?

On Mon, Jan 9, 2012 at 2:35 PM, John-David Dalton <
reply@reply.github.com

wrote:

Here is a blog post I did over a nice single page memory leak in IE
associated with removeChild.

http://allyoucanleet.com/2011/02/16/exploring-the-removechild-single-page-memory-leak/


Reply to this email directly or view it on GitHub:
#2127 (comment)

Member

ibolmo commented Jan 9, 2012

Nice screencast. I guess process explorer is really handy to just notice if
there's a leak. I liked that sIEve gave some nodes to help me find the
leak. How did you narrow the leak to replaceChild?

On Mon, Jan 9, 2012 at 2:35 PM, John-David Dalton <
reply@reply.github.com

wrote:

Here is a blog post I did over a nice single page memory leak in IE
associated with removeChild.

http://allyoucanleet.com/2011/02/16/exploring-the-removechild-single-page-memory-leak/


Reply to this email directly or view it on GitHub:
#2127 (comment)

@jdalton

This comment has been minimized.

Show comment
Hide comment
@jdalton

jdalton Jan 9, 2012

How did you narrow the leak to replaceChild?

The removeChild leak was known at the time. I remember reading about it in an excerpt here or some place similar.

code snippet

jdalton commented Jan 9, 2012

How did you narrow the leak to replaceChild?

The removeChild leak was known at the time. I remember reading about it in an excerpt here or some place similar.

code snippet

@jdalton

This comment has been minimized.

Show comment
Hide comment
@jdalton

jdalton Jan 9, 2012

When doing research into IE's event handler leaks I had to whittle the code down to a small snippet, which took some time (I used Process Explorer here again).

The result can be seen in @bryanforbes post over the Internet Explorer event handler leak under the "Fixing It By Global Reference" section.

jdalton commented Jan 9, 2012

When doing research into IE's event handler leaks I had to whittle the code down to a small snippet, which took some time (I used Process Explorer here again).

The result can be seen in @bryanforbes post over the Internet Explorer event handler leak under the "Fixing It By Global Reference" section.

@pronouncedJerry

This comment has been minimized.

Show comment
Hide comment
@pronouncedJerry

pronouncedJerry Jan 24, 2012

Big thanks to all of you!

Big thanks to all of you!

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