198133 - Leaks from zlibAlloc

Status: RESOLVED FIXED

(Core :: XPCOM, defect, P1)

Description

[Brian Ryner (not reading)]

alecf's checkin for bug 189528 caused leaks on the 'brad' tinderbox to jump by
over 1MB.  According to purify, there are a ton of leaks with this stack:

    calloc         [MSVCRT.dll]
    nsRecyclingAllocator::Malloc(UINT,int) [nsRecyclingAllocator.cpp:179]
    zlibAlloc      [nsZipArchive.cpp:400]
    inflate_blocks [infblock.c:229]
    inflate        [inflate.c:221]
    nsZipReadState::ContinueInflate(char *,UINT,UINT *) [nsZipArchive.cpp:1327]
    nsZipReadState::Read(char *,UINT,UINT *) [nsZipArchive.cpp:1251]
    nsJARInputStream::Read(char *,UINT,UINT *) [nsJARInputStream.cpp:53]

I'll attach the full bloat/leak log from that cycle too, before it goes away.

Comment 1

[Brian Ryner (not reading)]

Attachment: leaks from refcount logging

Comment 2

[Brian Ryner (not reading)]

Attachment: leaks and bloat reported by tracemalloc

Comment 3

[Alec Flett]

I'm definitely surprised by this, but I'm investigating. I'm really curious what
some of the stacks look like below nsJARInputStream::Read() - we agressively
free up the zlib structure once zlib has finished inflating a stream...so one
guess I'd have is that a consumer isn't finishing up the read.

when talking with dveditz I had guessed that maybe we waited until the stream
was closed, but that is definitely not the case.

anyhow, I'm waiting on my build so I can do some tracemalloc work.

Comment 4

[Alec Flett]

ok, so there are two leaks here
-  when we initialize the nsZipReadState, we always call into inflateInit2()
whether or not the item is deflated or not. So for every STORED item that we
read out of the JAR, we're leaking stuff out of inflateInit2() - this is an easy
fix.

- the other leak seems to be happening when a stream isn't completely read by
certain files. I don't know whats going on there but basically we're never
getting a Z_STREAM_END back from zlib on about 20% of the files we're reading
from JARs.

I expect to have both leaks plugged today.. 

Comment 5

[Alec Flett]

Ok, I found the other leak. For some strange reason, with SOME files, we never
get a Z_STREAM_END even though we've read the complete stream. So I've updated
the check. 

I've got a fix in hand, patch forthcoming.

Comment 6

[Daniel Veditz [:dveditz]]

I'd like to be in the review loop for this one.

Comment 7

[Alec Flett]

Attachment: fix leaks

ok, this is a diff -bw but basically it covers both cases:
first the if (mItem->compression != STORED) makes sure that we don't call
inflateInit2() on non-compressed files (Such as some of the .gifs and so forth)

second the call to verify that when we hit the end of a compressed file, that
we call inflateEnd()

That covers all the libjar leaks that I see. Can I get some reviews?

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 117751 [details] [diff] [review]
fix leaks

sr=bzbarsky

Comment 9

[Daniel Veditz [:dveditz]]

Comment on attachment 117751 [details] [diff] [review]
fix leaks

>+    if (mItem->compression != STORED) {

Please change this to == DEFLATED. Currently we only support DEFLATED and
STORED but this is open source, someday someone might support the other PKZIP
compression schemes.

>@@ -1345,7 +1347,6 @@
>                                      PRUint32* aBytesRead)
> {
>   // we still use the fields of mZs, we just use memcpy rather than inflate
>-  int zerr = Z_OK;
> 
>   if (mCurPos + aCount > mItem->realsize)
>     aCount = (mItem->realsize - mCurPos);

Just off the bottom here you don't return an error if you're not able to read
as much as you expect. You also don't CRC check STORED data; compression type
has nothing to do with the possibilies of corrupt data in a zip archive.

I suppose there's not much point, though, you've already checked this in :-(

Comment 10

[Alec Flett]

yay! Fix is in and leaks went down on tinderbox by about 1.2 megs!

Comment 11

[Brendan Eich [:brendan]]

How about dveditz's comment #9 ?

/be

Comment 12

[Alec Flett]

brendan: oops, the fix went in actually before dan marked this fixed.. I'm going
to switch this to == DEFLATED just to be sure though.

as for the other stuff, I'll look into the CRC checks, but the removal of that
variable was a warnings fix - I didn't actually change code in that function.

Comment 13

[Daniel Veditz [:dveditz]]

This patch didn't touch the code I commented on in my second point, but it looks
like it was part of your earlier re-writing of the nsJarInputStream behavior.
The original bad "inflate into a big buffer" code did CRC check both STORED and
DEFLATED file data.