Bug 1466577 - Race condition in WebSocketChannel::StopSession. r=hurley a=IanN CLOSED TREE DONTBUILD SEAMONKEY_2_49_ESR_RELBRANCH
authorMichal Novotny <michal.novotny@gmail.com>
Tue, 24 Jul 2018 17:18:58 -0400
branchSEAMONKEY_2_49_ESR_RELBRANCH
changeset 357539 30a49fef33b4374b5e19998cdab9e9e0f4988eb9
parent 357538 d6e809ebaef8c685c157fe89a3babea9a7e3c229
child 357540 ad03f53f55099a25dcc19a59844623b3adb95932
push id7834
push userfrgrahl@gmx.net
push dateSun, 13 Jan 2019 12:17:02 +0000
treeherdermozilla-esr52@6e4ad8a8f2e8 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewershurley, IanN
bugs1466577
milestone52.9.1
Bug 1466577 - Race condition in WebSocketChannel::StopSession. r=hurley a=IanN CLOSED TREE DONTBUILD mozilla-esr52 SEAMONKEY_2_49_ESR_RELBRANCH This patch prevents calling WebSocketChannel::StopSession at the same time on main thread from WebSocketChannel::Close and on socket thread from WebSocketChannel::AbortSession.
netwerk/protocol/websocket/WebSocketChannel.cpp
netwerk/protocol/websocket/WebSocketChannel.h
--- a/netwerk/protocol/websocket/WebSocketChannel.cpp
+++ b/netwerk/protocol/websocket/WebSocketChannel.cpp
@@ -1176,17 +1176,18 @@ WebSocketChannel::WebSocketChannel() :
   mCurrentOutSent(0),
   mDynamicOutputSize(0),
   mDynamicOutput(nullptr),
   mPrivateBrowsing(false),
   mConnectionLogService(nullptr),
   mCountRecv(0),
   mCountSent(0),
   mAppId(NECKO_NO_APP_ID),
-  mIsInIsolatedMozBrowser(false)
+  mIsInIsolatedMozBrowser(false),
+  mMutex("WebSocketChannel::mMutex")
 {
   MOZ_ASSERT(NS_IsMainThread(), "not main thread");
 
   LOG(("WebSocketChannel::WebSocketChannel() %p\n", this));
 
   nsWSAdmissionManager::Init();
 
   mFramePtr = mBuffer = static_cast<uint8_t *>(moz_xmalloc(mBufferSize));
@@ -2184,17 +2185,17 @@ WebSocketChannel::PrimeNewOutgoingMessag
     do {
       uint8_t *buffer;
       static_assert(4 == sizeof(mask), "Size of the mask should be equal to 4");
       nsresult rv = mRandomGenerator->GenerateRandomBytes(sizeof(mask),
                                                           &buffer);
       if (NS_FAILED(rv)) {
         LOG(("WebSocketChannel::PrimeNewOutgoingMessage(): "
              "GenerateRandomBytes failure %x\n", rv));
-        StopSession(rv);
+        AbortSession(rv);
         return;
       }
       memcpy(&mask, buffer, sizeof(mask));
       free(buffer);
     } while (!mask);
     NetworkEndian::writeUint32(payload - sizeof(uint32_t), mask);
   }
 
