Animated gif leaks memory

VERIFIED FIXED in M14

Status

()

Core
ImageLib
P3
critical
VERIFIED FIXED
18 years ago
12 years ago

People

(Reporter: waqar, Assigned: Gagan)

Tracking

({memory-leak})

Trunk
memory-leak
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [PDT+] Looks okay, but seeking verification w/Purify., URL)

Attachments

(2 attachments)

(Reporter)

Description

18 years ago
Some images on the above page leak memory.
(Reporter)

Comment 1

18 years ago
Created attachment 6085 [details]
One if the animated gif that leaks memory
(Reporter)

Updated

18 years ago
Keywords: beta1
(Reporter)

Updated

18 years ago
Blocks: 29862

Comment 2

18 years ago
I know you folks were working on this, and partial fixes.  The meg/minute leak 
rate is Very Bad. I think you folks have something that will bring it down to a 
tolerable level.  I'll PDT+ the fix (patch? hack?) that gets us out of real 
trouble, but please add info to indicate how bad the leak is if you want to go 
for further corrections.
On 3/7, we'll probably have to give up on this (unless we're still in the 
MB/minute rate :-( ).
thanks,
Jim
Whiteboard: [PDT+] w/b minus on 3/7

Comment 3

18 years ago
We leak about a meg a minute sitting on http://my.netscape.com/ as of last 
night. I filed a bug on that and it should be marked as a dup of this, but I 
can't find it in bugzilla at the moment (probably due to bugzilla being a bit 
messed up).

Comment 4

18 years ago
I was out Friday so I am seeing this for the first time now.
Do you happen to know if this is a new leak? A week ago, glowcode
gave us a ok on animateds.

I'll start looking at this with purify. If anyone else has a good
or better tool to look at leaks, I'd appreciate knowing about it/and/or
help.

-P
Status: NEW → ASSIGNED
Target Milestone: M14

Comment 5

18 years ago
http://www.cybersight.com/~bruce/mozilla-bin.20000305.log is a purify log for 
this.

A really easy way to reproduce this is to just go and visit 
http://my.netscape.com/ (what I did for that log) and then just sit there and 
watch the memory usage grow.
(Reporter)

Comment 6

18 years ago
Same thing happens if you go to the linuxstart.com page. Just visit the page and 

let it sit there and watch the memory usage go up, about a .5MB/s.

Comment 7

18 years ago
Thanks for the log, Bruce.
-P

Comment 8

18 years ago
*** Bug 30629 has been marked as a duplicate of this bug. ***

Comment 9

18 years ago
from my NT purify it looks like the leaks are in memcache code.
Also I get an assert that there is "a failure to shut down memcache cleanly".
in mozilla/netwerk/cache/memcache/nsMemCache.cpp.

Its a start.
-P

Comment 10

18 years ago
If this is what is also in my log, it looked to me like memcache stuff was 
leaking, at least in part, due to the nsHTTPChannel being leaked in its 
entirety.

MLK: 163800 bytes leaked in 975 blocks
  * This memory was allocated from:
        malloc         [rtlib.o]
        __bUiLtIn_nEw  [libxpcom.so]
        __builtin_new  [rtlib.o]
        nsHTTPHandler::NewChannel(const 
char*,nsIURI*,nsILoadGroup*,nsIInterfaceRequestor*,unsigned int,nsIURI*,unsigned 
int,unsigned int,nsIChannel**) [nsHTTPHandler.cpp:263]
        nsIOService::NewChannelFromURI(const 
char*,nsIURI*,nsILoadGroup*,nsIInterfaceRequestor*,unsigned int,nsIURI*,unsigned 
int,unsigned int,nsIChannel**) [nsIOService.cpp:241]
        
NS_OpenURI(nsIChannel**,nsIURI*,nsILoadGroup*,nsIInterfaceRequestor*,unsigned 
int,unsigned int,unsigned int) [nsNetUtil.h:79]
        ImageNetContextImpl::GetURL(ilIURL*,NET_ReloadMethod,ilINetReader*) 
[nsImageNetContextAsync.cpp:674]
        il_image_complete(il_container_struct*) [if.cpp:1551]
        ImgDCallbk::ImgDCBHaveImageAll() [if.cpp:185]
        process_buffered_gif_input_data(gif_struct*) [gif.cpp:692]
        gif_delay_time_callback(void*) [gif.cpp:725]
        timer_callback(nsITimer*,void*) [nsImageSystemServices.cpp:70]
        nsTimerGtk::FireTimeout() [nsTimerGtk.cpp:58]
        nsTimerExpired [nsTimerGtk.cpp:175]
        g_timeout_dispatch [gmain.c:1147]
        g_main_dispatch [gmain.c:647]
        g_main_iterate [gmain.c:854]
        g_main_run     [gmain.c:912]
        gtk_main       [gtkmain.c:475]
        nsAppShell::Run() [nsAppShell.cpp:304]
        nsAppShellService::Run() [nsAppShellService.cpp:392]
        main1(int,char**,nsISplashScreen*) [nsAppRunner.cpp:769]
        main           [nsAppRunner.cpp:889]
        _start         [crt1.o]
  * Block of 168 bytes (975 times); last block at 0x292c508

