Skip to content
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.

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

Memory leak if one of the images is not loaded #124

Open
d-u-a-l opened this issue Nov 6, 2013 · 9 comments
Open

Memory leak if one of the images is not loaded #124

d-u-a-l opened this issue Nov 6, 2013 · 9 comments

Comments

@d-u-a-l
Copy link

d-u-a-l commented Nov 6, 2013

It's in base.imagesLoaded function

ic = ('fileSize' in t[i] && t[i].fileSize < 0 && t[i].count > 10) ? true : t[i].complete;

must be like

ic = (('fileSize' in t[i] && t[i].fileSize < 0) || t[i].count > 10) ? true : t[i].complete;

because in your case ic would never be true, even after 10 iterations it would be t[i].complete, which is false, because img is not loaded

@Mottie
Copy link
Contributor

Mottie commented Nov 6, 2013

Hi @d-u-a-l!

Were you experiencing a problem with this?

IE is actually the only browser that would set the image complete property to false, other browsers would set it to true even if the image failed to load. Also, IE is the only browser with a fileSize property, and it returns -1 until the file has actually loaded.

Try it yourself with this demo. Click the "Load Images" button, then click on any image and it will show the image properties of complete, fileSize and count in the console.

@d-u-a-l
Copy link
Author

d-u-a-l commented Nov 6, 2013

Tried this just now - Opera does not set property to true, if this image is not loaded. I especially set src attr of one of images to non-existent file and value of property complete of this image remained false.
My corrections were wrong, I admit, but you have to change something, because when this happens in Opera I got heavy load on processor and dramatic increase in memory consumption

@d-u-a-l
Copy link
Author

d-u-a-l commented Nov 6, 2013

oh, by the way, I'm using Opera 12.16

@d-u-a-l
Copy link
Author

d-u-a-l commented Nov 6, 2013

One more problem - I'm experiencing problems with navigation arrows in IE10. They seems to be not working, but sometimes, when you click at them few times, they begin to work.

@Mottie
Copy link
Contributor

Mottie commented Nov 6, 2013

ok, try this code:

ic = ('fileSize' in t[i] && t[i].fileSize < 0 && t[i].count > 10) || t[i].count > 10 ? true : t[i].complete;

I just tried it in Opera, but since I'm on a Windows platform, I have version 17.0 and it does set the complete property to true when an image isn't found.

If that code works, I'll include it with the next update, thanks!

Edit: And I'll look into that IE issue.

@d-u-a-l
Copy link
Author

d-u-a-l commented Nov 6, 2013

Guess it will work. Opera 17 works on Webkit engine, like Chrome and Safari, so it sets complete to true. Opera 12 has it's own engine

I've made a website, where navigation is not working, it's on russian language, thou. It may help finding problem on IE
http://al.devep.ru/

@Mottie
Copy link
Contributor

Mottie commented Nov 6, 2013

Well, it looks like the problem is that base.initialized isn't set to true.... which is set when all images are loaded, which doesn't seem to be triggering the callback function (which sets the base.initialized flag), even with the above change.

I haven't had a chance to test it, but I think this additional change is needed:

if (c || img.length === 0) {
    // all complete, run the callback
    if (typeof callback === "function") { callback(); }
}

@d-u-a-l
Copy link
Author

d-u-a-l commented Nov 6, 2013

it didn't help, because img.length stays the same, as it was after first iteration, because you only push elements to array, but not delete them from it. The thing is, in my case, there's many images, that are hidden, so their height equals 0. That's why code
c = (c && ic && t[i].height !== 0)
always returns false after it checks height of hidden images
why is it so important to check height of images, if we know, that they already loaded?

There's another inconvenience - that is a period after page loaded, when you can't use navigation (something like timeout 2*animation speed, o.speed * 2) - but it's not that bad (even thou I use a fairly long animation speed)

@Mottie
Copy link
Contributor

Mottie commented Nov 6, 2013

Hmm, you're right... I didn't think about hidden images. Even failed images have a height (alt text), so yeah that height doesn't really do anything, so it should be removed as well.

You can always set the o.speed to a low number then elevate it after initialization:

$('#slider').on('initialized.movingBoxes', function(event, mb){
    mb.options.speed = 2000; // or whatever
});

Thanks!

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

No branches or pull requests

2 participants