@@ -2335,21 +2336,36 @@ WebSocketChannel::CleanupConnection()
 
   DecrementSessionCount();
 }
 
 void
 WebSocketChannel::StopSession(nsresult reason)
 {
   LOG(("WebSocketChannel::StopSession() %p [%x]\n", this, reason));
+  {
+    MutexAutoLock lock(mMutex);
+    if (mStopped) {
+      return;
+    }
+    mStopped = 1;
+  }
+
+  DoStopSession(reason);
+}
+
+void
+WebSocketChannel::DoStopSession(nsresult reason)
+{
+  LOG(("WebSocketChannel::DoStopSession() () %p [%x]\n", this, reason));
 
   // normally this should be called on socket thread, but it is ok to call it
   // from OnStartRequest before the socket thread machine has gotten underway
 
-  mStopped = 1;
+  MOZ_ASSERT(mStopped);
 
   if (!mOpenedHttpChannel) {
     // The HTTP channel information will never be used in this case
     NS_ReleaseOnMainThread(mChannel.forget());
     NS_ReleaseOnMainThread(mHttpChannel.forget());
     NS_ReleaseOnMainThread(mLoadGroup.forget());
     NS_ReleaseOnMainThread(mCallbacks.forget());
   }
@@ -2406,17 +2422,17 @@ WebSocketChannel::StopSession(nsresult r
     // that might be accrued by keeping this channel object around waiting for
     // the server. We handle the SHOULD by waiting a short time in the common
     // case, but not waiting in the case of high concurrency.
     //
     // Normally this will be taken care of in AbortSession() after mTCPClosed
     // is set when the server close arrives without waiting for the timeout to
     // expire.
 
-    LOG(("WebSocketChannel::StopSession: Wait for Server TCP close"));
+    LOG(("WebSocketChannel::DoStopSession: Wait for Server TCP close"));
 
     nsresult rv;
     mLingeringCloseTimer = do_CreateInstance("@mozilla.org/timer;1", &rv);
     if (NS_SUCCEEDED(rv))
       mLingeringCloseTimer->InitWithCallback(this, kLingeringCloseTimeout,
                                              nsITimer::TYPE_ONE_SHOT);
     else
       CleanupConnection();
@@ -2442,56 +2458,62 @@ WebSocketChannel::StopSession(nsresult r
 }
 
 void
 WebSocketChannel::AbortSession(nsresult reason)
 {
   LOG(("WebSocketChannel::AbortSession() %p [reason %x] stopped = %d\n",
        this, reason, !!mStopped));
 
+  MOZ_ASSERT(NS_FAILED(reason), "reason must be a failure!");
+
   // normally this should be called on socket thread, but it is ok to call it
   // from the main thread before StartWebsocketData() has completed
 
   // When we are failing we need to close the TCP connection immediately
   // as per 7.1.1
   mTCPClosed = true;
 
   if (mLingeringCloseTimer) {
     MOZ_ASSERT(mStopped, "Lingering without Stop");
     LOG(("WebSocketChannel:: Cleanup connection based on TCP Close"));
     CleanupConnection();
     return;
   }
 
-  if (mStopped)
-    return;
-  mStopped = 1;
-
-  if (mTransport && reason != NS_BASE_STREAM_CLOSED && !mRequestedClose &&
-      !mClientClosed && !mServerClosed && mConnecting == NOT_CONNECTING) {
-    mRequestedClose = 1;
-    mStopOnClose = reason;
-    mSocketThread->Dispatch(
-      new OutboundEnqueuer(this, new OutboundMessage(kMsgTypeFin, nullptr)),
-                           nsIEventTarget::DISPATCH_NORMAL);
-  } else {
-    StopSession(reason);
+  {
+    MutexAutoLock lock(mMutex);
+    if (mStopped) {
+      return;
+    }
+
+    if (mTransport && reason != NS_BASE_STREAM_CLOSED && !mRequestedClose &&
+        !mClientClosed && !mServerClosed && mDataStarted) {
+      mRequestedClose = 1;
+      mStopOnClose = reason;
+      mSocketThread->Dispatch(
+        new OutboundEnqueuer(this, new OutboundMessage(kMsgTypeFin, nullptr)),
+                             nsIEventTarget::DISPATCH_NORMAL);
+      return;
+    }
+
+    mStopped = 1;
   }
+
+  DoStopSession(reason);
 }
 
 // ReleaseSession is called on orderly shutdown
 void
 WebSocketChannel::ReleaseSession()
 {
   LOG(("WebSocketChannel::ReleaseSession() %p stopped = %d\n",
        this, !!mStopped));
   MOZ_ASSERT(PR_GetCurrentThread() == gSocketThread, "not socket thread");
 
-  if (mStopped)
-    return;
   StopSession(NS_OK);
 }
 
 void
 WebSocketChannel::IncrementSessionCount()
 {
   if (!mIncrementedSessionCount) {
     nsWSAdmissionManager::IncrementSessionCount();
@@ -2912,19 +2934,29 @@ WebSocketChannel::StartWebsocketData()
   nsresult rv;
 
   if (!IsOnTargetThread()) {
     return mTargetThread->Dispatch(
       NewRunnableMethod(this, &WebSocketChannel::StartWebsocketData),
       NS_DISPATCH_NORMAL);
   }
 
-  LOG(("WebSocketChannel::StartWebsocketData() %p", this));
-  MOZ_ASSERT(!mDataStarted, "StartWebsocketData twice");
-  mDataStarted = 1;
+  {
+    MutexAutoLock lock(mMutex);
+    LOG(("WebSocketChannel::StartWebsocketData() %p", this));
+    MOZ_ASSERT(!mDataStarted, "StartWebsocketData twice");
+
+    if (mStopped) {
+      LOG(("WebSocketChannel::StartWebsocketData channel already closed, not "
+           "starting data"));
+      return NS_ERROR_NOT_AVAILABLE;
+    }
+
+    mDataStarted = 1;
+  }
 
   rv = mSocketIn->AsyncWait(this, 0, 0, mSocketThread);
   if (NS_FAILED(rv)) {
     LOG(("WebSocketChannel::StartWebsocketData mSocketIn->AsyncWait() failed "
          "with error 0x%08x", rv));
     return mSocketThread->Dispatch(
       NewRunnableMethod<nsresult>(this,
                                   &WebSocketChannel::AbortSession,
@@ -3522,45 +3554,56 @@ NS_IMETHODIMP
 WebSocketChannel::Close(uint16_t code, const nsACString & reason)
 {
   LOG(("WebSocketChannel::Close() %p\n", this));
   MOZ_ASSERT(NS_IsMainThread(), "not main thread");
 
   // save the networkstats (bug 855949)
   SaveNetworkStats(true);
 
-  if (mRequestedClose) {
-    return NS_OK;
+  {
+    MutexAutoLock lock(mMutex);
+
+    if (mRequestedClose) {
+      return NS_OK;
+    }
+
+    if (mStopped) {
+      return NS_ERROR_NOT_AVAILABLE;
+    }
+
+    // The API requires the UTF-8 string to be 123 or less bytes
+    if (reason.Length() > 123)
+      return NS_ERROR_ILLEGAL_VALUE;
+
+    mRequestedClose = 1;
+    mScriptCloseReason = reason;
+    mScriptCloseCode = code;
+
+    if (mDataStarted) {
+      return mSocketThread->Dispatch(
+        new OutboundEnqueuer(this, new OutboundMessage(kMsgTypeFin, nullptr)),
+                             nsIEventTarget::DISPATCH_NORMAL);
+    }
+
+    mStopped = 1;
   }
 
-  // The API requires the UTF-8 string to be 123 or less bytes
-  if (reason.Length() > 123)
-    return NS_ERROR_ILLEGAL_VALUE;
-
-  mRequestedClose = 1;
-  mScriptCloseReason = reason;
-  mScriptCloseCode = code;
-
-  if (!mTransport || mConnecting != NOT_CONNECTING) {
-    nsresult rv;
-    if (code == CLOSE_GOING_AWAY) {
-      // Not an error: for example, tab has closed or navigated away
-      LOG(("WebSocketChannel::Close() GOING_AWAY without transport."));
-      rv = NS_OK;
-    } else {
-      LOG(("WebSocketChannel::Close() without transport - error."));
-      rv = NS_ERROR_NOT_CONNECTED;
-    }
-    StopSession(rv);
-    return rv;
-  }
-
-  return mSocketThread->Dispatch(
-      new OutboundEnqueuer(this, new OutboundMessage(kMsgTypeFin, nullptr)),
-                           nsIEventTarget::DISPATCH_NORMAL);
+  nsresult rv;
+  if (code == CLOSE_GOING_AWAY) {
+    // Not an error: for example, tab has closed or navigated away
+    LOG(("WebSocketChannel::Close() GOING_AWAY without transport."));
+    rv = NS_OK;
+  } else {
+    LOG(("WebSocketChannel::Close() without transport - error."));
+    rv = NS_ERROR_NOT_CONNECTED;
+ }
+
+  DoStopSession(rv);
+  return rv;
 }
 
 NS_IMETHODIMP
 WebSocketChannel::SendMsg(const nsACString &aMsg)
 {
   LOG(("WebSocketChannel::SendMsg() %p\n", this));
 
   return SendMsgCommon(&aMsg, false, aMsg.Length());
@@ -3924,23 +3967,21 @@ WebSocketChannel::OnInputStreamReady(nsI
     CountRecvBytes(count);
 
     if (rv == NS_BASE_STREAM_WOULD_BLOCK) {
       mSocketIn->AsyncWait(this, 0, 0, mSocketThread);
       return NS_OK;
     }
 
     if (NS_FAILED(rv)) {
-      mTCPClosed = true;
       AbortSession(rv);
       return rv;
     }
 
     if (count == 0) {
-      mTCPClosed = true;
       AbortSession(NS_BASE_STREAM_CLOSED);
       return NS_OK;
     }
 
     if (mStopped) {
       continue;
     }
 
--- a/netwerk/protocol/websocket/WebSocketChannel.h
+++ b/netwerk/protocol/websocket/WebSocketChannel.h
@@ -161,16 +161,17 @@ private:
   nsresult SetupRequest();
   nsresult ApplyForAdmission();
   nsresult DoAdmissionDNS();
   nsresult StartWebsocketData();
   uint16_t ResultToCloseCode(nsresult resultCode);
   void     ReportConnectionTelemetry();
 
   void StopSession(nsresult reason);
+  void DoStopSession(nsresult reason);
   void AbortSession(nsresult reason);
   void ReleaseSession();
   void CleanupConnection();
   void IncrementSessionCount();
   void DecrementSessionCount();
 
   void EnsureHdrOut(uint32_t size);
 
@@ -296,16 +297,18 @@ private:
   uint8_t                         mOutHeader[kCopyBreak + 16];
   nsAutoPtr<PMCECompression>      mPMCECompressor;
   uint32_t                        mDynamicOutputSize;
   uint8_t                        *mDynamicOutput;
   bool                            mPrivateBrowsing;
 
   nsCOMPtr<nsIDashboardEventNotifier> mConnectionLogService;
 
+  mozilla::Mutex mMutex;
+
 // These members are used for network per-app metering (bug 855949)
 // Currently, they are only available on gonk.
   Atomic<uint64_t, Relaxed>       mCountRecv;
   Atomic<uint64_t, Relaxed>       mCountSent;
   uint32_t                        mAppId;
   bool                            mIsInIsolatedMozBrowser;
 #ifdef MOZ_WIDGET_GONK
   nsMainThreadPtrHandle<nsINetworkInfo> mActiveNetworkInfo;