Why are we leaking 975 channels?  Why are we even creating that many?  
Everything that I see coming from the memcache appears to originate with these 
nsHTTPChannel objects.

Comment 11

18 years ago
Although this is a significant leak.... I have to hope we can survice a beta1.
The 3/7 "give up" date.
Marking PDT- for beta1
PDT+ for beta2
Whiteboard: [PDT+] w/b minus on 3/7 → [PDT-] plus for beta2

Comment 12

18 years ago
I give up.

Comment 13

18 years ago
I really hope the PDT will reconsider this one.  As jar said on 3/5/00, a
"meg/minute leak is Very Bad".  So bad, in fact, that I will not be able to use
a netscape beta product that leaks like this.  I think basically any solution is
better than this leak, even Bruce's idea of disabling animated gifs.  Realize
that this affects my.netscape.com very badly.  Someone on the PDT should just
let their browser sit on my.netscape.com for an hour or so and see what happens
to their performance.  It's not pretty.

To reiterate:  I am convinced that you cannot survive a beta with this bug.
More concretely, I will not use the netscape commericial beta if it has this
problem.  I've tried to just ignore it, but it degrades performance too much to
allow me to do so.

I'm now quitting this M15 build and starting up my nice dependable m14 build.
Thanks for reconsidering.

Comment 14

18 years ago
Maybe this has something to do with using the url string vs. uri object in the 
memcache, as mentioned in bug 29853, since the image is refetched every cycle.

Comment 15

18 years ago
status:
Gagan is looking at channel code & asked me to hold off
for a day.  Any news Gagan?
-P

Comment 16

