IE8 iframe memory leak with non-detached click event #1840

Closed
SergioMorchon opened this Issue Nov 5, 2014 · 11 comments

Comments

Projects
None yet
5 participants
@SergioMorchon

If you load jQuery 1.X (i'm testing with edge) inside an iframe, alone, and reload the content (eg. by submiting a dummy form), you could see a memory leak under IE8 (not IE10+ with IE8 compat, no. A really production IE8 legacy enterprise browser).

The bad lines are:

if ( div.attachEvent ) {
    div.attachEvent( "onclick", function() {
        support.noCloneEvent = false;
    });

    div.cloneNode( true ).click();
}

By changing them for something like the following...:

if (div.attachEvent) {
    function cloneTest() {
        support.noCloneEvent = false;
    }
    div.attachEvent("onclick", cloneTest);
    var div2 = div.cloneNode(true);
    div2.click();
    div.detachEvent("onclick", cloneTest);
    div2.detachEvent("onclick", cloneTest);
}

... the memory leak dissapears, and the test continues working as espected.

(- sorry about my non-linted code -)

One of the known solutions for the well-known issues with iframes, memory leaks, events and IE8 is to detach all the events before the garbage collection.

@markelog

This comment has been minimized.

Show comment
Hide comment
@markelog

markelog Nov 5, 2014

Member

How many iframes do you create to make this a real problem?

Member

markelog commented Nov 5, 2014

How many iframes do you create to make this a real problem?

@SergioMorchon

This comment has been minimized.

Show comment
Hide comment
@SergioMorchon

SergioMorchon Nov 5, 2014

2 pages (1 with 1 iframe):
index.html:

<!DOCTYPE html>
<html>
    <head>
        <style>
            html, body, iframe {
                margin: 0;
                padding: 0;
                border: 0;
                height: 100%;
                width: 100%;
            }
        </style>
    </head>
    <body>
        <iframe src="leak.html"/>
    </body>
</html>

leak.html:

<!DOCTYPE html>
<html lang="es">
<head>
    <meta charset="UTF-8" />
    <script src="http://code.jquery.com/jquery-1.11.1.min.js"></script>
</head>
<body>
    <button onclick="navigate()">Cerrar</button>
</body>
<script>
    function navigate() {
        var form = document.createElement("form");
        while (document.body.firstChild) {
            form.appendChild(document.body.firstChild);
        }
        document.body.appendChild(form);
        form.submit();
    }
</script>
</html>

2 pages (1 with 1 iframe):
index.html:

<!DOCTYPE html>
<html>
    <head>
        <style>
            html, body, iframe {
                margin: 0;
                padding: 0;
                border: 0;
                height: 100%;
                width: 100%;
            }
        </style>
    </head>
    <body>
        <iframe src="leak.html"/>
    </body>
</html>

leak.html:

<!DOCTYPE html>
<html lang="es">
<head>
    <meta charset="UTF-8" />
    <script src="http://code.jquery.com/jquery-1.11.1.min.js"></script>
</head>
<body>
    <button onclick="navigate()">Cerrar</button>
</body>
<script>
    function navigate() {
        var form = document.createElement("form");
        while (document.body.firstChild) {
            form.appendChild(document.body.firstChild);
        }
        document.body.appendChild(form);
        form.submit();
    }
</script>
</html>
@markelog

This comment has been minimized.

Show comment
Hide comment
@markelog

markelog Nov 5, 2014

Member

Right, so one onclick is taking a lot of memory?

Member

markelog commented Nov 5, 2014

Right, so one onclick is taking a lot of memory?

@SergioMorchon

This comment has been minimized.

Show comment
Hide comment
@SergioMorchon

SergioMorchon Nov 5, 2014

No.
The onclick is there only to perform a submit and reload the inner page, to see how the memory leaks.
If you open it with a real IE8 and click the button, could see how the IE memory process grows and grows.
But if you download the development edge jquery sourcecode, modify those lines I posted in the first post, and re-open the test with that modified version, you will see how this memory leak dissapears.

The problem is that those lines of code (and only those) are creating one div node (with the .clone(true)) and attaching a onclick event function which contains a closure variable reference (support) to it, without detaching it later, wich is a well known, even documented issue with IE8 (see here: http://msdn.microsoft.com/en-us/library/bb250448(v=vs.85).aspx, "Closures" section).

No.
The onclick is there only to perform a submit and reload the inner page, to see how the memory leaks.
If you open it with a real IE8 and click the button, could see how the IE memory process grows and grows.
But if you download the development edge jquery sourcecode, modify those lines I posted in the first post, and re-open the test with that modified version, you will see how this memory leak dissapears.

The problem is that those lines of code (and only those) are creating one div node (with the .clone(true)) and attaching a onclick event function which contains a closure variable reference (support) to it, without detaching it later, wich is a well known, even documented issue with IE8 (see here: http://msdn.microsoft.com/en-us/library/bb250448(v=vs.85).aspx, "Closures" section).

@dmethvin

This comment has been minimized.

Show comment
Hide comment
@dmethvin

dmethvin Nov 11, 2014

Member

Something similar was removed in b8fc9d1, but your code also removes the binding for the cloned div. Is that necessary, or does simply unbinding for the original div happen to fix the issue?

Member

dmethvin commented Nov 11, 2014

Something similar was removed in b8fc9d1, but your code also removes the binding for the cloned div. Is that necessary, or does simply unbinding for the original div happen to fix the issue?

@SergioMorchon

This comment has been minimized.

Show comment
Hide comment
@SergioMorchon

SergioMorchon Nov 11, 2014

It is necessary according to Microsoft's documentation (remove all the attached events in parent-less nodes). That piece of jQuery's code is a simple test to detect if the browser is able to clone not only the node itself, but also its attached events.

Detach them is transparent and fully functional and backward-compatible.

It is necessary according to Microsoft's documentation (remove all the attached events in parent-less nodes). That piece of jQuery's code is a simple test to detect if the browser is able to clone not only the node itself, but also its attached events.

Detach them is transparent and fully functional and backward-compatible.

@dmethvin

This comment has been minimized.

Show comment
Hide comment
@dmethvin

dmethvin Nov 12, 2014

Member

Yes, it does seem like both elements would need to have the event handler detached, but in that case the code I referred to above was not preventing a memory leak since the cloned element still had the handler.

It's too bad we don't have a good way to do automated tests on these issues.

Member

dmethvin commented Nov 12, 2014

Yes, it does seem like both elements would need to have the event handler detached, but in that case the code I referred to above was not preventing a memory leak since the cloned element still had the handler.

It's too bad we don't have a good way to do automated tests on these issues.

@dmethvin dmethvin added this to the 3.0.0 milestone Nov 12, 2014

@dmethvin dmethvin added the Event label Nov 12, 2014

@dmethvin dmethvin self-assigned this Dec 1, 2014

@dmethvin dmethvin added the 1.x-only label Dec 1, 2014

@dmethvin

This comment has been minimized.

Show comment
Hide comment
@dmethvin

dmethvin Dec 1, 2014

Member

How about we dump our direct feature detect and switch to an indirect detect? That reduces this whole mess to this without any resource leaks or cleanup:

support.noCloneEvent = !!div.addEventListener && !!div.attachEvent;

It could be even simpler but we need the second part for Opera 12 support if the comment is right.

Member

dmethvin commented Dec 1, 2014

How about we dump our direct feature detect and switch to an indirect detect? That reduces this whole mess to this without any resource leaks or cleanup:

support.noCloneEvent = !!div.addEventListener && !!div.attachEvent;

It could be even simpler but we need the second part for Opera 12 support if the comment is right.

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Dec 1, 2014

Member

It could be even simpler but we need the second part for Opera 12 support if the comment is right.

We're dropping Opera Presto support so that shouldn't matter.

Member

mgol commented Dec 1, 2014

It could be even simpler but we need the second part for Opera 12 support if the comment is right.

We're dropping Opera Presto support so that shouldn't matter.

dmethvin added a commit that referenced this issue Dec 6, 2014

Manipulation: Plug an IE8 memory leak in noCloneEvent feature detect
Fixes gh-1840

This feature detect could be simplified now that the only supported browser
with this problem is IE8.
@dmethvin

This comment has been minimized.

Show comment
Hide comment
@dmethvin

dmethvin Dec 6, 2014

Member

Landed faf295a to fix

Member

dmethvin commented Dec 6, 2014

Landed faf295a to fix

@dmethvin dmethvin closed this Dec 6, 2014

@svenmeier

This comment has been minimized.

Show comment
Hide comment
@svenmeier

svenmeier Jul 3, 2015

Just as a note: this regression was caused by #1518

Just as a note: this regression was caused by #1518

@dmethvin dmethvin modified the milestones: 1.12/2.2, 3.0.0 Jan 7, 2016

@cssmagic cssmagic referenced this issue in cssmagic/ChangeLog May 18, 2016

Open

jQuery #5

@jquery jquery locked as resolved and limited conversation to collaborators Jun 19, 2018

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