Bug 1011354 - Use a mutex to guard access to nsHttpTransaction::mConnection. r=mcmanus, r=honzab, a=abillings
authorValentin Gosu <valentin.gosu@gmail.com>
Thu, 18 Sep 2014 22:21:59 +0300
changeset 216860 ac926de428c3
parent 216859 16e19b9cec72
child 216861 fd4720dd6a46
push id3944
push userryanvm@gmail.com
push date2014-09-26 21:25 +0000
treeherdermozilla-beta@fd4720dd6a46 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmcmanus, honzab, abillings
bugs1011354
milestone33.0
Bug 1011354 - Use a mutex to guard access to nsHttpTransaction::mConnection. r=mcmanus, r=honzab, a=abillings
netwerk/protocol/http/nsHttpChannel.cpp
netwerk/protocol/http/nsHttpTransaction.cpp
netwerk/protocol/http/nsHttpTransaction.h
--- a/netwerk/protocol/http/nsHttpChannel.cpp
+++ b/netwerk/protocol/http/nsHttpChannel.cpp
@@ -5059,27 +5059,27 @@ nsHttpChannel::OnStopRequest(nsIRequest 
         // to another protocol.
         //
         // this code relies on the code in nsHttpTransaction::Close, which
         // tests for NS_HTTP_STICKY_CONNECTION to determine whether or not to
         // keep the connection around after the transaction is finished.
         //
         nsRefPtr<nsAHttpConnection> conn;
         if (authRetry && (mCaps & NS_HTTP_STICKY_CONNECTION)) {
-            conn = mTransaction->Connection();
+            conn = mTransaction->GetConnectionReference();
             // This is so far a workaround to fix leak when reusing unpersistent
             // connection for authentication retry. See bug 459620 comment 4
             // for details.
             if (conn && !conn->IsPersistent())
                 conn = nullptr;
         }
 
         nsRefPtr<nsAHttpConnection> stickyConn;
         if (mCaps & NS_HTTP_STICKY_CONNECTION)
-            stickyConn = mTransaction->Connection();
+            stickyConn = mTransaction->GetConnectionReference();
 
         // at this point, we're done with the transaction
         mTransactionTimings = mTransaction->Timings();
         mTransaction = nullptr;
         mTransactionPump = nullptr;
 
         // We no longer need the dns prefetch object
         if (mDNSPrefetch && mDNSPrefetch->TimingsValid()) {
--- a/netwerk/protocol/http/nsHttpTransaction.cpp
+++ b/netwerk/protocol/http/nsHttpTransaction.cpp
@@ -83,17 +83,17 @@ LogHeaders(const char *lineStart)
 }
 #endif
 
 //-----------------------------------------------------------------------------
 // nsHttpTransaction <public>
 //-----------------------------------------------------------------------------
 
 nsHttpTransaction::nsHttpTransaction()
-    : mCallbacksLock("transaction mCallbacks lock")
+    : mLock("transaction lock")
     , mRequestSize(0)
     , mConnection(nullptr)
     , mConnInfo(nullptr)
     , mRequestHead(nullptr)
     , mResponseHead(nullptr)
     , mContentLength(-1)
     , mContentRead(0)
     , mInvalidResponseBytesRead(0)
@@ -374,22 +374,31 @@ nsHttpTransaction::Init(uint32_t caps,
     if (NS_FAILED(rv)) return rv;
 
     Classify();
 
     NS_ADDREF(*responseBody = mPipeIn);
     return NS_OK;
 }
 
+// This method should only be used on the socket thread
 nsAHttpConnection *
 nsHttpTransaction::Connection()
 {
     return mConnection;
 }
 
+already_AddRefed<nsAHttpConnection>
+nsHttpTransaction::GetConnectionReference()
+{
+    MutexAutoLock lock(mLock);
+    nsRefPtr<nsAHttpConnection> connection = mConnection;
+    return connection.forget();
+}
+
 nsHttpResponseHead *
 nsHttpTransaction::TakeResponseHead()
 {
     MOZ_ASSERT(!mResponseHeadTaken, "TakeResponseHead called 2x");
 
     // Lock RestartInProgress() and TakeResponseHead() against main thread
     MutexAutoLock lock(*nsHttp::GetLock());
 
@@ -443,37 +452,40 @@ nsHttpTransaction::TakeSubTransactions(
 
 //----------------------------------------------------------------------------
 // nsHttpTransaction::nsAHttpTransaction
 //----------------------------------------------------------------------------
 
 void
 nsHttpTransaction::SetConnection(nsAHttpConnection *conn)
 {
-    NS_IF_RELEASE(mConnection);
-    NS_IF_ADDREF(mConnection = conn);
+    {
+        MutexAutoLock lock(mLock);
+        NS_IF_RELEASE(mConnection);
+        NS_IF_ADDREF(mConnection = conn);
+    }
 
     if (conn) {
         MOZ_EVENT_TRACER_EXEC(static_cast<nsAHttpTransaction*>(this),
                               "net::http::transaction");
     }
 }
 
 void
 nsHttpTransaction::GetSecurityCallbacks(nsIInterfaceRequestor **cb)
 {
-    MutexAutoLock lock(mCallbacksLock);
+    MutexAutoLock lock(mLock);
     NS_IF_ADDREF(*cb = mCallbacks);
 }
 
 void
 nsHttpTransaction::SetSecurityCallbacks(nsIInterfaceRequestor* aCallbacks)
 {
     {
-        MutexAutoLock lock(mCallbacksLock);
+        MutexAutoLock lock(mLock);
         mCallbacks = aCallbacks;
     }
 
     if (gSocketTransportService) {
         nsRefPtr<UpdateSecurityCallbacks> event = new UpdateSecurityCallbacks(this, aCallbacks);
         gSocketTransportService->Dispatch(event, nsIEventTarget::DISPATCH_NORMAL);
     }
 }
@@ -917,18 +929,20 @@ nsHttpTransaction::Close(nsresult reason
 
     // mTimings.responseEnd is normally recorded based on the end of a
     // HTTP delimiter such as chunked-encodings or content-length. However,
     // EOF or an error still require an end time be recorded.
     if (TimingEnabled() &&
         mTimings.responseEnd.IsNull() && !mTimings.responseStart.IsNull())
         mTimings.responseEnd = TimeStamp::Now();
 
-    if (relConn && mConnection)
+    if (relConn && mConnection) {
+        MutexAutoLock lock(mLock);
         NS_RELEASE(mConnection);
+    }
 
     // save network statistics in the end of transaction
     SaveNetworkStats(true);
 
     mStatus = reason;
     mTransactionDone = true; // forcibly flag the transaction as complete
     mClosed = true;
     ReleaseBlockingTransaction();
@@ -1081,16 +1095,17 @@ nsHttpTransaction::Restart()
     nsCOMPtr<nsISeekableStream> seekable = do_QueryInterface(mRequestStream);
     if (seekable)
         seekable->Seek(nsISeekableStream::NS_SEEK_SET, 0);
 
     // clear old connection state...
     mSecurityInfo = 0;
     if (mConnection) {
         mConnection->DontReuse();
+        MutexAutoLock lock(mLock);
         NS_RELEASE(mConnection);
     }
 
     // disable pipelining for the next attempt in case pipelining caused the
     // reset.  this is being overly cautious since we don't know if pipelining
     // was the problem here.
     mCaps &= ~NS_HTTP_ALLOW_PIPELINING;
     SetPipelinePosition(0);
--- a/netwerk/protocol/http/nsHttpTransaction.h
+++ b/netwerk/protocol/http/nsHttpTransaction.h
@@ -91,16 +91,20 @@ public:
     nsIEventTarget        *ConsumerTarget() { return mConsumerTarget; }
 
     void SetSecurityCallbacks(nsIInterfaceRequestor* aCallbacks);
 
     // Called to take ownership of the response headers; the transaction
     // will drop any reference to the response headers after this call.
     nsHttpResponseHead *TakeResponseHead();
 
+    // Provides a thread safe reference of the connection
+    // nsHttpTransaction::Connection should only be used on the socket thread
+    already_AddRefed<nsAHttpConnection> GetConnectionReference();
+
     // Called to find out if the transaction generated a complete response.
     bool ResponseIsComplete() { return mResponseIsComplete; }
 
     bool      ProxyConnectFailed() { return mProxyConnectFailed; }
 
     // setting mDontRouteViaWildCard to true means the transaction should only
     // be dispatched on a specific ConnectionInfo Hash Key (as opposed to a
     // generic wild card one). That means in the specific case of carrying this
@@ -176,17 +180,17 @@ private:
                 mTrans->mConnection->SetSecurityCallbacks(mCallbacks);
             return NS_OK;
         }
       private:
         nsRefPtr<nsHttpTransaction> mTrans;
         nsCOMPtr<nsIInterfaceRequestor> mCallbacks;
     };
 
-    Mutex mCallbacksLock;
+    Mutex mLock;
 
     nsCOMPtr<nsIInterfaceRequestor> mCallbacks;
     nsCOMPtr<nsITransportEventSink> mTransportSink;
     nsCOMPtr<nsIEventTarget>        mConsumerTarget;
     nsCOMPtr<nsISupports>           mSecurityInfo;
     nsCOMPtr<nsIAsyncInputStream>   mPipeIn;
     nsCOMPtr<nsIAsyncOutputStream>  mPipeOut;
     nsCOMPtr<nsILoadGroupConnectionInfo> mLoadGroupCI;