82370 - nsDiskCacheMap::WriteDiskCacheEntry leaks diskEntry

Status: VERIFIED FIXED

(Core :: Networking: Cache, defect)

Description

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

According to the boehm GC, we leak objects allocated at the following stack:

CreateDiskCacheEntry(nsDiskCacheBinding *)
nsDiskCacheMap::WriteDiskCacheEntry(nsDiskCacheBinding *)
nsDiskCacheDevice::DeactivateEntry(nsCacheEntry *)
nsCacheService::DeactivateEntry(nsCacheEntry *)
nsCacheService::CloseDescriptor(nsCacheEntryDescriptor *)
nsCacheEntryDescriptor::Close(void)
nsCacheEntryDescriptor::~nsCacheEntryDescriptor(void)
nsCacheEntryDescriptor::Release(void)
nsCOMPtr::assign_assuming_AddRef(nsICacheEntryDescriptor *)
nsCOMPtr::assign_with_AddRef(nsISupports *)
nsCOMPtr::operator=(nsICacheEntryDescriptor *)
nsHttpChannel::CloseCacheEntry(unsigned int)

It looks to me like the problem is that nsDiskCacheMap::WriteDiskCacheEntry
leaks the memory allocated by CreateDiskCacheEntry.

Also, it looks like there are some free memory mismatches in this code.  Since
the memory is allocated with new[], it should be freed with delete[].  It might
be a good idea to have static methods on nsDiskCacheEntry for creating and
freeing padded entries, or something like that...

Comment 1

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Attachment: leak log starting mozilla and loading my homepage

Comment 2

[gordon]

This is a pretty serious memory leak and should be fixed for 0.9.1.  I'll submit 
a patch later today.

Comment 3

[gordon]

Attachment: Patch to fix leak and clean up deletions.

Comment 4

[Darin Fisher]

sr=darin

Comment 5

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

r=dbaron (although I think in the future it would be nice to have a typesafe way
of doing this, where you have the casts all in one central pair of methods for
allocating and freeing the nsDiskCacheEntry).

Comment 6

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Actually, one question:  when you call nsCRT::zero, isn't metaSize going to be
zero since that's what nsCacheEntry::FlattenMetaData sets it to?  Is that what
you want?

Comment 7

[gordon]

FlattenMetaData() initializes *size to 0, but then calls PL_DHashTableEnumerate(&
table, CalculateSize, size), which sets size to the metadata byte count.

Comment 8

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Ah, I see.  I missed the |return| in the middle of the function.  r=dbaron

Comment 9

[Asa Dotzler [:asa]]

a=asa on behalf of drivers

Comment 10

[gordon]

Fix checked in.

Comment 11

[benc]

Checkins confirmed in LXR, the patches show the correct revised file except:
nsDiskCacheEntry.cpp, 1.9 is patched, instead of 1.8.