18 years ago
I would dearly love to have a fix, or a work-around to stop this leak.  If folks
can provide a safe work-around (in the extreme, I agree, disabling animated gifs
would be considered), then remove the PDT-, and re-nominate.  I think the leak
has been reduced *greatly* (and is no longer as bad as "megs/minute").
If there is new info (work-around? fix?) or evidence that this is worse than I'm
understanding, please give details, and renominate (re: remove "PDT-" to attract
attention of the PDT).
We have to ship... dates are real... so we'll have to take pain if we have
nothing to help with this in short order :-( :-(.

Comment 17

18 years ago
Please reconsider this for beta1! I was sitting on linuxstart.com, walked away
for 10 minutes, and came back to find mozilla with a 200 meg memory partition
(last night's build, 3/8/2000 midnight)!

This is *NOT* acceptable for beta1, and I don't think it has gotten better. It
will bring lesser machine to their knees before even a few pages have been
rendered. We can't afford that kind of severe, obvious bug...

Whiteboard: [PDT-] plus for beta2 → plus for beta2
(Reporter)

Comment 18

18 years ago
I will be checking in the fix for the 29862 bug, it has been approved. This will 
reduce the memory leak quite a bit.

Comment 19

18 years ago
The JS changes that waqar is landing *should* take care of the large
(meg/minute) leak that I mentioned earlier.  Apparently JS was holding refs, and
leaking memory, for animated gifs that were no longer visible.
I expect the leak amounts to be very "tolerable."  If my information is in
error, please add info to this bug (after the landing from waqar has done its
magic).
Thanks.

Comment 20

18 years ago
waqar:
thanks for the good news...
Should this bug be reassigned to you, or should
I hold it for any other animated image related leaks?

-P

Comment 21

18 years ago
Putting on PDT- radar for beta1. 
Keywords: beta2
Whiteboard: plus for beta2 → [PDT-]plus for beta2
(Reporter)

Comment 22

18 years ago
Pam, you should hold this bug for other animated gifs leak, 29862 does not fix 
the animated leaks, it fixes the javascript realted memory leaks.

Comment 23

18 years ago
You mean like the animated gifs leaks like the one
due to the nsHTTPChannel...... ;-)
-P
(Reporter)

Comment 24

18 years ago
I think so, if just load an animated gif, the one I added here, you will see 
memory being leaked. linuxstart.com has lots of animated gifs, so the problem was 
compounded.

Comment 25

18 years ago
Leaks _anywhere_ are more obvious with an animated gif page.

Comment 26

18 years ago
gagan indicated to me that he had a fix for some of this (the leaking 
nsHTTPChannel objects).  However, that was somewhere around 4-5am, so it'll be a 
bit longer before he's awake.
(Assignee)

Comment 27

18 years ago
I am awake now, and I have a fix... essentially the problem was that we were
addreffing an nsHTTPChannel (quite unnecessarily) for the cacheNetData objects.
I think this was a left over of the changes rpotts was making. Anyhow, I cleaned
that up and the nsHTTPChannel leak per animation cycle is now gone! I will check
it in soon... Bruce will have to run his magic to verify this and identify other
leaks...
(Assignee)

Comment 28

18 years ago
Created attachment 6337 [details] [diff] [review]
diffs for fixing the mem leak...
(Assignee)

Comment 29

18 years ago
diffs attached as above... need someone to r/a it...

Comment 30

18 years ago
thanks for the news, Bruce.
-p
(Assignee)

Comment 31

18 years ago
this should be plus for beta1! cc'ing jevering per his suggestion...
(Assignee)

Comment 32

18 years ago
BTW the diffs are from mozilla/netwerk directory and so they missed out on the
diff for imap-

Index: nsImapProtocol.cpp
===================================================================
RCS file: /cvsroot/mozilla/mailnews/imap/src/nsImapProtocol.cpp,v
retrieving revision 1.234
diff -r1.234 nsImapProtocol.cpp
6566c6566
<     rv = cacheEntry->NewChannel(m_loadGroup, this,
getter_AddRefs(cacheChannel));
---
>     rv = cacheEntry->NewChannel(m_loadGroup, getter_AddRefs(cacheChannel));
Keywords: mlk

Updated

18 years ago
Whiteboard: [PDT-]plus for beta2

Comment 33

18 years ago
PDT, reconsider this.  I think this *needs* to be in the beta.  It is leaking
like 2 meg every 10 seconds on animated images.  This is a HUGE leak and the fix
is simple.
(Assignee)

Comment 34

18 years ago
*** Bug 31371 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 35

18 years ago
taking over... to stomp on it with more force... other verifyable links which
cause disaster without this fix are http://www.hampsterdance.com/ and
http://www.cowdance.com/ 
Assignee: pnunn → gagan
Status: ASSIGNED → NEW
Sorry about the dup, don't know how I missed it.

The fix looks good to me.

Comment 37

18 years ago
Putting on PDT+ radar for beta1.
Whiteboard: [PDT+]

Comment 38

18 years ago
I saw gagan had a fix in hand (with several days of testing on all three
platforms)... and I even approved it for checkin (subject to chofmann's metering
etc.)  What happened to the fix? When will it land???
Whiteboard: [PDT+] → [PDT+] fix in hand (tested, reviewed, and approved)
(Assignee)

Comment 39

18 years ago
the branch wasn't ready till when I left on Friday. I will check it in today as
soon as the tree opens (since I need to do the same with the tip)
Status: NEW → ASSIGNED
(Assignee)

Comment 40

18 years ago
checked in on the branch, still waiting for tree open for tip...
(Assignee)

Comment 41

18 years ago
fix checked in on tip as well... marking fixed. 
Status: ASSIGNED → RESOLVED
Last Resolved: 18 years ago
Resolution: --- → FIXED

Comment 42

18 years ago
[Holding verification until tomorrow AM. Not currently alert enough to verify
this well.]

Comment 43

18 years ago
Using this morning's Win32 build, I'm seeing an increase in application memory
usage of 4 megabytes after leaving the www.linuxstart.com page running for 20
minutes unattended.

However, I'm not sure if this increase in memory usage is related to the
animated GIF or not, since I don't see a regular memory increase on a minute by
minute basis in the 7 minutes since I came back.

bruce@cybersight.com, might you be open to giving this a Purify check on Really
Short Notice?
Whiteboard: [PDT+] fix in hand (tested, reviewed, and approved) → [PDT+] Looks okay, but seeking verification w/Purify.

Comment 44

18 years ago
I fear asking this, but do you have purify Eli?  I know that suresh in mail/news 
QA does.

Comment 45

18 years ago
Sorry, no such fancy tools in browser QA. ;)

Comment 46

18 years ago
Order some?  I'll continue this offline though I think.

Comment 47

18 years ago
Using this morning's commercial beta-branch builds:
	* After about 1.5 hours displaying http://www.mozilla.org/quality/browser/
front-end/testcases/imaging/img-anim-stability/img-anim-stability.html, 
Netscape's memory partition increases by 200K
	* After about 30 minutes displaying the same page on Mac OS, the memory 
partition remains unchanged (rounded off at hundreds of kilobytes)
	* After about 10 minutes on Linux (assuming VSZ in ps actually means memory)

Thus, verifying as fixed. Anyone with tools to do a more detailed check on why 
the Win32 partition is increasing slightly is welcome to.
Status: RESOLVED → VERIFIED

Updated

18 years ago
Keywords: nsbeta2

Comment 48

12 years ago
The problem is still in the very last firefox release (but not in mozilla
suite) try http://facs.scripps.edu/surf/images/euranim.gif
REOPEN
You need to log in before you can comment on or make changes to this bug.