318193 - Input streams are not implemented consistently

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Darin Fisher]

Input streams are not implemented consistently

This is a spin-off from bug 316372.

Comment 1

[Darin Fisher]

Attachment: v1 patch

(same as the v1 patch attached to bug 316372 minus the nsInputStreamChannel.cpp changes.)

Comment 2

[Darin Fisher]

Would it help if I broke this patch up into smaller pieces?  I really want to get this landed as it is causing conflicts in my tree.  Thanks!

Comment 3

[Boris Zbarsky [:bzbarsky]]

I won't be able to review anything until at least the 21st, and more likely Jan 2, no matter the size.  :(

Comment 4

[Darin Fisher]

> I won't be able to review anything until at least the 21st, and more likely Jan
> 2, no matter the size.  :(

OK, thanks for the update.

Comment 5

[Christian :Biesinger (don't email me, ping me on IRC)]

I'll try to get to this tomorrow.

Comment 6

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 204500 [details] [diff] [review]
v1 patch

review of the stringstream.* part of the patch:

xpcom/io/nsStringStream.cpp
+protected:
     ~nsStringInputStream()

should you make it private, as it's nonvirtual?

+    // NOTE: We do not use nsCRT::strndup here because that does not handle
+    // null bytes in the middle of the given data.
...
+    memcpy(copy, data, dataLen + 1);

So if this is binary data, and the exact length was passed to us... won't this read past the given boundaries? In fact, why do this at all? There is no need for nulltermination of string data here, is there?

+    // We may be at end-of-file due to a Seek...

well, we could also be at EOF because we read all the data, right?

+    switch (whence) {
...
+    default:
+        return NS_ERROR_INVALID_ARG;

Might want to put an NS_ERROR here as well?

NS_NewCStringInputStream:
couldn't you use aStringToRead.BeginReading() and aStringToRead.Length() now?

nsIStringStream.idl:
+enum {
+    NS_STRINGINPUTSTREAM_COPY,
+    NS_STRINGINPUTSTREAM_DEPEND,
+    NS_STRINGINPUTSTREAM_ADOPT
+};

hm... why an anonymous enum, instead of using a named one here and as the argument?

+// If aMode is NS_STRINGINPUTSTREAM_ADOPT, then the resulting stream refers
+// directly to the given buffer (aStringToRead), and will free aStringToRead
+// once the stream is closed.  The caller is free to discard aStringToRead
+// after this function returns.

Hm.. you are using "discard" here and in the COPY docs, with two different meanings :) for COPY, the caller may free the data, but not here...

Comment 7

[Darin Fisher]

Thanks for the feedback.  Some comments:


> NS_NewCStringInputStream:
> couldn't you use aStringToRead.BeginReading() and aStringToRead.Length() now?

Yes, but the generated code is a bit less when you use the version of BeginReading that returns an iterator.


> nsIStringStream.idl:
> +enum {
> +    NS_STRINGINPUTSTREAM_COPY,
> +    NS_STRINGINPUTSTREAM_DEPEND,
> +    NS_STRINGINPUTSTREAM_ADOPT
> +};
> 
> hm... why an anonymous enum, instead of using a named one here and as the
> argument?

No good reason.  I'm not sure I like nsStringInputStreamMode as the enum type.  What do you think about a general purpose enum type like this:

  enum nsAssignmentType {
    NS_ASSIGNMENT_COPY,
    NS_ASSIGNMENT_DEPEND,
    NS_ASSIGNMENT_ADOPT
  };

It seems like something that might be useful elsewhere.

Comment 8

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 204500 [details] [diff] [review]
v1 patch

nsDOMParser.cpp:
nice removals in nsDOMParser :)

+  rv = listener->OnStartRequest(parserChannel, nsnull);

hmm, passing a channel that's not pending to OnStartRequest... but then, the code always did that

+  rv = listener->OnStopRequest(parserChannel, nsnull, status);
 
   if (NS_FAILED(rv)) {

I'd think it should ignore it, although I guess nsIRequestObserver does allow this kind of thing... anyway, not something you changed either

Hm... so if nsStringStream.h can be used outside of xpcom (you use it in netwerk/streamconv/converters/nsHTTPCompressConv.cpp at least), then why not move the %{C++ %} block from the IDL to that file?

nice to see nsByteArrayInputStream gone

also nice to see improved comments on nsIInputStream! :)

Hm... it looks like nsStringStream.cpp does get Read/ReadSegments wrong when it's closed - it returns NS_BASE_STREAM_CLOSED, contrary to most other streams and to this documentation.

nsIInputStream.idl:
+     * @throws NS_BASE_STREAM_CLOSED if the stream is closed normally or at
+     *   end-of-file

hm.. does that imply that streams should close themselves when at eof? or does it just refer to things like file stream's CLOSE_ON_EOF?

xpcom/io/nsIScriptableInputStream.idl
+ * nsIScriptableInputStream provides scriptable access to a nsIInputStream

not "an nsIInputStream"?

should read's documentation mention that on EOF, it will return an empty string?

xpcom/io/nsMultiplexInputStream.cpp
-    nsSupportsArray mStreams;
+    nsCOMArray<nsIInputStream> mStreams;

nice :)

xpcom/io/nsMultiplexInputStream.cpp
nsMultiplexInputStream::Read(char * aBuf, PRUint32 aCount, PRUint32 *_retval)
+    // It is tempting to implement this method in terms of ReadSegments

really? does it matter if Read() copies, or the writer called by ReadSegments?


Hm, I wonder why the multiplex stream goes to such great effort to read exactly aCount bytes of data... nsIInputStream allows it to return less.

why did you not implement Read using ReadSegments here?

xpcom/io/nsStorageStream.cpp
+    LOG(("Creating nsStorageStream [%x].\n", this));

not %p? seems like %x would be wrong on 64-bit archs. happens various times in this file, might want to fix it while touching these lines.


modules/libjar/nsJARInputStream.cpp
 nsJARInputStream::Read(char* buf, PRUint32 count, PRUint32 *bytesRead)
 {
     if (!mJAR)
         return NS_BASE_STREAM_CLOSED;

shouldn't that return a zero count and NS_OK?

modules/libpr0n/encoders/jpeg/nsJPEGEncoder.cpp
+// Input streams that do not implement nsIAsyncInputStream should be threadsafe
+// so that they may be used with nsIInputStreamPump and nsIInputStreamChannel,
+// which read such a stream on a background thread.

maybe that'd be a good comment to put in some general input stream documentation. I guess nsIInputStream isn't a good place, though (as nsIInputStreamPump is necko...)

modules/libpr0n/encoders/jpeg/nsJPEGEncoder.cpp
 NS_IMETHODIMP nsJPEGEncoder::ReadSegments(nsWriteSegmentFun aWriter, void *aClosure, PRUint32 aCount, PRUint32 *_retval)
+  if (!mImageBuffer)
+    return NS_BASE_STREAM_CLOSED;

again... not zero/NS_OK?
(PNG encoder too)


This is an awesome patch. r=biesi with those changes, and sorry about the delay.

Comment 9

[Christian :Biesinger (don't email me, ping me on IRC)]

> What do you think about a general purpose enum type like this:

Interesting idea. I think I like it. Where to put it, though?

Comment 10

[Darin Fisher]

> Interesting idea. I think I like it. Where to put it, though?

I was considering nsMemory.h ... what do you think?

Comment 11

[Darin Fisher]

> +  rv = listener->OnStartRequest(parserChannel, nsnull);
> 
> hmm, passing a channel that's not pending to OnStartRequest... but then, the
> code always did that

Yeah, I think it's okay here since the channel is only being passed to the parser.  If there were a load group involved, then I would be more concerned about the isPending flag.


> +  rv = listener->OnStopRequest(parserChannel, nsnull, status);
> 
>    if (NS_FAILED(rv)) {
> 
> I'd think it should ignore it, although I guess nsIRequestObserver does allow
> this kind of thing... anyway, not something you changed either

Yeah, I think I'm going to leave it alone.  I agree that we should be able to ignore the return value from OnStopRequest, but to be sure that this code is adhering to that rule would require some additional investigation that I'd prefer to punt on for now.


> Hm... so if nsStringStream.h can be used outside of xpcom (you use it in
> netwerk/streamconv/converters/nsHTTPCompressConv.cpp at least), then why not
> move the %{C++ %} block from the IDL to that file?

Good suggestion.  Done.


> nsIInputStream.idl:
> +     * @throws NS_BASE_STREAM_CLOSED if the stream is closed normally or at
> +     *   end-of-file
> 
> hm.. does that imply that streams should close themselves when at eof? or 
> does it just refer to things like file stream's CLOSE_ON_EOF?

Whether a stream closes itself when it reaches EOF or not depends on the implementation.  If you compare a socket to a file, you can see how it would differ depending on the implementation.  When a socket stream reaches EOF, it means that the TCP layer has shutdown the connection, so there is technically no stream anymore.  However, when a file stream reaches EOF, it can simply be rewound to be beginning, so the stream shouldn't just close on EOF (unless it is told to do so).


> should read's documentation mention that on EOF, it will return an empty
> string?

Good suggestion.


> +    // It is tempting to implement this method in terms of ReadSegments
> 
> really? does it matter if Read() copies, or the writer called by ReadSegments?

The point is that some of the inner streams may not implement ReadSegments, and the consumer of the multiplexed input stream knows what kinds of stream he put in the container, and so he can know to only call Read if some of the inner streams don't implement ReadSegments.  We could also teach the multiplexed input stream to try ReadSegments and then failover to Read if not supported, but that would result in even more complex code.


> Hm, I wonder why the multiplex stream goes to such great effort to read 
> exactly aCount bytes of data... nsIInputStream allows it to return less.

Yes, it may return less, but I think it makes sense to fill the given buffer as much as possible.  That probably helps reduce memory fragmentation.


Thanks for catching the places where I didn't fixup Read and ReadSegments to return NS_OK and zero bytes read.

Comment 12

[Darin Fisher]

Attachment: v1.1 patch

To boris with love.

Comment 13

[Darin Fisher]

BTW, I spoke with biesi and bsmedberg over IRC about adding nsAssignmentType to nsMemory.h.  They seemed to be fine with the idea.  The v1.1 patch includes that change.

Comment 14

[Christian :Biesinger (don't email me, ping me on IRC)]

for my Read/ReadSegments comment... what I meant was that I don't see how using ReadSegments would improve anything. As this is Read(), some function has to copy anyway. right?

Comment 15

[Darin Fisher]

> for my Read/ReadSegments comment... what I meant was that I don't see how using
> ReadSegments would improve anything. As this is Read(), some function has to
> copy anyway. right?

Indeed, implementing Read in terms of ReadSegments does not reduce the number of buffer copies.  It only reduces the amount of code ;-)

Comment 16

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 206530 [details] [diff] [review]
v1.1 patch

>Index: embedding/tests/wxEmbed/GeckoProtocolHandler.cpp
>+            rv = NS_NewByteInputStream(getter_AddRefs(mContentStream),
>+                                       (char *) mData, mContentLength,
>+                                       NS_ASSIGNMENT_ADOPT);
>             if (NS_FAILED(rv)) return rv;

So this leaks mData if NS_NewByteInputStream fails, right?  I realize this is a test, so that's not a huge deal, but may be worth fixing up.

>Index: modules/libpr0n/encoders/jpeg/nsJPEGEncoder.cpp

>+// Input streams that do not implement nsIAsyncInputStream should be threadsafe

That should be documented on nsIInputStream, then.  And we should audit our existing stream impls. But really, perhaps we should just make it clear that streams should typically be threadsafe?

> NS_IMETHODIMP nsJPEGEncoder::ReadSegments(nsWriteSegmentFun aWriter, void *aClosure, PRUint32 aCount, PRUint32 *_retval)
>+  if (aCount > maxCount)
>+    aCount = maxCount;

Why not just aCount = PR_MAX(aCount, maxCount)?  I guess either way's fine.

> +  *_retval = PR_FALSE;  // We don't implement nsIAsyncInputStream

That's not documented on nsIInputStream that I can see.  It probably should be.  I certainly wasn't aware of this restriction.  And perhaps we need to audit all of our input stream impls for this issue...

>Index: modules/libpr0n/encoders/png/nsPNGEncoder.cpp
>+NS_IMETHODIMP nsPNGEncoder::ReadSegments(nsWriteSegmentFun aWriter,
>+  if (aCount > maxCount)
>+    aCount = maxCount;

Again, perhaps PR_MAX.

> NS_IMETHODIMP nsPNGEncoder::IsNonBlocking(PRBool *_retval)

Is there a reason this wasn't changed the way the JPEG version was?

>Index: extensions/xmlextras/base/src/nsDOMParser.cpp
>+    rv = listener->OnDataAvailable(parserChannel, nsnull, stream, 0,
>+                                   contentLength);

We never look at that rv, so perhaps we shouldn't even be bothering to assign to it here?

>Index: xpcom/io/nsMultiplexInputStream.cpp
> nsMultiplexInputStream::IsNonBlocking(PRBool *aNonBlocking)
>+        // (except that we don't implement nsIAsyncInputStream, so there's
>+        //  not much for the caller to do if Read returns "would block")

Caller can wait a bit and try again....

>Index: xpcom/io/nsStringStream.cpp
>+nsStringInputStream::ReadSegments(nsWriteSegmentFun writer, void *closure,
>     if (maxCount == 0) {
>         *result = 0;
>         return NS_OK;
>     }

After this block, add:

  NS_ASSERTION(mData, "Must have data if maxCount != 0");

or something?

As far as I can tell, the following classes need additional modifications to comply with the newly-clarified API:

  nsBufferedInputStream (read() and readSegments() shouldn't be throwing
     NS_BASE_STREAM_CLOSED, read() can call readSegments() if !mBufferDisabled,
     available() should throw NS_BASE_STREAM_CLOSED as needed).
  nsFileUploadContentStream (readSegments() throws status if the stream is
     closed; it probably should not).
  FileImpl (though perhaps it's just better to not touch that stuff...)
  nsPipeInputStream (tell() should throw if stream is closed)

And the following classes might need such modifications:

  mozSqlResultStream (calls the readSegments callback with zero length
     sometimes, close() is a no-op, etc).
  nsProxyLoadStream (in nsXULDocument.cpp; calls the readSegments callback
     with zero length sometimes, close() is no-op)
  ProxyStream (in nsRDFXMLDataSource.cpp; calls the readSegments callback
     with zero length sometimes, close() is no-op)
  mozXMLTermStream (pretty bogus close() impl;  Might not care so much about
     this one).
  CPluginInputStream (in /modules/plugin; again may not care too much since
     it's just an example that's missing some method impls so it shouldn't
     even link as far as I can tell).
  CPluginInputStream (in /plugin/oji)

Comment 17

[Darin Fisher]

> >+// Input streams that do not implement nsIAsyncInputStream should be threadsafe
> 
> That should be documented on nsIInputStream, then.  And we should audit our
> existing stream impls. But really, perhaps we should just make it clear that
> streams should typically be threadsafe?

No, it has more to do with how nsIInputStream may be used with other subsystems such as Necko.  The interface, nsIInputStream, shouldn't really put forth such restrictions.  The same goes for IsNonBlocking and nsIAsyncInputStream.    nsIAsyncInputStream probably should document its connection to IsNonBlocking.  I don't think nsIInputStream should document that relation because as you said, an input stream may in fact be non-blocking and not implement nsIAsyncInputStream.  And, somewhere in Necko documentation, probably the nsIInputStreamPump (or nsIInputStreamChannel), we should talk about the significance of thread-safety, IsNonBlocking, and nsIAsyncInputStream.  I think there is already some mention of those things there, but perhaps it could be improved.


> >+  if (aCount > maxCount)
> >+    aCount = maxCount;
> 
> Why not just aCount = PR_MAX(aCount, maxCount)?  I guess either way's fine.

A matter of taste I suppose... though nice to only re-assign aCount if necessary.


> > NS_IMETHODIMP nsPNGEncoder::IsNonBlocking(PRBool *_retval)
> 
> Is there a reason this wasn't changed the way the JPEG version was?

Error on my part.  Glad you filed that bug about merging the two impls.


> > nsMultiplexInputStream::IsNonBlocking(PRBool *aNonBlocking)
> >+        // (except that we don't implement nsIAsyncInputStream, so there's
> >+        //  not much for the caller to do if Read returns "would block")
> 
> Caller can wait a bit and try again....

Right, "not much" in the way of anything good... ;-)


> As far as I can tell, the following classes need additional modifications to
> comply with the newly-clarified API:

OK, I will go through that list and update them accordingly.

Comment 18

[Boris Zbarsky [:bzbarsky]]

> I don't think nsIInputStream should document that relation because as you said,
> an input stream may in fact be non-blocking and not implement
> nsIAsyncInputStream.

I guess I'm not sure why we're changing the isNonBlocking impl of the JPEG/PNG stuff and changing whether refcounting is threadsafe just because it doesn't implement nsIAsyncInputStream, then...  Put another way, it seems to me that nsIInputStream should have whatever documentation is needed to correctly implement the interface.  If it does, then I'm not sure why the image encoder changes are needed; if it does not, we should fix it.

Comment 19

[Darin Fisher]

> I guess I'm not sure why we're changing the isNonBlocking impl of the 
> JPEG/PNG stuff and changing whether refcounting is threadsafe just because it 
> doesn't implement nsIAsyncInputStream, then...

I agree that the comment I added above the NS_IMPL_THREADSAFE macro could be better.  The real reason for changing it is that I wanted to make sure that the encoders worked properly if someone wanted to use them with nsIInputStreamChannel (or nsIInputStreamPump).  I was simply trying to avoid the random crashes that would occur later on if someone failed to realize the necessary implementation changes required to use the encoder with Necko.


> Put another way, it seems to me that
> nsIInputStream should have whatever documentation is needed to correctly
> implement the interface.  If it does, then I'm not sure why the image encoder
> changes are needed; if it does not, we should fix it.

OK, here's the deal... nsIInputStream does not have to be implemented in a thread-safe way to be useful in some contexts.  Indeed, there is no such requirement for the way these encoders are used today.  When used with nsIInputStreamPump, the input stream implementation has additional contracts placed on it.  I don't see why it is correct to make those additional contracts apply to all nsIInputStream implementations.

Perhaps I'll add some comments to nsIInputStream::IsNonBlocking saying more about what it means to be non-blocking and I'll mention nsIAsyncInputStream (not as a requirement but as a suggestion).  Likewise, I'll mention somewhere that many input stream consumers expect to be able to read an input stream from a thread other than the thread on which it was created.  I'll do the same for nsIOutputStream.

Comment 20

[Boris Zbarsky [:bzbarsky]]

> Perhaps I'll add some comments to nsIInputStream::IsNonBlocking ...

Sounds like a plan.  I just want to make sure we don't run into similar issues in the future, basically.

Comment 21

[Darin Fisher]

+// Input streams that do not implement nsIAsyncInputStream should be threadsafe
+// so that they may be used with nsIInputStreamPump and nsIInputStreamChannel,
+// which read such a stream on a background thread.
+NS_IMPL_THREADSAFE_ISUPPORTS2(nsJPEGEncoder, imgIEncoder, nsIInputStream)

Wait a second... Boris: why was this comment confusing?  (You only quoted the first line, and I forgot that I had written more.)  Clearly, I'm not saying that the JPEG/PNG encoder should be threadsafe because it implements nsIInputStream.  I'm saying that it should be threadsafe so that it may be used with those Necko interfaces.

Comment 22

[Boris Zbarsky [:bzbarsky]]

But that blanket statement is true of all input streams; I guess the question is why this stream should have to worry about being used with nsIInputStreamPump and streams in general do not.

Put another way, how could Brett have known to make this stream threadsafe when he wrote this code?

Comment 23

[Darin Fisher]

> But that blanket statement is true of all input streams; I guess the question
> is why this stream should have to worry about being used with
> nsIInputStreamPump and streams in general do not.

I think the statement is only true when the nsIInputStream implementation may be used with other, unknown modules in the future.  Since the encoders use nsIInputStream as their API, it stands to reason that someone might use an encoder with nsIInputStreamPump.


> Put another way, how could Brett have known to make this stream threadsafe 
> when he wrote this code?

Yeah, that is definitely the main concern here.  I've gone ahead and added some comments to nsIInputStream/nsIOutputStream about these issues.

Comment 24

[Boris Zbarsky [:bzbarsky]]

Ah, fair enough.  If you have full control over how the input stream is used, then there's a lot less in the way of restrictions, I agree.

Thanks for adding those comments!

Comment 25

[Darin Fisher]

nsFileUploadContentStream
nsBufferedInputStream
  - fixed

nsPipeInputStream
  - implementing Tell properly meant entering the pipe's monitor... ugh :(
nsProxyLoadStream
  - this class is completely unused, so i just removed it
ProxyStream
  - this class can be replaced with NS_NewBufferedInputStream

FileImpl
mozSqlResultStream
mozXMLTermStream
  - yuck... i don't want to touch 'em... and the latter two aren't even part of any mozilla products.

CPluginInputStream
  - doesn't implement nsIInputStream

Comment 26

[Darin Fisher]

Attachment: v1.2 patch

Comment 27

[Darin Fisher]

Attachment: interdiff v1.1 - v1.2

Boris: Here are the differences between the v1.1 and v1.2 patch for easier reviewing.

Comment 28

[Boris Zbarsky [:bzbarsky]]

(In reply to comment #25)
> nsProxyLoadStream
>   - this class is completely unused, so i just removed it
> ProxyStream
>   - this class can be replaced with NS_NewBufferedInputStream

Nice!

> FileImpl
> mozSqlResultStream
> mozXMLTermStream
>   - yuck... i don't want to touch 'em... and the latter two aren't even part
> of any mozilla products.

In the case of mozSqlResultStream that might change, no?  I thought we were going to start using that stuff for history, etc.  Separate bug on it is fine, I guess; with blocking1.9 requested.  I agree that we don't really want to touch the other two.

> CPluginInputStream
>   - doesn't implement nsIInputStream

Good catch.  I saw that it inherited from nsIInputStream, but didn't check the QI impl.

Comment 29

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 207309 [details] [diff] [review]
v1.2 patch

>+++ extensions/Makefile.in	1 Jan 2006 19:13:23 >+DIRS		= $(MOZ_EXTENSIONS) layout-debug

Is that meant to be part of this patch?

>+++ content/html/document/src/nsHTMLDocument.h	1 Jan 
2006 19:13:26 -0000

And the changes in this file?

With those two changes removed or explained, sr=bzbarsky

Comment 30

[Christian :Biesinger (don't email me, ping me on IRC)]

>In the case of mozSqlResultStream that might change, no?  I thought we were
>going to start using that stuff for history, etc. 

That'll use storage, not extension/sql, right?

Comment 31

[Boris Zbarsky [:bzbarsky]]

> That'll use storage, not extension/sql, right?

No idea.  I've sort of lost track of the new modules we're adding.

Comment 32

[Darin Fisher]

> That'll use storage, not extension/sql, right?

Right, and storage == mozilla/storage

Boris: Yeah, those two changes won't be checked in.  Thanks for the code review!

Comment 33

[Darin Fisher]

fixed-on-trunk

Comment 34

[James Ross]

So am I right in thinking I have to convert my code to nsIStringStream from nsIByteArrayInputSteam, after the later's unceremonious removal? Or maybe fight with nsStorageStream?

I have a byte* of data and need to read it through nsIBinaryInputStream - which nsIByteArrayInputSteam was perfect for.

Comment 35

[Bob Clary [:bc] (inactive)]

I am getting a flood of these assertions with a winxp debug build from today.

###!!! ASSERTION: OnDataAvailable implementation consumed no data: 'Error', file c:/work/mozilla/builds/ff/trunk/mozilla/netwerk/base/src/nsInputStreamPump.cpp, line 459

due to this checkin?

Comment 36

[Frank Wein [:mcsmurf]]

Maybe this also caused Bug 322116?

Comment 37

[Darin Fisher]

> So am I right in thinking I have to convert my code to nsIStringStream from
> nsIByteArrayInputSteam, after the later's unceremonious removal? Or maybe 
> fight with nsStorageStream?

nsIStringInputStream provides the exact same functionality as nsIByteArrayInputStream.  Use the AdoptData function (or NS_NewByteInputStream with NS_ASSIGNMENT_ADOPT) to emmulate NS_NewByteArrayInputStream.

Comment 38

[Darin Fisher]

> I am getting a flood of these assertions with a winxp debug build from today.
> 
> ###!!! ASSERTION: OnDataAvailable implementation consumed no data: 'Error',
> file
> c:/work/mozilla/builds/ff/trunk/mozilla/netwerk/base/src/nsInputStreamPump.cpp,
> line 459
> 
> due to this checkin?

Yeah, it must be due to this checkin... my guess is that the change to nsPipeInputStream::Tell is causing the problem.  It is probably on the last ODA event that assertion occurs.  For some reason, I'm not seeing this assertion in my debug winxp build, but it could very well be dependent on a thread race.  Do you have a multi-cpu/multi-core system?

Comment 39

[Bob Clary [:bc] (inactive)]

No, I see this on a Pentium M. I'm rebuilding on another machine that is multi-cpu in case that will help.

Comment 40

[Darin Fisher]

bclary: yeah, I'm also using an M processor.. looking at the code in nsInputStreamPump.cpp, I think the assertion could depend on uninitialized memory in some cases, which would explain why you see the assertion and I do not.  I'm fixing that bug in my tree to see if i get the assertion.

Comment 41

[Darin Fisher]

Yup, variables being un-initialized was masking the assertion on my system.  Working on a fix to the assertion...

Comment 42

[Darin Fisher]

Attachment: fix input stream pump assertion [fixed-on-trunk]

This fixes the problem bclary reported.  The solution I chose is to simply bypass the offset comparision if Tell returns an error.  That seems like the most robust solution to me, and it does the right thing in the case of a pipe.

Comment 43

[Darin Fisher]

Also, note that with this patch I've removed use of nsInt64/nsUint64 in favor of using PRInt64/PRUint64 directly since that is perfectly fine now-a-days.  (We do it in NSPR and elsewhere in Necko.)

Comment 44

[Darin Fisher]

Comment on attachment 207422 [details] [diff] [review]
fix input stream pump assertion [fixed-on-trunk]

OK, I went ahead and landed this patch on the trunk since w/o it we sometimes crash on startup or fail in other wonderous ways.  Boris, please let me know if you see anything else that should be tweaked with this patch.  Thanks!

Comment 45

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 207422 [details] [diff] [review]
fix input stream pump assertion [fixed-on-trunk]

>Index: nsInputStreamPump.cpp
>+            PRInt64 offsetBefore = 0;
>             nsCOMPtr<nsISeekableStream> seekable = do_QueryInterface(mAsyncStream);
>             if (seekable)
>                 seekable->Tell(&offsetBefore);

So should we assert that this succeeds?  Or are we assuming that failure will not modify the out param?  If the latter, I'd really rather we didn't (and set it to 0 ourselves on failure).

With that nit picked, sr=bzbarsky

Comment 46

[Darin Fisher]

Attachment: fixup nsInputStreamPump.cpp according to bz's review comments [fixed-on-trunk]

I also made some small benign changes to the way we get the current event queue.

Comment 47

[Darin Fisher]

Comment on attachment 207437 [details] [diff] [review]
fixup nsInputStreamPump.cpp according to bz's review comments [fixed-on-trunk]

I landed this patch on the trunk.

Comment 48

[Ruediger Lahl]

(In reply to comment #47)

> (From update of attachment 207437 [details] [diff] [review] [edit])
> I landed this patch on the trunk.

Testing SM build 2006010314 and it looks like the patch works and solve the problem from Bug 322116

Thank you.

Comment 49

[Stephen Donner [:stephend] Not actively reading bugmail]

*** Bug 322292 has been marked as a duplicate of this bug. ***

Comment 50

[Stephen Donner [:stephend] Not actively reading bugmail]

*** Bug 322360 has been marked as a duplicate of this bug. ***

Comment 51

[Boris Zbarsky [:bzbarsky]]

Adding that |data| attribute doesn't seem so happy -- see bug 322314.

Comment 52

[Bill Gianopoulos [:WG9s]]

This appears to have somehow caused an interaction with the Live HTTP Headers extension that triggers the issue in bug 323407.

Comment 53

[Mike Shaver (:shaver emeritus)]

So, hey, just throwing this out there...

Would it be feasible to take this patch -- except for the one API removal bit -- onto 1.8.1?  It seems pretty well-baked, improves consistency in a way that pleases me greatly, and fixes at least one caller-error crash bug (passing NULL to stringstream's SetData).