188328 - Reading a nsMIMEInputStream totally would cause a NS_BASE_STREAM_CLOSED exception

Status: NEW

(Core :: XPCOM, defect, P3)

Description

[Daniel Savard]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.3a) Gecko/20021212
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.3a) Gecko/20021212

In the code of xpcom/io/nsMultiplexInputStream.cpp, the NS_BASE_STREAM_CLOSED
exception is not considered as 'success'.  This would cause the following code
to fail in javascript (via a nsScriptableInputStream):
(for all sample below, inputStream is a nsMIMEInputStream)

This line will fail with a NS_BASE_STREAM_CLOSED exception
inputStream.read(inputStream.available())

But the following will work (but is very slow)
for (var i=0; i<inputStream.available(); i++) inputStream.read(1);

-----------------------
To correct this, all it is needed is to correct the read function of
xpcom/io/MultiplexInputStream:

This function:
--------------START ORIG---------------
/* [noscript] unsigned long read (in charPtr buf, in unsigned long count); */
NS_IMETHODIMP
nsMultiplexInputStream::Read(char * aBuf, PRUint32 aCount, PRUint32 *_retval)
{
    nsresult rv;
    PRUint32 len;

    mStreams.Count(&len);
    PRUint32 read = 0;
    while (mCurrentStream < len && aCount > 0) {
        nsCOMPtr<nsIInputStream> stream(do_QueryElementAt(&mStreams,
                                                          mCurrentStream));
        rv = stream->Read(aBuf, aCount, _retval);
        if (rv == NS_BASE_STREAM_WOULD_BLOCK) {
            *_retval = read;
            return read == 0 ? NS_BASE_STREAM_WOULD_BLOCK : NS_OK;
        }
        NS_ENSURE_SUCCESS(rv, rv);
        if (!*_retval) {
            // we're done with this stream, proceed to next
            ++mCurrentStream;
            mStartedReadingCurrent = PR_FALSE;
        }
        else {
            read += *_retval;
            aBuf += *_retval;
            aCount -= *_retval;
            mStartedReadingCurrent = PR_TRUE;
        }
    }
    *_retval = read;
    return NS_OK;
}
----------------STOP ORIG-----------------

Can become this one
----------------START V1------------------
/* [noscript] unsigned long read (in charPtr buf, in unsigned long count); */
NS_IMETHODIMP
nsMultiplexInputStream::Read(char * aBuf, PRUint32 aCount, PRUint32 *_retval)
{
    nsresult rv;
    PRUint32 len;

    mStreams.Count(&len);
    PRUint32 read = 0;
    while (mCurrentStream < len && aCount > 0) {
        nsCOMPtr<nsIInputStream> stream(do_QueryElementAt(&mStreams,
                                                          mCurrentStream));
        rv = stream->Read(aBuf, aCount, _retval);
        if (rv == NS_BASE_STREAM_WOULD_BLOCK) {
            *_retval = read;
            return read == 0 ? NS_BASE_STREAM_WOULD_BLOCK : NS_OK;
        }

        // Should accepd NS_BASE_STREAM_CLOSED
        if (rv == NS_BASE_STREAM_CLOSED) {
            // we're done with this stream, proceed to next
            ++mCurrentStream;
            mStartedReadingCurrent = PR_FALSE;
            continue;
        }

        NS_ENSURE_SUCCESS(rv, rv);
        if (!*_retval) {
            // we're done with this stream, proceed to next
            ++mCurrentStream;
            mStartedReadingCurrent = PR_FALSE;
        }
        else {
            read += *_retval;
            aBuf += *_retval;
            aCount -= *_retval;
            mStartedReadingCurrent = PR_TRUE;
        }
    }
    *_retval = read;
    return NS_OK;
}
----------------STOP V1-------------------

