Closed Bug 638031 Opened 13 years ago Closed 13 years ago

nsStringStreams should support using a refcounted buffer

Categories

(Core :: XPCOM, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: khuey, Assigned: neil)

Details

(Keywords: student-project, Whiteboard: [good first bug])

Attachments

(1 file, 2 obsolete files)

Right now they support using a buffer that something else is keeping alive, copying the contents of a buffer, or adopting a buffer.  They should also support refcounted buffers (be that an nsStringBuffer which seems to be the closest thing we have to a generic refcounted buffer or something else).
Keywords: student-project
Whiteboard: [good first bug]
Assignee: nobody → ejpbruel
I'm almost ready
My previous comment got posted prematurely. It should have said:

I'm almost ready with a patch for this bug, but before I put it online for review I would like to know how I can test this particular part of the codebase? Are there any specific unit tests I can run, and if so, how?
Attached patch Proposed patch (obsolete) — Splinter Review
Well, in that case, here is my proposed patch. I initially tried a somewhat smarter solution, storing the two booleans as flags in the mData field, but then I realized that only the mData->buffer subfield is guaranteed to be 4-byte aligned, whereas the mData->string subfield is not, so that solution was off. The current solution stores the two booleans explicitly, in a packed format.

Note that I wasn't 100% sure how to expose the API for using StringBuffers in IDL. I've opted to define nsStringBuffer as a native type, but if there is a better or preferred way of doing this, please let me know.
Attachment #550656 - Flags: review?(khuey)
The code in ShareBuffer in the dataLen < 0 case looks wrong.  Was that codepath tested?

I don't see you actually addrefing or releasing the buffer anywhere.  I'm surprised this passed whtever tests you _did_ do; it should be crashing when trying to read from deallocated buffers.

Is there a reason to not just keep |const char* mData| and just put the buffer's data pointer there?  Then you just need to look at mIsBuffer (which is preferable to mBuffer, I think) in the destructor to decide whether it needs releasing.

As far as the IDL goes, that part looks fine.
Being fairly new to Mozilla, I wasn't sure which tests I should run for this patch and how I should run them. As you can see in the earlier comments, Khuey suggested that just running the browser may be enough of a workout, so that is what I did (of course, the browser currently doesn't use the ShareBuffer method, so I'm not surprised that it turns out there's still a major bug in there). I apologize if the quality of this patch is below par as a result. If there are any other tests you would like me to run, please let me know, and I will look into it asap.

Having said that, you are right about the buffer not being AddReffed. This is a simple oversight, and of course should have happened in the ShareBuffer method. The buffer *is* Released however. This happens in the Clear() method, which is also called from the destructor if necessary. I also agree that mIsBuffer is preferable to mBuffer, but for consistency with the existing code (specifically, with mOwned) I decided against that. However, the use of a union makes it explicit that there can be two different entities stored in the same field, so personally I would like to keep it like that.

In general, and for future reference, how should I test these kind of patches that add some new functionality to Firefox that is not actually used anywhere at the time the patch is submitted?


(In reply to comment #5)
> The code in ShareBuffer in the dataLen < 0 case looks wrong.  Was that
> codepath tested?
> 
> I don't see you actually addrefing or releasing the buffer anywhere.  I'm
> surprised this passed whtever tests you _did_ do; it should be crashing when
> trying to read from deallocated buffers.
> 
> Is there a reason to not just keep |const char* mData| and just put the
> buffer's data pointer there?  Then you just need to look at mIsBuffer (which
> is preferable to mBuffer, I think) in the destructor to decide whether it
> needs releasing.
> 
> As far as the IDL goes, that part looks fine.
The ideal thing as far as tests go in this situation is to add some tests that exercise the new code.  I wonder whether the .cpp tests in xpcom/tests actually get run... if so, adding one of those would be the simplest approach.  Kyle?

The existing code is almost certainly already exercised by other tests, in this case, as Kyle said.

Good point on Clear() releasing the buffer.  I'd missed that.

I'd vote we rename mOwned to mIsOwned too.  ;)

As far as the union goes, the only place that really matters is during Clear() and when setting the value, right?  It seems to me like it would be nice if all the other code didn't have to worry about that detail, just like it doesn't have to worry about the value of mOwned right now.  In all cases we're holding a char*; the only question is how to properly clean it up...  Or at least that's my mental model.  Maybe that's not the common case.

