From b197d037c78a0b073964fff6401c09a1a3decf2c Mon Sep 17 00:00:00 2001 From: Valentin Gosu Date: Wed, 25 Feb 2015 20:23:23 +0200 Subject: [PATCH] Bug 1132572 - Fix data race for nsHttpTransaction's timing info r=mcmanus --- netwerk/protocol/http/nsHttpChannel.cpp | 14 +- netwerk/protocol/http/nsHttpTransaction.cpp | 160 ++++++++++++++++++-- netwerk/protocol/http/nsHttpTransaction.h | 19 ++- 3 files changed, 171 insertions(+), 22 deletions(-) diff --git a/netwerk/protocol/http/nsHttpChannel.cpp b/netwerk/protocol/http/nsHttpChannel.cpp index 7e3a01a0036e..a8d0de2ffa6f 100644 --- a/netwerk/protocol/http/nsHttpChannel.cpp +++ b/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; diff --git a/netwerk/protocol/http/nsHttpTransaction.cpp b/netwerk/protocol/http/nsHttpTransaction.cpp index b1adf3a82775..684646419b32 100644 --- a/netwerk/protocol/http/nsHttpTransaction.cpp +++ b/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 //----------------------------------------------------------------------------- diff --git a/netwerk/protocol/http/nsHttpTransaction.h b/netwerk/protocol/http/nsHttpTransaction.h index 30252d6cd052..ed479c874af4 100644 --- a/netwerk/protocol/http/nsHttpTransaction.h +++ b/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();