Bug 1563538 - Propagate close reason through TLSFilterTransaction to allow tunnel stream reuse when possible + close the undelying tunnel stream when a tunneling connection is closed, r=dragana,kershaw+kershaw

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

--HG--
extra : moz-landing-system : lando

modules/libpref/init/all.js

@@ -2175,6 +2175,9 @@ pref("network.http.tailing.delay-max", 6000);
 // Total limit we delay tailed requests since a page load beginning.
 pref("network.http.tailing.total-max", 45000);
 
+// Enable or disable the whole fix from bug 1563538
+pref("network.http.spdy.bug1563538", true);
+
 pref("permissions.default.image",           1); // 1-Accept, 2-Deny, 3-dontAcceptForeign
 
 pref("network.proxy.type",                  5);

netwerk/protocol/http/TunnelUtils.cpp

@@ -48,6 +48,7 @@ TLSFilterTransaction::TLSFilterTransaction(nsAHttpTransaction* aWrapped,
       mFilterReadCode(NS_ERROR_NOT_INITIALIZED),
       mForce(false),
       mReadSegmentReturnValue(NS_OK),
+      mCloseReason(NS_ERROR_UNEXPECTED),
       mNudgeCounter(0) {
   MOZ_ASSERT(OnSocketThread(), "not on socket thread");
   LOG(("TLSFilterTransaction ctor %p\n", this));
@@ -130,6 +131,16 @@ void TLSFilterTransaction::Close(nsresult aReason) {
   }
   mTransaction->Close(aReason);
   mTransaction = nullptr;
+
+  if (gHttpHandler->Bug1563538()) {
+    if (NS_FAILED(aReason)) {
+      mCloseReason = aReason;
+    } else {
+      mCloseReason = NS_BASE_STREAM_CLOSED;
+    }
+  } else {
+    MOZ_ASSERT(NS_ERROR_UNEXPECTED == mCloseReason);
+  }
 }
 
 nsresult TLSFilterTransaction::OnReadSegment(const char* aData, uint32_t aCount,
@@ -326,7 +337,7 @@ nsresult TLSFilterTransaction::ReadSegments(nsAHttpSegmentReader* aReader,
   LOG(("TLSFilterTransaction::ReadSegments %p max=%d\n", this, aCount));
 
   if (!mTransaction) {
-    return NS_ERROR_UNEXPECTED;
+    return mCloseReason;
   }
 
   mReadSegmentReturnValue = NS_OK;
@@ -351,7 +362,7 @@ nsresult TLSFilterTransaction::WriteSegments(nsAHttpSegmentWriter* aWriter,
   LOG(("TLSFilterTransaction::WriteSegments %p max=%d\n", this, aCount));
 
   if (!mTransaction) {
-    return NS_ERROR_UNEXPECTED;
+    return mCloseReason;
   }
 
   mSegmentWriter = aWriter;
@@ -649,6 +660,11 @@ nsresult TLSFilterTransaction::SetProxiedTransaction(
        this, aTrans, aSpdyConnectTransaction));
 
   mTransaction = aTrans;
+
+  // Reverting mCloseReason to the default value for consistency to indicate we
+  // are no longer in closed state.
+  mCloseReason = NS_ERROR_UNEXPECTED;
+
   nsCOMPtr<nsIInterfaceRequestor> callbacks;
   mTransaction->GetSecurityCallbacks(getter_AddRefs(callbacks));
   nsCOMPtr<nsISSLSocketControl> secCtrl(do_QueryInterface(mSecInfo));
@@ -829,23 +845,6 @@ SpdyConnectTransaction* TLSFilterTransaction::QuerySpdyConnectTransaction() {
   return mTransaction->QuerySpdyConnectTransaction();
 }
 
-class SocketTransportShim : public nsISocketTransport {
- public:
-  NS_DECL_THREADSAFE_ISUPPORTS
-  NS_DECL_NSITRANSPORT
-  NS_DECL_NSISOCKETTRANSPORT
-
-  explicit SocketTransportShim(nsISocketTransport* aWrapped, bool aIsWebsocket)
-      : mWrapped(aWrapped), mIsWebsocket(aIsWebsocket){};
-
- private:
-  virtual ~SocketTransportShim() = default;
-
-  nsCOMPtr<nsISocketTransport> mWrapped;
-  bool mIsWebsocket;
-  nsCOMPtr<nsIInterfaceRequestor> mSecurityCallbacks;
-};
-
 class WeakTransProxy final : public nsISupports {
  public:
   NS_DECL_THREADSAFE_ISUPPORTS
@@ -891,6 +890,33 @@ class WeakTransFreeProxy final : public Runnable {
   RefPtr<WeakTransProxy> mProxy;
 };
 
+class SocketTransportShim : public nsISocketTransport {
+ public:
+  NS_DECL_THREADSAFE_ISUPPORTS
+  NS_DECL_NSITRANSPORT
+  NS_DECL_NSISOCKETTRANSPORT
+
+  explicit SocketTransportShim(SpdyConnectTransaction* aTrans,
+                               nsISocketTransport* aWrapped, bool aIsWebsocket)
+      : mWrapped(aWrapped), mIsWebsocket(aIsWebsocket) {
+    mWeakTrans = new WeakTransProxy(aTrans);
+  }
+
+ private:
+  virtual ~SocketTransportShim() {
+    if (!OnSocketThread()) {
+      RefPtr<WeakTransFreeProxy> p = new WeakTransFreeProxy(mWeakTrans);
+      mWeakTrans = nullptr;
+      p->Dispatch();
+    }
+  }
+
+  nsCOMPtr<nsISocketTransport> mWrapped;
+  bool mIsWebsocket;
+  nsCOMPtr<nsIInterfaceRequestor> mSecurityCallbacks;
+  RefPtr<WeakTransProxy> mWeakTrans;  // SpdyConnectTransaction *
+};
+
 class OutputStreamShim : public nsIAsyncOutputStream {
  public:
   NS_DECL_THREADSAFE_ISUPPORTS
@@ -1030,7 +1056,7 @@ void SpdyConnectTransaction::MapStreamToHttpConnection(
     int32_t httpResponseCode) {
   mConnInfo = aConnInfo;
 
-  mTunnelTransport = new SocketTransportShim(aTransport, mIsWebsocket);
+  mTunnelTransport = new SocketTransportShim(this, aTransport, mIsWebsocket);
   mTunnelStreamIn = new InputStreamShim(this, mIsWebsocket);
   mTunnelStreamOut = new OutputStreamShim(this, mIsWebsocket);
   mTunneledConn = new nsHttpConnection();
@@ -1801,7 +1827,36 @@ NS_IMETHODIMP
 SocketTransportShim::Close(nsresult aReason) {
   if (mIsWebsocket) {
     LOG3(("WARNING: SocketTransportShim::Close %p", this));
+  } else {
+    LOG(("SocketTransportShim::Close %p", this));
   }
+
+  if (gHttpHandler->Bug1563538()) {
+    // Must always post, because mSession->CloseTransaction releases the
+    // Http2Stream which is still on stack.
+    RefPtr<SocketTransportShim> self(this);
+
+    nsCOMPtr<nsIEventTarget> sts =
+        do_GetService("@mozilla.org/network/socket-transport-service;1");
+    Unused << sts->Dispatch(NS_NewRunnableFunction(
+        "SocketTransportShim::Close", [self = std::move(self), aReason]() {
+          RefPtr<NullHttpTransaction> baseTrans =
+              self->mWeakTrans->QueryTransaction();
+          if (!baseTrans) {
+            return;
+          }
+          SpdyConnectTransaction* trans =
+              baseTrans->QuerySpdyConnectTransaction();
+          MOZ_ASSERT(trans);
+          if (!trans) {
+            return;
+          }
+
+          trans->mSession->CloseTransaction(trans, aReason);
+        }));
+    return NS_OK;
+  }
+
   return NS_ERROR_NOT_IMPLEMENTED;
 }
 

netwerk/protocol/http/TunnelUtils.h

@@ -182,6 +182,11 @@ class TLSFilterTransaction final : public nsAHttpTransaction,
   nsresult mFilterReadCode;
   bool mForce;
   nsresult mReadSegmentReturnValue;
+  // Before Close() is called this is NS_ERROR_UNEXPECTED, in Close() we either
+  // take the reason, if it is a failure, or we change to
+  // NS_ERROR_BASE_STREAM_CLOSE.  This is returned when Write/ReadSegments is
+  // called after Close, when we don't have mTransaction any more.
+  nsresult mCloseReason;
   uint32_t mNudgeCounter;
 };
 
@@ -227,6 +232,7 @@ class SpdyConnectTransaction final : public NullHttpTransaction {
   void SetConnRefTaken();
 
  private:
+  friend class SocketTransportShim;
   friend class InputStreamShim;
   friend class OutputStreamShim;
 

netwerk/protocol/http/nsHttpHandler.cpp

@@ -274,6 +274,7 @@ nsHttpHandler::nsHttpHandler()
       mTCPKeepaliveLongLivedIdleTimeS(600),
       mEnforceH1Framing(FRAMECHECK_BARELY),
       mDefaultHpackBuffer(4096),
+      mBug1563538(true),
       mMaxHttpResponseHeaderSize(393216),
       mFocusedWindowTransactionRatio(0.9f),
       mSpeculativeConnectEnabled(false),
@@ -1878,6 +1879,13 @@ void nsHttpHandler::PrefsChanged(const char* pref) {
     }
   }
 
+  if (PREF_CHANGED(HTTP_PREF("spdy.bug1563538"))) {
+    rv = Preferences::GetBool(HTTP_PREF("spdy.bug1563538"), &cVar);
+    if (NS_SUCCEEDED(rv)) {
+      mBug1563538 = cVar;
+    }
+  }
+
   // Enable HTTP response timeout if TCP Keepalives are disabled.
   mResponseTimeoutEnabled =
       !mTCPKeepaliveShortLivedEnabled && !mTCPKeepaliveLongLivedEnabled;

netwerk/protocol/http/nsHttpHandler.h

@@ -429,6 +429,8 @@ class nsHttpHandler final : public nsIHttpProtocolHandler,
 
   uint32_t DefaultHpackBuffer() const { return mDefaultHpackBuffer; }
 
+  bool Bug1563538() const { return mBug1563538; }
+
   uint32_t MaxHttpResponseHeaderSize() const {
     return mMaxHttpResponseHeaderSize;
   }
@@ -671,6 +673,9 @@ class nsHttpHandler final : public nsIHttpProtocolHandler,
   // The default size (in bytes) of the HPACK decompressor table.
   uint32_t mDefaultHpackBuffer;
 
+  // Pref for the whole fix that bug provides
+  Atomic<bool, Relaxed> mBug1563538;
+
   // The max size (in bytes) for received Http response header.
   uint32_t mMaxHttpResponseHeaderSize;