And as far as apologizing... don't.  :)  I'm sorry the way review works (with one person pointing out problems in something another one did) is so adversarial on the surface.
(In reply to Boris Zbarsky (:bz) from comment #7)
> The ideal thing as far as tests go in this situation is to add some tests
> that exercise the new code.  I wonder whether the .cpp tests in xpcom/tests
> actually get run... if so, adding one of those would be the simplest
> approach.  Kyle?

Yes, adding a .cpp test to xpcom/tests is the right way to go.  Ping me on IRC if you need more guidance here.

> The existing code is almost certainly already exercised by other tests, in
> this case, as Kyle said.

The existing code looks correct.

> I'd vote we rename mOwned to mIsOwned too.  ;)

Agreed.
 
> As far as the union goes, the only place that really matters is during
> Clear() and when setting the value, right?  It seems to me like it would be
> nice if all the other code didn't have to worry about that detail, just like
> it doesn't have to worry about the value of mOwned right now.  In all cases
> we're holding a char*; the only question is how to properly clean it up... 
> Or at least that's my mental model.  Maybe that's not the common case.
>
> And as far as apologizing... don't.  :)  I'm sorry the way review works
> (with one person pointing out problems in something another one did) is so
> adversarial on the surface.

Indeed.
Comment on attachment 550656 [details] [diff] [review]
Proposed patch

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

Overall, looks pretty good.  Some comments, and needs the addref plus tests.

::: xpcom/io/nsIStringStream.idl
@@ +96,5 @@
>       * @param dataLen   - stream data length (-1 if length should be computed)
>       */
>      [noscript] void shareData(in string data, in long dataLen);
> +
> +    [noscript] void shareBuffer(in StringBuffer buffer, in long dataLen);

Add doc comments for this?  Also, make it clear that this length is in bytes.

::: xpcom/io/nsStringStream.cpp
@@ +132,5 @@
>  
> +    union {
> +        const char      *string;
> +        nsStringBuffer  *buffer;
> +    } mData;

I really should land that tagged union template I wrote a while back :-)

@@ +136,5 @@
> +    } mData;
> +    PRUint32        mOffset;
> +    PRUint32        mLength;
> +    PRPackedBool    mBuffer : 1;
> +    PRPackedBool    mOwned  : 1;

If you're going to use bitfields, don't bother making these PRPackedBools, just use regular bool.

