Memory Leaks when dynamically loading #4419

Closed
rid00z opened this Issue May 23, 2012 · 12 comments

5 participants

@rid00z

Found some JQuery Mobile widgets attach closures to the window and document object and JQuery Mobile was not cleaning them up.

So code below would result in a memory leak.

$('#reload').click(function () {

    $('#content').empty();

    $.ajax({
        url: 'injectHtml.htm',
        async: false,
        success: function (data) { $('#content').append(data).trigger("create"); }
    });

});
@toddparker

@rid00z - Thanks for reporting. Mind creating a test page illustrating the issue? Template: http://jsbin.com/uqenom/edit

@rid00z

Hi

That should reproduce it. http://jsbin.com/uqenom/22/edit

@gabrielschulhof

OK, so .empty() ends up calling _destroy() on widget instances that have this. We need to implement this for all widgets that attach global handlers.

@rid00z
@gabrielschulhof

@rid00z

Thanks for the PR!

There are a few more leaky places you could look at, if you'd like:

  • fixedToolbar: It does a lot of $el.closest( ".ui-page" ).bind( ... ) meaning that, if you remove it from the page, the binding stays attached to the page.
  • navbar also attaches handlers to the closest page, and so it would leak similarly to fixedToolbar
  • loader does a bunch of $window.bind(...) and $window.unbind(...), but I can't tell from a cursory glance whether or not it leaves some bindings in place if you were to destroy the widget.

Fixing these should allow us to close this issue.

@gabrielschulhof

The new widget factory has a this.bindings object where we can list all the elements to which we bind things and, as long as they're bound with a namespace, they will be removed.

@gabrielschulhof

... so instead of the problematic $( document ).bind( "eventname", function() {} ) way of doing things, now you can simply this._on( document, { eventname: function() {} ) or even simpler this._on( document, { eventname: "_memberFunction" } ) and the handler will be namespaced uniquely to the widget instance and it will be removed upon _destroy().

@toddparker

@rid00z - thanks for your help. If you'd like to help out, hop on #jquerymobile-dev on freenode and we'll find some uses for your talents.

@jzaefferer
jQuery Foundation member

Saw this in the meeting notes: "the new widget factory has this._on() which nicely prevents memory leaks caused by stale attached event handlers, however, we should implement _destroy() on our widgets, because handlers attached to elements introduced during widget creation need not be removed if the elements are removed." - as long as you're using _on to bind events, you don't need to implement _destroy yourself - am I missing something?

@gabrielschulhof

@jzaefferer

Our widgets need to implement _destroy() so as to roll back changes they made during _create() (such as e.g. new DOM elements), but they can rely on the _destroy() inside $.Widget to remove the bindings.

@jzaefferer
jQuery Foundation member

Are there still widgets that don't clean up after themselves? If so, maybe individual tickets for each affected widget might help get this resolved.

@jaspermdegroot
jQuery Foundation member

Yes, this is still work in progress. We have a generic ticket for adding the missing _destroy() method (#5293) but I agree it's better if we have separate tickets for each widget.

@arschmitz @gabrielschulhof - Can you open tickets for widgets that aren't done yet and close #5293? If this issue is completely resolved when all widgets have _destroy() methods it can be closed as well to avoid duplication.

Thanks!

Update: See See #5293 (comment). Closing this ticket as duplicate. Closing this ticket as duplicate.

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