dimsemenov / Magnific-Popup

Light and responsive lightbox script with focus on performance.
http://dimsemenov.com/plugins/magnific-popup/
MIT License
11.39k stars 3.48k forks source link

Memory Leaks #580

Open maimairel opened 10 years ago

maimairel commented 10 years ago

When working with Magnific Popup on a fully AJAX based site, there will be memory leaks when the user has already opened a popup.

Is there any specific method that can be used to clear all cached objects? For now I managed to fix this by doing the following:

$.magnificPopup.instance.items = null;
$.magnificPopup.instance.ev = null;
$.magnificPopup.instance.st = null
$.magnificPopup.instance.contentContainer = null

I hope such method can be added as a convenient method.

Thanks!

maimairel commented 10 years ago

Additionally, the gallery module also causes memory leak because the item.img .mfploader namespaced event is not unbound when closing the popup.

Here I added some code in the gallery module's _preloadItem method, seems like you forgot to unbind the .mfploader event

if(item.type === 'image') {
    item.img = $('<img class="mfp-img" />').on('load.mfploader', function() {
        item.hasSize = true;
        item.img.off('.mfploader');
    }).on('error.mfploader', function() {
        item.hasSize = true;
        item.loadError = true;
        item.img.off('.mfploader');
        _mfpTrigger('LazyLoadError', item);
    }).attr('src', item.src);
}

I don't really know about the structure of MFP, so I don't know if there are more possible memory leaks.

Thanks!

maimairel commented 10 years ago

Hi Dmitry,

I've tested it thoroughly with many heap shots, and each time there is always an instance of images that is still bound with the mfploader event.

This is what I did to reproduce the issue:

  1. Open a page with an MFP gallery
  2. Open MFP by clicking on one of the images.
  3. Go to another page that's loaded via AJAX and take the heap shot.

Here's a screenshot: mfp

Is this caused by jQuery's caching mechanism or something else?

dimsemenov commented 10 years ago

My fault, I tested it with jQuery 2.x and Zepto which don't have this issue when loading images.

Can you confirm that after adding such code:

$.each( $.magnificPopup.instance.items, function() {
    if( this.img ) {
        this.img.off( '.mfploader' );
    }
});
$.magnificPopup.instance.close();
$.magnificPopup.instance = null;

... you don't see leaks after switching pages via ajax from open gallery?

maimairel commented 10 years ago

Unfortunately, that doesn't fixed it. The leaks are still there after doing that :(