Or this one:
----------------START V2------------------
/* [noscript] unsigned long read (in charPtr buf, in unsigned long count); */
NS_IMETHODIMP
nsMultiplexInputStream::Read(char * aBuf, PRUint32 aCount, PRUint32 *_retval)
{
    nsresult rv;
    PRUint32 len;

    mStreams.Count(&len);
    PRUint32 read = 0;
    while (mCurrentStream < len && aCount > 0) {
        nsCOMPtr<nsIInputStream> stream(do_QueryElementAt(&mStreams,
                                                          mCurrentStream));
        rv = stream->Read(aBuf, aCount, _retval);
        if (rv == NS_BASE_STREAM_WOULD_BLOCK) {
            *_retval = read;
            return read == 0 ? NS_BASE_STREAM_WOULD_BLOCK : NS_OK;
        }

        // Should accepd NS_BASE_STREAM_CLOSED
        if (rv == NS_BASE_STREAM_CLOSED) { rv = NS_OK; }

        NS_ENSURE_SUCCESS(rv, rv);
        if (!*_retval) {
            // we're done with this stream, proceed to next
            ++mCurrentStream;
            mStartedReadingCurrent = PR_FALSE;
        }
        else {
            read += *_retval;
            aBuf += *_retval;
            aCount -= *_retval;
            mStartedReadingCurrent = PR_TRUE;
        }
    }
    *_retval = read;
    return NS_OK;
}
----------------STOP V2-------------------

This step (verify NS_BASE_STREAM_CLOSED) is already in the readSegment function
but not on the read function.  As the read function is the only one available to
script via ScriptableInputStream, this cause problem for reading a
MultiplexInputStream or MIMEInputStream (as it use MultiplexInputStream internaly).


Reproducible: Always

Steps to Reproduce:
Use the following code on the a page able to call the getPostData function
(comm.jar/content/communicator/contentAreaUtils.js).  This will work in a new
tab of PageInfo if the line "var postis = getPostData();" is changed for "var
postis = window.opener.getPostData();"

------------START------------
const JS_FILE_I_STREAM_CID             = "@mozilla.org/scriptableinputstream;1";
const JS_FILE_I_SCRIPTABLE_IN_STREAM   = "nsIScriptableInputStream";
const JS_FILE_InputStream  = new Components.Constructor
  ( JS_FILE_I_STREAM_CID, JS_FILE_I_SCRIPTABLE_IN_STREAM );
var postis = getPostData();
var postiss = postis.QueryInterface(Components.interfaces.nsISeekableStream);
postiss.seek(0,0);
var is = new JS_FILE_InputStream();
is.init(postiss);
dump("Available:" + is.available()+"\n");
dump("Read:" + is.read(is.available())+"\n");
-----------------STOP---------------

Actual Results:  
Get an NS_BASE_STREAM_CLOSED exception

Expected Results:  
Get all the data available in the input stream

if I change the following line in the 'Step to reproduce code':
  dump("Read:" + is.read(is.available())+"\n");
for the following lines:
  dump("Read:");
  while(is.available() > 0) dump(is.read(1);
  dump("\n");
this will works but very slowly.

I tested the two 'solution' above (V1 and V2).  However, I don't know which one
is more consistent with Mozilla developing pratice.

Comment 1

[Boris Zbarsky [:bzbarsky]]

ccing multiplexinputstream buddies

Comment 2

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

[e-mail]: looks good, could you produce a patch?

Comment 3

[Daniel Savard]

Attachment: Patch for the solution v1

Comment 4

[Daniel Savard]

Attachment: Patch for the solution v2

Comment 5

[Daniel Savard]

I sent two patch.  One for each proposed solution.  I don't know which one
follows the mozilla's programming pratice.  Only one of these patch is needed.

Comment 6

[Boris Zbarsky [:bzbarsky]]

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

Comment 7

[Boris Zbarsky [:bzbarsky]]

Let me see if I understand this correctly... we read some data from a stream and
it returns the data _and_ NS_BASE_STREAM_CLOSED?  Or does it return the data on
the first call to Read() and NS_BASE_STREAM_CLOSED on the second one?

Comment 8

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 111130 [details] [diff] [review]
Patch for the solution v1

r=sicking

I like this version better. You shouldn't rely on the values if an errorcode is
returned (for example, you should not rely on that *_retval is 0 if
NS_BASE_STREAM_CLOSED is returned)

Comment 9

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I think tt returns data the first X times and then returns
NS_BASE_STREAM_CLOSED. X >= 0. Or it might be that it always returns
NS_BASE_STREAM_CLOSED. In any case we should treat returning
NS_BASE_STREAM_CLOSED not as an error but rather that we have reached the end of
the stream and there is no more data available.

Comment 10

[Daniel Savard]

It returns the data and also NS_BASE_STREAM_CLOSED.
The patch I made is based on the for nsMultiplexInputStream::Read is based on the
code already present on nsMultiplexInputStream::ReadSegments.

Comment 11

[Boris Zbarsky [:bzbarsky]]

> It returns the data and also NS_BASE_STREAM_CLOSED.

That seems wrong... darin?

Comment 12

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

but it doesn't do it during _the_same_ call, right?

Comment 13

[Darin Fisher]

a nsIInputStream::Read implementation should return NS_OK and zero bytes read to
indicate EOF.  returning NS_BASE_STREAM_CLOSED is treated by the API just like
NS_ERROR_FAILURE.

Comment 14

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Then the code in ReadSegments should be removed afaict.

Daniel: why did the current behaviour cause you problem? Maybe the problem is in
the underlying datastream that returns NS_BASE_STREAM_CLOSED when it shouldn't

Comment 15

[Daniel Savard]

I need to get the inputStream from a Post request (postData).  This input stream
is 'nsMIMEInputStream' which use a nsMultiplexInputStream internaly.  In
javascript, the only function that we can use to get the data is Read (through
nsScriptableInputStream).  Trying to get all the available data will throw the
exception and I only get the data up to the exception.

The reason the InputStream works well in postData is because the Socket uses
ReadSegments instead of Read to get its data.  ReadSegments is able to work
around NS_BASE_STREAM_CLOSED unlike Read.

It is always possible to use inputStream.Read(1) (one byte at the time) but it
is very slow.  inputStream.Read(inputStream.available()) is very performant. 
And as the postData may be very big (file upload, big forms), this have a big
impact on performance when trying to get post Data (for the next version of the
LiveHTTPHeaders mozdev project).

I can't tell if this is the underlying stream that is wrong by returning
NS_BASE_STREAM_CLOSED.  I can only tell that the ReadSegments function in
nsMultiplexInputStream know that fact but the Read function doesn't.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Daniel, does the postdata include a file input?  Or just normal form inputs?  I
seem to recall that we close the file stream once we finish reading the file;
that could be the problem here....

Comment 17

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

jkeiser: you added the NS_BASE_STREAM_CLOSED-code as part of bug 126829, do you
know which stream that code originally came from?

Comment 18

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

bz: yes, the stream contains (or at least can contain) file-streams. The
question is however if they should be throwing this error-value or not

Comment 19

[Darin Fisher]

ooh.. in fact, nsFileInputStream::read is all wrong.  it should return NS_OK and
zero bytes read if mFD is null.  for all other methods, returning
NS_BASE_STREAM_CLOSED is valid.

Comment 20

[Daniel Savard]

After the comments from Darin Fisher, what should be patched ?
nsFileInputStream::read or nsMultiplexInputStream::Read ?

Comment 21

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

nsFileInputStream::read should be patched. I'm on it.

Comment 22

[Wladimir Palant]

Attachment: Patch for nsMultiplexInputStream::Available()

This bug hasn't been marked FIXED yet but it seems that the patch has been applied - nsMultiplexInputStream::Read() doesn't consider NS_BASE_STREAM_CLOSED an error. There is however the same issue with nsMultiplexInputStream::Available(), nsFileInputStream will return NS_BASE_STREAM_CLOSED here as well indicating EOF. My suggestion is to fix nsMultiplexInputStream::Available() in the same way nsMultiplexInputStream::Read() has been fixed.

Comment 23

[Darin Fisher]

See also bug 318193.  I have a patch there waiting for review that also touches nsMultiplexInputStream::Available.  nsIInputStream::Available is supposed to throw NS_BASE_STREAM_CLOSED if the stream has been closed.  It will throw a different exception if the stream was closed due to some error.

Perhaps this patch should be incorporated into the patch for bug 318193.

Comment 24

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 205908 [details] [diff] [review]
Patch for nsMultiplexInputStream::Available()

>+        if (rv == NS_BASE_STREAM_CLOSED) {
>+            rv = NS_OK;
>+            streamAvail = 0;
>+        }
>+        else
>+            NS_ENSURE_SUCCESS(rv, rv);

You don't really need the |else| here. But if you do want to it please use { }

r=me with that.

Comment 25

[Wladimir Palant]

Comment on attachment 205908 [details] [diff] [review]
Patch for nsMultiplexInputStream::Available()

I don't think brackets on a one line else are style in that file, so I would rather not change that.

Comment 26

[Darin Fisher]

Comment on attachment 205908 [details] [diff] [review]
Patch for nsMultiplexInputStream::Available()

I think it may be good to assert that "i == mCurrentStream" when we ignore an rv of NS_BASE_STREAM_CLOSED.  In fact, I might even say that we should make that a runtime test since we could be dealing with badly implemented streams.

Otherwise, this patch is fine, but I would encourage you to think about being a little more cautious here.