Bug 613977 - Intermittent invalid certificate error prompt (partial) r=honzab a=blocking-beta9
authorPatrick McManus <mcmanus@ducksong.com>
Thu, 16 Dec 2010 08:50:36 -0500
changeset 59398 e465d98c56c2
parent 59397 1ae27e2eb959
child 59399 3857de12f888
push id17607
push usermcmanus@ducksong.com
push dateThu, 16 Dec 2010 14:12:22 +0000
treeherdermozilla-central@e465d98c56c2 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewershonzab, blocking-beta9
bugs613977, 614677, 614950, 592284
milestone2.0b9pre
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 613977 - Intermittent invalid certificate error prompt (partial) r=honzab a=blocking-beta9 Bug 614677 - Connection is reset message appears intermittently Bug 614950 - Connections stall occasionally after 592284 landed A couple of follow-on changes to 592284 rolled together to prevent diff conflicts 1] Set the securitycallback information for unused speculative connections in the connection manager to be the new cloned connection rather than the one they originated on. (613977) 2] When adding unused speculative connections to the connection manager, due so with a short timeout (<= 5 seconds) as some servers get grumpy if they haven't seen a request in that time. Most will close the connection, but some will just sit there quietly and RST things when the connection is used - so if you don't use the connection quickly don't use it at all. This is probably a L4 load balancer issue, actually. Mozillazine illustrates the problem. Connections are made in bursts anyhow, so the reuse optimization is likely still quite useful. (614677 and 614950) 3] mark every connection in the connection manager persistent conneciton pool as "reused". This allows the transaction to be restarted if a RST is recvd upon sending the requests (see #2) - with the conservative timeout this is now a rare event, but still possible so recovery is the right thing to do. (614677 and 614950) 4] obtain an nshttpconnection object from the connection manager, subject to the max connection constraints, at the same time as starting the backup conneciton. If we defer that until recycling time the exceeded limits of the SocketService can cause problems for other connections. also re-enables the syn retry feature by default. r+ honzab
modules/libpref/src/init/all.js
netwerk/protocol/http/nsHttpConnection.cpp
netwerk/protocol/http/nsHttpConnection.h
netwerk/protocol/http/nsHttpConnectionMgr.cpp
netwerk/protocol/http/nsHttpConnectionMgr.h
--- a/modules/libpref/src/init/all.js
+++ b/modules/libpref/src/init/all.js
@@ -735,18 +735,17 @@ pref("network.http.prompt-temp-redirect"
 // default value for HTTP
 // in a DSCP environment this should be 40 (0x28, or AF11), per RFC-4594,
 // Section 4.8 "High-Throughput Data Service Class"
 pref("network.http.qos", 0);
 
 // The number of milliseconds after sending a SYN for an HTTP connection,
 // to wait before trying a different connection. 0 means do not use a second
 // connection.
-// Temporarily Disabled for 4.0 Beta 8 - bug 614677
-pref("network.http.connection-retry-timeout", 0);
+pref("network.http.connection-retry-timeout", 250);
 
 // default values for FTP
 // in a DSCP environment this should be 40 (0x28, or AF11), per RFC-4594,
 // Section 4.8 "High-Throughput Data Service Class", and 80 (0x50, or AF22)
 // per Section 4.7 "Low-Latency Data Service Class".
 pref("network.ftp.data.qos", 0);
 pref("network.ftp.control.qos", 0);
 
--- a/netwerk/protocol/http/nsHttpConnection.cpp
+++ b/netwerk/protocol/http/nsHttpConnection.cpp
@@ -98,16 +98,21 @@ nsHttpConnection::nsHttpConnection()
 nsHttpConnection::~nsHttpConnection()
 {
     LOG(("Destroying nsHttpConnection @%x\n", this));
  
     if (mIdleSynTimer) {
         mIdleSynTimer->Cancel();
         mIdleSynTimer = nsnull;
     }
+
+    if (mBackupConnection) {
+        gHttpHandler->ReclaimConnection(mBackupConnection);
+        mBackupConnection = nsnull;
+    }
     
     NS_IF_RELEASE(mConnInfo);
     NS_IF_RELEASE(mTransaction);
 
     if (mLock) {
         PR_DestroyLock(mLock);
         mLock = nsnull;
     }
@@ -149,25 +154,31 @@ nsHttpConnection::IdleSynTimeout(nsITime
     self->mIdleSynTimer = nsnull;
 
     if (!self->mSocketTransport) {
         NS_ABORT_IF_FALSE(self->mSocketTransport1 && !self->mSocketTransport2,
                           "establishing backup tranport");
 
         LOG(("SocketTransport hit idle timer - starting backup socket"));
 
-        sCreateTransport2++;
+        gHttpHandler->ConnMgr()->GetConnection(self->mConnInfo,
+                                               self->mSocketCaps,
+                                               getter_AddRefs(
+                                                   self->mBackupConnection));
+        if (!self->mBackupConnection)
+            return;
         nsresult rv =
             self->CreateTransport(self->mSocketCaps,
                                   getter_AddRefs(self->mSocketTransport2),
                                   getter_AddRefs(self->mSocketIn2),
                                   getter_AddRefs(self->mSocketOut2));
-        
-        if (NS_SUCCEEDED(rv))
+        if (NS_SUCCEEDED(rv)) {
+            sCreateTransport2++;
             self->mSocketOut2->AsyncWait(self, 0, 0, nsnull);
+        }
     }
 
     return;
 }
 
 // called on the socket thread
 nsresult
 nsHttpConnection::Activate(nsAHttpTransaction *trans, PRUint8 caps)
@@ -627,16 +638,31 @@ nsHttpConnection::CreateTransport(PRUint
 
     strans.forget(sock);
     CallQueryInterface(sin, instream);
     CallQueryInterface(sout, outstream);
 
     return NS_OK;
 }
 
+nsresult
+nsHttpConnection::AssignTransport(nsISocketTransport *sock,
+                                  nsIAsyncOutputStream *outs,
+                                  nsIAsyncInputStream *ins)
+{
+    nsresult rv = sock->SetEventSink(this, nsnull);
+    NS_ENSURE_SUCCESS(rv, rv);
+    rv =sock->SetSecurityCallbacks(this);
+    NS_ENSURE_SUCCESS(rv, rv);
+    mSocketTransport = sock;
+    mSocketOut = outs;
+    mSocketIn = ins;
+    return NS_OK;
+}
+
 void
 nsHttpConnection::CloseTransaction(nsAHttpTransaction *trans, nsresult reason)
 {
     LOG(("nsHttpConnection::CloseTransaction[this=%x trans=%x reason=%x]\n",
         this, trans, reason));
 
     NS_ASSERTION(trans == mTransaction, "wrong transaction");
     NS_ASSERTION(PR_GetCurrentThread() == gSocketThread, "wrong thread");
@@ -877,40 +903,38 @@ nsHttpConnection::SetupSSLProxyConnect()
 
     buf.Truncate();
     request.Flatten(buf, PR_FALSE);
     buf.AppendLiteral("\r\n");
 
     return NS_NewCStringInputStream(getter_AddRefs(mSSLProxyConnectStream), buf);
 }
 
-nsresult
+void
 nsHttpConnection::ReleaseBackupTransport(nsISocketTransport *sock,
                                          nsIAsyncOutputStream *outs,
                                          nsIAsyncInputStream *ins)
 {
-    nsRefPtr<nsHttpConnection> clone = new nsHttpConnection();
-    nsresult rv = clone->Init(mConnInfo, mMaxHangTime);
-    if (NS_SUCCEEDED(rv)) {
-        // We need to establish a non-zero idle timeout so the connection mgr
-        // perceives this socket as suitable for persistent connection reuse
-        clone->mIdleTimeout = gHttpHandler->IdleTimeout();
-        clone->mSocketTransport = sock;
-        clone->mSocketOut = outs;
-        clone->mSocketIn = ins;
-        gHttpHandler->ReclaimConnection(clone);
-    }
-    return rv;
+    // We need to establish a small non-zero idle timeout so the connection
+    // mgr perceives this socket as suitable for persistent connection reuse
+    NS_ABORT_IF_FALSE(sock && outs && ins, "release Backup precond");
+    mBackupConnection->mIdleTimeout = NS_MIN((PRUint16) 5,
+                                             gHttpHandler->IdleTimeout());
+    mBackupConnection->mIsReused = PR_TRUE;
+    nsresult rv = mBackupConnection->AssignTransport(sock, outs, ins);
+    if (NS_SUCCEEDED(rv))
+        rv = gHttpHandler->ReclaimConnection(mBackupConnection);
+    if (NS_FAILED(rv))
+        NS_WARNING("Backup nsHttpConnection could not be reclaimed");
+    mBackupConnection = nsnull;
 }
 
-nsresult
+void
 nsHttpConnection::SelectPrimaryTransport(nsIAsyncOutputStream *out)
 {
-    nsresult rv = NS_OK;
-    
     if (!mSocketOut) {
         // Setup the Main Socket
         
         if (mIdleSynTimer) {
             mIdleSynTimer->Cancel();
             mIdleSynTimer = nsnull;
         }
         
@@ -929,45 +953,43 @@ nsHttpConnection::SelectPrimaryTransport
 
             sSuccessTransport2++;
             mSocketTransport.swap(mSocketTransport2);
             mSocketOut.swap(mSocketOut2);
             mSocketIn.swap(mSocketIn2);
         }
         else {
             NS_ABORT_IF_FALSE(0, "setup on unexpected socket");
-            return NS_ERROR_UNEXPECTED;
+            return;
         }
     }
     else if (out == mSocketOut1) {
         // Socket2 became the primary socket but Socket1 is now valid - give it
         // to the connection manager
 
-        rv = ReleaseBackupTransport(mSocketTransport1,
-                                    mSocketOut1,
-                                    mSocketIn1);
+        ReleaseBackupTransport(mSocketTransport1,
+                               mSocketOut1,
+                               mSocketIn1);
         sSuccessTransport1++;
         mSocketTransport1 = nsnull;
         mSocketOut1 = nsnull;
         mSocketIn1 = nsnull;
     }
     else if (out == mSocketOut2) {
         // Socket1 became the primary socket but Socket2 is now valid - give it
         // to the connectionmanager
 
-        rv = ReleaseBackupTransport(mSocketTransport2,
-                                    mSocketOut2,
-                                    mSocketIn2);
+        ReleaseBackupTransport(mSocketTransport2,
+                               mSocketOut2,
+                               mSocketIn2);
         sSuccessTransport2++;
         mSocketTransport2 = nsnull;
         mSocketOut2 = nsnull;
         mSocketIn2 = nsnull;
     }
-
-    return rv;
 }
 
 //-----------------------------------------------------------------------------
 // nsHttpConnection::nsISupports
 //-----------------------------------------------------------------------------
 
 NS_IMPL_THREADSAFE_ISUPPORTS4(nsHttpConnection,
                               nsIInputStreamCallback,
@@ -1012,30 +1034,26 @@ nsHttpConnection::OnOutputStreamReady(ns
     if (!mTransaction) {
         LOG(("  no transaction; ignoring event\n"));
         return NS_OK;
     }
 
     NS_ABORT_IF_FALSE(out == mSocketOut  ||
                       out == mSocketOut1 ||
                       out == mSocketOut2    , "unexpected socket");
-    nsresult rv;
     if (out != mSocketOut)
-        rv = SelectPrimaryTransport(out);
-    else
-        rv = NS_OK;
+        SelectPrimaryTransport(out);
 
-    if (NS_SUCCEEDED(rv) && (mSocketOut == out)) {
+    if (mSocketOut == out) {
         NS_ABORT_IF_FALSE(!mIdleSynTimer,"IdleSynTimer should not be set");
-        rv = OnSocketWritable();
+        nsresult rv = OnSocketWritable();
+        if (NS_FAILED(rv))
+            CloseTransaction(mTransaction, rv);
     }
 
-    if (NS_FAILED(rv))
-        CloseTransaction(mTransaction, rv);
-
     return NS_OK;
 }
 
 //-----------------------------------------------------------------------------
 // nsHttpConnection::nsITransportEventSink
 //-----------------------------------------------------------------------------
 
 NS_IMETHODIMP
--- a/netwerk/protocol/http/nsHttpConnection.h
+++ b/netwerk/protocol/http/nsHttpConnection.h
@@ -130,29 +130,32 @@ private:
     // called to cause the underlying socket to start speaking SSL
     nsresult ProxyStartSSL();
 
     nsresult CreateTransport(PRUint8 caps);
     nsresult CreateTransport(PRUint8 caps,
                              nsISocketTransport **sock,
                              nsIAsyncInputStream **instream,
                              nsIAsyncOutputStream **outstream);
+    nsresult AssignTransport(nsISocketTransport *sock,
+                             nsIAsyncOutputStream *outs,
+                             nsIAsyncInputStream *ins);
 
     nsresult OnTransactionDone(nsresult reason);
     nsresult OnSocketWritable();
     nsresult OnSocketReadable();
 
     nsresult SetupSSLProxyConnect();
 
     PRBool   IsAlive();
     PRBool   SupportsPipelining(nsHttpResponseHead *);
     
     static void  IdleSynTimeout(nsITimer *, void *);
-    nsresult     SelectPrimaryTransport(nsIAsyncOutputStream *out);
-    nsresult     ReleaseBackupTransport(nsISocketTransport *sock,
+    void         SelectPrimaryTransport(nsIAsyncOutputStream *out);
+    void         ReleaseBackupTransport(nsISocketTransport *sock,
                                         nsIAsyncOutputStream *outs,
                                         nsIAsyncInputStream *ins);
 private:
     nsCOMPtr<nsISocketTransport>    mSocketTransport;
     nsCOMPtr<nsIAsyncInputStream>   mSocketIn;
     nsCOMPtr<nsIAsyncOutputStream>  mSocketOut;
 
     nsresult                        mSocketInCondition;
@@ -177,16 +180,17 @@ private:
     PRPackedBool                    mCompletedSSLConnect;
 
     PRUint32                        mActivationCount;
 
     // These items are used to implement a parallel connection opening
     // attempt when network.http.connection-retry-timeout has expired
     PRUint8                         mSocketCaps;
     nsCOMPtr<nsITimer>              mIdleSynTimer;
+    nsRefPtr<nsHttpConnection>      mBackupConnection;
 
     nsCOMPtr<nsISocketTransport>    mSocketTransport1;
     nsCOMPtr<nsIAsyncInputStream>   mSocketIn1;
     nsCOMPtr<nsIAsyncOutputStream>  mSocketOut1;
 
     nsCOMPtr<nsISocketTransport>    mSocketTransport2;
     nsCOMPtr<nsIAsyncInputStream>   mSocketIn2;
     nsCOMPtr<nsIAsyncOutputStream>  mSocketOut2;
--- a/netwerk/protocol/http/nsHttpConnectionMgr.cpp
+++ b/netwerk/protocol/http/nsHttpConnectionMgr.cpp
@@ -593,18 +593,18 @@ nsHttpConnectionMgr::ProcessPendingQForE
 PRBool
 nsHttpConnectionMgr::AtActiveConnectionLimit(nsConnectionEntry *ent, PRUint8 caps)
 {
     nsHttpConnectionInfo *ci = ent->mConnInfo;
 
     LOG(("nsHttpConnectionMgr::AtActiveConnectionLimit [ci=%s caps=%x]\n",
         ci->HashKey().get(), caps));
 
-    // If we have more active connections than the limit, then we're done --
-    // purging idle connections won't get us below it.
+    // If there are more active connections than the global limit, then we're
+    // done. Purging idle connections won't get us below it.
     if (mNumActiveConns >= mMaxConns) {
         LOG(("  num active conns == max conns\n"));
         return PR_TRUE;
     }
 
     nsHttpConnection *conn;
     PRInt32 i, totalCount, persistCount = 0;
     
@@ -632,16 +632,28 @@ nsHttpConnectionMgr::AtActiveConnectionL
     }
 
     // use >= just to be safe
     return (totalCount >= maxConns) || ( (caps & NS_HTTP_ALLOW_KEEPALIVE) &&
                                          (persistCount >= maxPersistConns) );
 }
 
 void
+nsHttpConnectionMgr::GetConnection(nsHttpConnectionInfo *ci,
+                                   PRUint8 caps,
+                                   nsHttpConnection **result)
+{
+    NS_ASSERTION(PR_GetCurrentThread() == gSocketThread, "wrong thread");
+    nsCStringKey key(ci->HashKey());
+    nsConnectionEntry *ent = (nsConnectionEntry *) mCT.Get(&key);
+    if (!ent) return;
+    GetConnection(ent, caps, result);
+}
+
+void
 nsHttpConnectionMgr::GetConnection(nsConnectionEntry *ent, PRUint8 caps,
                                    nsHttpConnection **result)
 {
     LOG(("nsHttpConnectionMgr::GetConnection [ci=%s caps=%x]\n",
         ent->mConnInfo->HashKey().get(), PRUint32(caps)));
 
     // First, see if an idle persistent connection may be reused instead of
     // establishing a new socket. We do not need to check the connection limits
@@ -699,16 +711,21 @@ nsHttpConnectionMgr::GetConnection(nsCon
         NS_ADDREF(conn);
         nsresult rv = conn->Init(ent->mConnInfo, mMaxRequestDelay);
         if (NS_FAILED(rv)) {
             NS_RELEASE(conn);
             return;
         }
     }
 
+    // hold an owning ref to this connection
+    ent->mActiveConns.AppendElement(conn);
+    mNumActiveConns++;
+    NS_ADDREF(conn);
+
     *result = conn;
 }
 
 nsresult
 nsHttpConnectionMgr::DispatchTransaction(nsConnectionEntry *ent,
                                          nsAHttpTransaction *trans,
                                          PRUint8 caps,
                                          nsHttpConnection *conn)
@@ -723,21 +740,16 @@ nsHttpConnectionMgr::DispatchTransaction
 
     nsHttpPipeline *pipeline = nsnull;
     if (conn->SupportsPipelining() && (caps & NS_HTTP_ALLOW_PIPELINING)) {
         LOG(("  looking to build pipeline...\n"));
         if (BuildPipeline(ent, trans, &pipeline))
             trans = pipeline;
     }
 
-    // hold an owning ref to this connection
-    ent->mActiveConns.AppendElement(conn);
-    mNumActiveConns++;
-    NS_ADDREF(conn);
-
     // give the transaction the indirect reference to the connection.
     trans->SetConnection(handle);
 
     nsresult rv = conn->Activate(trans, caps);
 
     if (NS_FAILED(rv)) {
         LOG(("  conn->Activate failed [rv=%x]\n", rv));
         ent->mActiveConns.RemoveElement(conn);
@@ -842,25 +854,16 @@ nsHttpConnectionMgr::ProcessNewTransacti
 
         // steal reference from connection handle.
         // XXX prevent SetConnection(nsnull) from calling ReclaimConnection
         conn = handle->mConn;
         handle->mConn = nsnull;
 
         // destroy connection handle.
         trans->SetConnection(nsnull);
-
-        // remove sticky connection from active connection list; we'll add it
-        // right back in DispatchTransaction.
-        if (ent->mActiveConns.RemoveElement(conn))
-            mNumActiveConns--;
-        else {
-            NS_ERROR("sticky connection not found in active list");
-            return NS_ERROR_UNEXPECTED;
-        }
     }
     else
         GetConnection(ent, caps, &conn);
 
     nsresult rv;
     if (!conn) {
         LOG(("  adding transaction to pending queue [trans=%x pending-count=%u]\n",
             trans, ent->mPendingQ.Length()+1));
--- a/netwerk/protocol/http/nsHttpConnectionMgr.h
+++ b/netwerk/protocol/http/nsHttpConnectionMgr.h
@@ -132,16 +132,19 @@ public:
     // removes the next transaction for the specified connection from the
     // pending transaction queue.
     void AddTransactionToPipeline(nsHttpPipeline *);
 
     // called to force the transaction queue to be processed once more, giving
     // preference to the specified connection.
     nsresult ProcessPendingQ(nsHttpConnectionInfo *);
 
+    // called to reserve a nshttpconnection object with the manager
+    void GetConnection(nsHttpConnectionInfo *, PRUint8 caps,
+                       nsHttpConnection **);
 private:
     virtual ~nsHttpConnectionMgr();
 
     // nsConnectionEntry
     //
     // mCT maps connection info hash key to nsConnectionEntry object, which
     // contains list of active and idle connections as well as the list of
     // pending transactions.