Bug 654201 - avoid nsHttpConnection::IsAlive() running event loop for unused SSL connections r=honzab sr=biesi
authorPatrick McManus <mcmanus@ducksong.com>
Thu, 19 May 2011 15:06:44 -0400
changeset 69727 f1d79c22fd712766d2e030f4677b546e1d286741
parent 69726 f594c196fac77e757c370ec978057a79a72e4502
child 69728 99436764a9261c60e758f28be50408b85a26b83a
push id20083
push usermcmanus@ducksong.com
push dateThu, 19 May 2011 19:33:38 +0000
treeherdermozilla-central@f1d79c22fd71 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewershonzab, biesi
bugs654201
milestone6.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 654201 - avoid nsHttpConnection::IsAlive() running event loop for unused SSL connections r=honzab sr=biesi
netwerk/base/public/nsISocketTransport.idl
netwerk/base/src/nsSocketTransport2.cpp
netwerk/protocol/ftp/nsFtpConnectionThread.cpp
netwerk/protocol/ftp/nsFtpControlConnection.cpp
netwerk/protocol/http/nsHttpConnection.cpp
netwerk/protocol/http/nsHttpConnection.h
--- a/netwerk/base/public/nsISocketTransport.idl
+++ b/netwerk/base/public/nsISocketTransport.idl
@@ -46,17 +46,17 @@ native PRNetAddr(union PRNetAddr);
  *
  * NOTE: Connection setup is triggered by opening an input or output stream,
  * it does not start on its own. Completion of the connection setup is
  * indicated by a STATUS_CONNECTED_TO notification to the event sink (if set).
  *
  * NOTE: This is a free-threaded interface, meaning that the methods on
  * this interface may be called from any thread.
  */
-[scriptable, uuid(19c37caa-fb41-4c32-bbf1-c6b31b75d789)]
+[scriptable, uuid(bbee8fef-6ac2-4819-9089-61169bdf5074)]
 interface nsISocketTransport : nsITransport 
 {
     /**
      * Get the host for the underlying socket connection.
      */
     readonly attribute AUTF8String host;
 
     /**
@@ -90,18 +90,22 @@ interface nsISocketTransport : nsITransp
      * via nsISSLSocketControl at socket creation time.
      *
      * NOTE: this attribute cannot be changed once a stream has been opened.
      */
     attribute nsIInterfaceRequestor securityCallbacks;
    
     /**
      * Test if this socket transport is (still) connected.
+     *
+     * @param aPassive indicates that the IsAlive() method should not
+     *                 read, even non-destructively, from the network.
+     *                 SSL based transports may deadlock if called without this.
      */
-    boolean isAlive();
+    boolean isAlive(in boolean aPassive);
 
     /**
      * Socket timeouts in seconds.  To specify no timeout, pass PR_UINT32_MAX
      * as aValue to setTimeout.  The implementation may truncate timeout values
      * to a smaller range of values (e.g., 0 to 0xFFFF).
      */
     unsigned long getTimeout(in unsigned long aType);
     void          setTimeout(in unsigned long aType, in unsigned long aValue);
--- a/netwerk/base/src/nsSocketTransport2.cpp
+++ b/netwerk/base/src/nsSocketTransport2.cpp
@@ -1827,38 +1827,58 @@ nsSocketTransport::SetEventSink(nsITrans
     }
 
     MutexAutoLock lock(mLock);
     mEventSink = sink;
     return NS_OK;
 }
 
 NS_IMETHODIMP
-nsSocketTransport::IsAlive(PRBool *result)
+nsSocketTransport::IsAlive(PRBool aPassive, PRBool *result)
 {
     *result = PR_FALSE;
 
     PRFileDesc *fd;
     {
         MutexAutoLock lock(mLock);
         if (NS_FAILED(mCondition))
             return NS_OK;
         fd = GetFD_Locked();
         if (!fd)
             return NS_OK;
     }
 
     // XXX do some idle-time based checks??
 
-    char c;
-    PRInt32 rval = PR_Recv(fd, &c, 1, PR_MSG_PEEK, 0);
+    if (aPassive) {
+        *result = PR_TRUE;                        /* presume true */
+
+        PRPollDesc desc;
+        desc.fd = mFD;
+
+        // include POLL_READ in the in_flags in order to take
+        // conditions PK11LoggedOut / AlreadyShutDown into account on SSL
+        // sockets as a workaround for bug 658138
+        desc.in_flags = PR_POLL_READ | PR_POLL_EXCEPT;
+        desc.out_flags = 0;
 
-    if ((rval > 0) || (rval < 0 && PR_GetError() == PR_WOULD_BLOCK_ERROR))
-        *result = PR_TRUE;
-
+        if ((PR_Poll(&desc, 1, 0) == 1) &&
+            (desc.out_flags &
+             (PR_POLL_EXCEPT | PR_POLL_ERR | PR_POLL_NVAL | PR_POLL_HUP))) {
+            *result = PR_FALSE;
+        }
+    }
+    else {
+        char c;
+        PRInt32 rval = PR_Recv(fd, &c, 1, PR_MSG_PEEK, 0);
+        
+        if ((rval > 0) || (rval < 0 && PR_GetError() == PR_WOULD_BLOCK_ERROR))
+            *result = PR_TRUE;
+    }
+    
     {
         MutexAutoLock lock(mLock);
         ReleaseFD_Locked(fd);
     }
     return NS_OK;
 }
 
 NS_IMETHODIMP
