IE memory leak in jquery support function

Reported by:	agnes@atlassian.com	Owned by:	shghs
Priority:	low	Milestone:	1.next
Component:	support	Version:	1.5.2
Keywords:		Cc:
Blocked by:		Blocking:

Description

The following code in the jquery support detection function will cause a memory leak in IE.

if ( !div.addEventListener && div.attachEvent && div.fireEvent ) {
		div.attachEvent("onclick", function click() {
			// Cloning a node shouldn't copy over any
			// bound event handlers (IE does this)
			jQuery.support.noCloneEvent = false;
			div.detachEvent("onclick", click);
		});
		div.cloneNode(true).fireEvent("onclick");
	}

This is because the detachEvent is only ever called on the div and not never on the cloned div.

It can be fixed simply with this code:

    if ( !div.addEventListener && div.attachEvent && div.fireEvent ) {
        var click = function() {
			// Cloning a node shouldn't copy over any
			// bound event handlers (IE does this)
			jQuery.support.noCloneEvent = false;
			this.detachEvent("onclick", click);
		};
		div.attachEvent("onclick", click);
		div.cloneNode(true).fireEvent("onclick");
        div.detachEvent("onclick", click);
	}

Attachments (0)

Change History (10)

Changed April 14, 2011 02:19PM UTC by timmywil comment:1

component:	unfiled → support

That makes sense to me. Can we confirm the leak in IE?

Changed April 14, 2011 04:20PM UTC by dmethvin comment:2

yes, memory leaks can be elusive so a test case is appreciated.

Changed April 15, 2011 02:05AM UTC by timmywil comment:3

owner:	→ agnes@atlassian.com
status:	new → pending

Changed April 29, 2011 07:55AM UTC by trac-o-bot comment:4

resolution:	→ invalid
status:	pending → closed

Because we get so many tickets, we often need to return them to the initial reporter for more information. If that person does not reply within 14 days, the ticket will automatically be closed, and that has happened in this case. If you still are interested in pursuing this issue, feel free to add a comment with the requested information and we will be happy to reopen the ticket if it is still valid. Thanks!

Changed May 24, 2011 07:04AM UTC by shghs comment:5

The leak is present in IE6 and IE7. A test case can be produced by including jQuery in a blank HTML page with no other JavaScript. Reloading the page causes IE's memory usage to increase constantly. With the fix that Agnes suggests, the same test case doesn't result in memory leakage.

I've issued a pull request: https://github.com/jquery/jquery/pull/387

Changed May 24, 2011 02:27PM UTC by timmywil comment:6

priority:	undecided → low
resolution:	invalid
status:	closed → reopened

Changed May 24, 2011 02:27PM UTC by timmywil comment:7

owner:	agnes@atlassian.com → shghs
status:	reopened → assigned

Changed May 24, 2011 04:43PM UTC by rwaldron comment:8

This might be more effective and has less change to the existing code:

https://github.com/rwldrn/jquery/commit/34e6c0637bb8ac9420761232078ccfcc15ca74ad

Changed May 24, 2011 05:57PM UTC by timmywil comment:9

I'm thinking both should be done, but I'll do some testing.

Changed May 25, 2011 07:09PM UTC by timmywil comment:10

resolution:	→ fixed
status:	assigned → closed

detachEvent is unnecessary since we're nulling div. Fixes #8873.

Changeset: b8fc9d14a134f91f17bf96194da86d38231da005

Side navigation

#8873 closed bug (fixed)