Bug 1405739 P1 Don't expose internal redirect start/end/count through performance timing API. r=valentin
authorBen Kelly <ben@wanderview.com>
Fri, 06 Oct 2017 09:04:54 -0700
changeset 676133 6c5488ade824e460bfe19d4b347b78cfa8960a79
parent 676132 fd99f3454c510f766a19f7c6ea86f6278227b5a0
child 676134 ba89a80854b5064befa08ee115a36ff0e5ac2a9e
push id83398
push userbmo:rail@mozilla.com
push dateFri, 06 Oct 2017 17:12:44 +0000
reviewersvalentin
bugs1405739
milestone58.0a1
Bug 1405739 P1 Don't expose internal redirect start/end/count through performance timing API. r=valentin
dom/performance/PerformanceTiming.cpp
dom/performance/PerformanceTiming.h
netwerk/base/nsITimedChannel.idl
netwerk/protocol/http/HttpBaseChannel.cpp
netwerk/protocol/http/HttpBaseChannel.h
netwerk/protocol/http/HttpChannelParent.cpp
netwerk/protocol/http/NullHttpChannel.cpp
netwerk/protocol/http/nsHttpChannel.cpp
--- a/dom/performance/PerformanceTiming.cpp
+++ b/dom/performance/PerformanceTiming.cpp
@@ -173,17 +173,17 @@ PerformanceTiming::CheckAllowedOrigin(ns
 }
 
 bool
 PerformanceTiming::TimingAllowed() const
 {
   return mTimingAllowed;
 }
 
