http-connection-reclamation
author Benjamin Smedberg <benjamin@smedbergs.us>
Sat, 26 Jul 2008 22:49:39 -0400
changeset 167 a4da40849f5436e629c5732f4368c6c48189637f
parent 114 5efab25e10a8ef260156617cfb3d300235e1050c
permissions -rw-r--r--
State as of now

Bug 429728 - rework nsHTTPConnectionHandle so it's not finalizer-dependent. This is, IMO, a terrible hack, but it appears to work. It causes some MMgc assertions which jorendorff claims are incorrect, because of the way we're using the postsweep hook to do "real" work. That will be filed separately.

diff --git a/netwerk/protocol/http/src/nsHttpConnectionMgr.cpp b/netwerk/protocol/http/src/nsHttpConnectionMgr.cpp
--- a/netwerk/protocol/http/src/nsHttpConnectionMgr.cpp
+++ b/netwerk/protocol/http/src/nsHttpConnectionMgr.cpp
@@ -73,7 +73,7 @@ InsertTransactionSorted(nsVoidArray &pen
 //-----------------------------------------------------------------------------
 
 nsHttpConnectionMgr::nsHttpConnectionMgr()
-    : mRef(0)
+    : MMgc::GCCallback(NS_GetGC())
     , mMonitor(nsAutoMonitor::NewMonitor("nsHttpConnectionMgr"))
     , mMaxConns(0)
     , mMaxConnsPerHost(0)
@@ -106,7 +106,7 @@ nsHttpConnectionMgr::Init(PRUint16 maxCo
     LOG(("nsHttpConnectionMgr::Init\n"));
 
     nsresult rv;
-    nsCOMPtr<nsIEventTarget> sts = do_GetService(kSocketTransportServiceCID, &rv);
+    nsIEventTarget* sts = do_GetService(kSocketTransportServiceCID, &rv);
     if (NS_FAILED(rv)) return rv;
 
     nsAutoMonitor mon(mMonitor);
@@ -168,7 +168,7 @@ nsHttpConnectionMgr::PostEvent(nsConnEve
         rv = NS_ERROR_NOT_INITIALIZED;
     }
     else {
-        nsRefPtr<nsIRunnable> event = new nsConnEvent(this, handler, iparam, vparam);
+        nsIRunnable* event = new nsConnEvent(this, handler, iparam, vparam);
         if (!event)
             rv = NS_ERROR_OUT_OF_MEMORY;
         else
@@ -178,13 +178,11 @@ nsHttpConnectionMgr::PostEvent(nsConnEve
 }
 
 //-----------------------------------------------------------------------------
-
 nsresult
 nsHttpConnectionMgr::AddTransaction(nsHttpTransaction *trans, PRInt32 priority)
 {
     LOG(("nsHttpConnectionMgr::AddTransaction [trans=%x %d]\n", trans, priority));
 
-    NS_ADDREF(trans);
     nsresult rv = PostEvent(&nsHttpConnectionMgr::OnMsgNewTransaction, priority, trans);
     if (NS_FAILED(rv))
         NS_RELEASE(trans);
@@ -196,7 +194,6 @@ nsHttpConnectionMgr::RescheduleTransacti
 {
     LOG(("nsHttpConnectionMgr::RescheduleTransaction [trans=%x %d]\n", trans, priority));
 
-    NS_ADDREF(trans);
     nsresult rv = PostEvent(&nsHttpConnectionMgr::OnMsgReschedTransaction, priority, trans);
     if (NS_FAILED(rv))
         NS_RELEASE(trans);
@@ -208,7 +205,6 @@ nsHttpConnectionMgr::CancelTransaction(n
 {
     LOG(("nsHttpConnectionMgr::CancelTransaction [trans=%x reason=%x]\n", trans, reason));
 
-    NS_ADDREF(trans);
     nsresult rv = PostEvent(&nsHttpConnectionMgr::OnMsgCancelTransaction, reason, trans);
     if (NS_FAILED(rv))
         NS_RELEASE(trans);
@@ -225,7 +221,7 @@ nsHttpConnectionMgr::GetSocketThreadTarg
 nsHttpConnectionMgr::GetSocketThreadTarget(nsIEventTarget **target)
 {
     nsAutoMonitor mon(mMonitor);
-    NS_IF_ADDREF(*target = mSocketThreadTarget);
+    *target = mSocketThreadTarget;
     return NS_OK;
 }
 
@@ -236,8 +232,8 @@ nsHttpConnectionMgr::AddTransactionToPip
 
     NS_ASSERTION(PR_GetCurrentThread() == gSocketThread, "wrong thread");
 
-    nsRefPtr<nsHttpConnectionInfo> ci;
-    pipeline->GetConnectionInfo(getter_AddRefs(ci));
+    nsHttpConnectionInfo* ci = nsnull;
+    pipeline->GetConnectionInfo(&ci);
     if (ci) {
         nsCStringKey key(ci->HashKey());
         nsConnectionEntry *ent = (nsConnectionEntry *) mCT.Get(&key);
@@ -264,7 +260,6 @@ nsHttpConnectionMgr::ReclaimConnection(n
 {
     LOG(("nsHttpConnectionMgr::ReclaimConnection [conn=%x]\n", conn));
 
-    NS_ADDREF(conn);
     nsresult rv = PostEvent(&nsHttpConnectionMgr::OnMsgReclaimConnection, 0, conn);
     if (NS_FAILED(rv))
         NS_RELEASE(conn);
@@ -283,7 +278,6 @@ nsHttpConnectionMgr::ProcessPendingQ(nsH
 {
     LOG(("nsHttpConnectionMgr::ProcessPendingQ [ci=%s]\n", ci->HashKey().get()));
 
-    NS_ADDREF(ci);
     nsresult rv = PostEvent(&nsHttpConnectionMgr::OnMsgProcessPendingQ, 0, ci);
     if (NS_FAILED(rv))
         NS_RELEASE(ci);
@@ -546,7 +540,6 @@ nsHttpConnectionMgr::GetConnection(nsCon
         conn = new nsHttpConnection();
         if (!conn)
             return;
-        NS_ADDREF(conn);
 
         nsresult rv = conn->Init(ent->mConnInfo, mMaxRequestDelay);
         if (NS_FAILED(rv)) {
@@ -575,10 +568,9 @@ nsHttpConnectionMgr::DispatchTransaction
     LOG(("nsHttpConnectionMgr::DispatchTransaction [ci=%s trans=%x caps=%x conn=%x]\n",
         ent->mConnInfo->HashKey().get(), trans, caps, conn));
 
-    nsConnectionHandle *handle = new nsConnectionHandle(conn);
+    nsConnectionHandle *handle = new nsConnectionHandle(conn, mHandleList);
     if (!handle)
         return NS_ERROR_OUT_OF_MEMORY;
-    NS_ADDREF(handle);
 
     nsHttpPipeline *pipeline = nsnull;
     if (conn->SupportsPipelining() && (caps & NS_HTTP_ALLOW_PIPELINING)) {
@@ -590,7 +582,6 @@ nsHttpConnectionMgr::DispatchTransaction
     // 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);
@@ -607,6 +598,9 @@ nsHttpConnectionMgr::DispatchTransaction
         NS_RELEASE(handle->mConn);
         // destroy the connection
         NS_RELEASE(conn);
+    }
+    else {
+	mHandleList = handle;
     }
 
     // if we were unable to activate the pipeline, then this will destroy
@@ -657,7 +651,7 @@ nsHttpConnectionMgr::BuildPipeline(nsCon
         return PR_FALSE;
 
     LOG(("  pipelined %u transactions\n", numAdded));
-    NS_ADDREF(*result = pipeline);
+    *result = pipeline;
     return PR_TRUE;
 }
 
@@ -722,7 +716,6 @@ nsHttpConnectionMgr::ProcessNewTransacti
             trans, ent->mPendingQ.Count()+1));
         // put this transaction on the pending queue...
         InsertTransactionSorted(ent->mPendingQ, trans);
-        NS_ADDREF(trans);
         rv = NS_OK;
     }
     else {
@@ -855,7 +848,6 @@ nsHttpConnectionMgr::OnMsgReclaimConnect
     //
 
     nsHttpConnectionInfo *ci = conn->ConnectionInfo();
-    NS_ADDREF(ci);
 
     nsCStringKey key(ci->HashKey());
     nsConnectionEntry *ent = (nsConnectionEntry *) mCT.Get(&key);
@@ -921,15 +913,45 @@ nsHttpConnectionMgr::OnMsgUpdateParam(PR
 //-----------------------------------------------------------------------------
 // nsHttpConnectionMgr::nsConnectionHandle
 
-nsHttpConnectionMgr::nsConnectionHandle::~nsConnectionHandle()
+
+// Garbage collection of connection handles is... ugly.
+
+void
+nsHttpConnectionMgr::presweep()
 {
-    if (mConn) {
-        gHttpHandler->ReclaimConnection(mConn);
-        NS_RELEASE(mConn);
+    NS_ASSERTION(mConnectionsToReclaim.Length() == 0,
+		 "Should have been cleared in postsweep!");
+
+    MMgc::GCHiddenPointer<nsConnectionHandle*> *handle = &mHandleList;
+    while (*handle) {
+	nsHttpConnection *conn = (*handle)->mConn;
+	if (NS_GetGC()->GetMark(*handle)) {
+	    NS_ASSERTION(NS_GetGC()->GetMark(conn),
+			 "Connection handle is marked but connection isn't.");
+
+	    handle = &(*handle)->mNext;
+	}
+	else {
+	    if (NS_GetGC()->GetMark(conn)) {
+		mConnectionsToReclaim.AppendElement(conn);
+	    }
+	    *handle = (*handle)->mNext;
+	}
     }
 }
 
-NS_IMPL_THREADSAFE_ISUPPORTS0(nsHttpConnectionMgr::nsConnectionHandle)
+void
+nsHttpConnectionMgr::postsweep()
+{
+    nsHttpConnection** conn = mConnectionsToReclaim.Elements();
+    nsHttpConnection** last = conn + mConnectionsToReclaim.Length();
+    for (; conn < last; conn++) {
+	ReclaimConnection(*conn);
+    }
+    mConnectionsToReclaim.Clear();
+}
+
+NS_IMPL_ISUPPORTS0(nsHttpConnectionMgr::nsConnectionHandle)
 
 nsresult
 nsHttpConnectionMgr::nsConnectionHandle::OnHeadersAvailable(nsAHttpTransaction *trans,
diff --git a/netwerk/protocol/http/src/nsHttpConnectionMgr.h b/netwerk/protocol/http/src/nsHttpConnectionMgr.h
--- a/netwerk/protocol/http/src/nsHttpConnectionMgr.h
+++ b/netwerk/protocol/http/src/nsHttpConnectionMgr.h
@@ -54,6 +54,7 @@ class nsHttpPipeline;
 
 class nsHttpConnectionMgr : public XPCOMGCFinalizedObject
                           , public MMgc::GCFinalizable
+			  , public MMgc::GCCallback
 {
 public:
 
@@ -140,7 +141,6 @@ private:
         nsConnectionEntry(nsHttpConnectionInfo *ci)
             : mConnInfo(ci)
         {
-            NS_ADDREF(mConnInfo);
         }
        ~nsConnectionEntry() { NS_RELEASE(mConnInfo); }
 
@@ -159,23 +159,28 @@ private:
     // need for consumer code to know when to give the connection back to the
     // connection manager.
     //
-    class nsConnectionHandle : public nsAHttpConnection
+    class nsConnectionHandle : public XPCOMGCObject, public nsAHttpConnection
     {
     public:
-        NS_DECL_ISUPPORTS
+	NS_DECL_ISUPPORTS
         NS_DECL_NSAHTTPCONNECTION
 
-        nsConnectionHandle(nsHttpConnection *conn) { NS_ADDREF(mConn = conn); }
-        virtual ~nsConnectionHandle();
+        nsConnectionHandle(nsHttpConnection *conn, nsConnectionHandle *next)
+	    : mConn(conn)
+	    , mNext(next)
+	{ }
 
         nsHttpConnection *mConn;
+
+	// linked list of connection handles for presweep/postsweep hooks
+	// managed by the connection manager
+	MMgc::GCHiddenPointer<nsConnectionHandle*> mNext;
     };
 
     //-------------------------------------------------------------------------
     // NOTE: these members may be accessed from any thread (use mMonitor)
     //-------------------------------------------------------------------------
 
-    PRInt32                      mRef;
     PRMonitor                   *mMonitor;
     nsCOMPtr<nsIEventTarget>     mSocketThreadTarget;
 
@@ -227,7 +232,6 @@ private:
             , mIParam(iparam)
             , mVParam(vparam)
         {
-            NS_ADDREF(mMgr);
         }
 
         NS_IMETHOD Run()
@@ -266,6 +270,14 @@ private:
     PRUint16 mNumActiveConns;
     PRUint16 mNumIdleConns;
 
+    // Connection reclamation tracking... this array should only have
+    // entries during garbage collection.
+    MMgc::GCHiddenPointer<nsConnectionHandle*> mHandleList;
+    nsTArray<nsHttpConnection*, CAllocator> mConnectionsToReclaim;
+
+    virtual void presweep();
+    virtual void postsweep();
+
     //
     // the connection table
     //