Closed
Bug 638031
Opened 13 years ago
Closed 13 years ago
nsStringStreams should support using a refcounted buffer
Categories
(Core :: XPCOM, defect)
Core
XPCOM
Tracking
()
RESOLVED
FIXED
People
(Reporter: khuey, Assigned: neil)
Details
(Keywords: student-project, Whiteboard: [good first bug])
Attachments
(1 file, 2 obsolete files)
9.65 KB,
patch
|
benjamin
:
review+
bzbarsky
:
feedback+
|
Details | Diff | Splinter Review |
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).
Updated•13 years ago
|
Keywords: student-project
Whiteboard: [good first bug]
Updated•13 years ago
|
Assignee: nobody → ejpbruel
Comment 1•13 years ago
|
||
I'm almost ready
Comment 2•13 years ago
|
||
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?
Reporter | ||
Comment 3•13 years ago
|
||
http://mxr.mozilla.org/mozilla-central/source/xpcom/tests/unit/test_stringstream.js Just running the browser may be enough of a workout.
Comment 4•13 years ago
|
||
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)
Comment 5•13 years ago
|
||
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.
Comment 6•13 years ago
|
||
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.
Comment 7•13 years ago
|
||
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.
Reporter | ||
Comment 8•13 years ago
|
||
(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.
Reporter | ||
Comment 9•13 years ago
|
||
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 10•13 years ago
|
||
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;
Comment 11•13 years ago
|
||
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.
Comment 12•13 years ago
|
||
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; }
Comment 13•13 years ago
|
||
Is the test not linking against libxul? If so, can we change that?
Reporter | ||
Comment 14•13 years ago
|
||
(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.
Comment 15•13 years ago
|
||
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.
Comment 16•13 years ago
|
||
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?
Assignee | ||
Comment 17•13 years ago
|
||
(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.
Updated•13 years ago
|
Attachment #550656 -
Attachment is obsolete: true
Comment 18•13 years ago
|
||
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.
Assignee | ||
Comment 19•13 years ago
|
||
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...
Assignee | ||
Comment 20•13 years ago
|
||
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).
Assignee | ||
Updated•13 years ago
|
Attachment #569071 -
Flags: feedback?(bzbarsky)
Comment 21•13 years ago
|
||
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+
Assignee | ||
Comment 22•13 years ago
|
||
(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? ;-)
Comment 23•13 years ago
|
||
Good question. Do they expect it to throw on read, or report EOF?
Assignee | ||
Comment 24•13 years ago
|
||
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.
Comment 25•13 years ago
|
||
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
Updated•13 years ago
|
Assignee: ejpbruel → neil
Assignee | ||
Updated•13 years ago
|
Attachment #569071 -
Attachment description: Untested nsDependentCString patch → Tested nsDependentCString patch
Attachment #569071 -
Flags: review?(benjamin)
Updated•13 years ago
|
Attachment #569071 -
Flags: review?(benjamin) → review+
Assignee | ||
Comment 26•13 years ago
|
||
Pushed changeset f79946f0bb2a to mozilla-central.
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Updated•13 years ago
|
Attachment #552383 -
Attachment is obsolete: true
You need to log in
before you can comment on or make changes to this bug.
Description
•