66493 - SocketTransport changes break in-process PSM

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Brian Ryner (not reading)]

As we discussed, the changes to nsSocketTransport break pipnss because of the
assumptions it makes about being able to write/read a socket after a PR_Poll
returns.

Comment 1

[Darin Fisher]

Attachment: changes under netwerk/

Comment 2

[Darin Fisher]

Attachment: changes under xpcom/io/

Comment 3

[Darin Fisher]

There's two patches.  The first (for netwerk files) is pretty straightforward.
My solution is to record the fact that PR_Write/PR_Read returned WOULD_BLOCK
and then check for that condition if zero bytes were written by the provider
or read by the listener.

To make this work, however, I had to fix a bug in nsPipe2.cpp.  WriteSegments
incorrectly returns NS_BASE_STREAM_WOULD_BLOCK when the "reader" returns 
NS_BASE_STREAM_WOULD_BLOCK.  This is incorrect because it blurs the meaning
of NS_BASE_STREAM_WOULD_BLOCK as a return value from WriteSegments.  It should
only mean that the write failed because the pipe is full.  My patch makes it
so that if the "reader" returns NS_BASE_STREAM_WOULD_BLOCK, WriteSegments 
returns pipe->mCondition, which is normally just NS_OK unless an error occured
elsewhere.  Seeing NS_OK returned from WriteSegments, the caller would check
the number of bytes written to determine what to do.  If zero bytes were 
written (as in the case of this bug), the caller would probably assume that
the "reader" had no more data to provide.  But, right now, we only meet this
condition in nsStreamListenerProxy.cpp, which expects an OnStopRequest from
the underlying transport to signify the end-of-file, so it just ignores the
case of zero bytes written.  I'm not sure if this solution is correct in 
general.  Brendan: what do you think?

I've tested these changes and they seem to do the trick.

Comment 4

[Brian Ryner (not reading)]

There is one small problem with this patch -- you removed the empty definition
of nsSocketOutputStream::~nsSocketOutputStream and replaced it with a second
declaration.  In other words, change line 166 from:

virtual ~nsSocketOutputStream();

to

virtual ~nsSocketOutputStream() {}

as it was before.

Comment 5

[Darin Fisher]

oops.. typo.. resubmitting.

Comment 6

[Darin Fisher]

Attachment: fixed typo -- new changes under netwerk/

Comment 7

[Darin Fisher]

bryner can i get an r= from you?

Comment 8

[Brian Ryner (not reading)]

r=bryner

Comment 9

[Darin Fisher]

Unfortunately, it looks like this breaks FTP.  FTP tries to write data to
the socket transport by connecting AsyncWrite to a pipe via a simple stream
provider (nsISimpleStreamProvider).  This is broken because the simple stream
provider assumes that the entire data to be written is available.  But, since
FTP wants the simple stream provider to feed from a pipe, this won't work.

FTP needs to implement nsIStreamProvider.  Working on a patch (cc-ing dougt)...

Comment 10

[Darin Fisher]

Attachment: Fixes FTP bustage incurred by the other patches.

Comment 11

[Darin Fisher]

r=gagan on the FTP patch (discussed my changes with dougt, but he's not around
right now to review the patch).

Comment 12

[Brendan Eich [:brendan]]

Nits on latest (FTP) patch:

+        if (avail > 0) {
+            PRUint32 bytesWritten;
+            return aOutStream->WriteFrom(mInStream, PR_MIN(avail, aCount),
&bytesWritten);
+        }
+        else {
+            NS_STATIC_CAST(nsFtpControlConnection*, aContext)->mSuspendedWrite
= PR_TRUE;
+            return NS_BASE_STREAM_WOULD_BLOCK;
+        }

else after return is a non-sequitur, and why not cast out the odd case first:

+        if (avail == 0) {
+            NS_STATIC_CAST(nsFtpControlConnection*, aContext)->mSuspendedWrite
= PR_TRUE;
+            return NS_BASE_STREAM_WOULD_BLOCK;
+        }
         PRUint32 bytesWritten;
+        return aOutStream->WriteFrom(mInStream, PR_MIN(avail, aCount),
&bytesWritten);

Sorry if I'm flailing here, but isn't that NS_STATIC_CAST ambiguous?  Which
nsISupports* is being passed in, the one in nsIStreamListener or the one in
nsIStreamObserver?

More static cast frobbing:

+    nsCOMPtr<nsIStreamProvider> provider;
+    NS_NEWXPCOM(provider, nsFtpStreamProvider);
+    if (!provider) return NS_ERROR_OUT_OF_MEMORY;
+
+    // setup the stream provider to get data from the pipe.
+    NS_STATIC_CAST(nsFtpStreamProvider*,
+        NS_STATIC_CAST(nsIStreamProvider*, provider))->mInStream = inStream;

Can't the inner one be replaced by provider.get()?

Looks good otherwise.  I'll look at the earlier patches too, if that'll help. 
Your change to unoverload the WOULD_BLOCK return value from WriteSegments is in
line with earlier rants and empty threats I made in similar bugs.  For truth in
naming, I move that writeSegments be renamed something that connotes both read
and write, like transferSegments.  Does the 0 WriteSegments return when the
reader has returned WOULD_BLOCK upset any other implementations?  More in a bit.

/be

Comment 13

[Darin Fisher]

Brendan, thanks for cleaning up my casting madness (posting an improved patch).

I think that writeSegments is okay... you want something that describes writing
to the underlying segments of the output stream.  Segments, in this context,
applies to the output stream and nothing else, so transferSegments seems a
little confusing to me.

Comment 14

[Darin Fisher]

Attachment: Revised FTP patch per Brendan's comments

Comment 15

[Darin Fisher]

Brendan, provider.get() returns an nsDerivedSafe<T> which doesn't cast to a
nsFtpStreamProvider* either; hence, the nested NS_STATIC_CAST's.

Comment 16

[Darin Fisher]

> Does the 0 WriteSegments return when the reader has returned WOULD_BLOCK upset
> any other implementations?

I am very concerned about this (it's a scary thing making changes to nsPipe2).
I have tested most areas that might be effected (http, https, ftp, file, imap,
and news).  I've also run through chofmann's browser buster without any
surprises, so I feel fairly confident that this change shouldn't upset anything.

Comment 17

[Scott MacGregor]

sr=mscott on the netwerk and xpcom changes. I'm still digesting the ftp patch. 
Of course changes to nsPipe are always high risk but I can't think of anything
in particular to test for other than what you've already done.

Comment 18

[Scott MacGregor]

Okay the last ftp patch looks good to me to. 

Any chance this is causing some problems I'm having in yesterday's and today's
release builds? If I leave the mail window open and idle for a little while we
are hitting 100% CPU. It's not the UI thread that's pinging the CPU either.
Seems to be another thread. No one else in mailnews seems to be seeing this. But
I use imap ssl which uses PSM. I'm wondering if the PSM thread is spinning. 

Any way I have more investigative work to do before I figure out what's causing
that in my release builds. Just curious.

Comment 19

[Darin Fisher]

It's probably the socket thread that is spinning.  I will try imap over ssl
before submitting this patch.

Comment 20

[Darin Fisher]

Thanks for the reviews... these changes have landed!