545673 - NS_InputStreamIsBuffered should initialize count or TestInputStream should set the count to zero

Status: NEW

(Core :: XPCOM, defect)

Description

[John J. Barton]

I have built FF 3.6 with DEBUG and I am trying to get Chromebug+Firebug to run on it without asserts.

I assert here:
---
     nsresult rv = tee->mWriter(in, tee->mClosure, fromSegment, offset, count, writeCount);
    if (NS_FAILED(rv) || (*writeCount == 0)) {
        NS_ASSERTION((NS_FAILED(rv) ? (*writeCount == 0) : PR_TRUE),
                "writer returned an error with non-zero writeCount");
        return rv;
    }
---
in nsInputStreamTee.cpp. The reason is that mWriter is
---
static NS_METHOD
TestInputStream(nsIInputStream *inStr,
                void *closure,
                const char *buffer,
                PRUint32 offset,
                PRUint32 count,
                PRUint32 *countWritten)
{
    PRBool *result = static_cast<PRBool *>(closure);
    *result = PR_TRUE;
    return NS_ERROR_ABORT;  // don't call me anymore
}
---
The rv will be NS_ERROR_ABORT, but the writeCount will be what ever garbage value happened to be in that location? 

There seems to be two ways to fix this.

1) set writeCount to zero in TestInputStream
http://mxr.mozilla.org/mozilla1.9.2/source/xpcom/io/nsStreamUtils.cpp#695
or
2) initialize 'n' to zero in
http://mxr.mozilla.org/mozilla1.9.2/source/xpcom/io/nsStreamUtils.cpp#702

I think both should be done.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Or the assert should be fixed to not assert bogus things about out params of functions that return error?

Comment 2

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

Well, the point of the assert is to catch callees who want to consume some data of the stream but then return an error. That doesn't seem entirely unreasonable, and also, this is not an xpcom function - just a C++ callback that happens to be defined in IDL.

I think 2) should be done, 1) doesn't seem necessary to me.

Comment 3

[John J. Barton]

Attachment: (trivial) patch for 1.9.2

I've been running my local copy of FF 3.6 with this patch. WFM ;-)

Comment 4

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

I'm sorry, I didn't read comment 0 carefully enough when I said 2) should be done. What I meant was that nsInputStreamTee::WriteSegmentFun should initialize *writeCount before calling mWriter (not nsStreamUtils)

Comment 5

[John J. Barton]

Attachment: (trivial) patch for 1.9.2

per comment 4, a refreshed patch

Comment 6

[John J. Barton]

I guess I need to do something to cause this to move forward, but I don't know what.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Nathan, could you check whether this still makes sense (I suspect it does) and get it landed if so?