Bug 1527890 - WebSocketChannel can hang waiting for OnTransportAvailable when HTTP request isn't upgraded, r=valentin

Canceling mOpenTimer is moved to CallStartWebsocketData which is called after calling OnStartRequest as well as OnTransportAvailable.

netwerk/protocol/websocket/WebSocketChannel.cpp

@@ -2839,8 +2839,14 @@ nsresult WebSocketChannel::ApplyForAdmission() {
 // Called after both OnStartRequest and OnTransportAvailable have
 // executed. This essentially ends the handshake and starts the websockets
 // protocol state machine.
-nsresult WebSocketChannel::StartWebsocketData() {
-  nsresult rv;
+nsresult WebSocketChannel::CallStartWebsocketData() {
+  LOG(("WebSocketChannel::CallStartWebsocketData() %p", this));
+  MOZ_ASSERT(NS_IsMainThread(), "not main thread");
+
+  if (mOpenTimer) {
+    mOpenTimer->Cancel();
+    mOpenTimer = nullptr;
+  }
 
   if (!IsOnTargetThread()) {
     return mTargetThread->Dispatch(
@@ -2849,6 +2855,12 @@ nsresult WebSocketChannel::StartWebsocketData() {
         NS_DISPATCH_NORMAL);
   }
 
+  return StartWebsocketData();
+}
+
+nsresult WebSocketChannel::StartWebsocketData() {
+  nsresult rv;
+
   {
     MutexAutoLock lock(mMutex);
     LOG(("WebSocketChannel::StartWebsocketData() %p", this));
@@ -2925,7 +2937,7 @@ nsresult WebSocketChannel::StartPinging() {
   return NS_OK;
 }
 
-void WebSocketChannel::ReportConnectionTelemetry() {
+void WebSocketChannel::ReportConnectionTelemetry(nsresult aStatusCode) {
   // 3 bits are used. high bit is for wss, middle bit for failed,
   // and low bit for proxy..
   // 0 - 7 : ws-ok-plain, ws-ok-proxy, ws-failed-plain, ws-failed-proxy,
@@ -2943,8 +2955,10 @@ void WebSocketChannel::ReportConnectionTelemetry() {
       didProxy = true;
   }
 
-  uint8_t value = (mEncrypted ? (1 << 2) : 0) |
-                  (!mGotUpgradeOK ? (1 << 1) : 0) | (didProxy ? (1 << 0) : 0);
+  uint8_t value =
+      (mEncrypted ? (1 << 2) : 0) |
+      (!(mGotUpgradeOK && NS_SUCCEEDED(aStatusCode)) ? (1 << 1) : 0) |
+      (didProxy ? (1 << 0) : 0);
 
   LOG(("WebSocketChannel::ReportConnectionTelemetry() %p %d", this, value));
   Telemetry::Accumulate(Telemetry::WEBSOCKETS_HANDSHAKE_TYPE, value);
@@ -3165,7 +3179,6 @@ WebSocketChannel::Notify(nsITimer *timer) {
     LOG(("WebSocketChannel:: Expecting Server Close - Timed Out\n"));
     AbortSession(NS_ERROR_NET_TIMEOUT);
   } else if (timer == mOpenTimer) {
-    MOZ_ASSERT(!mGotUpgradeOK, "Open Timer after open complete");
     MOZ_ASSERT(NS_IsMainThread(), "not main thread");
 
     mOpenTimer = nullptr;
@@ -3597,7 +3610,7 @@ WebSocketChannel::OnTransportAvailable(nsISocketTransport *aTransport,
     // is pending
     nsWSAdmissionManager::OnConnected(this);
 
-    return StartWebsocketData();
+    return CallStartWebsocketData();
   }
 
   if (mIsServerSide) {
@@ -3640,7 +3653,7 @@ WebSocketChannel::OnTransportAvailable(nsISocketTransport *aTransport,
       }
     }
 
-    return StartWebsocketData();
+    return CallStartWebsocketData();
   }
 
   return NS_OK;
@@ -3655,11 +3668,6 @@ WebSocketChannel::OnStartRequest(nsIRequest *aRequest, nsISupports *aContext) {
   MOZ_ASSERT(NS_IsMainThread(), "not main thread");
   MOZ_ASSERT(!mGotUpgradeOK, "OTA duplicated");
 
-  if (mOpenTimer) {
-    mOpenTimer->Cancel();
-    mOpenTimer = nullptr;
-  }
-
   if (mStopped) {
     LOG(("WebSocketChannel::OnStartRequest: Channel Already Done\n"));
     AbortSession(NS_ERROR_CONNECTION_REFUSED);
@@ -3832,7 +3840,7 @@ WebSocketChannel::OnStartRequest(nsIRequest *aRequest, nsISupports *aContext) {
     // is pending
     nsWSAdmissionManager::OnConnected(this);
 
-    return StartWebsocketData();
+    return CallStartWebsocketData();
   }
 
   return NS_OK;
@@ -3845,7 +3853,13 @@ WebSocketChannel::OnStopRequest(nsIRequest *aRequest, nsISupports *aContext,
        aRequest, mHttpChannel.get(), static_cast<uint32_t>(aStatusCode)));
   MOZ_ASSERT(NS_IsMainThread(), "not main thread");
 
-  ReportConnectionTelemetry();
+  // OnTransportAvailable won't be called if the request is stopped with
+  // an error. Abort the session now instead of waiting for timeout.
+  if (NS_FAILED(aStatusCode) && !mRecvdHttpUpgradeTransport) {
+    AbortSession(aStatusCode);
+  }
+
+  ReportConnectionTelemetry(aStatusCode);
 
   // This is the end of the HTTP upgrade transaction, the
   // upgraded streams live on

netwerk/protocol/websocket/WebSocketChannel.h

@@ -157,9 +157,10 @@ class WebSocketChannel : public BaseWebSocketChannel,
   MOZ_MUST_USE nsresult SetupRequest();
   MOZ_MUST_USE nsresult ApplyForAdmission();
   MOZ_MUST_USE nsresult DoAdmissionDNS();
+  MOZ_MUST_USE nsresult CallStartWebsocketData();
   MOZ_MUST_USE nsresult StartWebsocketData();
   uint16_t ResultToCloseCode(nsresult resultCode);
-  void ReportConnectionTelemetry();
+  void ReportConnectionTelemetry(nsresult aStatusCode);
 
   void StopSession(nsresult reason);
   void DoStopSession(nsresult reason);