--- a/netwerk/protocol/ftp/nsFtpConnectionThread.cpp
+++ b/netwerk/protocol/ftp/nsFtpConnectionThread.cpp
@@ -1423,17 +1423,18 @@ nsFtpState::R_pasv() {
         // is the same
         nsCOMPtr<nsISocketTransport> strans = do_QueryInterface(mDataTransport);
         if (strans) {
             PRInt32 oldPort;
             nsresult rv = strans->GetPort(&oldPort);
             if (NS_SUCCEEDED(rv)) {
                 if (oldPort == port) {
                     PRBool isAlive;
-                    if (NS_SUCCEEDED(strans->IsAlive(&isAlive)) && isAlive)
+                    if (NS_SUCCEEDED(strans->IsAlive(PR_FALSE, &isAlive)) &&
+                        isAlive)
                         newDataConn = PR_FALSE;
                 }
             }
         }
 
         if (newDataConn) {
             mDataTransport->Close(NS_ERROR_ABORT);
             mDataTransport = nsnull;
--- a/netwerk/protocol/ftp/nsFtpControlConnection.cpp
+++ b/netwerk/protocol/ftp/nsFtpControlConnection.cpp
@@ -111,17 +111,17 @@ nsFtpControlConnection::~nsFtpControlCon
 
 PRBool
 nsFtpControlConnection::IsAlive()
 {
     if (!mSocket) 
         return PR_FALSE;
 
     PRBool isAlive = PR_FALSE;
-    mSocket->IsAlive(&isAlive);
+    mSocket->IsAlive(PR_FALSE, &isAlive);
     return isAlive;
 }
 nsresult 
 nsFtpControlConnection::Connect(nsIProxyInfo* proxyInfo,
                                 nsITransportEventSink* eventSink)
 {
     if (mSocket)
         return NS_OK;
--- a/netwerk/protocol/http/nsHttpConnection.cpp
+++ b/netwerk/protocol/http/nsHttpConnection.cpp
@@ -71,16 +71,17 @@ nsHttpConnection::nsHttpConnection()
     , mConsiderReusedAfterInterval(0)
     , mConsiderReusedAfterEpoch(0)
     , mCurrentBytesRead(0)
     , mMaxBytesRead(0)
     , mKeepAlive(PR_TRUE) // assume to keep-alive by default
     , mKeepAliveMask(PR_TRUE)
     , mSupportsPipelining(PR_FALSE) // assume low-grade server
     , mIsReused(PR_FALSE)
+    , mIsActivated(PR_FALSE)
     , mCompletedProxyConnect(PR_FALSE)
     , mLastTransactionExpectedNoContent(PR_FALSE)
 {
     LOG(("Creating nsHttpConnection @%x\n", this));
 
     // grab a reference to the handler to ensure that it doesn't go away.
     nsHttpHandler *handler = gHttpHandler;
     NS_ADDREF(handler);
@@ -159,16 +160,17 @@ nsHttpConnection::Activate(nsAHttpTransa
         mCallbacks.swap(callbacks);
         if (callbacks)
             NS_ProxyRelease(mCallbackTarget, callbacks);
         mCallbackTarget = callbackTarget;
     }
 
     // take ownership of the transaction
     mTransaction = trans;
+    mIsActivated = PR_TRUE;
 
     // set mKeepAlive according to what will be requested
     mKeepAliveMask = mKeepAlive = (caps & NS_HTTP_ALLOW_KEEPALIVE);
 
     // need to handle HTTP CONNECT tunnels if this is the first time if
     // we are tunneling through a proxy
     if (((mConnInfo->UsingSSL() && mConnInfo->UsingHttpProxy()) ||
          mConnInfo->ShouldForceConnectMethod()) && !mCompletedProxyConnect) {
@@ -260,18 +262,25 @@ PRUint32 nsHttpConnection::TimeToLive()
 }
 
 PRBool
 nsHttpConnection::IsAlive()
 {
     if (!mSocketTransport)
         return PR_FALSE;
 
+    // Calling IsAlive() non passively on an SSL socket transport that has
+    // not yet completed the SSL handshake can result
+    // in the event loop being run. All code that calls
+    // nsHttpConnection::IsAlive() is not re-entrant so we need to avoid
+    // having IsAlive() trigger a real SSL read in that circumstance.
+
     PRBool alive;
-    nsresult rv = mSocketTransport->IsAlive(&alive);
+    PRBool passiveRead = mConnInfo->UsingSSL() && !mIsActivated;
+    nsresult rv = mSocketTransport->IsAlive(passiveRead, &alive);
     if (NS_FAILED(rv))
         alive = PR_FALSE;
 
 //#define TEST_RESTART_LOGIC
 #ifdef TEST_RESTART_LOGIC
     if (!alive) {
         LOG(("pretending socket is still alive to test restart logic\n"));
         alive = PR_TRUE;
--- a/netwerk/protocol/http/nsHttpConnection.h
+++ b/netwerk/protocol/http/nsHttpConnection.h
@@ -183,13 +183,14 @@ private:
     PRIntervalTime                  mConsiderReusedAfterEpoch;
     PRInt64                         mCurrentBytesRead;   // data read per activation
     PRInt64                         mMaxBytesRead;       // max read in 1 activation
 
     PRPackedBool                    mKeepAlive;
     PRPackedBool                    mKeepAliveMask;
     PRPackedBool                    mSupportsPipelining;
     PRPackedBool                    mIsReused;
+    PRPackedBool                    mIsActivated;
     PRPackedBool                    mCompletedProxyConnect;
     PRPackedBool                    mLastTransactionExpectedNoContent;
 };
 
 #endif // nsHttpConnection_h__