683286 - Setting the src attribute for an Image causes us to decode right away even if it is not visible

Status: RESOLVED WORKSFORME

(Core :: Graphics: ImageLib, defect)

Description

[Jeff Muizelaar [:jrmuizel]]

None of the other browsers seem to do this and it can cause us to use a lot more memory.

Comment 1

[Jeff Muizelaar [:jrmuizel]]

Attachment: Don't request a decode when we don't need it

Comment 2

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Can we take the patch here?  What else needs to be done?

Comment 3

[Xavier Antoviaque]

We can confirm this is an issue, especially for web applications which do pre-loading.

On our game (cf http://cardstories.org/), visitors go through a loading screen, which pre-loads several large sprites using Image objects. On Firefox post-4.0 we see a pretty huge spike of memory consumption of ~250MB during the preloading, compared to 20-25MB on other browsers, or previous versions of Firefox.

The memory is freed pretty quickly - the garbage collector seem to properly clean things up right after, but in the meantime the uncompressed images heavily bloat the memory, creating crashes/slowdowns for players who use Firefox. 

I see there is a patch for this issue, but I'm unsure about the status as it's sitting there quietly since September. Any chance to get it merged for Firefox 11? :D

Comment 4

[Xavier Antoviaque]

Attachment: Benchmark - CardStories.org preloading

I'm attaching a quick benchmark of the memory usage for 3 different versions: Firefox 4.0-b13pre, Firefox 8.0, Firefox nightly build (9/12). 

Details of the issue and ODS of the results available at http://tickets.farsides.com/issues/528#note-17

Comment 5

[Josh Matthews [:jdm]]

Comment on attachment 557495 [details] [diff] [review]
Don't request a decode when we don't need it

Joe, I choose you!

Comment 6

[Joe Drew (not getting mail)]

Comment on attachment 557495 [details] [diff] [review]
Don't request a decode when we don't need it

Review of attachment 557495 [details] [diff] [review]:
-----------------------------------------------------------------

Josh uses JOE! It's super effective!

Comment 7

[Joe Drew (not getting mail)]

Jeff and I agree that we should put this in at the beginning of the next cycle.

Comment 8

[Nicholas Nethercote [inactive]]

Jeff, can you land this as soon as you're back from holidays?  Thanks.

Comment 9

[Jeff Muizelaar [:jrmuizel]]

I'd like to write a test for this before landing, and need to think about the implications of this, i.e. will it keep us from firing onload?

Comment 10

[David Rees]

(In reply to Xavier Antoviaque from comment #3)
...
> The memory is freed pretty quickly - the garbage collector seem to properly
> clean things up right after, but in the meantime the uncompressed images
> heavily bloat the memory, creating crashes/slowdowns for players who use
> Firefox. 
> 

Can someone confirm the memory is released correctly when using something like new Image().src = url" for preloading? I worry that bug 661304 implies that memory will not be freed if the JavaScript calling it is in the context of the current tab (even though the image object is orphaned its part of the current tab)?

In the meantime is there a workaround that will help like explicitly setting the src to null after the image is loaded or reusing Image objects?

Comment 11

[Justin Lebar (not reading bugmail)]

> Can someone confirm the memory is released correctly when using something like new Image().src = 
> url" for preloading?

You can check pretty easily using about:memory.

> I worry that bug 661304 implies that memory will not be freed if the JavaScript calling it is in the 
> context of the current tab (even though the image object is orphaned its part of the current tab)?

That should only apply to images which are in the document.

Comment 12

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to Justin Lebar [:jlebar] from comment #11)
> > I worry that bug 661304 implies that memory will not be freed if the JavaScript calling it is in the 
> > context of the current tab (even though the image object is orphaned its part of the current tab)?
> 
> That should only apply to images which are in the document.

Why do you think that?  Bug 683290 is still open ...

Comment 13

[Justin Lebar (not reading bugmail)]

> Why do you think that?  Bug 683290 is still open ...

Ah, right.  We really should fix that...

Comment 14

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to Justin Lebar [:jlebar] from comment #13)
> > Why do you think that?  Bug 683290 is still open ...
> 
> Ah, right.  We really should fix that...

I'm working on it!  Hopefully some more movement on the dependencies next week.

Comment 15

[Tim Taubert [:ttaubert] (inactive)]

Jeff, what's the status of this patch? Did you have a chance to think about its side-effects? I'd really like to have this for bug 650968.

Comment 16

[Jeff Muizelaar [:jrmuizel]]

(In reply to Tim Taubert [:ttaubert] from comment #15)
> Jeff, what's the status of this patch? Did you have a chance to think about
> its side-effects? I'd really like to have this for bug 650968.

No, I haven't had a chance to look at this anymore since I wrote last.

Comment 17

[Eric Rahm [:erahm]]

Comment on attachment 557495 [details] [diff] [review]
Don't request a decode when we don't need it

This code no longer exists.

Comment 18

[Eric Rahm [:erahm]]

Jeff should we just close this bug? It seems like we did a big refactor of image loading since filing.

Comment 19

[BugBot [:suhaib / :marco/ :calixte]]

The severity field for this bug is relatively low, S3. However, the bug has 18 votes.
:jrmuizel, could you consider increasing the bug severity?

For more information, please visit auto_nag documentation.

Comment 20

[BugBot (nomail) [:suhaib / :marco/ :calixte]]

The last needinfo from me was triggered in error by recent activity on the bug. I'm clearing the needinfo since this is a very old bug and I don't know if it's still relevant.

Comment 21

[Timothy Nikkel (:tnikkel)]

I think this is fixed now. I checked setting src on an img that was not visible because: it was display none, or it was visibility hidden, or it was scrolled far out of view. If there are cases where this is still happening please let me know.