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

Status: NEW

(Core :: XPCOM, defect)

Description

[Jonathan Kew [:jfkthame]]

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?

Comment 2

[Mike Hommey [:glandium]]

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.

Comment 3

[Jonathan Kew [:jfkthame]]

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.

Comment 4

[Mike Hommey [:glandium]]

If we get rid of GetBuffer and return an nsIObjectInputStream instead, we would also get rid of the use of nsZipItemPtr::Forget.

Comment 5

[Mike Hommey [:glandium]]

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.

Comment 6

[Mike Hommey [:glandium]]

(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