Bug 1274509 - Use ReentrantMonitor for nsHttpRequestHead because VisitHeaders calls Visitor lock. r=mcmanus

netwerk/protocol/http/HttpBaseChannel.cpp

@@ -1243,7 +1243,10 @@ HttpBaseChannel::SetReferrerWithPolicy(nsIURI *referrer,
 
   // 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) {

netwerk/protocol/http/HttpChannelParent.cpp

@@ -1057,7 +1057,7 @@ HttpChannelParent::OnStartRequest(nsIRequest *aRequest, nsISupports *aContext)
   }
 
   // !!! 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,
@@ -1073,7 +1073,7 @@ HttpChannelParent::OnStartRequest(nsIRequest *aRequest, nsISupports *aContext)
   {
     rv = NS_ERROR_UNEXPECTED;
   }
-  requestHead->Unlock();
+  requestHead->Exit();
   return rv;
 }
 

netwerk/protocol/http/nsHttpRequestHead.cpp

@@ -21,7 +21,8 @@ nsHttpRequestHead::nsHttpRequestHead()
     , mVersion(NS_HTTP_VERSION_1_1)
     , mParsedMethod(kMethod_Get)
     , mHTTPS(false)
-    , mLock("nsHttpRequestHead.mLock")
+    , mReentrantMonitor("nsHttpRequestHead.mReentrantMonitor")
+    , mInVisitHeaders(false)
 {
     MOZ_COUNT_CTOR(nsHttpRequestHead);
 }
@@ -36,42 +37,43 @@ nsHttpRequestHead::~nsHttpRequestHead()
 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();
 }
 
@@ -79,49 +81,52 @@ nsresult
 nsHttpRequestHead::VisitHeaders(nsIHttpHeaderVisitor *visitor,
                                 nsHttpHeaderArray::VisitorFilter filter /* = nsHttpHeaderArray::eFilterAll*/)
 {
-    MutexAutoLock lock(mLock);
+    ReentrantMonitorAutoEnter mon(mReentrantMonitor);
-    return mHeaders.VisitHeaders(visitor, filter);
+    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;
 }
 
@@ -129,7 +134,12 @@ 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,
                               nsHttpHeaderArray::eVarietyRequestOverride);
 }
@@ -138,14 +148,24 @@ 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,
                                    nsHttpHeaderArray::eVarietyRequestOverride);
 }
@@ -154,35 +174,45 @@ nsresult
 nsHttpRequestHead::GetHeader(nsHttpAtom h, nsACString &v)
 {
     v.Truncate();
-    MutexAutoLock lock(mLock);
+    ReentrantMonitorAutoEnter mon(mReentrantMonitor);
     return mHeaders.GetHeader(h, v);
 }
 
 void
 nsHttpRequestHead::ClearHeader(nsHttpAtom h)
 {
-    MutexAutoLock lock(mLock);
+    ReentrantMonitorAutoEnter mon(mReentrantMonitor);
+
+    if (mInVisitHeaders) {
+        return;
+    }
+
     mHeaders.ClearHeader(h);
 }
 
 void
 nsHttpRequestHead::ClearHeaders()
 {
-    MutexAutoLock lock(mLock);
+    ReentrantMonitorAutoEnter mon(mReentrantMonitor);
+
+    if (mInVisitHeaders) {
+        return;
+    }
+
     mHeaders.Clear();
 }
 
 bool
 nsHttpRequestHead::HasHeader(nsHttpAtom h)
 {
-  MutexAutoLock lock(mLock);
+    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);
 }
 
@@ -190,7 +220,12 @@ 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,
                                   nsHttpHeaderArray::eVarietyRequestOverride);
@@ -201,21 +236,21 @@ nsHttpRequestHead::SetHeaderOnce(nsHttpAtom h, const char *v,
 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);
     nsHttpAtom hdr;
     char *val;
     while (buffer) {
@@ -239,14 +274,14 @@ nsHttpRequestHead::ParseHeaderSet(char *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")) {
@@ -270,7 +305,7 @@ 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);
@@ -283,7 +318,7 @@ nsHttpRequestHead::SetOrigin(const nsACString &scheme, const nsACString &host,
 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) ||
@@ -304,7 +339,7 @@ nsHttpRequestHead::IsSafeMethod()
 void
 nsHttpRequestHead::Flatten(nsACString &buf, bool pruneProxyHeaders)
 {
-    MutexAutoLock lock(mLock);
+    ReentrantMonitorAutoEnter mon(mReentrantMonitor);
     // note: the first append is intentional.
 
     buf.Append(mMethod);

netwerk/protocol/http/nsHttpRequestHead.h

@@ -9,7 +9,7 @@
 #include "nsHttp.h"
 #include "nsHttpHeaderArray.h"
 #include "nsString.h"
-#include "mozilla/Mutex.h"
+#include "mozilla/ReentrantMonitor.h"
 
 class nsIHttpHeaderVisitor;
 
@@ -30,8 +30,8 @@ public:
     // copying headers. If you use it be careful to do it only under
     // nsHttpRequestHead lock!!!
     const nsHttpHeaderArray &Headers() const;
-    void Lock() { mLock.Lock(); }
+    void Enter() { mReentrantMonitor.Enter(); }
-    void Unlock() { mLock.Unlock(); }
+    void Exit() { mReentrantMonitor.Exit(); }
 
     void SetHeaders(const nsHttpHeaderArray& aHeaders);
 
@@ -114,7 +114,12 @@ private:
     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

netwerk/protocol/http/nsIHttpChannel.idl

@@ -52,6 +52,8 @@ interface nsIHttpChannel : nsIChannel
      * 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;
 
@@ -86,6 +88,7 @@ interface nsIHttpChannel : nsIChannel
 
     /**
      * 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);
 
@@ -152,6 +155,7 @@ interface nsIHttpChannel : nsIChannel
      *
      * @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,
@@ -171,6 +175,7 @@ interface nsIHttpChannel : nsIChannel
      *
      * @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);