Bug 1132572 - Fix data race for nsHttpTransaction's timing info r=mcmanus

netwerk/protocol/http/nsHttpChannel.cpp

@@ -5155,7 +5155,7 @@ nsHttpChannel::GetDomainLookupStart(TimeStamp* _retval) {
     if (mDNSPrefetch && mDNSPrefetch->TimingsValid())
         *_retval = mDNSPrefetch->StartTimestamp();
     else if (mTransaction)
-        *_retval = mTransaction->Timings().domainLookupStart;
+        *_retval = mTransaction->GetDomainLookupStart();
     else
         *_retval = mTransactionTimings.domainLookupStart;
     return NS_OK;
@@ -5166,7 +5166,7 @@ nsHttpChannel::GetDomainLookupEnd(TimeStamp* _retval) {
     if (mDNSPrefetch && mDNSPrefetch->TimingsValid())
         *_retval = mDNSPrefetch->EndTimestamp();
     else if (mTransaction)
-        *_retval = mTransaction->Timings().domainLookupEnd;
+        *_retval = mTransaction->GetDomainLookupEnd();
     else
         *_retval = mTransactionTimings.domainLookupEnd;
     return NS_OK;
@@ -5175,7 +5175,7 @@ nsHttpChannel::GetDomainLookupEnd(TimeStamp* _retval) {
 NS_IMETHODIMP
 nsHttpChannel::GetConnectStart(TimeStamp* _retval) {
     if (mTransaction)
-        *_retval = mTransaction->Timings().connectStart;
+        *_retval = mTransaction->GetConnectStart();
     else
         *_retval = mTransactionTimings.connectStart;
     return NS_OK;
@@ -5184,7 +5184,7 @@ nsHttpChannel::GetConnectStart(TimeStamp* _retval) {
 NS_IMETHODIMP
 nsHttpChannel::GetConnectEnd(TimeStamp* _retval) {
     if (mTransaction)
-        *_retval = mTransaction->Timings().connectEnd;
+        *_retval = mTransaction->GetConnectEnd();
     else
         *_retval = mTransactionTimings.connectEnd;
     return NS_OK;
@@ -5193,7 +5193,7 @@ nsHttpChannel::GetConnectEnd(TimeStamp* _retval) {
 NS_IMETHODIMP
 nsHttpChannel::GetRequestStart(TimeStamp* _retval) {
     if (mTransaction)
-        *_retval = mTransaction->Timings().requestStart;
+        *_retval = mTransaction->GetRequestStart();
     else
         *_retval = mTransactionTimings.requestStart;
     return NS_OK;
@@ -5202,7 +5202,7 @@ nsHttpChannel::GetRequestStart(TimeStamp* _retval) {
 NS_IMETHODIMP
 nsHttpChannel::GetResponseStart(TimeStamp* _retval) {
     if (mTransaction)
-        *_retval = mTransaction->Timings().responseStart;
+        *_retval = mTransaction->GetResponseStart();
     else
         *_retval = mTransactionTimings.responseStart;
     return NS_OK;
@@ -5211,7 +5211,7 @@ nsHttpChannel::GetResponseStart(TimeStamp* _retval) {
 NS_IMETHODIMP
 nsHttpChannel::GetResponseEnd(TimeStamp* _retval) {
     if (mTransaction)
-        *_retval = mTransaction->Timings().responseEnd;
+        *_retval = mTransaction->GetResponseEnd();
     else
         *_retval = mTransactionTimings.responseEnd;
     return NS_OK;

netwerk/protocol/http/nsHttpTransaction.cpp

@@ -513,13 +513,13 @@ nsHttpTransaction::OnTransportStatus(nsITransport* transport,
 
     if (TimingEnabled()) {
         if (status == NS_NET_STATUS_RESOLVING_HOST) {
-            mTimings.domainLookupStart = TimeStamp::Now();
+            SetDomainLookupStart(TimeStamp::Now());
         } else if (status == NS_NET_STATUS_RESOLVED_HOST) {
-            mTimings.domainLookupEnd = TimeStamp::Now();
+            SetDomainLookupEnd(TimeStamp::Now());
         } else if (status == NS_NET_STATUS_CONNECTING_TO) {
-            mTimings.connectStart = TimeStamp::Now();
+            SetConnectStart(TimeStamp::Now());
         } else if (status == NS_NET_STATUS_CONNECTED_TO) {
-            mTimings.connectEnd = TimeStamp::Now();
+            SetConnectEnd(TimeStamp::Now());
         }
     }
 
@@ -634,9 +634,9 @@ nsHttpTransaction::ReadRequestSegment(nsIInputStream *stream,
     nsresult rv = trans->mReader->OnReadSegment(buf, count, countRead);
     if (NS_FAILED(rv)) return rv;
 
-    if (trans->TimingEnabled() && trans->mTimings.requestStart.IsNull()) {
-        // First data we're sending -> this is requestStart
-        trans->mTimings.requestStart = TimeStamp::Now();
+    if (trans->TimingEnabled()) {
+        // Set the timestamp to Now(), only if it null
+        trans->SetRequestStart(TimeStamp::Now(), true);
     }
 
     if (!trans->mSentData) {
@@ -704,8 +704,9 @@ nsHttpTransaction::WritePipeSegment(nsIOutputStream *stream,
     if (trans->mTransactionDone)
         return NS_BASE_STREAM_CLOSED; // stop iterating
 
-    if (trans->TimingEnabled() && trans->mTimings.responseStart.IsNull()) {
-        trans->mTimings.responseStart = TimeStamp::Now();
+    if (trans->TimingEnabled()) {
+        // Set the timestamp to Now(), only if it null
+        trans->SetResponseStart(TimeStamp::Now(), true);
     }
 
     nsresult rv;
@@ -972,9 +973,12 @@ 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 (TimingEnabled()) {
+        const TimingStruct timings = Timings();
+        if (timings.responseEnd.IsNull() && !timings.responseStart.IsNull()) {
+            SetResponseEnd(TimeStamp::Now());
+        }
+    }
 
     if (relConn && mConnection) {
         MutexAutoLock lock(mLock);
@@ -1655,8 +1659,9 @@ nsHttpTransaction::HandleContent(char *buf,
         mResponseIsComplete = true;
         ReleaseBlockingTransaction();
 
-        if (TimingEnabled())
-            mTimings.responseEnd = TimeStamp::Now();
+        if (TimingEnabled()) {
+            SetResponseEnd(TimeStamp::Now());
+        }
 
         // report the entire response has arrived
         if (mActivityDistributor)
@@ -1844,6 +1849,133 @@ nsHttpTransaction::DisableSpdy()
     }
 }
 
+const TimingStruct
+nsHttpTransaction::Timings()
+{
+    mozilla::MutexAutoLock lock(mLock);
+    TimingStruct timings = mTimings;
+    return timings;
+}
+
+void
+nsHttpTransaction::SetDomainLookupStart(mozilla::TimeStamp timeStamp, bool onlyIfNull)
+{
+    mozilla::MutexAutoLock lock(mLock);
+    if (onlyIfNull && !mTimings.domainLookupStart.IsNull()) {
+        return; // We only set the timestamp if it was previously null
+    }
+    mTimings.domainLookupStart = timeStamp;
+}
+
+void
+nsHttpTransaction::SetDomainLookupEnd(mozilla::TimeStamp timeStamp, bool onlyIfNull)
+{
+    mozilla::MutexAutoLock lock(mLock);
+    if (onlyIfNull && !mTimings.domainLookupEnd.IsNull()) {
+        return; // We only set the timestamp if it was previously null
+    }
+    mTimings.domainLookupEnd = timeStamp;
+}
+
+void
+nsHttpTransaction::SetConnectStart(mozilla::TimeStamp timeStamp, bool onlyIfNull)
+{
+    mozilla::MutexAutoLock lock(mLock);
+    if (onlyIfNull && !mTimings.connectStart.IsNull()) {
+        return; // We only set the timestamp if it was previously null
+    }
+    mTimings.connectStart = timeStamp;
+}
+
+void
+nsHttpTransaction::SetConnectEnd(mozilla::TimeStamp timeStamp, bool onlyIfNull)
+{
+    mozilla::MutexAutoLock lock(mLock);
+    if (onlyIfNull && !mTimings.connectEnd.IsNull()) {
+        return; // We only set the timestamp if it was previously null
+    }
+    mTimings.connectEnd = timeStamp;
+}
+
+void
+nsHttpTransaction::SetRequestStart(mozilla::TimeStamp timeStamp, bool onlyIfNull)
+{
+    mozilla::MutexAutoLock lock(mLock);
+    if (onlyIfNull && !mTimings.requestStart.IsNull()) {
+        return; // We only set the timestamp if it was previously null
+    }
+    mTimings.requestStart = timeStamp;
+}
+
+void
+nsHttpTransaction::SetResponseStart(mozilla::TimeStamp timeStamp, bool onlyIfNull)
+{
+    mozilla::MutexAutoLock lock(mLock);
+    if (onlyIfNull && !mTimings.responseStart.IsNull()) {
+        return; // We only set the timestamp if it was previously null
+    }
+    mTimings.responseStart = timeStamp;
+}
+
+void
+nsHttpTransaction::SetResponseEnd(mozilla::TimeStamp timeStamp, bool onlyIfNull)
+{
+    mozilla::MutexAutoLock lock(mLock);
+    if (onlyIfNull && !mTimings.responseEnd.IsNull()) {
+        return; // We only set the timestamp if it was previously null
+    }
+    mTimings.responseEnd = timeStamp;
+}
+
+mozilla::TimeStamp
+nsHttpTransaction::GetDomainLookupStart()
+{
+    mozilla::MutexAutoLock lock(mLock);
+    return mTimings.domainLookupStart;
+}
+
+mozilla::TimeStamp
+nsHttpTransaction::GetDomainLookupEnd()
+{
+    mozilla::MutexAutoLock lock(mLock);
+    return mTimings.domainLookupEnd;
+}
+
+mozilla::TimeStamp
+nsHttpTransaction::GetConnectStart()
+{
+    mozilla::MutexAutoLock lock(mLock);
+    return mTimings.connectStart;
+}
+
+mozilla::TimeStamp
+nsHttpTransaction::GetConnectEnd()
+{
+    mozilla::MutexAutoLock lock(mLock);
+    return mTimings.connectEnd;
+}
+
+mozilla::TimeStamp
+nsHttpTransaction::GetRequestStart()
+{
+    mozilla::MutexAutoLock lock(mLock);
+    return mTimings.requestStart;
+}
+
+mozilla::TimeStamp
+nsHttpTransaction::GetResponseStart()
+{
+    mozilla::MutexAutoLock lock(mLock);
+    return mTimings.responseStart;
+}
+
+mozilla::TimeStamp
+nsHttpTransaction::GetResponseEnd()
+{
+    mozilla::MutexAutoLock lock(mLock);
+    return mTimings.responseEnd;
+}
+
 //-----------------------------------------------------------------------------
 // nsHttpTransaction deletion event
 //-----------------------------------------------------------------------------

netwerk/protocol/http/nsHttpTransaction.h

@@ -114,7 +114,6 @@ public:
     void    SetPriority(int32_t priority) { mPriority = priority; }
     int32_t    Priority()                 { return mPriority; }
 
-    const TimingStruct& Timings() const { return mTimings; }
     enum Classifier Classification() { return mClassification; }
 
     void PrintDiagnostics(nsCString &log);
@@ -141,6 +140,24 @@ public:
     }
     void SetPushedStream(Http2PushedStream *push) { mPushedStream = push; }
 
+    // Locked methods to get and set timing info
+    const TimingStruct Timings();
+    void SetDomainLookupStart(mozilla::TimeStamp timeStamp, bool onlyIfNull = false);
+    void SetDomainLookupEnd(mozilla::TimeStamp timeStamp, bool onlyIfNull = false);
+    void SetConnectStart(mozilla::TimeStamp timeStamp, bool onlyIfNull = false);
+    void SetConnectEnd(mozilla::TimeStamp timeStamp, bool onlyIfNull = false);
+    void SetRequestStart(mozilla::TimeStamp timeStamp, bool onlyIfNull = false);
+    void SetResponseStart(mozilla::TimeStamp timeStamp, bool onlyIfNull = false);
+    void SetResponseEnd(mozilla::TimeStamp timeStamp, bool onlyIfNull = false);
+
+    mozilla::TimeStamp GetDomainLookupStart();
+    mozilla::TimeStamp GetDomainLookupEnd();
+    mozilla::TimeStamp GetConnectStart();
+    mozilla::TimeStamp GetConnectEnd();
+    mozilla::TimeStamp GetRequestStart();
+    mozilla::TimeStamp GetResponseStart();
+    mozilla::TimeStamp GetResponseEnd();
+
 private:
     friend class DeleteHttpTransaction;
     virtual ~nsHttpTransaction();