Bug Tracker

Opened 7 years ago

Closed 6 years ago

#8873 closed bug (fixed)

IE memory leak in jquery support function

Reported by: agnes@… 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);
	}

Change History (10)

comment:1 Changed 7 years ago by timmywil

Component: unfiledsupport

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

comment:2 Changed 7 years ago by dmethvin

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

comment:3 Changed 7 years ago by timmywil

Owner: set to agnes@…
Status: newpending

comment:4 Changed 7 years ago by trac-o-bot

Resolution: invalid
Status: pendingclosed

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!

comment:5 Changed 6 years ago by shghs

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

comment:6 Changed 6 years ago by timmywil

Priority: undecidedlow
Resolution: invalid
Status: closedreopened

comment:7 Changed 6 years ago by timmywil

Owner: changed from agnes@… to shghs
Status: reopenedassigned

comment:8 Changed 6 years ago by Rick Waldron

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

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

comment:9 Changed 6 years ago by timmywil

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

comment:10 Changed 6 years ago by timmywil

Resolution: fixed
Status: assignedclosed

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

Changeset: b8fc9d14a134f91f17bf96194da86d38231da005

Note: See TracTickets for help on using tickets.