372801 - Fix and improve xpcom/ds/nsByteBuffer.cpp and nsIByteBuffer.h

Status: RESOLVED INCOMPLETE

(Core :: XPCOM, defect)

Description

[Johnny Accot]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a3pre) Gecko/20070304 Firefox/3.0a3pre
Build Identifier: 

I wanted to use ByteBufferImpl, which implements nsIByteBuffer, and I think there are quite a few issues with that component.

---

1. When the method |Grow| is called, if the new size is smaller than the old size, the method will still do a memcpy of the old content to the new buffer, which is too small.

2. |Grow| does not update the |mSpace| variable, such that we lose track of the buffer size.

3. The |NS_NewByteBuffer| factory method has a |aOuter| parameter just to throw NS_ERROR_NO_AGGREGATION if one passes a outer object.

4. In the |Fill| method, the |aKeep| parameter is supposed to say what amount of data should be kept in the buffer.  I would think that the data that is kept is at the *beginning* of the buffer, but it seems as if it is the end of the buffer that is kept, and moved to the front of the buffer.  If one reads a few times from an input stream using the |aKeep| parameter, the data will end up mixed in some strange order.

5. The signature of the |Fill| and |Grow| methods are quite odd.  Instead of:

NS_IMETHOD_(PRBool) Grow(PRUint32 aNewSize);
NS_IMETHOD_(PRInt32) Fill(nsresult* aErrorCode, nsIInputStream* aStream, PRUint32 aKeep);

I would expect something like:

NS_IMETHOD Grow(PRUint32 aNewSize);
NS_IMETHOD Fill(nsIInputStream* aStream, PRUint32 aKeep, PRUint32 *_retval);

|Grow| would throw NS_ERROR_OUT_OF_MEMORY instead of returning PR_FALSE when there is an error.  |Fill| would have a closer signature to that of nsIInputStream::Read, which it does call almost directly.

6. I don't think testing for a MIN_BUFFER_SIZE is very useful.  At least it's not as important as testing for a shrinking buffer in |Grow|.

---

There are two components that seem to use the ByteBufferImpl implementation:

./intl/uconv/src/nsConverterInputStream.cpp
./xpcom/io/nsUnicharInputStream.cpp

The two components show why |aKeep| has the semantic it has:  some data was read from a stream, and some unicode character might have been split between two reads, such that we want to keep the data that was not used.  Yet I think this is a very unusual way to use a byte buffer, and it should be possible to push data to the buffer and keep what was there before.  Either add a different method to nsIByteBuffer.h, like a |Push| method with a |aOffset| parameter, or add a flag to |Fill| to know if it is supposed to keep the front bytes or the end bytes.  At least mention that it keeps the end bytes in the installed header file.

Hope it helps.

Reproducible: Always

Steps to Reproduce:
1.
2.
3.

Comment 1

[Benjamin Smedberg]

This bug got lost. I suspect that bytebuffer is a relic and we should be using JS bytearray nowadays. If somebody cares about this, please email me and we can talk about usecases.