Bug 1520483 - Return proper error if the nss layer encounters an error on the http tunnel. r=valentin,kershaw, a=RyanVM
authorDragana Damjanovic <dd.mozilla@gmail.com>
Fri, 08 Feb 2019 09:13:21 +0000
changeset 512983 81b95ee7a2f3a2e09ac4428bafe6957f5942781a
parent 512982 2b77d6958f310d832164971626a3686d8826cd40
child 512984 f05abb753b64609a9e169a08c66fc237615b3b50
push id10669
push userryanvm@gmail.com
push dateMon, 11 Feb 2019 12:53:17 +0000
treeherdermozilla-beta@2305c930b771 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersvalentin, kershaw, RyanVM
bugs1520483
milestone66.0
Bug 1520483 - Return proper error if the nss layer encounters an error on the http tunnel. r=valentin,kershaw, a=RyanVM Differential Revision: https://phabricator.services.mozilla.com/D17952
netwerk/protocol/http/TunnelUtils.cpp
netwerk/protocol/http/TunnelUtils.h
netwerk/protocol/http/nsHttpConnection.cpp
--- a/netwerk/protocol/http/TunnelUtils.cpp
+++ b/netwerk/protocol/http/TunnelUtils.cpp
@@ -42,17 +42,17 @@ TLSFilterTransaction::TLSFilterTransacti
                                            nsAHttpSegmentWriter *aWriter)
     : mTransaction(aWrapped),
       mEncryptedTextUsed(0),
       mEncryptedTextSize(0),
       mSegmentReader(aReader),
       mSegmentWriter(aWriter),
       mFilterReadCode(NS_ERROR_NOT_INITIALIZED),
       mForce(false),
