457215 - lcms leaks 12,288 bytes on exit

Status: RESOLVED FIXED

(Core :: GFX: Color Management, defect)

Description

[Mats Palmgren (:mats)]

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==

Comment 1

[Mats Palmgren (:mats)]

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.

Comment 2

[Bobby Holley (:bholley)]

Seems plausible enough. Looks like there are some issues with the freeing for the precache code.

Assigning to myself.

Comment 3

[Bobby Holley (:bholley)]

Attachment: fix

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 4

[Mats Palmgren (:mats)]

Comment on attachment 340770 [details] [diff] [review]
fix

This fixed it for me.

Comment 5

[Mats Palmgren (:mats)]

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 6

[Bobby Holley (:bholley)]

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.

Comment 7

[Bobby Holley (:bholley)]

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 8

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 340770 [details] [diff] [review]
fix

a191=beltzner

Comment 9

[Bobby Holley (:bholley)]

Attachment: Same final patch as above, Modified with user/commit message. [Check me in].

Added a version of the patch with user and commit messages set for metered checkin.

Comment 10

[Bobby Holley (:bholley)]

Attachment: MQ formatted, proper username. [Check Me In]

Screwed up the username string. Updating.

Comment 11

[Peter Van der Beken [:peterv]]

http://hg.mozilla.org/mozilla-central/rev/527249758771

Comment 12

[Shawn Wilsher :sdwilsh]

Backed out to investigate bug 467102:
http://hg.mozilla.org/mozilla-central/rev/3957258880e7

Comment 13

[Shawn Wilsher :sdwilsh]

relanded http://hg.mozilla.org/mozilla-central/rev/fc1c0104ec4d

Comment 14

[Bobby Holley (:bholley)]

um, why did you flag this checkin-needed Dao? It landed finally on both trees (before the branch) almost half a year ago.

Comment 15

[Bobby Holley (:bholley)]

also FWIW LCMS is obsoleted by bug 481926.

Comment 16

[Dão Gottwald [::dao]]

Trying to make the "has 1.9.1 approval but didn't land" list useful...