Bug 1749786 - websocketchannel cleanup r=necko-reviewers,kershaw

Differential Revision: https://phabricator.services.mozilla.com/D135727

Author: Randell Jesup

netwerk/protocol/websocket/BaseWebSocketChannel.h

@@ -103,7 +103,8 @@ class BaseWebSocketChannel : public nsIWebSocketChannel,
   nsCOMPtr<nsILoadInfo> mLoadInfo;
   nsCOMPtr<nsITransportProvider> mServerTransportProvider;
 
-  // Used to ensure atomicity of mTargetThread
+  // Used to ensure atomicity of mTargetThread.
+  // Set before AsyncOpen via RetargetDeliveryTo or in AsyncOpen, never changed after AsyncOpen
   DataMutex<nsCOMPtr<nsIEventTarget>> mTargetThread{
       "BaseWebSocketChannel::EventTargetMutex"};
 

netwerk/protocol/websocket/WebSocketChannel.cpp

@@ -240,6 +240,7 @@ class FailDelayManager {
         if (remainingDelay) {
           // reconnecting within delay interval: delay by remaining time
           nsresult rv;
+          MutexAutoLock lock(ws->mMutex);
           rv = NS_NewTimerWithCallback(getter_AddRefs(ws->mReconnectDelayTimer),
                                        ws, remainingDelay,
                                        nsITimer::TYPE_ONE_SHOT);
@@ -427,9 +428,14 @@ class nsWSAdmissionManager {
     // Only way a connecting channel may get here w/o failing is if it
     // was closed with GOING_AWAY (1001) because of navigation, tab
     // close, etc.
-    MOZ_ASSERT(
+#ifdef DEBUG
+    {
+      MutexAutoLock lock(aChannel->mMutex);
+      MOZ_ASSERT(
         NS_FAILED(aReason) || aChannel->mScriptCloseCode == CLOSE_GOING_AWAY,
         "websocket closed while connecting w/o failing?");
+    }
+#endif
     Unused << aReason;
 
     sManager->RemoveFromQueue(aChannel);
@@ -1331,8 +1337,10 @@ void WebSocketChannel::BeginOpen(bool aCalledFromAdmissionManager) {
   }
 }
 
+// MainThread
 void WebSocketChannel::BeginOpenInternal() {
   LOG(("WebSocketChannel::BeginOpenInternal() %p\n", this));
+  MOZ_ASSERT(NS_IsMainThread(), "not main thread");
 
   nsresult rv;
 
@@ -2017,6 +2025,7 @@ void WebSocketChannel::PrimeNewOutgoingMessage() {
     // If the channel user provided a code and reason during Close()
     // and there isn't an internal error, use that.
     if (NS_SUCCEEDED(mStopOnClose)) {
+      MutexAutoLock lock(mMutex);
       if (mScriptCloseCode) {
         NetworkEndian::writeUint16(payload, mScriptCloseCode);
         mOutHeader[1] += 2;
@@ -2236,7 +2245,17 @@ class RemoveObserverRunnable : public Runnable {
 }  // namespace
 
 void WebSocketChannel::CleanupConnection() {
+  // normally this should be called on socket thread, but it may be called
+  // on MainThread
+  
   LOG(("WebSocketChannel::CleanupConnection() %p", this));
+  // This needs to run on the IOThread so we don't need to lock a bunch of these
+  if (!mIOThread->IsOnCurrentThread()) {
+    mIOThread->Dispatch(
+        NewRunnableMethod("net::WebSocketChannel::CleanupConnection", this,
+                          &WebSocketChannel::CleanupConnection),
+        NS_DISPATCH_NORMAL);
+  }
 
   if (mLingeringCloseTimer) {
     mLingeringCloseTimer->Cancel();
@@ -2271,8 +2290,7 @@ void WebSocketChannel::CleanupConnection() {
     mConnectionLogService->RemoveHost(mHost, mSerial);
   }
 
-  // This method can run in any thread, but the observer has to be removed on
-  // the main-thread.
+  // The observer has to be removed on the main-thread.
   NS_DispatchToMainThread(new RemoveObserverRunnable(this));
 
   DecrementSessionCount();
@@ -2298,7 +2316,9 @@ void WebSocketChannel::DoStopSession(nsresult reason) {
        static_cast<uint32_t>(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
+  // from OnStartRequest before the socket thread machine has gotten underway.
+  // If mDataStarted is false, this is called on MainThread for Close().
+  // Otherwise it should be called on the IO thread
 
   MOZ_ASSERT(mStopped);
   MOZ_ASSERT(mIOThread->IsOnCurrentThread() || mTCPClosed || !mDataStarted);
@@ -2317,14 +2337,19 @@ void WebSocketChannel::DoStopSession(nsresult reason) {
     mCloseTimer = nullptr;
   }
 
+  // mOpenTimer must be null if mDataStarted is true and we're not on MainThread
   if (mOpenTimer) {
+    MOZ_ASSERT(NS_IsMainThread(), "not main thread");
     mOpenTimer->Cancel();
     mOpenTimer = nullptr;
   }
 
-  if (mReconnectDelayTimer) {
-    mReconnectDelayTimer->Cancel();
-    mReconnectDelayTimer = nullptr;
+  {
+    MutexAutoLock lock(mMutex);
+    if (mReconnectDelayTimer) {
+      mReconnectDelayTimer->Cancel();
+      NS_ReleaseOnMainThread("WebSocketChannel::mMutex", mReconnectDelayTimer.forget());
+    }
   }
 
   if (mPingTimer) {
@@ -2406,6 +2431,8 @@ void WebSocketChannel::DoStopSession(nsresult reason) {
   }
 }
 
+// Called from MainThread, and called from IOThread in
+// PrimeNewOutgoingMessage
 void WebSocketChannel::AbortSession(nsresult reason) {
   LOG(("WebSocketChannel::AbortSession() %p [reason %" PRIx32
        "] stopped = %d\n",
@@ -2415,6 +2442,7 @@ void WebSocketChannel::AbortSession(nsresult reason) {
 
   // normally this should be called on socket thread, but it is ok to call it
   // from the main thread before StartWebsocketData() has completed
+  MOZ_ASSERT(mIOThread->IsOnCurrentThread() || !mDataStarted);
 
   // When we are failing we need to close the TCP connection immediately
   // as per 7.1.1
@@ -3234,12 +3262,16 @@ WebSocketChannel::Notify(nsITimer* timer) {
     }
 
     AbortSession(NS_ERROR_NET_TIMEOUT);
+  // mReconnectDelayTimer is only modified on MainThread
   } else if (timer == mReconnectDelayTimer) {
-    MOZ_ASSERT(mConnecting == CONNECTING_DELAYED,
-               "woke up from delay w/o being delayed?");
+    MOZ_RELEASE_ASSERT(mConnecting == CONNECTING_DELAYED,
+                       "woke up from delay w/o being delayed?");
     MOZ_ASSERT(NS_IsMainThread(), "not main thread");
 
-    mReconnectDelayTimer = nullptr;
+    {
+      MutexAutoLock lock(mMutex);
+      mReconnectDelayTimer = nullptr;
+    }
     LOG(("WebSocketChannel: connecting [this=%p] after reconnect delay", this));
     BeginOpen(false);
   } else if (timer == mPingTimer) {
@@ -3344,7 +3376,7 @@ WebSocketChannel::AsyncOpenNative(nsIURI* aURI, const nsACString& aOrigin,
 
   nsresult rv;
 
-  // Ensure target thread is set.
+  // Ensure target thread is set if RetargetDeliveryTo isn't called
   {
     auto lock = mTargetThread.Lock();
     if (!lock.ref()) {
@@ -4034,7 +4066,7 @@ WebSocketChannel::OnInputStreamReady(nsIAsyncInputStream* aStream) {
   nsresult rv;
 
   do {
-    rv = mSocketIn->Read((char*)buffer, 2048, &count);
+    rv = mSocketIn->Read((char*)buffer, sizeof(buffer), &count);
     LOG(("WebSocketChannel::OnInputStreamReady: read %u rv %" PRIx32 "\n",
          count, static_cast<uint32_t>(rv)));
 

netwerk/protocol/websocket/WebSocketChannel.h

@@ -211,77 +211,104 @@ class WebSocketChannel : public BaseWebSocketChannel,
   }
 
   nsCOMPtr<nsIEventTarget> mIOThread;
+  // Set in AsyncOpenNative and AsyncOnChannelRedirect, modified in
+  // DoStopSession on IO thread (.forget()).   Probably ok...
   nsCOMPtr<nsIHttpChannelInternal> mChannel;
   nsCOMPtr<nsIHttpChannel> mHttpChannel;
+
   nsCOMPtr<nsICancelable> mCancelable;
+  // Mainthread only
   nsCOMPtr<nsIAsyncVerifyRedirectCallback> mRedirectCallback;
+  // Set on Mainthread during AsyncOpen, used on IO thread and Mainthread
   nsCOMPtr<nsIRandomGenerator> mRandomGenerator;
 
-  nsCString mHashedSecret;
+  nsCString mHashedSecret;  // MainThread only
 
   // Used as key for connection managment: Initially set to hostname from URI,
   // then to IP address (unless we're leaving DNS resolution to a proxy server)
+  // MainThread only
   nsCString mAddress;
   int32_t mPort;  // WS server port
   // Secondary key for the connection queue. Used by nsWSAdmissionManager.
-  nsCString mOriginSuffix;
+  nsCString mOriginSuffix;  // MainThread only
 
   // Used for off main thread access to the URI string.
+  // Set on MainThread in AsyncOpenNative, used on TargetThread and IO thread
   nsCString mHost;
   nsString mEffectiveURL;
 
+  // Set on MainThread before multithread use, used on IO thread, cleared on IOThread
   nsCOMPtr<nsISocketTransport> mTransport;
   nsCOMPtr<nsIAsyncInputStream> mSocketIn;
   nsCOMPtr<nsIAsyncOutputStream> mSocketOut;
   RefPtr<WebSocketConnectionBase> mConnection;
 
+  // Only used on IO Thread (accessed when known-to-be-null in DoStopSession
+  // on MainThread before mDataStarted)
   nsCOMPtr<nsITimer> mCloseTimer;
+  // set in AsyncOpenInternal on MainThread, used on IO thread.
+  // No multithread use before it's set, no changes after that.
   uint32_t mCloseTimeout; /* milliseconds */
 
-  nsCOMPtr<nsITimer> mOpenTimer;
-  uint32_t mOpenTimeout;         /* milliseconds */
-  wsConnectingState mConnecting; /* 0 if not connecting */
+  nsCOMPtr<nsITimer> mOpenTimer; /* Mainthread only */
+  uint32_t mOpenTimeout;         /* milliseconds, MainThread only */
+  wsConnectingState mConnecting; /* 0 if not connecting, MainThread only */
+  // Set on MainThread, deleted on either MainThread mainthread, used on
+  // MainThread or IO Thread in DoStopSession
   nsCOMPtr<nsITimer> mReconnectDelayTimer;
 
+  // Only touched on IOThread (DoStopSession reads it on MainThread if
+  // we haven't connected yet (mDataStarted==false), and it's always null
+  // until mDataStarted=true)
   nsCOMPtr<nsITimer> mPingTimer;
 
+  // Created in DoStopSession on IO thread (mDataStarted=true), accessed
+  // only from IO Thread
   nsCOMPtr<nsITimer> mLingeringCloseTimer;
   const static int32_t kLingeringCloseTimeout = 1000;
   const static int32_t kLingeringCloseThreshold = 50;
 
-  RefPtr<WebSocketEventService> mService;
+  RefPtr<WebSocketEventService> mService;  // effectively const
 
-  int32_t mMaxConcurrentConnections;
+  int32_t
+      mMaxConcurrentConnections;  // only used in AsyncOpenNative on MainThread
 
+  // Set on MainThread in AsyncOpenNative; then used on IO thread
   uint64_t mInnerWindowID;
 
   // following members are accessed only on the main thread
   uint32_t mGotUpgradeOK : 1;
   uint32_t mRecvdHttpUpgradeTransport : 1;
   uint32_t mAutoFollowRedirects : 1;
   uint32_t mAllowPMCE : 1;
-  uint32_t : 0;
+  uint32_t : 0;  // ensure these aren't mixed with the next set
 
-  // following members are accessed only on the socket thread
+  // following members are accessed only on the IO thread
   uint32_t mPingOutstanding : 1;
   uint32_t mReleaseOnTransmit : 1;
   uint32_t : 0;
 
   Atomic<bool> mDataStarted;
+  // All changes to mRequestedClose happen under mutex, but since it's atomic,
+  // it can be read anywhere without a lock
   Atomic<bool> mRequestedClose;
+  // mServer/ClientClosed are only modified on IOThread
   Atomic<bool> mClientClosed;
   Atomic<bool> mServerClosed;
+  // All changes to mStopped happen under mutex, but since it's atomic, it
+  // can be read anywhere without a lock
   Atomic<bool> mStopped;
   Atomic<bool> mCalledOnStop;
   Atomic<bool> mTCPClosed;
   Atomic<bool> mOpenedHttpChannel;
   Atomic<bool> mIncrementedSessionCount;
   Atomic<bool> mDecrementedSessionCount;
 
-  int32_t mMaxMessageSize;
+  int32_t mMaxMessageSize;  // set on MainThread in AsyncOpenNative, read on IO thread
+  // Set on IOThread, or on MainThread before mDataStarted.  Used on IO Thread (after mDataStarted)
   nsresult mStopOnClose;
-  uint16_t mServerCloseCode;
-  nsCString mServerCloseReason;
+  uint16_t mServerCloseCode;     // only used on IO thread
+  nsCString mServerCloseReason;  // only used on IO thread
   uint16_t mScriptCloseCode;
   nsCString mScriptCloseReason;
 
@@ -292,6 +319,7 @@ class WebSocketChannel : public BaseWebSocketChannel,
   // increase the buffer temporarily, then drop back down to this size.
   const static uint32_t kIncomingBufferStableSize = 128 * 1024;
 
+  // Set at creation, used/modified only on IO thread
   uint8_t* mFramePtr;
   uint8_t* mBuffer;
   uint8_t mFragmentOpcode;
@@ -302,6 +330,7 @@ class WebSocketChannel : public BaseWebSocketChannel,
   // These are for the send buffers
   const static int32_t kCopyBreak = 1000;
 
+  // Only used on IO thread
   OutboundMessage* mCurrentOut;
   uint32_t mCurrentOutSent;
   nsDeque<OutboundMessage> mOutgoingMessages;
@@ -310,12 +339,20 @@ class WebSocketChannel : public BaseWebSocketChannel,
   uint32_t mHdrOutToSend;
   uint8_t* mHdrOut;
   uint8_t mOutHeader[kCopyBreak + 16]{0};
+  
+  // Set on MainThread in OnStartRequest (before mDataStarted), used on IO
+  // Thread (after mDataStarted), cleared in DoStopSession on IOThread or
+  // on MainThread (if mDataStarted == false)
   UniquePtr<PMCECompression> mPMCECompressor;
+
+  // Used by EnsureHdrOut, which isn't called anywhere
   uint32_t mDynamicOutputSize;
   uint8_t* mDynamicOutput;
-  bool mPrivateBrowsing;
+  // Set on creation and AsyncOpen, read on both threads
+  Atomic<bool> mPrivateBrowsing;
 
-  nsCOMPtr<nsIDashboardEventNotifier> mConnectionLogService;
+  nsCOMPtr<nsIDashboardEventNotifier>
+      mConnectionLogService;  // effectively const
 
   mozilla::Mutex mMutex;
 };