486080 - nsIByteBuffer and nsIUnicharBuffer should be removed

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Robert O'Callahan (:roc) (email my personal email if necessary)]

These interfaces are barely used. The code that uses them can use nsTArray<PRuint8> and nsTArray<PRUnichar> instead, taking care to ensure that buffer 'space' is mapped to nsTArray 'capacity' and the buffer 'length' is mapped to nsTArray's 'length'. The only tricky bit is nsIByteBuffer::Fill, which can be reimplemented as a utility function like
  nsresult NS_FillByteBuffer(nsTArray<PRUint8>* aBytes, PRUint32 aKeep);

Comment 1

[David Humphrey (:humph)]

Assigning at Adam's request.

Comment 2

[Adam Eberbach]

My first diff for review (two diffs as attachments). I have not made much effort to check return values and tidy it completely (nor delete files for nsIByteBuffer and nsIUnicharBuffer) because it would probably benefit from scrutiny to see if this is the right way to go about it. It seems to work and does not crash as I browse or squawk about leaks more than it did. gdb shows me the changed code is getting reasonable coverage.

Comment 3

[Adam Eberbach]

Attachment: diff of affected file

Comment 4

[Adam Eberbach]

Attachment: diff of affected file

Comment 5

[Adam Eberbach]

errr. long cc list appeared on attachments. Sorry if I have done something wrong here. Feel free to correct the procedure, I want to learn.

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

You should attach a patch as one diff that covers all affected files. This is easy if you use Mercurial to clone the repository, modify the files in the checkout area and use "hg diff" to generate the patch.

You should mark it as a patch when you attach it.

Code-wise, your idea is right. Your indentation is using tab characters, but we avoid those, use spaces instead --- you should be able to configure your editor to do this.

