Closed Bug 1305025 Opened 8 years ago Closed 8 years ago

Looping in nsPipe + stream copier (when source closed but buffered data available and sink would block)

Categories

(Core :: Networking, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 --- fixed

People

(Reporter: mayhemer, Assigned: mayhemer)

References

Details

(Whiteboard: [necko-active])

Attachments

(1 file, 1 obsolete file)

During work on the new windows specific socket polling code I found a bug in our nsPipe.  It manifests as following:

- have an nsAStreamCopier that copies from a string input stream to a socket
- the input is small (just a simple GET request)
- the copier performs a single copy operation (dispatches itself to the STS thread)
- it reads the input, all content fits the pipe buffer, the input stream is closed with NS_BASE_STREAM_CLOSED, state of the pipe is set to that failure code (all expected)
- it tries to write to the sink, but that returns WOULD_BLOCK
- the copier then waits for sink being writable AND source being closed (WAIT_CLOSURE_ONLY)

The problem is that we get OnInputStreamReady from the source immediately.  It makes the copy operation be performed immediately again.  Reason is that nsPipeInputStream::Status() returns the NS_BASE_STREAM_CLOSED set on the pipe despite that there are still data in the buffer available to read.

Hence, this causes a loop, until the sink becomes writable.

This may not happen that often, but may potentially manifest during long outstanding connections, not sure tho.

Still I believe worth a fix.  This is sensitive.
Assignee: nobody → honzab.moz
Status: NEW → ASSIGNED
Whiteboard: [necko-active]
Try push will be made later.
Attachment #8794173 - Flags: review?(jduell.mcbugs)
Comment on attachment 8794173 [details] [diff] [review]
v1 (don't let nsPipe::Status return the pipe's failure code when there are data available to read)

Ben, take a look at this change, since you've made some changes to this code recently.
Attachment #8794173 - Flags: feedback?(bkelly)
Blocks: 1297354
No longer blocks: 1297354
Comment on attachment 8794173 [details] [diff] [review]
v1 (don't let nsPipe::Status return the pipe's failure code when there are data available to read)

Review of attachment 8794173 [details] [diff] [review]:
-----------------------------------------------------------------

::: xpcom/io/nsPipe3.cpp
@@ +1527,5 @@
>  {
> +  if (NS_FAILED(mInputStatus)) {
> +    return mInputStatus;
> +  }
> +  

nit: trailing white space

@@ +1528,5 @@
> +  if (NS_FAILED(mInputStatus)) {
> +    return mInputStatus;
> +  }
> +  
> +  if (mReadState.mAvailable) {

I believe you should hold the monitor if you are inspecting mReadState.  For example:

https://dxr.mozilla.org/mozilla-central/source/xpcom/io/nsPipe3.cpp#1166

@@ +1532,5 @@
> +  if (mReadState.mAvailable) {
> +    // Still something to read and this input stream state is OK.
> +    return NS_OK;
> +  }
> +    

nit: trailing white space
Attachment #8794173 - Flags: feedback?(bkelly) → feedback+
(In reply to Ben Kelly [:bkelly] from comment #5)
> > +  if (mReadState.mAvailable) {
> 
> I believe you should hold the monitor if you are inspecting mReadState.

I believe I do.  I could change mReadState.mAvailable to call of (non-const*)this->Available() to gain an assertion automatically but I think the method signature already proves the lock sufficiently enough.
Also a note that we could only adjust the condition in nsPipeInputStream::AsyncWait, something like:

if (mReadState.mAvailable && !(aFlags & WAIT_CLOSURE_ONLY))
  notify->

if (!mReadState.mAvailable && NS_FAILED(Status(mon))
  notify->

And leave Status(&mon) as is to make this patch less risky.
Jason, ping on review here.  It's perfect time to land this now, it's early in the cycle.
Flags: needinfo?(jduell.mcbugs)
Attachment #8794173 - Flags: review?(jduell.mcbugs) → review+
Flags: needinfo?(jduell.mcbugs)
Thanks.
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4dd0aaa71fb0
Don't return error from nsPipe::Status when there are data to read to prevent potentiall loop. r=jduell
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/4dd0aaa71fb0
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: