Closed Bug 457215 Opened 13 years ago Closed 13 years ago

lcms leaks 12,288 bytes on exit

Categories

(Core :: GFX: Color Management, defect)

x86
Linux
defect
Not set
minor

Tracking

()

RESOLVED FIXED
mozilla1.9.1b3

People

(Reporter: mats, Assigned: bholley)

References

Details

(Keywords: fixed1.9.1, memory-leak, valgrind)

Attachments

(2 files, 1 obsolete file)

lcms leaks 12,288 bytes on exit.

STEPS TO REPRODUCE
1. Build Firefox (x86_64 Linux) with:
      ac_add_options --enable-jemalloc
      ac_add_options --with-valgrind
2. run Firefox under valgrind  (firefox -g -d valgrind)
3. File->Quit

ACTUAL RESULTS
==27750== 12,288 bytes in 3 blocks are definitely lost in loss record 160 of 168
==27750==    at 0x4082B7: arena_malloc_large (jemalloc.c:3939)
==27750==    by 0x408775: arena_malloc (jemalloc.c:3964)
==27750==    by 0x408E5E: imalloc (jemalloc.c:3974)
==27750==    by 0x409C91: malloc (jemalloc.c:5983)
==27750==    by 0x1493A56E: _cmsMalloc (lcms.h:1438)
==27750==    by 0x1493A8C0: cmsAllocGamma (cmsgamma.c:124)
==27750==    by 0x1493AA5C: cmsDupGamma (cmsgamma.c:159)
==27750==    by 0x1494350A: cmsReadICCGamma (cmsio1.c:1815)
==27750==    by 0x14959E77: cmsPrecacheProfile (cmsprecache.c:111)
==27750==    by 0x146A6658: gfxPlatform::GetCMSsRGBProfile() (gfxPlatform.cpp:641)
==27750==    by 0x146A68D4: gfxPlatform::GetCMSOutputProfile() (gfxPlatform.cpp:620)
==27750==    by 0x14B79CEE: info_callback(png_struct_def*, png_info_struct*) (nsPNGDecoder.cpp:566)
==27750==    by 0x14B95C61: MOZ_PNG_push_have_info (pngpread.c:1728)
==27750==    by 0x14B97180: MOZ_PNG_push_read_chunk (pngpread.c:410)
==27750==    by 0x14B9755B: MOZ_PNG_proc_some_data (pngpread.c:55)
==27750==    by 0x14B975BD: MOZ_PNG_process_data (pngpread.c:35)
==27750==    by 0x14B78AF6: ReadDataOut(nsIInputStream*, void*, char const*, unsigned, unsigned, unsigned*) (nsPNGDecoder.cpp:338)
==27750==    by 0x57946C2: nsPipeInputStream::ReadSegments(unsigned (*)(nsIInputStream*, void*, char const*, unsigned, unsigned, unsigned*), void*, unsigned, unsigned*) (nsPipe3.cpp:799)
==27750==    by 0x14B7893F: nsPNGDecoder::WriteFrom(nsIInputStream*, unsigned, unsigned*) (nsPNGDecoder.cpp:368)
==27750==    by 0x14B74F7C: imgRequest::OnDataAvailable(nsIRequest*, nsISupports*, nsIInputStream*, unsigned, unsigned) (imgRequest.cpp:878)
==27750==    by 0x14B6DD3C: ProxyListener::OnDataAvailable(nsIRequest*, nsISupports*, nsIInputStream*, unsigned, unsigned) (imgLoader.cpp:1400)
==27750==    by 0x12EFC691: nsJARChannel::OnDataAvailable(nsIRequest*, nsISupports*, nsIInputStream*, unsigned, unsigned) (nsJARChannel.cpp:872)
==27750==    by 0x125723B6: nsInputStreamPump::OnStateTransfer() (nsInputStreamPump.cpp:508)
==27750==    by 0x125724DF: nsInputStreamPump::OnInputStreamReady(nsIAsyncInputStream*) (nsInputStreamPump.cpp:398)
==27750==    by 0x5795D68: nsInputStreamReadyEvent::Run() (nsStreamUtils.cpp:111)
==27750==    by 0x57AF95F: nsThread::ProcessNextEvent(int, int*) (nsThread.cpp:510)
==27750==    by 0x5771B03: NS_ProcessNextEvent_P(nsIThread*, int) (nsThreadUtils.cpp:227)
==27750==    by 0x152363A1: nsBaseAppShell::Run() (nsBaseAppShell.cpp:170)
==27750==    by 0x1B150251: nsAppStartup::Run() (nsAppStartup.cpp:182)
==27750==    by 0x50606CC: XRE_main (nsAppRunner.cpp:3265)
==27750==    by 0x401BD6: main (nsBrowserApp.cpp:156)
==27750==
I have verified that ShutdownCMS() is called by adding a printf:
printf("ShutdownCMS: gCMSOutputProfile=%p gCMSsRGBProfile=%p\n", gCMSOutputProfile, gCMSsRGBProfile);
and got:
ShutdownCMS: gCMSOutputProfile=0x1f831000 gCMSsRGBProfile=0x1f831000
so it appears the leak is within lcms.
Seems plausible enough. Looks like there are some issues with the freeing for the precache code.

Assigning to myself.
Assignee: nobody → bholley
Blocks: 455056
Attached patch fixSplinter Review
Added a patch. I fixed this issue some time ago, but I for I guess it was in a patch that never landed.

Mats, can you confirm that this plugs the leak? I don't have a linux box so I can't valgrind.
Comment on attachment 340770 [details] [diff] [review]
fix

This fixed it for me.
FYI, I got this compile warning (unrelated to your patch):

lcms.h: In function '_cmsMalloc':
lcms.h:1436: warning: comparison of unsigned expression < 0 is always false
Comment on attachment 340770 [details] [diff] [review]
fix

Ok - flagging vlad for review

Those warnings have always been in LCMS. The problem is that lcms routes mallocs through a macro that checks to see if the malloc-ed amount is < 0. When an unsigned is passed in, it complains. I'll fix it sometime, but it's harmless.
Attachment #340770 - Flags: review?(vladimir)
Attachment #340770 - Flags: approval1.9.1?
Comment on attachment 340770 [details] [diff] [review]
fix

Seeking Approval-1.9.1. This patch fixes a memory leak in lcms, which is important for 3.1 because people generally complain if they find their browser doesn't free all its memory on shutdown, and could lead to negative press. This has very low regression risk, since it's 3 lines that free a pointer that was allocated but never freed.
Comment on attachment 340770 [details] [diff] [review]
fix

a191=beltzner
Attachment #340770 - Flags: approval1.9.1? → approval1.9.1+
Added a version of the patch with user and commit messages set for metered checkin.
Screwed up the username string. Updating.
Attachment #350413 - Attachment is obsolete: true
http://hg.mozilla.org/mozilla-central/rev/527249758771
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.1b3
Backed out to investigate bug 467102:
http://hg.mozilla.org/mozilla-central/rev/3957258880e7
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
relanded http://hg.mozilla.org/mozilla-central/rev/fc1c0104ec4d
Status: REOPENED → RESOLVED
Closed: 13 years ago13 years ago
Resolution: --- → FIXED
Product: Core → Core Graveyard
Component: GFX → GFX: Color Management
Product: Core Graveyard → Core
QA Contact: general → color-management
Keywords: checkin-needed
um, why did you flag this checkin-needed Dao? It landed finally on both trees (before the branch) almost half a year ago.
also FWIW LCMS is obsoleted by bug 481926.
Trying to make the "has 1.9.1 approval but didn't land" list useful...
You need to log in before you can comment on or make changes to this bug.