+  // how much available? Set length of nsTArray
+  PRUint32 streamAvailable;
+  mInput->Available(&streamAvailable);
+  mCharData.SetLength(mLeftOverBytes + streamAvailable);
+  PRUint32 nb;
+  *aErrorCode = mInput->Read(reinterpret_cast<char*>(mCharData.Elements()) + mLeftOverBytes, streamAvailable, &nb);
+
... 
-  NS_ASSERTION(PRUint32(nb) + mLeftOverBytes == mByteData->GetLength(),
+  NS_ASSERTION(PRUint32(nb) + mLeftOverBytes == mCharData.Length(),

This isn't quite right. The Read call may return a value in nb that is less than streamAvailable. In that case, the assertion will fire. Instead we need to do what Fill did, which is to effectively *set* the array length based on the amount of data read from mInput.

Comment 7

[timeless]

Comment on attachment 371645 [details] [diff] [review]
diff of affected file

please don't use tabs, and do make things line up properly

+    	mCharData(0),

is one space under-indented - it looks like you used a tab, did a diff and copied the output from the diff (having the tab serialized as spaces).

+	nsTArray<PRUint8>  mCharData;

and the following line have tabs instead of spaces.

Can you explain why you init mCharData w/ 0 but don't do anything similar for mUnicharData ?

Comment 8

[timeless]

Comment on attachment 371643 [details] [diff] [review]
diff of affected file

same general comments about alignment. also please try to combine related diffs in a single patch.

+  mCharData.SetLength(mLeftOverBytes + streamAvailable);

can this fail?

Comment 9

[Adam Eberbach]

Attachment: for this bug, revised to address comments and correct tab/space issues

Comment 10

[Adam Eberbach]

Comment on attachment 372798 [details] [diff] [review]
for this bug, revised to address comments and correct tab/space issues

addressed comments and fixed tab/spaces problem

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 372798 [details] [diff] [review]
for this bug, revised to address comments and correct tab/space issues

You can't mark review+ yourself, since you're not a reviewer.

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+    if(PR_TRUE != mCharData.SetLength(0))

Comparing to PR_TRUE/PR_FALSE is almost never a good idea. "PR_TRUE != mCharData.SetLength(0)" means "mCharData.SetLength() is false" which is better written "!mCharData.SetLEngth()"..

Comment 13

[Adam Eberbach]

Attachment: remove comparisons to PR_TRUE in favor of !

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 373000 [details] [diff] [review]
remove comparisons to PR_TRUE in favor of !

You can't request review from yourself ... not sure who should review, guessing bsemdberg?

Comment 15

[timeless]

Comment on attachment 373000 [details] [diff] [review]
remove comparisons to PR_TRUE in favor of !

>+    if(PR_TRUE != mCharData.SetLength(0))

Use .Truncate()

And again, don't compare directly against PR_TRUE/PR_FALSE.

if you think |x| will be true, right: if (x), if you think y will be false, write if (!y), and if you're trying to assign z to a PRBool from something which is not definitely PR_TRUE or PR_FALSE, write a=!!z;

>+        NS_ASSERTION(0, "Couldn't empty mCharData");

Don't assert for cases which you know could happen.

And don't use NS_ASSERTION(0,...) use NS_ERROR(...) instead.


>+  // how much available? Set length of nsTArray

how much <what> _is_ available.

>+  if(PR_TRUE != mCharData.SetLength(nb + mLeftOverBytes))
>+  {
>+    NS_ASSERTION(0, "Couldn't resize mCharData");

earlier in this file you had something that vaguely looked like 4 space indentation. please don't mix 2 and 4 space indentation and do follow file style (which I hope is 2 space);

note: i'm only giving r-'s that doesn't imply that if you address my comments you'd get an r+ from me. I rarely give r+'s in general.

Comment 16

[mjh563]

Do you still want this doing? If so, please assign the bug to me and I'll make a patch.

Comment 17

[Trevor Saunders (:tbsaunde)]

(In reply to mjh563 from comment #16)
> Do you still want this doing? If so, please assign the bug to me and I'll
> make a patch.


I can't imagine someone has a reason not to kill them

Comment 18

[mjh563]

Attachment: Remove nsIByteBuffer and nsIUnicharBuffer

Thanks! This patch removes nsIByteBuffer and nsIUnicharBuffer and replaces them with nsTArray<char> and nsTArray<PRUnichar> in the two classes where they were used (nsConverterInputStream and UTF8InputStream).

A couple of notes about the patch:

1. I replaced the calls to nsIByteBuffer::Fill with inline code, rather than using a separate function as suggested in comment 0. The duplicated code is only a few lines, but I can change it to a function if that would be preferable.

2. One 'gotcha' that I came across: if you set the length of an nsTArray to 0 when it's currently > 0, the array capacity is also set to 0. As a result, a simple re-write of this code from ByteBufferImpl::Fill:

mLength = aKeep;
uint32_t nb;
*aErrorCode = aStream->Read(mBuffer + aKeep, mSpace - aKeep, &nb);
if (NS_SUCCEEDED(*aErrorCode)) {
  mLength += nb;
}

to do the same thing but with an nsTArray (ie. setting the length of the array to the amount of left-over data before reading more) will result in an erroneous read request for 0 bytes if the amount of left-over data from the previous read is 0. The solution is to only call SetLength() on the array _after_ the read, not before. The array capacity (and length) can still become 0, but at that point you're at the end-of-stream or an error anyway, so I don't think it matters.

Comment 19

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 763300 [details] [diff] [review]
Remove nsIByteBuffer and nsIUnicharBuffer

Review of attachment 763300 [details] [diff] [review]:
-----------------------------------------------------------------

::: intl/uconv/src/nsConverterInputStream.cpp
@@ +39,5 @@
>      if (NS_FAILED(rv)) return rv;
>   
>      // set up our buffers
> +    mByteData.SetCapacity(aBufferSize);
> +    mUnicharData.SetCapacity(aBufferSize);

I think you want SetLength here, not SetCapacity. You should only ever call SetCapacity for well-understood performance reasons.

@@ -62,5 @@
>      mLineBuffer = nullptr;
>      mInput = nullptr;
>      mConverter = nullptr;
> -    mByteData = nullptr;
> -    mUnicharData = nullptr;

You should call Clear().

@@ -181,5 @@
>    // of a byte buffer we may end up in a situation where n bytes lead
>    // to n+1 unicode chars.  Thus we need to keep track of the leftover
>    // bytes as we convert.
>    
> -  int32_t nb = mByteData->Fill(aErrorCode, mInput, mLeftOverBytes);

Create a helper function to replace Fill(), instead of inlining it. Put it in XPCOM somewhere.

::: xpcom/io/nsUnicharInputStream.cpp
@@ +155,5 @@
>  nsresult 
>  UTF8InputStream::Init(nsIInputStream* aStream)
>  {
> +  mByteData.SetCapacity(STRING_BUFFER_SIZE);
> +  mUnicharData.SetCapacity(STRING_BUFFER_SIZE);

SetLength.

Comment 20

[Mats Palmgren (inactive)]

Also, SetLength() returns false if it fails to allocate, you need to return
NS_ERROR_OUT_OF_MEMORY in that case.

Comment 21

[Mats Palmgren (inactive)]

Attachment: updated patch

I took the liberty of updating the patch to address roc's comments
about Clear() and creating a helper function to replace Fill().
I think the suggested use of SetCapacity() here is correct though.
It's used to pre-allocate a buffer of a suitable size, and the
length reflects how many valid bytes it contains.  Using SetLength()
would be wrong, unless we keep a separate "valid length" member
somewhere and rewrite the code a lot to use that, but
capacity/length does this naturally.

I changed the array type to be fallible because that's what the
consumers currently expects.

A note on SetLength() in the helper function, NS_FillArray:

  aInput->Read(aBuffer + aKeep, aDest.Capacity() - aKeep, aNewBytes);
  aDest.SetLength(aKeep + *aNewBytes);

Here we read from the stream into the array beyond Length() but within
Capacity().  We use SetLength() to update how many bytes we now have.
BUT, this realies on the assumption that SetLength() itself does not
initialize the new slots since that would overwrite the bytes we just
read into it.  nsTArray explicitly avoids that for integral types:
http://hg.mozilla.org/mozilla-central/annotate/c8c9bd74cc40/xpcom/glue/nsTArray.h#l515

An alternative approach that does not rely on that would be to use
IncrementLength() instead:

  nsTArray<char>::size_type newLength = aKeep + *aNewBytes;
  int64_t grow = int64_t(newLength) - aDest.Length();
  if (grow > 0) {
    static_cast<FallibleTArrayAccessor&>(aDest).IncrementLength(grow);
  } else {
    aDest.TruncateLength(newLength);
  }

but IncrementLength is a protected member so we'd need a small helper
class (FallibleTArrayAccessor) to hack around that.

I chose SetLength() with an explicit comment on the hidden assumption.
Let me know if you prefer the IncrementLength solution.
(both are green on Try)

Comment 22

[mjh563]

Mats, thanks. Sorry for not following up on this.

Comment 23

[Mats Palmgren (inactive)]

(In reply to mjh563 from comment #22)
> Mats, thanks. Sorry for not following up on this.

No problem!  I think you did most of the work here so I'm assigning
this bug to you.

Landed on inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b58b09143e5e

Comment 24

[Mats Palmgren (inactive)]

Bah, I guess it should stay open until it lands on -central.

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b58b09143e5e

Comment 26

[Chris Tomlinson]

I've found a problem with the 2013-08-21 nightly build which looks likely to be related to this patch. I'm not sure if it's a real bug or more of a problem with the way my addon (https://addons.mozilla.org/en-US/firefox/addon/keefox/) handles network connections but I hope people familiar with this patch will be able to help me to understand why my add-on no longer works and hopefully find a way to fix it.

The add-on reads UTF8 data from a network stream using JavaScript code similar to this:

  // this.transport is an SSL connection created by 
  // Components.interfaces.nsISocketTransportService
  this.raw_istream = this.transport.openInputStream(0, 512, 0);
  this.istream = Cc["@mozilla.org/intl/converter-input-stream;1"]
               .createInstance(Ci.nsIConverterInputStream);
  this.istream.init(this.raw_istream, "UTF-8", 0,
    Ci..nsIConverterInputStream.DEFAULT_REPLACEMENT_CHARACTER);

  //... Set up an instance of Components.interfaces.nsIInputStreamPump 
  // to call the following function when the network buffers have new data
  //...

  this.onDataAvailable = function(request, ctx, inputStream, offset, count)
  {
    var fullString = "";
    var str = {};
    while (this.istream.readString(4096, str) != 0)
      fullString += str.value;
  }

The 2nd call to readString returns 0 in Nightly 2013-08-21 and later.

There is obviously a lot of other code around the edges so I can't say whether it is a problem with the 2nd readString call, 2nd and subsequent calls or just something about the data that gets sent during that 2nd call. The latter is unlikely since it's always just a small UTF8 encoded string (very similar to the data received in the first onDataAvailable call).

onDataAvailable is being called with the same offset and count parameters as in the working Nightly 2013-08-20 but I don't actually use those parameters because the istream offers no way to do so. Perhaps that is a reason for this problem but since it wasn't clear that this behaviour would be changed by this patch I thought it was worth asking for at least a bit more information.

I'm happy to help convert this information into a formal test and/or new bug if required but I'd need help knowing where to start (I've never built Firefox nor do I understand how the test systems at Mozilla work).

Comment 27

[Mats Palmgren (inactive)]

Chris, there shouldn't be any change in behavior from this bug, so please
file a new bug with the above information and CC me.  Could you also test
a Nightly debug build and include any error messages you see on the console?
http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2013-08-30-mozilla-central-debug/

For doing your own build:
https://developer.mozilla.org/en-US/docs/Simple_Firefox_build