@@ +260,5 @@
> +{
> +    NS_ENSURE_ARG_POINTER(buffer);
> +
> +    if (dataLen < 0)
> +        dataLen = strlen(reinterpret_cast<const char*>(buffer));

I'm very hesitant to compute a length here, because it's only going to be correct for ASCII strings.  Maybe we should just make the input length an unsigned long here.

@@ +501,5 @@
> +    NS_PRECONDITION(aStreamResult, "null out ptr");
> +
> +    nsStringInputStream* stream = new nsStringInputStream();
> +    if (!stream)
> +        return NS_ERROR_OUT_OF_MEMORY;

operator new is infallible in Gecko (we just abort if it fails) so you don't need this check.

::: xpcom/io/nsStringStream.h
@@ +105,5 @@
>  NS_NewCStringInputStream(nsIInputStream** aStreamResult,
>                           const nsACString& aStringToRead);
>  
> +extern NS_COM nsresult
> +NS_NewStringBufferInputStream(nsIInputStream** aStreamResult,

Total bikeshedding, but how about NS_NewSharedBufferInputStream?

@@ +106,5 @@
>                           const nsACString& aStringToRead);
>  
> +extern NS_COM nsresult
> +NS_NewStringBufferInputStream(nsIInputStream** aStreamResult,
> +                              nsStringBuffer* aBufferToRead, PRInt32 aLength);

Also needs comments so that it's clear that the length is in bytes.
Attachment #550656 - Flags: review?(khuey) → review-
Comment on attachment 550656 [details] [diff] [review]
Proposed patch

>--- a/xpcom/io/nsStringStream.cpp	Thu Jul 28 05:15:23 2011 -0700
>+++ b/xpcom/io/nsStringStream.cpp	Thu Aug 04 13:57:29 2011 +0200
>+NS_COM nsresult
>+NS_NewStringBufferInputStream(nsIInputStream** aStreamResult,
>+                              nsStringBuffer* aBufferToRead, PRInt32 aLength)
>+{
>+    NS_PRECONDITION(aStreamResult, "null out ptr");
>+
>+    nsStringInputStream* stream = new nsStringInputStream();
>+    if (!stream)
>+        return NS_ERROR_OUT_OF_MEMORY;
>+
>+    NS_ADDREF(stream);
>+
>+    nsresult rv = stream->ShareBuffer(aBufferToRead, aLength);
>+    if (NS_FAILED(rv)) {
>+        NS_RELEASE(stream);
>+        return rv;
>+    }
>+
>+    *aStreamResult = stream;
>+    return rv;
>+}

This could use some smart pointer love:

nsRefPtr<nsStringInputStream> stream = new nsStringInputStream();
nsresult rv = stream->ShareBuffer(aBufferToRead, aLength);
NS_ENSURE_SUCCESS(rv, rv);
stream.forget(aStreamResult);
return rv;
I've written a new patch which is ready for review, but I'm still struggling to write a good test for it. I've tried adding a C++ test to xpcom/tests, but the problem I'm running into is that nsStringBuffer is not exposed by the public String API.

Here are the steps I have tried so far:
1. I cannot use nsStringBuffer::FromString or any other static constructor to create an nsStringBuffer for my test, because the definition of that function is not linked with the test
2. To my knowledge, there is no library that contains this definition, so it's not possible to simply link the test against that library to make it work.
3. Explicitly copying the definition of nsStringBuffer::FromString to my test does not work either, because it relies on nsACStringAccessor, which is not declared by nsStringAPI.h
4. Including nsSubstring.h, which contains this declaration, is not allowed, because this is part of the private API (as opposed to nsStringAPI.h, which is the public API).
5. Defining MOZILLA_INTERNAL_API allows me to include nsSubstring.h, but now I'm no longer allowed to include nsStringAPI.h (which i need for nsCAutoString)
6. Including nsString.h to get nsCAutoString does not work because MOZILLA_INTERNAL_API causes nsACString to be renamed to nsACString_internal, and only the former seems to be linked against the test.
7. Sticking with the public nsStringAPI.h and explicitly compiling nsSubstring.cpp or linking the corresponding object file does not work either, because nsSubstring.cpp contains some definitions that are also defined in nsStringAPI.cpp, leading to linker conflicts.
Attached patch Revised patch (still untested) (obsolete) — Splinter Review
I've added a revised patch that addresses the issues mentioned in the comments above, but it is still untested due to the aforementioned problems.

Kyle, here are the few lines I've written so far for the test that I couldn't get to compile/link. I'd appreciate it if you could take a look at it and see if you can get any further with this:

#include "nsStringAPI.h"
#include "nsStringBuffer.h"

int main(int argc, char **argv)
{
    nsCAutoString string("This is a test");
    nsStringBuffer *buffer = nsStringBuffer::FromString(string);

    return 0;
}
Is the test not linking against libxul?  If so, can we change that?
(In reply to Boris Zbarsky (:bz) from comment #13)
> Is the test not linking against libxul?  If so, can we change that?

The problem is not that the test is not linking against libxul, it's that it wants to use symbols not exported from libxul.
Hey Kyle, have you made any progress with the C++ testing harness you proposed? This bug has been on hold for a while now, and I would like to wrap it up one way or another.
This bug has been shelved now for a while, and I would like to wrap it up. Kyle, you proposed writing some code that uses this patch as an alternative for writing a test. Do you have anything specific in mind?
(In reply to Kyle Huey from comment #0)
> Right now they support using a buffer that something else is keeping alive,
> copying the contents of a buffer, or adopting a buffer.  They should also
> support refcounted buffers (be that an nsStringBuffer which seems to be the
> closest thing we have to a generic refcounted buffer or something else).

Can't our string classes handle this? Using nsDependentCSubstring, you can:
* Use Assign(data, datalen) to copy data
* Use Adopt(data, datalen) to adopt data
* Use Rebind(data, datalen) to share data
* Use Assign(nsACString) to share a string buffer

So all the bases are covered.
Attachment #550656 - Attachment is obsolete: true
By khuey's suggestion, I'm going to proceed by rewriting  http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsDOMFile.cpp#74 to use the new nsStringStream. This will probably work the patch out good enough for it to land.
Well, not completely untested... it is known to compile ;-)

Sadly there isn't a (char*, PRInt32) version of Rebind on nsDependentCSubstring, so I had to leave the strlen in.

Also, IPC::WriteParam is weird template stuff that wouldn't let me pass a promised flat string directly...
Oh, and in case it isn't clear, this patch improves the existing nsACString creation and setter methods to share the string's buffer (if it has one).
Attachment #569071 - Flags: feedback?(bzbarsky)
Comment on attachment 569071 [details] [diff] [review]
Tested nsDependentCString patch

Seems fine, though the string form of SetData() needs to be careful when |data| is already IsVoid(), because Assign() on a string copies that bit.
Attachment #569071 - Flags: feedback?(bzbarsky) → feedback+
(In reply to Boris Zbarsky from comment #21)
> (From update of attachment 569071 [details] [diff] [review])
> Seems fine, though the string form of SetData() needs to be careful when
> |data| is already IsVoid(), because Assign() on a string copies that bit.
If someone calls SetData(null) on a stream, what do they expect back? ;-)
Good question.  Do they expect it to throw on read, or report EOF?
There are 16 existing callers to NS_NewCStringInputStream, plus I pushed the patch plus the conversion of three callers of NS_NewByteInputStream to try; the linux tests are a little backlogged right now but apart from that there doesn't seem to have been any problems apart from the usual randomorange.
Try run for 5bcf29374c43 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=5bcf29374c43
Results (out of 191 total builds):
    exception: 1
    success: 164
    warnings: 10
    failure: 16
Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/neil@parkwaycc.co.uk-5bcf29374c43
Assignee: ejpbruel → neil
Attachment #569071 - Attachment description: Untested nsDependentCString patch → Tested nsDependentCString patch
Attachment #569071 - Flags: review?(benjamin)
Attachment #569071 - Flags: review?(benjamin) → review+
Pushed changeset f79946f0bb2a to mozilla-central.
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Attachment #552383 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.