Bug 1514688 - In case we already have a h2 connection and it does not support websockets, we should not try again to use websockets over h2. r=michal, a=RyanVM
authorDragana Damjanovic <dd.mozilla@gmail.com>
Mon, 07 Jan 2019 23:09:35 +0000
changeset 509415 07265912b0c7eaccbe864f27f3b1ac60b4831700
parent 509414 5eeab947d0ff48e6340bbc6bedbb0f1ec774c707
child 509416 a902a8dcea8d2d51debed83ddf0f589ae4c88129
push id1905
push userffxbld-merge
push dateMon, 21 Jan 2019 12:33:13 +0000
treeherdermozilla-release@c2fca1944d8c [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmichal, RyanVM
bugs1514688
milestone65.0
Bug 1514688 - In case we already have a h2 connection and it does not support websockets, we should not try again to use websockets over h2. r=michal, a=RyanVM There are 2 parts of the patch: 1) we should not try to use websockets over h2 if we already know that they are not supported. 2) make sure that we clean up websockets waiting for the settings frame when we close a h2 connection. (the part 1) will fix the issue, this part is only to be 100% that we some how do not retrigger the issue) Differential Revision: https://phabricator.services.mozilla.com/D15296
netwerk/protocol/http/Http2Session.cpp
netwerk/protocol/http/nsHttpConnectionMgr.cpp
--- a/netwerk/protocol/http/Http2Session.cpp
+++ b/netwerk/protocol/http/Http2Session.cpp
@@ -3830,16 +3830,47 @@ void Http2Session::Close(nsresult aReaso
 
   mClosed = true;
 
   Shutdown();
 
   mStreamIDHash.Clear();
   mStreamTransactionHash.Clear();
 
+  // If we have any websocket transactions waiting for settings frame,
+  // reinitiate them. This can happend if we close a h2 connection before the
+  // settings frame is received.
+  if (mWaitingWebsockets.Length()) {
+    MOZ_ASSERT(!mProcessedWaitingWebsockets);
+    MOZ_ASSERT(mWaitingWebsockets.Length() ==
+               mWaitingWebsocketCallbacks.Length());
+
+    mProcessedWaitingWebsockets = true;
+    for (size_t i = 0; i < mWaitingWebsockets.Length(); ++i) {
+      RefPtr<nsAHttpTransaction> httpTransaction = mWaitingWebsockets[i];
+      LOG3(("Http2Session::Close %p Re-queuing websocket.", this));
+      httpTransaction->SetConnection(nullptr);
+      nsHttpTransaction *trans = httpTransaction->QueryHttpTransaction();
+      if (trans) {
+        nsresult rv =
+            gHttpHandler->InitiateTransaction(trans, trans->Priority());
+        if (NS_FAILED(rv)) {
+          LOG3(
+              ("Http2Session::Close %p failed to reinitiate websocket "
+               "transaction (%08x).\n",
+               this, static_cast<uint32_t>(rv)));
+        }
+      } else {
+        LOG3(("Http2Session::Close %p missing transaction?!", this));
+      }
+    }
+    mWaitingWebsockets.Clear();
+    mWaitingWebsocketCallbacks.Clear();
+  }
+
   uint32_t goAwayReason;
   if (mGoAwayReason != NO_HTTP_ERROR) {
     goAwayReason = mGoAwayReason;
   } else if (NS_SUCCEEDED(aReason)) {
     goAwayReason = NO_HTTP_ERROR;
   } else if (aReason == NS_ERROR_ILLEGAL_VALUE) {
     goAwayReason = PROTOCOL_ERROR;
   } else if (mCleanShutdown) {
--- a/netwerk/protocol/http/nsHttpConnectionMgr.cpp
+++ b/netwerk/protocol/http/nsHttpConnectionMgr.cpp
@@ -1484,28 +1484,34 @@ nsresult nsHttpConnectionMgr::TryDispatc
 
   // step 0
   // look for existing spdy connection - that's always best because it is
   // essentially pipelining without head of line blocking
 
   if (!(caps & NS_HTTP_DISALLOW_SPDY) && gHttpHandler->IsSpdyEnabled()) {
     RefPtr<nsHttpConnection> conn = GetSpdyActiveConn(ent);
     if (conn) {
-      bool websocketCheckOK =
-          trans->IsWebsocketUpgrade() ? conn->CanAcceptWebsocket() : true;
-      if (websocketCheckOK && ((caps & NS_HTTP_ALLOW_KEEPALIVE) ||
-                               (caps & NS_HTTP_ALLOW_SPDY_WITHOUT_KEEPALIVE) ||
-                               !conn->IsExperienced())) {
-        LOG(("   dispatch to spdy: [conn=%p]\n", conn.get()));
-        trans->RemoveDispatchedAsBlocking(); /* just in case */
-        nsresult rv = DispatchTransaction(ent, trans, conn);
-        NS_ENSURE_SUCCESS(rv, rv);
-        return NS_OK;
+      if (trans->IsWebsocketUpgrade() && !conn->CanAcceptWebsocket()) {
+        // This is a websocket transaction and we already have a h2 connection
+        // that do not support websockets, we should disable h2 for this
+        // transaction.
+        trans->DisableSpdy();
+        caps &= NS_HTTP_DISALLOW_SPDY;
+      } else {
+        if ((caps & NS_HTTP_ALLOW_KEEPALIVE) ||
+            (caps & NS_HTTP_ALLOW_SPDY_WITHOUT_KEEPALIVE) ||
+            !conn->IsExperienced()) {
+          LOG(("   dispatch to spdy: [conn=%p]\n", conn.get()));
+          trans->RemoveDispatchedAsBlocking(); /* just in case */
+          nsresult rv = DispatchTransaction(ent, trans, conn);
+          NS_ENSURE_SUCCESS(rv, rv);
+          return NS_OK;
+        }
+        unusedSpdyPersistentConnection = conn;
       }
-      unusedSpdyPersistentConnection = conn;
     }
   }
 
   // If this is not a blocking transaction and the request context for it is
   // currently processing one or more blocking transactions then we
   // need to just leave it in the queue until those are complete unless it is
   // explicitly marked as unblocked.
   if (!(caps & NS_HTTP_LOAD_AS_BLOCKING)) {