Status

()

Core
ImageLib
P3
major
VERIFIED FIXED
18 years ago
18 years ago

People

(Reporter: Suresh Duddi (gone), Assigned: pnunn)

Tracking

Trunk
x86
Windows NT
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [PDT+], URL)

Attachments

(4 attachments)

(Reporter)

Description

18 years ago
mscott says:

his may be a red herring but I'll mention it anyway. I was just debugging 
something else on windows this afternoon and I noticed something that might
be causing your problem. Steve's email mentioning a timer firing repeatedly 
made me think it might be related.

I had loaded my.netscape.com in the browser and had a breakpoint in the uri 
loader. I noticed that it continually was loading a url all the time. 

The stack trace looked like:
nsURILoader::OpenURI(nsURILoader * const 0x01dcbe80, nsIChannel * 0x042c9800, 
int 0x00000004, const char * 0x00000000, nsISupports *
0x042c63d4) line 486
ImageNetContextImpl::GetURL(ilIURL * 0x042c6ad0, NET_ReloadMethod 
IMG_NTWK_SERVER, ilINetReader * 0x042c6200) line 714 + 56 bytes
il_image_complete(il_container_struct * 0x044811e0) line 1547
ImgDCallbk::ImgDCBHaveImageAll(ImgDCallbk * const 0x04481030) line 185 + 12 
bytes
process_buffered_gif_input_data(gif_struct * 0x04327be0) line 694
gif_delay_time_callback(void * 0x044811e0) line 723 + 9 bytes
timer_callback(nsITimer * 0x042c6670, void * 0x042c0710) line 71 + 12 bytes
nsTimer::Fire() line 194 + 17 bytes
nsTimerManager::FireNextReadyTimer(nsTimerManager * const 0x01ead170, unsigned 
int 0x00000000) line 117
FireTimeout(HWND__ * 0x00000000, unsigned int 0x00000113, unsigned int 
0x00001477, unsigned long 0x2a736a98) line 89

This may not be wrong I don't know. But if for some reason there was a leak 
during the process of loading this image and this timeout keeps firing over
and over again, that could explain what Judson was seeing?

danm says:
----------

However, sitting at my.netscape.com, it looks like this sequence is executed
not once every 20 seconds, but something like 30 times per second. Just
sitting at there for ten seconds leaked something like 1200 objects. Most are
nsVoidKeys. Sprinkled within are a bunch of strings which look like they're
coming from somewhere in necko and I believe an occasional GIFDecoder.

pnunn says:
-----------
Yesterday I talked to mscott about the mynetscape.com has
a stealth animated gif, help.gif.  It doesn't _look_ animated.
Its a 2 frame animation. And the timer should kick off each loop of
the spectacular 2 frame show.
Try looking at it through a 4.x browser using view info.
(Reporter)

Comment 1

18 years ago
Affects stability, the #1 or #2 reason for the beta
Severity: normal → major
Keywords: beta1
(Assignee)

Comment 2

18 years ago
dp:

I don't think you made the right conclusion here.
I talked to mscott yesterday and he thought the
page had no animations. It does. 
The leak I believe is the timer not the animations.

Someone earlier had mentioned they had the same problem
with mozilla.org/index.html and there are no animated images
on that page.

-P
(Assignee)

Comment 3

18 years ago
Ok. I've talked to danm and have some call stacks.
I'm now a believer. There's an image leak in the
gif decoder.
-P
Status: NEW → ASSIGNED
Target Milestone: M14
(Assignee)

Comment 4

18 years ago
Created attachment 5399 [details] [diff] [review]
One line fix for mem leak on animations.
(Assignee)

Comment 5

18 years ago
I think I've got a fix...at least for the string related
leak. I'll attach diff. Dan, could you give me a code review
tomorrow? This is a one line fix.

-p
(Assignee)

Comment 6

18 years ago
danm checked the fix with glowcode and the
leak is still there. No joy.
-p
(Assignee)

Comment 7

18 years ago
I have checked in fix. Dan pointed out another location
in gif.cpp where there was a strdup without a test. On animated
gifs, since the image container is reused, in some places the
mime type is not a null string when we start a new data stream
for the next frame.
I put in tests before the strdup in if.cpp and gif.cpp.

This only fixes one of the leaks.
-P

Comment 8

18 years ago
Putting on the PDT+ radar for beta 1.
Whiteboard: [PDT+]
(Assignee)

Updated

18 years ago
Whiteboard: [PDT+] → [PDT+]finding last leak, ~2 days

Comment 9

18 years ago
I'm still seeing this today at:

         MLK: 350 bytes leaked in 35 blocks
         This memory was allocated from:
               malloc         [rtlib.o]
               PL_strdup      [strdup.c:30]
               nsCRT::strdup(const char*) [nsCRT.h:163]
               gif_delay_time_callback(void*) [gif.cpp:714]
               timer_callback(nsITimer*,void*) [nsImageSystemServices.cpp:71]
               nsTimerGtk::FireTimeout() [nsTimerGtk.cpp:52]
               nsTimerExpired [nsTimerGtk.cpp:169]
               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]
               nsNativeViewerApp::Run() [nsGtkMain.cpp:50]
               main           [nsGtkMain.cpp:168]
               _start         [crt1.o]
         Block of 10 bytes (35 times); last block at 0xc6d190
(Reporter)

Comment 10

18 years ago
Created attachment 5579 [details] [diff] [review]
Fixes for imgdecode leak with animated images and string leak
(Reporter)

Comment 11

18 years ago
Pam, you need to look over my fix. We seem to be creating an image decoder for 
every time we animate. The old image decoder was leaked. I fixed that leak.

It would be better if we work with the same decoder instead of creating a 
new one every cycle of animation. This will peg the cpu for 2 frame animations. 
Code vice, this makes sense as we are working with the same ic.

But then, you know better. See if you can improve on the attached fix.
(Assignee)

Comment 12

18 years ago
Created attachment 5610 [details] [diff] [review]
Latest/Best fix for imglib memleaks
(Assignee)

Comment 13

18 years ago
Created attachment 5613 [details] [diff] [review]
contextdiff for same changes.
(Assignee)

Comment 14

18 years ago
checked in fix.
-p
Status: ASSIGNED → RESOLVED
Last Resolved: 18 years ago
Resolution: --- → FIXED

Comment 15

18 years ago
dp or pnunn, could either of you possibly provide a reproducible test case, or verify this PDT+ bug personally? Thanks!
Whiteboard: [PDT+]finding last leak, ~2 days → [PDT+] Needs developer verification; QA cannot verify.

Comment 16

18 years ago
The only symptom suitable for a test case would be to leave Mozilla on an 
animated gif until it ran out of memory. Sounds boring. I'll verify later today.
(Reporter)

Comment 17

18 years ago
Thanks dan.
(Assignee)

Comment 18

18 years ago
danm:

here's a nice largish animated gif thats good
for testing:
http://jazz/users/pnunn/publish/redhead.gif

I'd verify, but since I checked in the fixes, I think
some other eyes would catch mistakes I wouldn't.
-p

Comment 19

18 years ago
Or you could use a tool, which is of course what I'm doing. Anyway, the only leak 
I see sitting on redhead.gif is the new one covered by bug 28902.
Status: RESOLVED → VERIFIED
Whiteboard: [PDT+] Needs developer verification; QA cannot verify. → [PDT+]

Comment 20

18 years ago
Yes; thanks, Dan!
You need to log in before you can comment on or make changes to this bug.