160768 - nsPipe2::ReadSegments goes into an infinite loop if writer returns NS_BASE_STREAM_WOULD_BLOCK and reads 0 bytes

Status: RESOLVED FIXED

(Core :: Networking, defect, P3)

Description

[Boris Zbarsky [:bzbarsky]]

If you look at http://lxr.mozilla.org/seamonkey/source/xpcom/io/nsPipe2.cpp#419
you can see that the return value of "writer" is explicitly checked against
NS_BASE_STREAM_WOULD_BLOCK to prevent a goto to "done".  Then "rv" is set to
|pipe->mCondition|, which wipes out information on whether the writer func
wanted to actaully get data or not.  Then we loop forever, since no data is
being consumed.

Comment 1

[Darin Fisher]

nice find.. i'm not sure if we tickle this bug or not.  yet, it may explain one
of the infinite loops i saw with pipelining enabled.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Note that
http://lxr.mozilla.org/seamonkey/source/modules/libpr0n/src/imgRequest.cpp#766
(sniff_mimetype_callback) returns NS_ERROR_FAILURE; possibly to get around this
problem....

Comment 3

[Darin Fisher]

whoa, wrong milestone.  not going to happen for mozilla 1.2.

Comment 4

[Judson Valeski]

do we have a reproducible test case that aggrivates this? if so, please
re-nominate for "topembed"

Comment 5

[Boris Zbarsky [:bzbarsky]]

We have at least two ugly workarounds in Mozilla (in the nsUnicharStreamLoader
and in imagelib) to handle the fact that the browser went into infinite loops
because of this problem).  The workarounds break the contract of the
ReadSegments api to deal with this situation.  Any embeddor attempting to use
ReadSegments on an nsIPipe's input stream would run into this problem and curse
the bugginess of the API (basically, the only way to get this API to work is to
disregard the comments in the IDL and read the source to figure out the
hackiness that needs to happen).

So renominating.

Comment 6

[Darin Fisher]

agreed.. fixing this bug requires rev'ing the documentation in
nsIInputStream.idl and probably nsIOutputStream.idl.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Um you mean _not_ fixing this bug would require that, right?

Comment 8

[Darin Fisher]

well, sort of... in addition to fixing the bug itself, i mean that the API for
ReadSegments has a problem.  if you call ReadSegments, and the segmentwriter
decides to not write anything by returning NS_BASE_STREAM_WOULD_BLOCK, we cannot
return WOULD_BLOCK from ReadSegments because that would imply that the pipe has
no data in it when clearly it does.  so what do we return?  NS_ERROR_UNEXPECTED?
 or, NS_OK and zero bytes read?  but, the latter means the stream has been closed :(

Comment 9

[Judson Valeski]

Boris, understood; thanks for the clarity. Still going to minus this one though
as nsIPipe is not intended to be public (see
http://www.mozilla.org/projects/embedding/apiReviewNotes.html#nsIPipe ).

Comment 10

[Darin Fisher]

jud: but nsIStreamListener provides access to nsIInputStream, and the impl of
that nsIInputStream could easily be a nsPipeInputStream.

Comment 11

[Darin Fisher]

this has been fixed with the patch for bug 176919.