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
authorDragana Damjanovic <dd.mozilla@gmail.com>
Mon, 07 Jan 2019 23:09:35 +0000
changeset 452821 62e3131970c01474e08e9ddd69c0d06fe966ce37
parent 452820 953e31ae0e5d51f97dcd7070bea05c0ffd7f1c2c
child 452822 e59729ade65262288e43f679455be1e5da8b13c3
push id75606
push userbtara@mozilla.com
push dateTue, 08 Jan 2019 03:56:42 +0000
treeherderautoland@62e3131970c0 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmichal
bugs1514688
milestone66.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
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 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
@@ -1483,28 +1483,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)) {