-uint16_t
+uint8_t
 PerformanceTiming::GetRedirectCount() const
 {
   if (!nsContentUtils::IsPerformanceTimingEnabled() || !IsInitialized() ||
       nsContentUtils::ShouldResistFingerprinting()) {
     return 0;
   }
   if (!mAllRedirectsSameOrigin) {
     return 0;
--- a/dom/performance/PerformanceTiming.h
+++ b/dom/performance/PerformanceTiming.h
@@ -136,17 +136,17 @@ public:
   {
     if (!nsContentUtils::IsPerformanceTimingEnabled() ||
         nsContentUtils::ShouldResistFingerprinting()) {
       return 0;
     }
     return GetDOMTiming()->GetUnloadEventEnd();
   }
 
-  uint16_t GetRedirectCount() const;
+  uint8_t GetRedirectCount() const;
 
   // Checks if the resource is either same origin as the page that started
   // the load, or if the response contains the Timing-Allow-Origin header
   // with a value of * or matching the domain of the loading Principal
   bool CheckAllowedOrigin(nsIHttpChannel* aResourceChannel, nsITimedChannel* aChannel);
 
   // Cached result of CheckAllowedOrigin. If false, security sensitive
   // attributes of the resourceTiming object will be set to 0
@@ -280,17 +280,17 @@ private:
   TimeStamp mConnectStart;
   TimeStamp mSecureConnectionStart;
   TimeStamp mConnectEnd;
   TimeStamp mRequestStart;
   TimeStamp mResponseStart;
   TimeStamp mCacheReadStart;
   TimeStamp mResponseEnd;
   TimeStamp mCacheReadEnd;
-  uint16_t mRedirectCount;
+  uint8_t mRedirectCount;
   bool mTimingAllowed;
   bool mAllRedirectsSameOrigin;
   bool mInitialized;
 
   // If the resourceTiming object should have non-zero redirectStart and
   // redirectEnd attributes. It is false if there were no redirects, or if
   // any of the responses didn't pass the timing-allow-check
   bool mReportCrossOriginRedirect;
--- a/netwerk/base/nsITimedChannel.idl
+++ b/netwerk/base/nsITimedChannel.idl
@@ -16,17 +16,18 @@ native TimeStamp(mozilla::TimeStamp);
 [scriptable, uuid(ca63784d-959c-4c3a-9a59-234a2a520de0)]
 interface nsITimedChannel : nsISupports {
   // Set this attribute to true to enable collection of timing data.
   // channelCreationTime will be available even with this attribute set to
   // false.
   attribute boolean timingEnabled;
 
   // The number of redirects
-  attribute uint16_t redirectCount;
+  attribute uint8_t redirectCount;
+  attribute uint8_t internalRedirectCount;
 
   [noscript] readonly attribute TimeStamp channelCreation;
   [noscript] readonly attribute TimeStamp asyncOpen;
 
   // The following are only set when the request is intercepted by a service
   // worker no matter the response is synthesized.
   [noscript] attribute TimeStamp launchServiceWorkerStart;
   [noscript] attribute TimeStamp launchServiceWorkerEnd;
--- a/netwerk/protocol/http/HttpBaseChannel.cpp
+++ b/netwerk/protocol/http/HttpBaseChannel.cpp
@@ -188,16 +188,17 @@ HttpBaseChannel::HttpBaseChannel()
   , mTlsFlags(0)
   , mSuspendCount(0)
   , mInitialRwin(0)
   , mProxyResolveFlags(0)
   , mContentDispositionHint(UINT32_MAX)
   , mHttpHandler(gHttpHandler)
   , mReferrerPolicy(NS_GetDefaultReferrerPolicy())
   , mRedirectCount(0)
+  , mInternalRedirectCount(0)
   , mForcePending(false)
   , mCorsIncludeCredentials(false)
   , mCorsMode(nsIHttpChannelInternal::CORS_MODE_NO_CORS)
   , mRedirectMode(nsIHttpChannelInternal::REDIRECT_MODE_FOLLOW)
   , mFetchCacheMode(nsIHttpChannelInternal::FETCH_CACHE_MODE_DEFAULT)
   , mOnStartRequestCalled(false)
   , mOnStopRequestCalled(false)
   , mAfterOnStartRequestBegun(false)
@@ -3483,23 +3484,16 @@ HttpBaseChannel::SetupReplacementChannel
   // convey the referrer if one was used for this channel to the next one
   if (mReferrer) {
     rv = httpChannel->SetReferrerWithPolicy(mReferrer, mReferrerPolicy);
     MOZ_ASSERT(NS_SUCCEEDED(rv));
   }
   // convey the mAllowSTS flags
   rv = httpChannel->SetAllowSTS(mAllowSTS);
   MOZ_ASSERT(NS_SUCCEEDED(rv));
-  // convey the new redirection limit
-  // make sure we don't underflow
-  uint32_t redirectionLimit = mRedirectionLimit
-    ? mRedirectionLimit - 1
-    : 0;
-  rv = httpChannel->SetRedirectionLimit(redirectionLimit);
-  MOZ_ASSERT(NS_SUCCEEDED(rv));
 
   // convey the Accept header value
   {
     nsAutoCString oldAcceptValue;
     nsresult hasHeader = mRequestHead.GetHeader(nsHttp::Accept, oldAcceptValue);
     if (NS_SUCCEEDED(hasHeader)) {
       rv = httpChannel->SetRequestHeader(NS_LITERAL_CSTRING("Accept"),
                                          oldAcceptValue,
@@ -3597,33 +3591,50 @@ HttpBaseChannel::SetupReplacementChannel
   }
 
   // Transfer the timing data (if we are dealing with an nsITimedChannel).
   nsCOMPtr<nsITimedChannel> newTimedChannel(do_QueryInterface(newChannel));
   nsCOMPtr<nsITimedChannel> oldTimedChannel(
       do_QueryInterface(static_cast<nsIHttpChannel*>(this)));
   if (oldTimedChannel && newTimedChannel) {
     newTimedChannel->SetTimingEnabled(mTimingEnabled);
-    newTimedChannel->SetRedirectCount(mRedirectCount + 1);
+
+    if (redirectFlags & nsIChannelEventSink::REDIRECT_INTERNAL) {
+      int8_t newCount = mInternalRedirectCount + 1;
+      newTimedChannel->SetInternalRedirectCount(
+        std::max(newCount, mInternalRedirectCount));
+    } else {
+      int8_t newCount = mRedirectCount + 1;
+      newTimedChannel->SetRedirectCount(
+        std::max(newCount, mRedirectCount));
+    }
 
     // If the RedirectStart is null, we will use the AsyncOpen value of the
     // previous channel (this is the first redirect in the redirects chain).
     if (mRedirectStartTimeStamp.IsNull()) {
-      TimeStamp asyncOpen;
-      oldTimedChannel->GetAsyncOpen(&asyncOpen);
-      newTimedChannel->SetRedirectStart(asyncOpen);
-    }
-    else {
+      // Only do this for real redirects.  Internal redirects should be hidden.
+      if (!(redirectFlags & nsIChannelEventSink::REDIRECT_INTERNAL)) {
+        TimeStamp asyncOpen;
+        oldTimedChannel->GetAsyncOpen(&asyncOpen);
+        newTimedChannel->SetRedirectStart(asyncOpen);
+      }
+    } else {
       newTimedChannel->SetRedirectStart(mRedirectStartTimeStamp);
     }
 
-    // The RedirectEnd timestamp is equal to the previous channel response end.
-    TimeStamp prevResponseEnd;
-    oldTimedChannel->GetResponseEnd(&prevResponseEnd);
-    newTimedChannel->SetRedirectEnd(prevResponseEnd);
+    // For internal redirects just propagate the last redirect end time
+    // forward.  Otherwise the new redirect end time is the last response
+    // end time.
+    TimeStamp newRedirectEnd;
+    if (redirectFlags & nsIChannelEventSink::REDIRECT_INTERNAL) {
+      oldTimedChannel->GetRedirectEnd(&newRedirectEnd);
+    } else {
+      oldTimedChannel->GetResponseEnd(&newRedirectEnd);
+    }
+    newTimedChannel->SetRedirectEnd(newRedirectEnd);
 
     nsAutoString initiatorType;
     oldTimedChannel->GetInitiatorType(initiatorType);
     newTimedChannel->SetInitiatorType(initiatorType);
 
     // Check whether or not this was a cross-domain redirect.
     newTimedChannel->SetAllRedirectsSameOrigin(
         mAllRedirectsSameOrigin && SameOriginWithOriginalUri(newURI));
@@ -3736,30 +3747,44 @@ HttpBaseChannel::GetAsyncOpen(TimeStamp*
   return NS_OK;
 }
 
 /**
  * @return the number of redirects. There is no check for cross-domain
  * redirects. This check must be done by the consumers.
  */
 NS_IMETHODIMP
-HttpBaseChannel::GetRedirectCount(uint16_t *aRedirectCount)
+HttpBaseChannel::GetRedirectCount(uint8_t *aRedirectCount)
 {
   *aRedirectCount = mRedirectCount;
   return NS_OK;
 }
 
 NS_IMETHODIMP
-HttpBaseChannel::SetRedirectCount(uint16_t aRedirectCount)
+HttpBaseChannel::SetRedirectCount(uint8_t aRedirectCount)
 {
   mRedirectCount = aRedirectCount;
   return NS_OK;
 }
 
 NS_IMETHODIMP
+HttpBaseChannel::GetInternalRedirectCount(uint8_t *aRedirectCount)
+{
+  *aRedirectCount = mInternalRedirectCount;
+  return NS_OK;
+}
+
+NS_IMETHODIMP
+HttpBaseChannel::SetInternalRedirectCount(uint8_t aRedirectCount)
+{
+  mInternalRedirectCount = aRedirectCount;
+  return NS_OK;
+}
+
+NS_IMETHODIMP
 HttpBaseChannel::GetRedirectStart(TimeStamp* _retval)
 {
   *_retval = mRedirectStartTimeStamp;
   return NS_OK;
 }
 
 NS_IMETHODIMP
 HttpBaseChannel::SetRedirectStart(TimeStamp aRedirectStart)
--- a/netwerk/protocol/http/HttpBaseChannel.h
+++ b/netwerk/protocol/http/HttpBaseChannel.h
@@ -566,17 +566,19 @@ protected:
 
   uint32_t                          mReferrerPolicy;
 
   // Performance tracking
   // The initiator type (for this resource) - how was the resource referenced in
   // the HTML file.
   nsString                          mInitiatorType;
   // Number of redirects that has occurred.
-  int16_t                           mRedirectCount;
+  int8_t                            mRedirectCount;
+  // Number of internal redirects that has occurred.
+  int8_t                            mInternalRedirectCount;
   // A time value equal to the starting time of the fetch that initiates the
   // redirect.
   mozilla::TimeStamp                mRedirectStartTimeStamp;
   // A time value equal to the time immediately after receiving the last byte of
   // the response of the last redirect.
   mozilla::TimeStamp                mRedirectEndTimeStamp;
 
   PRTime                            mChannelCreationTime;
--- a/netwerk/protocol/http/HttpChannelParent.cpp
+++ b/netwerk/protocol/http/HttpChannelParent.cpp
@@ -1468,17 +1468,17 @@ HttpChannelParent::OnStartRequest(nsIReq
   mCacheEntry = do_QueryInterface(cacheEntry);
 
   nsresult channelStatus = NS_OK;
   chan->GetStatus(&channelStatus);
 
   nsCString secInfoSerialization;
   UpdateAndSerializeSecurityInfo(secInfoSerialization);
 
-  uint16_t redirectCount = 0;
+  uint8_t redirectCount = 0;
   chan->GetRedirectCount(&redirectCount);
 
   nsCOMPtr<nsISupports> cacheKey;
   chan->GetCacheKey(getter_AddRefs(cacheKey));
   uint32_t cacheKeyValue = 0;
   if (cacheKey) {
     nsCOMPtr<nsISupportsPRUint32> container = do_QueryInterface(cacheKey);
     if (!container) {
--- a/netwerk/protocol/http/NullHttpChannel.cpp
+++ b/netwerk/protocol/http/NullHttpChannel.cpp
@@ -559,23 +559,35 @@ NullHttpChannel::GetTimingEnabled(bool *
 
 NS_IMETHODIMP
 NullHttpChannel::SetTimingEnabled(bool aTimingEnabled)
 {
   return NS_ERROR_NOT_IMPLEMENTED;
 }
 
 NS_IMETHODIMP
-NullHttpChannel::GetRedirectCount(uint16_t *aRedirectCount)
+NullHttpChannel::GetRedirectCount(uint8_t *aRedirectCount)
 {
   return NS_ERROR_NOT_IMPLEMENTED;
 }
 
 NS_IMETHODIMP
-NullHttpChannel::SetRedirectCount(uint16_t aRedirectCount)
+NullHttpChannel::SetRedirectCount(uint8_t aRedirectCount)
+{
+  return NS_ERROR_NOT_IMPLEMENTED;
+}
+
+NS_IMETHODIMP
+NullHttpChannel::GetInternalRedirectCount(uint8_t *aRedirectCount)
+{
+  return NS_ERROR_NOT_IMPLEMENTED;
+}
+
+NS_IMETHODIMP
+NullHttpChannel::SetInternalRedirectCount(uint8_t aRedirectCount)
 {
   return NS_ERROR_NOT_IMPLEMENTED;
 }
 
 NS_IMETHODIMP
 NullHttpChannel::GetChannelCreation(mozilla::TimeStamp *aChannelCreation)
 {
   *aChannelCreation = mChannelCreationTimestamp;
--- a/netwerk/protocol/http/nsHttpChannel.cpp
+++ b/netwerk/protocol/http/nsHttpChannel.cpp
@@ -5691,17 +5691,17 @@ nsHttpChannel::AsyncProcessRedirection(u
     if (NS_FAILED(mResponseHead->GetHeader(nsHttp::Location, location)))
         return NS_ERROR_FAILURE;
 
     // make sure non-ASCII characters in the location header are escaped.
     nsAutoCString locationBuf;
     if (NS_EscapeURL(location.get(), -1, esc_OnlyNonASCII, locationBuf))
         location = locationBuf;
 
-    if (mRedirectionLimit == 0) {
+    if (mRedirectCount >= mRedirectionLimit || mInternalRedirectCount >= mRedirectionLimit) {
         LOG(("redirection limit reached!\n"));
         return NS_ERROR_REDIRECT_LOOP;
     }
 
     mRedirectType = redirectType;
 
     LOG(("redirecting to: %s [redirection-limit=%u]\n",
         location.get(), uint32_t(mRedirectionLimit)));