Bug 1159280 - TSan: data race netwerk/protocol/websocket/WebSocketChannel.cpp:3156 WebSocketChannel::Close, r=mcmanus

netwerk/protocol/websocket/BaseWebSocketChannel.cpp

@@ -19,10 +19,10 @@ namespace mozilla {
 namespace net {
 
 BaseWebSocketChannel::BaseWebSocketChannel()
-  : mEncrypted(0)
-  , mWasOpened(0)
+  : mWasOpened(0)
   , mClientSetPingInterval(0)
   , mClientSetPingTimeout(0)
+  , mEncrypted(0)
   , mPingForced(0)
   , mPingInterval(0)
   , mPingResponseTimeout(10000)
@@ -145,6 +145,8 @@ BaseWebSocketChannel::GetPingInterval(uint32_t *aSeconds)
 NS_IMETHODIMP
 BaseWebSocketChannel::SetPingInterval(uint32_t aSeconds)
 {
+  MOZ_ASSERT(NS_IsMainThread());
+
   if (mWasOpened) {
     return NS_ERROR_IN_PROGRESS;
   }
@@ -168,6 +170,8 @@ BaseWebSocketChannel::GetPingTimeout(uint32_t *aSeconds)
 NS_IMETHODIMP
 BaseWebSocketChannel::SetPingTimeout(uint32_t aSeconds)
 {
+  MOZ_ASSERT(NS_IsMainThread());
+
   if (mWasOpened) {
     return NS_ERROR_IN_PROGRESS;
   }

netwerk/protocol/websocket/BaseWebSocketChannel.h

@@ -89,11 +89,12 @@ class BaseWebSocketChannel : public nsIWebSocketChannel,
 
   nsCString                       mNegotiatedExtensions;
 
-  uint32_t                        mEncrypted                 : 1;
   uint32_t                        mWasOpened                 : 1;
   uint32_t                        mClientSetPingInterval     : 1;
   uint32_t                        mClientSetPingTimeout      : 1;
-  uint32_t                        mPingForced                : 1;
+
+  Atomic<bool>                    mEncrypted;
+  bool                            mPingForced;
 
   uint32_t                        mPingInterval;         /* milliseconds */
   uint32_t                        mPingResponseTimeout;  /* milliseconds */

netwerk/protocol/websocket/WebSocketChannel.cpp

@@ -1114,20 +1114,20 @@ WebSocketChannel::WebSocketChannel() :
   mMaxConcurrentConnections(200),
   mGotUpgradeOK(0),
   mRecvdHttpUpgradeTransport(0),
+  mAutoFollowRedirects(0),
+  mAllowPMCE(1),
+  mPingOutstanding(0),
+  mReleaseOnTransmit(0),
+  mDataStarted(0),
   mRequestedClose(0),
   mClientClosed(0),
   mServerClosed(0),
   mStopped(0),
   mCalledOnStop(0),
-  mPingOutstanding(0),
-  mAutoFollowRedirects(0),
-  mReleaseOnTransmit(0),
   mTCPClosed(0),
   mOpenedHttpChannel(0),
-  mDataStarted(0),
   mIncrementedSessionCount(0),
   mDecrementedSessionCount(0),
-  mAllowPMCE(1),
   mMaxMessageSize(INT32_MAX),
   mStopOnClose(NS_OK),
   mServerCloseCode(CLOSE_ABNORMAL),
@@ -2228,6 +2228,13 @@ WebSocketChannel::StopSession(nsresult 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
+  if (NS_IsMainThread()) {
+    MOZ_DIAGNOSTIC_ASSERT(!mDataStarted);
+  } else {
+    MOZ_DIAGNOSTIC_ASSERT(PR_GetCurrentThread() == gSocketThread,
+                          "Called on unexpected thread!");
+    MOZ_DIAGNOSTIC_ASSERT(mDataStarted);
+  }
 
   mStopped = 1;
 
@@ -2330,10 +2337,17 @@ void
 WebSocketChannel::AbortSession(nsresult reason)
 {
   LOG(("WebSocketChannel::AbortSession() %p [reason %x] stopped = %d\n",
-       this, reason, mStopped));
+       this, reason, !!mStopped));
 
   // normally this should be called on socket thread, but it is ok to call it
   // from the main thread before StartWebsocketData() has completed
+  if (NS_IsMainThread()) {
+    MOZ_DIAGNOSTIC_ASSERT(!mDataStarted);
+  } else {
+    MOZ_DIAGNOSTIC_ASSERT(PR_GetCurrentThread() == gSocketThread,
+                          "Called on unexpected thread!");
+    MOZ_DIAGNOSTIC_ASSERT(mDataStarted);
+  }
 
   // When we are failing we need to close the TCP connection immediately
   // as per 7.1.1
@@ -2367,7 +2381,7 @@ void
 WebSocketChannel::ReleaseSession()
 {
   LOG(("WebSocketChannel::ReleaseSession() %p stopped = %d\n",
-       this, mStopped));
+       this, !!mStopped));
   MOZ_ASSERT(PR_GetCurrentThread() == gSocketThread, "not socket thread");
 
   if (mStopped)
@@ -3693,7 +3707,13 @@ WebSocketChannel::SaveNetworkStats(bool enforce)
     return NS_OK;
   }
 
-  if (mCountRecv <= 0 && mCountSent <= 0) {
+  uint64_t countRecv = 0;
+  uint64_t countSent = 0;
+
+  mCountRecv.exchange(countRecv);
+  mCountSent.exchange(countSent);
+
+  if (countRecv == 0 && countSent == 0) {
     // There is no traffic, no need to save.
     return NS_OK;
   }
@@ -3701,7 +3721,7 @@ WebSocketChannel::SaveNetworkStats(bool enforce)
   // If |enforce| is false, the traffic amount is saved
   // only when the total amount exceeds the predefined
   // threshold.
-  uint64_t totalBytes = mCountRecv + mCountSent;
+  uint64_t totalBytes = countRecv + countSent;
   if (!enforce && totalBytes < NETWORK_STATS_THRESHOLD) {
     return NS_OK;
   }
@@ -3710,13 +3730,9 @@ WebSocketChannel::SaveNetworkStats(bool enforce)
   // the event is then dispathed to the main thread.
   nsRefPtr<nsRunnable> event =
     new SaveNetworkStatsEvent(mAppId, mIsInBrowser, mActiveNetwork,
-                              mCountRecv, mCountSent, false);
+                              countRecv, countSent, false);
   NS_DispatchToMainThread(event);
 
-  // Reset the counters after saving.
-  mCountSent = 0;
-  mCountRecv = 0;
-
   return NS_OK;
 #else
   return NS_ERROR_NOT_IMPLEMENTED;

netwerk/protocol/websocket/WebSocketChannel.h

@@ -29,6 +29,7 @@
 #include "nsCOMPtr.h"
 #include "nsString.h"
 #include "nsDeque.h"
+#include "mozilla/Atomics.h"
 
 class nsIAsyncVerifyRedirectCallback;
 class nsIDashboardEventNotifier;
@@ -218,22 +219,28 @@ private:
 
   int32_t                         mMaxConcurrentConnections;
 
+  // following members are accessed only on the main thread
   uint32_t                        mGotUpgradeOK              : 1;
   uint32_t                        mRecvdHttpUpgradeTransport : 1;
-  uint32_t                        mRequestedClose            : 1;
-  uint32_t                        mClientClosed              : 1;
-  uint32_t                        mServerClosed              : 1;
-  uint32_t                        mStopped                   : 1;
-  uint32_t                        mCalledOnStop              : 1;
-  uint32_t                        mPingOutstanding           : 1;
   uint32_t                        mAutoFollowRedirects       : 1;
-  uint32_t                        mReleaseOnTransmit         : 1;
-  uint32_t                        mTCPClosed                 : 1;
-  uint32_t                        mOpenedHttpChannel         : 1;
-  uint32_t                        mDataStarted               : 1;
-  uint32_t                        mIncrementedSessionCount   : 1;
-  uint32_t                        mDecrementedSessionCount   : 1;
   uint32_t                        mAllowPMCE                 : 1;
+  uint32_t                                                   : 0;
+
+  // following members are accessed only on the socket thread
+  uint32_t                        mPingOutstanding           : 1;
+  uint32_t                        mReleaseOnTransmit         : 1;
+  uint32_t                                                   : 0;
+
+  Atomic<bool>                    mDataStarted;
+  Atomic<bool>                    mRequestedClose;
+  Atomic<bool>                    mClientClosed;
+  Atomic<bool>                    mServerClosed;
+  Atomic<bool>                    mStopped;
+  Atomic<bool>                    mCalledOnStop;
+  Atomic<bool>                    mTCPClosed;
+  Atomic<bool>                    mOpenedHttpChannel;
+  Atomic<bool>                    mIncrementedSessionCount;
+  Atomic<bool>                    mDecrementedSessionCount;
 
   int32_t                         mMaxMessageSize;
   nsresult                        mStopOnClose;
@@ -278,8 +285,8 @@ private:
 
 // These members are used for network per-app metering (bug 855949)
 // Currently, they are only available on gonk.
-  uint64_t                        mCountRecv;
-  uint64_t                        mCountSent;
+  Atomic<uint64_t, Relaxed>       mCountRecv;
+  Atomic<uint64_t, Relaxed>       mCountSent;
   uint32_t                        mAppId;
   bool                            mIsInBrowser;
 #ifdef MOZ_WIDGET_GONK