SocketTransport changes break in-process PSM

RESOLVED FIXED

Status

()

Core
Networking
RESOLVED FIXED
17 years ago
15 years ago

People

(Reporter: Brian Ryner (not reading), Assigned: Darin Fisher)

Tracking

Trunk
x86
All
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments)

(Reporter)

Description

17 years ago
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.
(Assignee)

Comment 1

17 years ago
Created attachment 23427 [details] [diff] [review]
changes under netwerk/
(Assignee)

Comment 2

17 years ago
Created attachment 23428 [details] [diff] [review]
changes under xpcom/io/
(Assignee)

Comment 3

17 years ago
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.
(Reporter)

Comment 4

17 years ago
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.
(Assignee)

Comment 5

17 years ago
oops.. typo.. resubmitting.
(Assignee)

Comment 6

17 years ago
Created attachment 23457 [details] [diff] [review]
fixed typo -- new changes under netwerk/
(Assignee)

Comment 7

17 years ago
bryner can i get an r= from you?
(Reporter)

Comment 8

17 years ago
r=bryner
(Assignee)

Comment 9

17 years ago
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)...
(Assignee)

Comment 10

17 years ago
Created attachment 23500 [details] [diff] [review]
Fixes FTP bustage incurred by the other patches.
(Assignee)

Comment 11

17 years ago
r=gagan on the FTP patch (discussed my changes with dougt, but he's not around
right now to review the patch).
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
(Assignee)

Comment 13

17 years ago
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.
(Assignee)

Comment 14

17 years ago
Created attachment 23510 [details] [diff] [review]
Revised FTP patch per Brendan's comments
(Assignee)

Comment 15

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

Comment 16

17 years ago
> 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

17 years ago
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

17 years ago
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.
(Assignee)

Comment 19

17 years ago
It's probably the socket thread that is spinning.  I will try imap over ssl
before submitting this patch.
(Assignee)

Comment 20

17 years ago
Thanks for the reviews... these changes have landed!
Status: NEW → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED

Updated

15 years ago
QA Contact: tever → benc
You need to log in before you can comment on or make changes to this bug.