185148 - nsStorageStream::ReadSegments(...) loops forever if the writer fails...

Status: VERIFIED FIXED

(Core :: Networking, defect)

Description

[rpotts (gone)]

If the consumer of a storage stream fails during write, it is possible for
nsStorageStream::ReadSegments(...) to enter an endless loop.

Below is the code snippet:
--------------------------
436     while (remainingCapacity) {
437         availableInSegment = mSegmentEnd - mReadCursor;
 :
447     
448         count = PR_MIN(availableInSegment, remainingCapacity);
449         writer(this, closure, mReadCursor, mLogicalCursor, count,
&bytesConsumed);
450         remainingCapacity -= bytesConsumed;
451         mReadCursor += bytesConsumed;
452         mLogicalCursor += bytesConsumed;
453     };

Unfortunately, the return code from writer(...) is lost.  So, if the function
fails and bytesConsumed is 0... Then the loop will never exit.

Comment 1

[Cathleen]

--> suresh :-)

Comment 2

[Suresh]

Attachment: patch attached.

Comment 3

[Darin Fisher]

Comment on attachment 110918 [details] [diff] [review]
patch attached.

>Index: nsStorageStream.cpp

>+    nsresult rv = NS_OK;

no need to initialize rv.  sr=darin with that change.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 110918 [details] [diff] [review]
patch attached.

+	 if (NS_FAILED(rv) || (bytesConsumed == 0))

take out the extra parens.

Comment 5

[Suresh]

whoo hooo! my first necko bug is fixed in trunk!!

Comment 6

[benc]

Are there steps I can use to blackbox verify, or is a straightforward code change?

Comment 7

[Suresh]

This is a simple straightforward code change. But, I found one consumer of this
StorageStream class while fixing this bug. The "Publish" feature in composer
uses this. You will run into this bug when write operation fails for some reason
(like, ran out of disk space. )

Comment 8

[benc]

VERIFIED: via lxr

http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&file=nsStorageStream.cpp&root=/cvsroot&subdir=mozilla/xpcom/io&command=DIFF_FRAMESET&rev1=1.22&rev2=1.23

Is this Necko or XPCOM? Here's the path of the file: 

mozilla/ xpcom/ io/ nsStorageStream.cpp 

Do we have reports or steps for composer QA to verify? If it is in another bug,
this bug should block that bug.