-      mReadSegmentBlocked(false),
+      mReadSegmentReturnValue(NS_OK),
       mNudgeCounter(0) {
   MOZ_ASSERT(OnSocketThread(), "not on socket thread");
   LOG(("TLSFilterTransaction ctor %p\n", this));
 
   nsCOMPtr<nsISocketProvider> provider;
   nsCOMPtr<nsISocketProviderService> spserv =
       nsSocketProviderService::GetOrCreate();
 
@@ -144,17 +144,17 @@ void TLSFilterTransaction::Close(nsresul
   }
 }
 
 nsresult TLSFilterTransaction::OnReadSegment(const char *aData, uint32_t aCount,
                                              uint32_t *outCountRead) {
   LOG(("TLSFilterTransaction %p OnReadSegment %d (buffered %d)\n", this, aCount,
        mEncryptedTextUsed));
 
-  mReadSegmentBlocked = false;
+  mReadSegmentReturnValue = NS_OK;
   MOZ_ASSERT(mSegmentReader);
   if (!mSecInfo) {
     return NS_ERROR_FAILURE;
   }
 
   nsresult rv;
   *outCountRead = 0;
 
@@ -191,27 +191,22 @@ nsresult TLSFilterTransaction::OnReadSeg
     LOG(("TLSFilterTransaction %p OnReadSegment PRWrite(%d) = %d %d\n", this,
          aCount, written, PR_GetError() == PR_WOULD_BLOCK_ERROR));
 
     if (written < 1) {
       if (*outCountRead) {
         return NS_OK;
       }
       // mTransaction ReadSegments actually obscures this code, so
-      // keep it in a member var for this::ReadSegments to insepct. Similar
+      // keep it in a member var for this::ReadSegments to inspect. Similar
       // to nsHttpConnection::mSocketOutCondition
       PRErrorCode code = PR_GetError();
-      mReadSegmentBlocked = (code == PR_WOULD_BLOCK_ERROR);
-      if (mReadSegmentBlocked) {
-        return NS_BASE_STREAM_WOULD_BLOCK;
-      }
+      mReadSegmentReturnValue = ErrorAccordingToNSPR(code);
 
-      nsresult rv = ErrorAccordingToNSPR(code);
-      Close(rv);
-      return rv;
+      return mReadSegmentReturnValue;
     }
     aCount -= written;
     aData += written;
     *outCountRead += written;
     mNudgeCounter = 0;
   }
 
   LOG(("TLSFilterTransaction %p OnReadSegment2 (buffered %d)\n", this,
@@ -282,19 +277,24 @@ nsresult TLSFilterTransaction::OnWriteSe
   // level connection before removing the local TLS layer
   mFilterReadCode = NS_OK;
   int32_t bytesRead = PR_Read(mFD, aData, aCount);
   if (bytesRead == -1) {
     PRErrorCode code = PR_GetError();
     if (code == PR_WOULD_BLOCK_ERROR) {
       return NS_BASE_STREAM_WOULD_BLOCK;
     }
-    nsresult rv = ErrorAccordingToNSPR(code);
-    Close(rv);
-    return rv;
+    // If reading from the socket succeeded (NS_SUCCEEDED(mFilterReadCode)),
+    // but the nss layer encountered an error remember the error.
+    if (NS_SUCCEEDED(mFilterReadCode)) {
+      mFilterReadCode = ErrorAccordingToNSPR(code);
+      LOG(("TLSFilterTransaction::OnWriteSegment %p nss error %" PRIx32 ".\n",
+           this, static_cast<uint32_t>(mFilterReadCode)));
+    }
+    return mFilterReadCode;
   }
   *outCountRead = bytesRead;
 
   if (NS_SUCCEEDED(mFilterReadCode) && !bytesRead) {
     LOG(
         ("TLSFilterTransaction::OnWriteSegment %p "
          "Second layer of TLS stripping results in STREAM_CLOSED\n",
          this));
@@ -315,17 +315,17 @@ int32_t TLSFilterTransaction::FilterInpu
   uint32_t outCountRead = 0;
   mFilterReadCode =
       mSegmentWriter->OnWriteSegment(aBuf, aAmount, &outCountRead);
   if (NS_SUCCEEDED(mFilterReadCode) && outCountRead) {
     LOG(("TLSFilterTransaction::FilterInput rv=%" PRIx32
          " read=%d input from net "
          "1 layer stripped, 1 still on\n",
          static_cast<uint32_t>(mFilterReadCode), outCountRead));
-    if (mReadSegmentBlocked) {
+    if (mReadSegmentReturnValue == NS_BASE_STREAM_WOULD_BLOCK) {
       mNudgeCounter = 0;
     }
   }
   if (mFilterReadCode == NS_BASE_STREAM_WOULD_BLOCK) {
     PR_SetError(PR_WOULD_BLOCK_ERROR, 0);
     return -1;
   }
   return outCountRead;
@@ -336,29 +336,28 @@ nsresult TLSFilterTransaction::ReadSegme
                                             uint32_t *outCountRead) {
   MOZ_ASSERT(OnSocketThread(), "not on socket thread");
   LOG(("TLSFilterTransaction::ReadSegments %p max=%d\n", this, aCount));
 
   if (!mTransaction) {
     return NS_ERROR_UNEXPECTED;
   }
 
-  mReadSegmentBlocked = false;
+  mReadSegmentReturnValue = NS_OK;
   mSegmentReader = aReader;
   nsresult rv = mTransaction->ReadSegments(this, aCount, outCountRead);
   LOG(("TLSFilterTransaction %p called trans->ReadSegments rv=%" PRIx32 " %d\n",
        this, static_cast<uint32_t>(rv), *outCountRead));
-  if (NS_SUCCEEDED(rv) && mReadSegmentBlocked) {
-    rv = NS_BASE_STREAM_WOULD_BLOCK;
+  if (NS_SUCCEEDED(rv) && (mReadSegmentReturnValue == NS_BASE_STREAM_WOULD_BLOCK)) {
     LOG(("TLSFilterTransaction %p read segment blocked found rv=%" PRIx32 "\n",
          this, static_cast<uint32_t>(rv)));
     Unused << Connection()->ForceSend();
   }
 
-  return rv;
+  return NS_SUCCEEDED(rv) ? mReadSegmentReturnValue : rv;
 }
 
 nsresult TLSFilterTransaction::WriteSegments(nsAHttpSegmentWriter *aWriter,
                                              uint32_t aCount,
                                              uint32_t *outCountWritten) {
   MOZ_ASSERT(OnSocketThread(), "not on socket thread");
   LOG(("TLSFilterTransaction::WriteSegments %p max=%d\n", this, aCount));
 
@@ -451,18 +450,20 @@ nsresult TLSFilterTransaction::NudgeTunn
 NS_IMETHODIMP
 TLSFilterTransaction::Notify(nsITimer *timer) {
   MOZ_ASSERT(OnSocketThread(), "not on socket thread");
   LOG(("TLSFilterTransaction %p NudgeTunnel notify\n", this));
 
   if (timer != mTimer) {
     return NS_ERROR_UNEXPECTED;
   }
-  DebugOnly<nsresult> rv = StartTimerCallback();
-  MOZ_ASSERT(NS_SUCCEEDED(rv));
+  nsresult rv = StartTimerCallback();
+  if (NS_FAILED(rv)) {
+    Close(rv);
+  }
   return NS_OK;
 }
 
 NS_IMETHODIMP
 TLSFilterTransaction::GetName(nsACString &aName) {
   aName.AssignLiteral("TLSFilterTransaction");
   return NS_OK;
 }
@@ -470,17 +471,17 @@ TLSFilterTransaction::GetName(nsACString
 nsresult TLSFilterTransaction::StartTimerCallback() {
   LOG(("TLSFilterTransaction %p NudgeTunnel StartTimerCallback %p\n", this,
        mNudgeCallback.get()));
 
   if (mNudgeCallback) {
     // This class can be called re-entrantly, so cleanup m* before ->on()
     RefPtr<NudgeTunnelCallback> cb(mNudgeCallback);
     mNudgeCallback = nullptr;
-    cb->OnTunnelNudged(this);
+    return cb->OnTunnelNudged(this);
   }
   return NS_OK;
 }
 
 PRStatus TLSFilterTransaction::GetPeerName(PRFileDesc *aFD, PRNetAddr *addr) {
   NetAddr peeraddr;
   TLSFilterTransaction *self =
       reinterpret_cast<TLSFilterTransaction *>(aFD->secret);
--- a/netwerk/protocol/http/TunnelUtils.h
+++ b/netwerk/protocol/http/TunnelUtils.h
@@ -95,21 +95,21 @@ namespace mozilla {
 namespace net {
 
 class nsHttpRequestHead;
 class NullHttpTransaction;
 class TLSFilterTransaction;
 
 class NudgeTunnelCallback : public nsISupports {
  public:
-  virtual void OnTunnelNudged(TLSFilterTransaction *) = 0;
+  virtual nsresult OnTunnelNudged(TLSFilterTransaction *) = 0;
 };
 
 #define NS_DECL_NUDGETUNNELCALLBACK \
-  void OnTunnelNudged(TLSFilterTransaction *) override;
+  nsresult OnTunnelNudged(TLSFilterTransaction *) override;
 
 class TLSFilterTransaction final : public nsAHttpTransaction,
                                    public nsAHttpSegmentReader,
                                    public nsAHttpSegmentWriter,
                                    public nsITimerCallback,
                                    public nsINamed {
   ~TLSFilterTransaction();
 
@@ -176,17 +176,17 @@ class TLSFilterTransaction final : publi
   uint32_t mEncryptedTextSize;
 
   PRFileDesc *mFD;
   nsAHttpSegmentReader *mSegmentReader;
   nsAHttpSegmentWriter *mSegmentWriter;
 
   nsresult mFilterReadCode;
   bool mForce;
-  bool mReadSegmentBlocked;
+  nsresult mReadSegmentReturnValue;
   uint32_t mNudgeCounter;
 };
 
 class SocketTransportShim;
 class InputStreamShim;
 class OutputStreamShim;
 class nsHttpConnection;
 
--- a/netwerk/protocol/http/nsHttpConnection.cpp
+++ b/netwerk/protocol/http/nsHttpConnection.cpp
@@ -696,24 +696,24 @@ npnComplete:
 
   if (rv == psm::GetXPCOMFromNSSError(
                 mozilla::pkix::MOZILLA_PKIX_ERROR_MITM_DETECTED)) {
     gSocketTransportService->SetNotTrustedMitmDetected();
   }
   return true;
 }
 
-void nsHttpConnection::OnTunnelNudged(TLSFilterTransaction *trans) {
+nsresult nsHttpConnection::OnTunnelNudged(TLSFilterTransaction *trans) {
   MOZ_ASSERT(OnSocketThread(), "not on socket thread");
   LOG(("nsHttpConnection::OnTunnelNudged %p\n", this));
   if (trans != mTLSFilter) {
-    return;
+    return NS_OK;
   }
   LOG(("nsHttpConnection::OnTunnelNudged %p Calling OnSocketWritable\n", this));
-  Unused << OnSocketWritable();
+  return OnSocketWritable();
 }
 
 // called on the socket thread
 nsresult nsHttpConnection::Activate(nsAHttpTransaction *trans, uint32_t caps,
                                     int32_t pri) {
   MOZ_ASSERT(OnSocketThread(), "not on socket thread");
   LOG1(("nsHttpConnection::Activate [this=%p trans=%p caps=%x]\n", this, trans,
         caps));