Bug 1120046 - Improve error reporting in nsBufferedStreams.cpp, r=honzab

netwerk/base/nsBufferedStreams.cpp

@@ -75,8 +75,9 @@ nsBufferedStream::Init(nsISupports* stream, uint32_t bufferSize)
     mBufferStartOffset = 0;
     mCursor = 0;
     mBuffer = new (mozilla::fallible) char[bufferSize];
-    if (mBuffer == nullptr)
+    if (mBuffer == nullptr) {
         return NS_ERROR_OUT_OF_MEMORY;
+    }
     return NS_OK;
 }
 
@@ -97,8 +98,9 @@ nsBufferedStream::Close()
         static FILE *tfp;
         if (!tfp) {
             tfp = fopen("/tmp/bufstats", "w");
-            if (tfp)
+            if (tfp) {
                 setvbuf(tfp, nullptr, _IOLBF, 0);
+            }
         }
         if (tfp) {
             fprintf(tfp, "seeks within buffer:    %u\n",
@@ -126,16 +128,22 @@ nsBufferedStream::Close()
 NS_IMETHODIMP
 nsBufferedStream::Seek(int32_t whence, int64_t offset)
 {
-    if (mStream == nullptr)
+    if (mStream == nullptr) {
         return NS_BASE_STREAM_CLOSED;
-    
+    }
+
     // If the underlying stream isn't a random access store, then fail early.
     // We could possibly succeed for the case where the seek position denotes
     // something that happens to be read into the buffer, but that would make
     // the failure data-dependent.
     nsresult rv;
     nsCOMPtr<nsISeekableStream> ras = do_QueryInterface(mStream, &rv);
-    if (NS_FAILED(rv)) return rv;
+    if (NS_FAILED(rv)) {
+#ifdef DEBUG
+        NS_ERROR("mStream doesn't QI to nsISeekableStream");
+#endif
+        return rv;
+    }
 
     int64_t absPos = 0;
     switch (whence) {
@@ -173,10 +181,20 @@ nsBufferedStream::Seek(int32_t whence, int64_t offset)
     METER(bufstats.mBufferReadUponSeek += mCursor);
     METER(bufstats.mBufferUnreadUponSeek += mFillPoint - mCursor);
     rv = Flush();
-    if (NS_FAILED(rv)) return rv;
+    if (NS_FAILED(rv)) {
+#ifdef DEBUG
+        NS_WARNING("(debug) Flush returned error within nsBufferedStream::Seek, so we exit early.");
+#endif
+        return rv;
+    }
 
     rv = ras->Seek(whence, offset);
-    if (NS_FAILED(rv)) return rv;
+    if (NS_FAILED(rv)) {
+#ifdef DEBUG
+        NS_WARNING("(debug) Error: ras->Seek() returned error within nsBufferedStream::Seek, so we exit early.");
+#endif
+        return rv;
+    }
 
     mEOF = false;
 
@@ -202,7 +220,9 @@ nsBufferedStream::Seek(int32_t whence, int64_t offset)
         int64_t tellPos;
         rv = ras->Tell(&tellPos);
         mBufferStartOffset = tellPos;
-        if (NS_FAILED(rv)) return rv;
+        if (NS_FAILED(rv)) {
+            return rv;
+        }
     }
     else {
         mBufferStartOffset = absPos;
@@ -218,9 +238,10 @@ nsBufferedStream::Seek(int32_t whence, int64_t offset)
 NS_IMETHODIMP
 nsBufferedStream::Tell(int64_t *result)
 {
-    if (mStream == nullptr)
+    if (mStream == nullptr) {
         return NS_BASE_STREAM_CLOSED;
-    
+    }
+
     int64_t result64 = mBufferStartOffset;
     result64 += mCursor;
     *result = result64;
@@ -230,16 +251,21 @@ nsBufferedStream::Tell(int64_t *result)
 NS_IMETHODIMP
 nsBufferedStream::SetEOF()
 {
-    if (mStream == nullptr)
+    if (mStream == nullptr) {
         return NS_BASE_STREAM_CLOSED;
-    
+    }
+
     nsresult rv;
     nsCOMPtr<nsISeekableStream> ras = do_QueryInterface(mStream, &rv);
-    if (NS_FAILED(rv)) return rv;
+    if (NS_FAILED(rv)) {
+        return rv;
+    }
 
     rv = ras->SetEOF();
-    if (NS_SUCCEEDED(rv))
+    if (NS_SUCCEEDED(rv)) {
         mEOF = true;
+    }
+
     return rv;
 }
 
@@ -272,8 +298,9 @@ nsBufferedInputStream::Create(nsISupports *aOuter, REFNSIID aIID, void **aResult
     NS_ENSURE_NO_AGGREGATION(aOuter);
 
     nsBufferedInputStream* stream = new nsBufferedInputStream();
-    if (stream == nullptr)
+    if (stream == nullptr) {
         return NS_ERROR_OUT_OF_MEMORY;
+    }
     NS_ADDREF(stream);
     nsresult rv = stream->QueryInterface(aIID, aResult);
     NS_RELEASE(stream);
@@ -292,10 +319,25 @@ nsBufferedInputStream::Close()
     nsresult rv1 = NS_OK, rv2;
     if (mStream) {
         rv1 = Source()->Close();
+#ifdef DEBUG
+        if (NS_FAILED(rv1)) {
+            NS_WARNING("(debug) Error: Source()->Close() returned error (rv1) in bsBuffedInputStream::Close().");
+        };
+#endif
         NS_RELEASE(mStream);
     }
+
     rv2 = nsBufferedStream::Close();
-    if (NS_FAILED(rv1)) return rv1;
+
+#ifdef DEBUG
+    if (NS_FAILED(rv2)) {
+        NS_WARNING("(debug) Error: nsBufferedStream::Close() returned error (rv2) within nsBufferedInputStream::Close().");
+    };
+#endif
+
+    if (NS_FAILED(rv1)) {
+        return rv1;
+    }
     return rv2;
 }
 
@@ -338,8 +380,9 @@ nsBufferedInputStream::ReadSegments(nsWriteSegmentFun writer, void *closure,
 {
     *result = 0;
 
-    if (!mStream)
+    if (!mStream) {
         return NS_OK;
+    }
 
     nsresult rv = NS_OK;
     while (count > 0) {
@@ -358,8 +401,9 @@ nsBufferedInputStream::ReadSegments(nsWriteSegmentFun writer, void *closure,
         }
         else {
             rv = Fill();
-            if (NS_FAILED(rv) || mFillPoint == mCursor)
+            if (NS_FAILED(rv) || mFillPoint == mCursor) {
                 break;
+            }
         }
     }
     return (*result > 0) ? NS_OK : rv;
@@ -368,16 +412,18 @@ nsBufferedInputStream::ReadSegments(nsWriteSegmentFun writer, void *closure,
 NS_IMETHODIMP
 nsBufferedInputStream::IsNonBlocking(bool *aNonBlocking)
 {
-    if (mStream)
+    if (mStream) {
         return Source()->IsNonBlocking(aNonBlocking);
+    }
     return NS_ERROR_NOT_INITIALIZED;
 }
 
 NS_IMETHODIMP
 nsBufferedInputStream::Fill()
 {
-    if (mBufferDisabled)
+    if (mBufferDisabled) {
         return NS_OK;
+    }
     NS_ENSURE_TRUE(mStream, NS_ERROR_NOT_INITIALIZED);
 
     nsresult rv;
@@ -394,10 +440,13 @@ nsBufferedInputStream::Fill()
 
     uint32_t amt;
     rv = Source()->Read(mBuffer + mFillPoint, mBufferSize - mFillPoint, &amt);
-    if (NS_FAILED(rv)) return rv;
+    if (NS_FAILED(rv)) {
+        return rv;
+    }
 
-    if (amt == 0)
+    if (amt == 0) {
         mEOF = true;
+    }
     
     mFillPoint += amt;
     return NS_OK;
@@ -407,17 +456,20 @@ NS_IMETHODIMP_(char*)
 nsBufferedInputStream::GetBuffer(uint32_t aLength, uint32_t aAlignMask)
 {
     NS_ASSERTION(mGetBufferCount == 0, "nested GetBuffer!");
-    if (mGetBufferCount != 0)
+    if (mGetBufferCount != 0) {
         return nullptr;
+    }
 
-    if (mBufferDisabled)
+    if (mBufferDisabled) {
         return nullptr;
+    }
 
     char* buf = mBuffer + mCursor;
     uint32_t rem = mFillPoint - mCursor;
     if (rem == 0) {
-        if (NS_FAILED(Fill()))
+        if (NS_FAILED(Fill())) {
             return nullptr;
+        }
         buf = mBuffer + mCursor;
         rem = mFillPoint - mCursor;
     }
@@ -425,8 +477,9 @@ nsBufferedInputStream::GetBuffer(uint32_t aLength, uint32_t aAlignMask)
     uint32_t mod = (NS_PTR_TO_INT32(buf) & aAlignMask);
     if (mod) {
         uint32_t pad = aAlignMask + 1 - mod;
-        if (pad > rem)
+        if (pad > rem) {
             return nullptr;
+        }
 
         memset(buf, 0, pad);
         mCursor += pad;
@@ -434,8 +487,9 @@ nsBufferedInputStream::GetBuffer(uint32_t aLength, uint32_t aAlignMask)
         rem -= pad;
     }
 
-    if (aLength > rem)
+    if (aLength > rem) {
         return nullptr;
+    }
     mGetBufferCount++;
     return buf;
 }
@@ -444,8 +498,9 @@ NS_IMETHODIMP_(void)
 nsBufferedInputStream::PutBuffer(char* aBuffer, uint32_t aLength)
 {
     NS_ASSERTION(mGetBufferCount == 1, "stray PutBuffer!");
-    if (--mGetBufferCount != 0)
+    if (--mGetBufferCount != 0) {
         return;
+    }
 
     NS_ASSERTION(mCursor + aLength <= mFillPoint, "PutBuffer botch");
     mCursor += aLength;
@@ -457,8 +512,9 @@ nsBufferedInputStream::DisableBuffering()
     NS_ASSERTION(!mBufferDisabled, "redundant call to DisableBuffering!");
     NS_ASSERTION(mGetBufferCount == 0,
                  "DisableBuffer call between GetBuffer and PutBuffer!");
-    if (mGetBufferCount != 0)
+    if (mGetBufferCount != 0) {
         return NS_ERROR_UNEXPECTED;
+    }
 
     // Empty the buffer so nsBufferedStream::Tell works.
     mBufferStartOffset += mCursor;
@@ -574,8 +630,9 @@ nsBufferedOutputStream::Create(nsISupports *aOuter, REFNSIID aIID, void **aResul
     NS_ENSURE_NO_AGGREGATION(aOuter);
 
     nsBufferedOutputStream* stream = new nsBufferedOutputStream();
-    if (stream == nullptr)
+    if (stream == nullptr) {
         return NS_ERROR_OUT_OF_MEMORY;
+    }
     NS_ADDREF(stream);
     nsresult rv = stream->QueryInterface(aIID, aResult);
     NS_RELEASE(stream);
@@ -595,17 +652,41 @@ NS_IMETHODIMP
 nsBufferedOutputStream::Close()
 {
     nsresult rv1, rv2 = NS_OK, rv3;
+
     rv1 = Flush();
+
+#ifdef DEBUG
+    if (NS_FAILED(rv1)) {
+        NS_WARNING("(debug) Flush() inside nsBufferedOutputStream::Close() returned error (rv1).");
+    }
+#endif
+
     // If we fail to Flush all the data, then we close anyway and drop the
     // remaining data in the buffer. We do this because it's what Unix does
     // for fclose and close. However, we report the error from Flush anyway.
     if (mStream) {
         rv2 = Sink()->Close();
+#ifdef DEBUG
+        if (NS_FAILED(rv2)) {
+            NS_WARNING("(debug) Sink->Close() inside nsBufferedOutputStream::Close() returned error (rv2).");
+        }
+#endif
         NS_RELEASE(mStream);
     }
     rv3 = nsBufferedStream::Close();
-    if (NS_FAILED(rv1)) return rv1;
-    if (NS_FAILED(rv2)) return rv2;
+
+#ifdef DEBUG
+    if (NS_FAILED(rv3)) {
+        NS_WARNING("(debug) nsBufferedStream:Close() inside nsBufferedOutputStream::Close() returned error (rv3).");
+    }
+#endif
+
+    if (NS_FAILED(rv1)) {
+        return rv1;
+    }
+    if (NS_FAILED(rv2)) {
+        return rv2;
+    }
     return rv3;
 }
 
@@ -614,6 +695,21 @@ nsBufferedOutputStream::Write(const char *buf, uint32_t count, uint32_t *result)
 {
     nsresult rv = NS_OK;
     uint32_t written = 0;
+    *result = 0;
+    if (!mStream) {
+        // We special case this situtaion.
+        // We should catch the failure, NS_BASE_STREAM_CLOSED ASAP, here.
+        // If we don't, eventually Flush() is called in the while loop below
+        // after so many writes.
+        // However, Flush() returns NS_OK when mStream is null (!!),
+        // and we don't get a meaningful error, NS_BASE_STREAM_CLOSED,
+        // soon enough when we use buffered output.
+#ifdef DEBUG
+        NS_WARNING("(info) nsBufferedOutputStream::Write returns NS_BASE_STREAM_CLOSED immediately (mStream==null).");
+#endif
+        return NS_BASE_STREAM_CLOSED;
+    }
+
     while (count > 0) {
         uint32_t amt = std::min(count, mBufferSize - mCursor);
         if (amt > 0) {
@@ -625,9 +721,14 @@ nsBufferedOutputStream::Write(const char *buf, uint32_t count, uint32_t *result)
                 mFillPoint = mCursor;
         }
         else {
-            NS_ASSERTION(mFillPoint, "iloop in nsBufferedOutputStream::Write!");
+            NS_ASSERTION(mFillPoint, "loop in nsBufferedOutputStream::Write!");
             rv = Flush();
-            if (NS_FAILED(rv)) break;
+            if (NS_FAILED(rv)) {
+#ifdef DEBUG
+                NS_WARNING("(debug) Flush() returned error in nsBufferedOutputStream::Write.");
+#endif
+                break;
+            }
         }
     }
     *result = written;
@@ -643,8 +744,14 @@ nsBufferedOutputStream::Flush()
         // Stream already cancelled/flushed; probably because of previous error.
         return NS_OK;
     }
+    // optimize : some code within C-C needs to call Seek -> Flush() often.
+    if (mFillPoint == 0) {
+        return NS_OK;
+    }
     rv = Sink()->Write(mBuffer, mFillPoint, &amt);
-    if (NS_FAILED(rv)) return rv;
+    if (NS_FAILED(rv)) {
+        return rv;
+    }
     mBufferStartOffset += amt;
     if (amt == mFillPoint) {
         mFillPoint = mCursor = 0;
@@ -665,21 +772,38 @@ NS_IMETHODIMP
 nsBufferedOutputStream::Finish()
 {
     // flush the stream, to write out any buffered data...
-    nsresult rv = nsBufferedOutputStream::Flush();
-    if (NS_FAILED(rv))
-        NS_WARNING("failed to flush buffered data! possible dataloss");
+    nsresult rv1 = nsBufferedOutputStream::Flush();
+    nsresult rv2 = NS_OK, rv3;
 
-    // ... and finish the underlying stream...
-    if (NS_SUCCEEDED(rv))
-        rv = mSafeStream->Finish();
-    else
-        Sink()->Close();
+    if (NS_FAILED(rv1)) {
+        NS_WARNING("(debug) nsBufferedOutputStream::Flush() failed in nsBufferedOutputStream::Finish()! Possible dataloss.");
+
+        rv2 = Sink()->Close();
+        if (NS_FAILED(rv2)) {
+            NS_WARNING("(debug) Sink()->Close() failed in nsBufferedOutputStream::Finish()! Possible dataloss.");
+        }
+    } else {
+        rv2 = mSafeStream->Finish();
+        if (NS_FAILED(rv2)) {
+            NS_WARNING("(debug) mSafeStream->Finish() failed within nsBufferedOutputStream::Flush()! Possible dataloss.");
+        }
+    }
 
     // ... and close the buffered stream, so any further attempts to flush/close
     // the buffered stream won't cause errors.
-    nsBufferedStream::Close();
+    rv3 = nsBufferedStream::Close();
 
-    return rv;
+    // We want to return the errors precisely from Finish()
+    // and mimick the existing error handling in
+    // nsBufferedOutputStream::Close() as reference.
+
+    if (NS_FAILED(rv1)) {
+        return rv1;
+    }
+    if (NS_FAILED(rv2)) {
+        return rv2;
+    }
+    return rv3;
 }
 
 static nsresult
@@ -709,8 +833,9 @@ nsBufferedOutputStream::WriteSegments(nsReadSegmentFun reader, void * closure, u
         uint32_t left = std::min(count, mBufferSize - mCursor);
         if (left == 0) {
             rv = Flush();
-            if (NS_FAILED(rv))
+            if (NS_FAILED(rv)) {
                 return (*_retval > 0) ? NS_OK : rv;
+            }
 
             continue;
         }
@@ -718,8 +843,9 @@ nsBufferedOutputStream::WriteSegments(nsReadSegmentFun reader, void * closure, u
         uint32_t read = 0;
         rv = reader(this, closure, mBuffer + mCursor, *_retval, left, &read);
 
-        if (NS_FAILED(rv)) // If we have written some data, return ok
+        if (NS_FAILED(rv)) { // If we have read some data, return ok
             return (*_retval > 0) ? NS_OK : rv;
+        }
         mCursor += read;
         *_retval += read;
         count -= read;
@@ -731,8 +857,9 @@ nsBufferedOutputStream::WriteSegments(nsReadSegmentFun reader, void * closure, u
 NS_IMETHODIMP
 nsBufferedOutputStream::IsNonBlocking(bool *aNonBlocking)
 {
-    if (mStream)
+    if (mStream) {
         return Sink()->IsNonBlocking(aNonBlocking);
+    }
     return NS_ERROR_NOT_INITIALIZED;
 }
 
@@ -740,17 +867,20 @@ NS_IMETHODIMP_(char*)
 nsBufferedOutputStream::GetBuffer(uint32_t aLength, uint32_t aAlignMask)
 {
     NS_ASSERTION(mGetBufferCount == 0, "nested GetBuffer!");
-    if (mGetBufferCount != 0)
+    if (mGetBufferCount != 0) {
         return nullptr;
+    }
 
-    if (mBufferDisabled)
+    if (mBufferDisabled) {
         return nullptr;
+    }
 
     char* buf = mBuffer + mCursor;
     uint32_t rem = mBufferSize - mCursor;
     if (rem == 0) {
-        if (NS_FAILED(Flush()))
+        if (NS_FAILED(Flush())) {
             return nullptr;
+        }
         buf = mBuffer + mCursor;
         rem = mBufferSize - mCursor;
     }
@@ -758,8 +888,9 @@ nsBufferedOutputStream::GetBuffer(uint32_t aLength, uint32_t aAlignMask)
     uint32_t mod = (NS_PTR_TO_INT32(buf) & aAlignMask);
     if (mod) {
         uint32_t pad = aAlignMask + 1 - mod;
-        if (pad > rem)
+        if (pad > rem) {
             return nullptr;
+        }
 
         memset(buf, 0, pad);
         mCursor += pad;
@@ -767,8 +898,9 @@ nsBufferedOutputStream::GetBuffer(uint32_t aLength, uint32_t aAlignMask)
         rem -= pad;
     }
 
-    if (aLength > rem)
+    if (aLength > rem) {
         return nullptr;
+    }
     mGetBufferCount++;
     return buf;
 }
@@ -777,13 +909,15 @@ NS_IMETHODIMP_(void)
 nsBufferedOutputStream::PutBuffer(char* aBuffer, uint32_t aLength)
 {
     NS_ASSERTION(mGetBufferCount == 1, "stray PutBuffer!");
-    if (--mGetBufferCount != 0)
+    if (--mGetBufferCount != 0) {
         return;
+    }
 
     NS_ASSERTION(mCursor + aLength <= mBufferSize, "PutBuffer botch");
     mCursor += aLength;
-    if (mFillPoint < mCursor)
+    if (mFillPoint < mCursor) {
         mFillPoint = mCursor;
+    }
 }
 
 NS_IMETHODIMP
@@ -792,13 +926,15 @@ nsBufferedOutputStream::DisableBuffering()
     NS_ASSERTION(!mBufferDisabled, "redundant call to DisableBuffering!");
     NS_ASSERTION(mGetBufferCount == 0,
                  "DisableBuffer call between GetBuffer and PutBuffer!");
-    if (mGetBufferCount != 0)
+    if (mGetBufferCount != 0) {
         return NS_ERROR_UNEXPECTED;
+    }
 
     // Empty the buffer so nsBufferedStream::Tell works.
     nsresult rv = Flush();
-    if (NS_FAILED(rv))
+    if (NS_FAILED(rv)) {
         return rv;
+    }
 
     mBufferDisabled = true;
     return NS_OK;
@@ -818,8 +954,9 @@ nsBufferedOutputStream::GetUnbufferedStream(nsISupports* *aStream)
     // Empty the buffer so subsequent i/o trumps any buffered data.
     if (mFillPoint) {
         nsresult rv = Flush();
-        if (NS_FAILED(rv))
+        if (NS_FAILED(rv)) {
             return rv;
+        }
     }
 
     *aStream = mStream;