Bug 1274509 - Use ReentrantMonitor for nsHttpRequestHead because VisitHeaders calls Visitor lock. r=mcmanus, a=sylvestre
authorDragana Damjanovic dd.mozilla@gmail.com
Sat, 04 Jun 2016 01:27:00 +0200
changeset 335370 52db915c585a7d3cb174e6cf722cde00e889c874
parent 335369 b142c2e3bf73c86c64ef9ec503dbb0dff08d85d0
child 335371 da61e46cf3c514597cdb9f7f1f5891eb60645dce
push id1146
push userCallek@gmail.com
push dateMon, 25 Jul 2016 16:35:44 +0000
treeherdermozilla-release@a55778f9cd5a [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmcmanus, sylvestre
bugs1274509
milestone48.0
Bug 1274509 - Use ReentrantMonitor for nsHttpRequestHead because VisitHeaders calls Visitor lock. r=mcmanus, a=sylvestre
netwerk/protocol/http/HttpBaseChannel.cpp
netwerk/protocol/http/HttpChannelParent.cpp
netwerk/protocol/http/nsHttpRequestHead.cpp
netwerk/protocol/http/nsHttpRequestHead.h
netwerk/protocol/http/nsIHttpChannel.idl
--- a/netwerk/protocol/http/HttpBaseChannel.cpp
+++ b/netwerk/protocol/http/HttpBaseChannel.cpp
@@ -1239,17 +1239,20 @@ HttpBaseChannel::GetReferrerPolicy(uint3
 NS_IMETHODIMP
 HttpBaseChannel::SetReferrerWithPolicy(nsIURI *referrer,
                                        uint32_t referrerPolicy)
 {
   ENSURE_CALLED_BEFORE_CONNECT();
 
   // clear existing referrer, if any
   mReferrer = nullptr;
-  mRequestHead.ClearHeader(nsHttp::Referer);
+  nsresult rv = mRequestHead.ClearHeader(nsHttp::Referer);
+  if(NS_FAILED(rv)) {
+    return rv;
+  }
   mReferrerPolicy = REFERRER_POLICY_NO_REFERRER_WHEN_DOWNGRADE;
 
   if (!referrer) {
     return NS_OK;
   }
 
   // Don't send referrer at all when the meta referrer setting is "no-referrer"
   if (referrerPolicy == REFERRER_POLICY_NO_REFERRER) {
@@ -1283,17 +1286,16 @@ HttpBaseChannel::SetReferrerWithPolicy(n
   } else {
     referrerLevel = 2; // inline content
   }
   if (userReferrerLevel < referrerLevel) {
     return NS_OK;
   }
 
   nsCOMPtr<nsIURI> referrerGrip;
-  nsresult rv;
   bool match;
 
   //
   // Strip off "wyciwyg://123/" from wyciwyg referrers.
   //
   // XXX this really belongs elsewhere since wyciwyg URLs aren't part of necko.
   //   perhaps some sort of generic nsINestedURI could be used.  then, if an URI
   //   fails the whitelist test, then we could check for an inner URI and try
--- a/netwerk/protocol/http/HttpChannelParent.cpp
+++ b/netwerk/protocol/http/HttpChannelParent.cpp
@@ -1053,33 +1053,33 @@ HttpChannelParent::OnStartRequest(nsIReq
 
     nsresult rv = container->GetData(&cacheKeyValue);
     if (NS_FAILED(rv)) {
       return rv;
     }
   }
 
   // !!! We need to lock headers and please don't forget to unlock them !!!
-  requestHead->Lock();
+  requestHead->Enter();
   nsresult rv = NS_OK;
   if (mIPCClosed ||
       !SendOnStartRequest(channelStatus,
                           responseHead ? *responseHead : nsHttpResponseHead(),
                           !!responseHead,
                           requestHead->Headers(),
                           isFromCache,
                           mCacheEntry ? true : false,
                           expirationTime, cachedCharset, secInfoSerialization,
                           mChannel->GetSelfAddr(), mChannel->GetPeerAddr(),
                           redirectCount,
                           cacheKeyValue))
   {
     rv = NS_ERROR_UNEXPECTED;
   }
-  requestHead->Unlock();
+  requestHead->Exit();
   return rv;
 }
 
 NS_IMETHODIMP
 HttpChannelParent::OnStopRequest(nsIRequest *aRequest,
                                  nsISupports *aContext,
                                  nsresult aStatusCode)
 {
--- a/netwerk/protocol/http/nsHttpRequestHead.cpp
+++ b/netwerk/protocol/http/nsHttpRequestHead.cpp
@@ -16,222 +16,258 @@
 namespace mozilla {
 namespace net {
 
 nsHttpRequestHead::nsHttpRequestHead()
     : mMethod(NS_LITERAL_CSTRING("GET"))
     , mVersion(NS_HTTP_VERSION_1_1)
     , mParsedMethod(kMethod_Get)
     , mHTTPS(false)
-    , mLock("nsHttpRequestHead.mLock")
+    , mReentrantMonitor("nsHttpRequestHead.mReentrantMonitor")
+    , mInVisitHeaders(false)
 {
     MOZ_COUNT_CTOR(nsHttpRequestHead);
 }
 
 nsHttpRequestHead::~nsHttpRequestHead()
 {
     MOZ_COUNT_DTOR(nsHttpRequestHead);
 }
 
 // Don't use this function. It is only used by HttpChannelParent to avoid
 // copying of request headers!!!
 const nsHttpHeaderArray &
 nsHttpRequestHead::Headers() const
 {
-    mLock.AssertCurrentThreadOwns();
+    nsHttpRequestHead &curr = const_cast<nsHttpRequestHead&>(*this);
+    curr.mReentrantMonitor.AssertCurrentThreadIn();
     return mHeaders;
 }
 
 void
 nsHttpRequestHead::SetHeaders(const nsHttpHeaderArray& aHeaders)
 {
-    MutexAutoLock lock(mLock);
+    ReentrantMonitorAutoEnter mon(mReentrantMonitor);
     mHeaders = aHeaders;
 }
 
 void
 nsHttpRequestHead::SetVersion(nsHttpVersion version)
 {
-    MutexAutoLock lock(mLock);
+    ReentrantMonitorAutoEnter mon(mReentrantMonitor);
     mVersion = version;
 }
 
 void
 nsHttpRequestHead::SetRequestURI(const nsCSubstring &s)
 {
-    MutexAutoLock lock(mLock);
+    ReentrantMonitorAutoEnter mon(mReentrantMonitor);
     mRequestURI = s;
 }
 
 void
 nsHttpRequestHead::SetPath(const nsCSubstring &s)
 {
-    MutexAutoLock lock(mLock);
+    ReentrantMonitorAutoEnter mon(mReentrantMonitor);
     mPath = s;
 }
 
 uint32_t
 nsHttpRequestHead::HeaderCount()
 {
-    MutexAutoLock lock(mLock);
+    ReentrantMonitorAutoEnter mon(mReentrantMonitor);
     return mHeaders.Count();
 }
 
 nsresult
 nsHttpRequestHead::VisitHeaders(nsIHttpHeaderVisitor *visitor,
                                 nsHttpHeaderArray::VisitorFilter filter /* = nsHttpHeaderArray::eFilterAll*/)
 {
-    MutexAutoLock lock(mLock);
-    return mHeaders.VisitHeaders(visitor, filter);
+    ReentrantMonitorAutoEnter mon(mReentrantMonitor);
+    mInVisitHeaders = true;
+    nsresult rv = mHeaders.VisitHeaders(visitor, filter);
+    mInVisitHeaders = false;
+    return rv;
 }
 
 void
 nsHttpRequestHead::Method(nsACString &aMethod)
 {
-    MutexAutoLock lock(mLock);
+    ReentrantMonitorAutoEnter mon(mReentrantMonitor);
     aMethod = mMethod;
 }
 
 nsHttpVersion
 nsHttpRequestHead::Version()
 {
-    MutexAutoLock lock(mLock);
+    ReentrantMonitorAutoEnter mon(mReentrantMonitor);
     return mVersion;
 }
 
 void
 nsHttpRequestHead::RequestURI(nsACString &aRequestURI)
 {
-    MutexAutoLock lock(mLock);
+    ReentrantMonitorAutoEnter mon(mReentrantMonitor);
     aRequestURI = mRequestURI;
 }
 
 void
 nsHttpRequestHead::Path(nsACString &aPath)
 {
-    MutexAutoLock lock(mLock);
+    ReentrantMonitorAutoEnter mon(mReentrantMonitor);
     aPath = mPath.IsEmpty() ? mRequestURI : mPath;
 }
 
 void
 nsHttpRequestHead::SetHTTPS(bool val)
 {
-    MutexAutoLock lock(mLock);
+    ReentrantMonitorAutoEnter monk(mReentrantMonitor);
     mHTTPS = val;
 }
 
 void
 nsHttpRequestHead::Origin(nsACString &aOrigin)
 {
-    MutexAutoLock lock(mLock);
+    ReentrantMonitorAutoEnter mon(mReentrantMonitor);
     aOrigin = mOrigin;
 }
 
 nsresult
 nsHttpRequestHead::SetHeader(nsHttpAtom h, const nsACString &v,
                              bool m /*= false*/)
 {
-    MutexAutoLock lock(mLock);
+    ReentrantMonitorAutoEnter mon(mReentrantMonitor);
+
+    if (mInVisitHeaders) {
+        return NS_ERROR_FAILURE;
+    }
+
     return mHeaders.SetHeader(h, v, m);
 }
 
 nsresult
 nsHttpRequestHead::SetHeader(nsHttpAtom h, const nsACString &v, bool m,
                              nsHttpHeaderArray::HeaderVariety variety)
 {
-    MutexAutoLock lock(mLock);
+    ReentrantMonitorAutoEnter mon(mReentrantMonitor);
+
+    if (mInVisitHeaders) {
+        return NS_ERROR_FAILURE;
+    }
+
     return mHeaders.SetHeader(h, v, m, variety);
 }
 
 nsresult
 nsHttpRequestHead::SetEmptyHeader(nsHttpAtom h)
 {
-    MutexAutoLock lock(mLock);
+    ReentrantMonitorAutoEnter mon(mReentrantMonitor);
+
+    if (mInVisitHeaders) {
+        return NS_ERROR_FAILURE;
+    }
+
     return mHeaders.SetEmptyHeader(h);
 }
 
 nsresult
 nsHttpRequestHead::GetHeader(nsHttpAtom h, nsACString &v)
 {
     v.Truncate();
-    MutexAutoLock lock(mLock);
+    ReentrantMonitorAutoEnter mon(mReentrantMonitor);
     return mHeaders.GetHeader(h, v);
 }
 
-void
+nsresult
 nsHttpRequestHead::ClearHeader(nsHttpAtom h)
 {
-    MutexAutoLock lock(mLock);
+    ReentrantMonitorAutoEnter mon(mReentrantMonitor);
+
+    if (mInVisitHeaders) {
+        return NS_ERROR_FAILURE;
+    }
+
     mHeaders.ClearHeader(h);
+    return NS_OK;
 }
 
 void
 nsHttpRequestHead::ClearHeaders()
 {
-    MutexAutoLock lock(mLock);
+    ReentrantMonitorAutoEnter mon(mReentrantMonitor);
+
+    if (mInVisitHeaders) {
+        return;
+    }
+
     mHeaders.Clear();
 }
 
 bool
 nsHttpRequestHead::HasHeader(nsHttpAtom h)
 {
-  MutexAutoLock lock(mLock);
-  return mHeaders.HasHeader(h);
+    ReentrantMonitorAutoEnter mon(mReentrantMonitor);
+    return mHeaders.HasHeader(h);
 }
 
 bool
 nsHttpRequestHead::HasHeaderValue(nsHttpAtom h, const char *v)
 {
-    MutexAutoLock lock(mLock);
+    ReentrantMonitorAutoEnter mon(mReentrantMonitor);
     return mHeaders.HasHeaderValue(h, v);
 }
 
 nsresult
 nsHttpRequestHead::SetHeaderOnce(nsHttpAtom h, const char *v,
                                  bool merge /*= false */)
 {
-    MutexAutoLock lock(mLock);
+    ReentrantMonitorAutoEnter mon(mReentrantMonitor);
+
+    if (mInVisitHeaders) {
+        return NS_ERROR_FAILURE;
+    }
+
     if (!merge || !mHeaders.HasHeaderValue(h, v)) {
         return mHeaders.SetHeader(h, nsDependentCString(v), merge);
     }
     return NS_OK;
 }
 
 nsHttpRequestHead::ParsedMethodType
 nsHttpRequestHead::ParsedMethod()
 {
-    MutexAutoLock lock(mLock);
+    ReentrantMonitorAutoEnter mon(mReentrantMonitor);
     return mParsedMethod;
 }
 
 bool
 nsHttpRequestHead::EqualsMethod(ParsedMethodType aType)
 {
-    MutexAutoLock lock(mLock);
+    ReentrantMonitorAutoEnter mon(mReentrantMonitor);
     return mParsedMethod == aType;
 }
 
 void
 nsHttpRequestHead::ParseHeaderSet(char *buffer)
 {
-    MutexAutoLock lock(mLock);
+    ReentrantMonitorAutoEnter mon(mReentrantMonitor);
     mHeaders.ParseHeaderSet(buffer);
 }
 
 bool
 nsHttpRequestHead::IsHTTPS()
 {
-    MutexAutoLock lock(mLock);
+    ReentrantMonitorAutoEnter mon(mReentrantMonitor);
     return mHTTPS;
 }
 
 void
 nsHttpRequestHead::SetMethod(const nsACString &method)
 {
-    MutexAutoLock lock(mLock);
+    ReentrantMonitorAutoEnter mon(mReentrantMonitor);
     mParsedMethod = kMethod_Custom;
     mMethod = method;
     if (!strcmp(mMethod.get(), "GET")) {
         mParsedMethod = kMethod_Get;
     } else if (!strcmp(mMethod.get(), "POST")) {
         mParsedMethod = kMethod_Post;
     } else if (!strcmp(mMethod.get(), "OPTIONS")) {
         mParsedMethod = kMethod_Options;
@@ -245,30 +281,30 @@ nsHttpRequestHead::SetMethod(const nsACS
         mParsedMethod = kMethod_Trace;
     }
 }
 
 void
 nsHttpRequestHead::SetOrigin(const nsACString &scheme, const nsACString &host,
                              int32_t port)
 {
-    MutexAutoLock lock(mLock);
+    ReentrantMonitorAutoEnter mon(mReentrantMonitor);
     mOrigin.Assign(scheme);
     mOrigin.Append(NS_LITERAL_CSTRING("://"));
     mOrigin.Append(host);
     if (port >= 0) {
         mOrigin.Append(NS_LITERAL_CSTRING(":"));
         mOrigin.AppendInt(port);
     }
 }
 
 bool
 nsHttpRequestHead::IsSafeMethod()
 {
-    MutexAutoLock lock(mLock);
+    ReentrantMonitorAutoEnter mon(mReentrantMonitor);
     // This code will need to be extended for new safe methods, otherwise
     // they'll default to "not safe".
     if ((mParsedMethod == kMethod_Get) || (mParsedMethod == kMethod_Head) ||
         (mParsedMethod == kMethod_Options) || (mParsedMethod == kMethod_Trace)
        ) {
         return true;
     }
 
@@ -279,17 +315,17 @@ nsHttpRequestHead::IsSafeMethod()
     return (!strcmp(mMethod.get(), "PROPFIND") ||
             !strcmp(mMethod.get(), "REPORT") ||
             !strcmp(mMethod.get(), "SEARCH"));
 }
 
 void
 nsHttpRequestHead::Flatten(nsACString &buf, bool pruneProxyHeaders)
 {
-    MutexAutoLock lock(mLock);
+    ReentrantMonitorAutoEnter mon(mReentrantMonitor);
     // note: the first append is intentional.
 
     buf.Append(mMethod);
     buf.Append(' ');
     buf.Append(mRequestURI);
     buf.AppendLiteral(" HTTP/");
 
     switch (mVersion) {
--- a/netwerk/protocol/http/nsHttpRequestHead.h
+++ b/netwerk/protocol/http/nsHttpRequestHead.h
@@ -4,17 +4,17 @@
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 #ifndef nsHttpRequestHead_h__
 #define nsHttpRequestHead_h__
 
 #include "nsHttp.h"
 #include "nsHttpHeaderArray.h"
 #include "nsString.h"
-#include "mozilla/Mutex.h"
+#include "mozilla/ReentrantMonitor.h"
 
 class nsIHttpHeaderVisitor;
 
 namespace mozilla { namespace net {
 
 //-----------------------------------------------------------------------------
 // nsHttpRequestHead represents the request line and headers from an HTTP
 // request.
@@ -25,18 +25,18 @@ class nsHttpRequestHead
 public:
     nsHttpRequestHead();
     ~nsHttpRequestHead();
 
     // The following function is only used in HttpChannelParent to avoid
     // copying headers. If you use it be careful to do it only under
     // nsHttpRequestHead lock!!!
     const nsHttpHeaderArray &Headers() const;
-    void Lock() { mLock.Lock(); }
-    void Unlock() { mLock.Unlock(); }
+    void Enter() { mReentrantMonitor.Enter(); }
+    void Exit() { mReentrantMonitor.Exit(); }
 
     void SetHeaders(const nsHttpHeaderArray& aHeaders);
 
     void SetMethod(const nsACString &method);
     void SetVersion(nsHttpVersion version);
     void SetRequestURI(const nsCSubstring &s);
     void SetPath(const nsCSubstring &s);
     uint32_t HeaderCount();
@@ -58,17 +58,17 @@ public:
     void Origin(nsACString &aOrigin);
 
     nsresult SetHeader(nsHttpAtom h, const nsACString &v, bool m=false);
     nsresult SetHeader(nsHttpAtom h, const nsACString &v, bool m,
                        nsHttpHeaderArray::HeaderVariety variety);
     nsresult SetEmptyHeader(nsHttpAtom h);
     nsresult GetHeader(nsHttpAtom h, nsACString &v);
 
-    void ClearHeader(nsHttpAtom h);
+    nsresult ClearHeader(nsHttpAtom h);
     void ClearHeaders();
 
     bool HasHeaderValue(nsHttpAtom h, const char *v);
     // This function returns true if header is set even if it is an empty
     // header.
     bool HasHeader(nsHttpAtom h);
     void Flatten(nsACString &, bool pruneProxyHeaders = false);
 
@@ -109,15 +109,20 @@ private:
     // because this is used off the main thread
     nsCString         mRequestURI;
     nsCString         mPath;
 
     nsCString         mOrigin;
     ParsedMethodType  mParsedMethod;
     bool              mHTTPS;
 
-    Mutex             mLock;
+    // We are using ReentrantMonitor instead of a Mutex because VisitHeader
+    // function calls nsIHttpHeaderVisitor::VisitHeader while under lock. 
+    ReentrantMonitor  mReentrantMonitor;
+
+    // During VisitHeader we sould not allow cal to SetHeader.
+    bool mInVisitHeaders;
 };
 
 } // namespace net
 } // namespace mozilla
 
 #endif // nsHttpRequestHead_h__
--- a/netwerk/protocol/http/nsIHttpChannel.idl
+++ b/netwerk/protocol/http/nsIHttpChannel.idl
@@ -47,16 +47,18 @@ interface nsIHttpChannel : nsIChannel
      *
      * NOTE: The channel may silently refuse to set the Referer header if the
      * URI does not pass certain security checks (e.g., a "https://" URL will
      * never be sent as the referrer for a plaintext HTTP request).  The
      * implementation is not required to throw an exception when the referrer
      * URI is rejected.
      *
      * @throws NS_ERROR_IN_PROGRESS if set after the channel has been opened.
+     * @throws NS_ERROR_FAILURE if used for setting referrer during
+     *         visitRequestHeaders. Getting the value will not throw.
      */
     attribute nsIURI referrer;
 
     /**
      * Referrer policies. See ReferrerPolicy.h for more details.
      */
 
     /*   default state, a shorter name for no-referrer-when-downgrade   */
@@ -76,16 +78,17 @@ interface nsIHttpChannel : nsIChannel
      * Get the HTTP referrer policy.  The policy is retrieved from the meta
      * referrer tag, which can be one of many values (see ReferrerPolicy.h for
      * more details).
      */
     readonly attribute unsigned long referrerPolicy;
 
     /**
      * Set the HTTP referrer URI with a referrer policy.
+     * @throws NS_ERROR_FAILURE if called during visitRequestHeaders.
      */
     void setReferrerWithPolicy(in nsIURI referrer, in unsigned long referrerPolicy);
 
     /**
      * Returns the network protocol used to fetch the resource as identified
      * by the ALPN Protocol ID.
      *
      * @throws NS_ERROR_NOT_AVAILABLE if called before the response
@@ -142,16 +145,17 @@ interface nsIHttpChannel : nsIChannel
      *        which this flag is ignored is an implementation detail.  If this
      *        flag is false, then the header value will be replaced with the
      *        contents of |aValue|.
      *
      * If aValue is empty and aMerge is false, the header will be cleared.
      *
      * @throws NS_ERROR_IN_PROGRESS if called after the channel has been
      *         opened.
+     * @throws NS_ERROR_FAILURE if called during visitRequestHeaders.
      */
     void setRequestHeader(in ACString aHeader,
                           in ACString aValue,
                           in boolean aMerge);
 
     /**
      * Set a request header with empty value.
      *
@@ -161,16 +165,17 @@ interface nsIHttpChannel : nsIChannel
      * This method may only be called before the channel is opened.
      *
      * @param aHeader
      *        The case-insensitive name of the request header to set (e.g.,
      *        "Cookie").
      *
      * @throws NS_ERROR_IN_PROGRESS if called after the channel has been
      *         opened.
+     * @throws NS_ERROR_FAILURE if called during visitRequestHeaders.
      */
     void setEmptyRequestHeader(in ACString aHeader);
 
     /**
      * Call this method to visit all request headers.  Calling setRequestHeader
      * while visiting request headers has undefined behavior.  Don't do it!
      *
      * @param aVisitor