StartupCache::GetBuffer needs to clarify how the returned buffer was allocated

NEW
Unassigned

Status

()

defect
8 years ago
7 years ago

People

(Reporter: jfkthame, Unassigned)

Tracking

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

The StartupCache::GetBuffer() method says that ownership of the returned buffer is passed to the caller:

http://mxr.mozilla.org/mozilla-central/source/startupcache/StartupCache.h#134

However, it does not specify how the returned buffer was allocated (malloc, NS_Alloc, new[], some-other-allocator?) and therefore the caller doesn't know how to release it.

Looking at the implementation does not help, as it appears that the buffer might have been allocated with C++ new[]:

http://mxr.mozilla.org/mozilla-central/source/startupcache/StartupCache.cpp#240

But alternatively, it might come from nsZipItemPtr::forget():

http://mxr.mozilla.org/mozilla-central/source/startupcache/StartupCache.cpp#250

(or line 260 or 270); and checking nsZipItemPtr, it appears that this may be a malloc'ed buffer:

http://mxr.mozilla.org/mozilla-central/source/modules/libjar/nsZipArchive.h#368

So what's a caller to do? delete[] it, or free() it?
Duplicate of this bug: 697449
Blocks: 94199
Blocks: 684889
How about we just get rid of GetBuffer? Except gfx/thebes/gfxFT2FontList.cpp, all uses take the returned buffer/length pair and feed it to NewObjectInputStreamFromBuffer, which returns an nsIObjectInputStream. We could instead have a GetObjectInputStream function that returns the nsIObjectInputStream directly, and adapt gfxFT2FontList.cpp accordingly.
It looks like this problem goes deeper than just StartupCache::GetBuffer(); the same kind of uncertainty exists for the buffer returned from nsZipItemPtr::Forget() (which is why you can't just say that GetBuffer()'s result is to be deallocated using delete[]).

Can we fix nsZipItemPtr::Forget(), and any other ZipItem code involved here, and then have consistent, documented behavior for GetBuffer()?

Seems like unless we fix that, the problem is still going to exist somewhere, as whoever is dealing with the zipitem's buffer - whether it's within StartupCache or its client code - can't be sure what to do with it.
If we get rid of GetBuffer and return an nsIObjectInputStream instead, we would also get rid of the use of nsZipItemPtr::Forget.
Also note that bug 695843 will change Omnijar::GetReader such that it will return an nsIZipArchive, which could be used to retrieve an nsIInputStream. If mArchive is changed to an nsIZipArchive as well, we can get rid of the use of nsZipItemPtr.
(In reply to Mike Hommey [:glandium] from comment #5)
> Also note that bug 695843 will change Omnijar::GetReader such that it will
> return an nsIZipArchive, which could be used to retrieve an nsIInputStream.
> If mArchive is changed to an nsIZipArchive as well, we can get rid of the
> use of nsZipItemPtr.

nsIZipReader instead of nsIZipArchive
You need to log in before you can comment on or make changes to this bug.