Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
SergioMorchon opened this issue Nov 5, 2014 · 11 comments
Closed

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

SergioMorchon opened this issue Nov 5, 2014 · 11 comments
Assignees
@dmethvin
Labels
1.x-only Event
Milestone
1.12.0/2.2.0

Comments

@SergioMorchon
Copy link

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
Copy link
Member

markelog commented Nov 5, 2014

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

@SergioMorchon
Copy link
Author

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
Copy link
Member

markelog commented Nov 5, 2014

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

@SergioMorchon
Copy link
Author

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
Copy link
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?

@SergioMorchon
Copy link
Author

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

@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
Copy link
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
Copy link
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
@dmethvin
Manipulation: Plug an IE8 memory leak in noCloneEvent feature detect
faf295a 
Fixes gh-1840

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

dmethvin commented Dec 6, 2014

Landed faf295a to fix

@dmethvin dmethvin closed this as completed Dec 6, 2014
@svenmeier
Copy link

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 mentioned this issue May 18, 2016
Open
@lock lock bot 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.
Assignees

@dmethvin dmethvin

Labels
1.x-only Event
Milestone
1.12.0/2.2.0
Development

No branches or pull requests
5 participants
@dmethvin @svenmeier @markelog @mgol @SergioMorchon