Bug 1247205 - dont loop on http2 softerror r=dragana
authorPatrick McManus <mcmanus@ducksong.com>
Sat, 13 Feb 2016 20:54:24 -0500
changeset 331401 9b6d687214543f0c21035b3d7f32520419314c2b
parent 331400 221698ba7d337a3195bab02ad71e857dd4e8e7de
child 331402 37a288bd14768f6b960874b61322224b3f926e02
push id10977
push usercykesiopka.bmo@gmail.com
push dateWed, 17 Feb 2016 01:27:23 +0000
reviewersdragana
bugs1247205
milestone47.0a1
Bug 1247205 - dont loop on http2 softerror r=dragana
netwerk/protocol/http/Http2Session.cpp
netwerk/protocol/http/Http2Session.h
netwerk/protocol/http/SpdySession31.cpp
netwerk/protocol/http/SpdySession31.h
netwerk/protocol/http/nsAHttpTransaction.h
netwerk/protocol/http/nsHttpConnection.cpp
--- a/netwerk/protocol/http/Http2Session.cpp
+++ b/netwerk/protocol/http/Http2Session.cpp
@@ -2254,18 +2254,18 @@ Http2Session::OnTransportStatus(nsITrans
 }
 
 // ReadSegments() is used to write data to the network. Generally, HTTP
 // request data is pulled from the approriate transaction and
 // converted to http/2 data. Sometimes control data like window-update are
 // generated instead.
 
 nsresult
-Http2Session::ReadSegments(nsAHttpSegmentReader *reader,
-                           uint32_t count, uint32_t *countRead)
+Http2Session::ReadSegmentsAgain(nsAHttpSegmentReader *reader,
+                                uint32_t count, uint32_t *countRead, bool *again)
 {
   MOZ_ASSERT(PR_GetCurrentThread() == gSocketThread);
 
   MOZ_ASSERT(!mSegmentReader || !reader || (mSegmentReader == reader),
              "Inconsistent Write Function Callback");
 
   nsresult rv = ConfirmTLSProfile();
   if (NS_FAILED(rv))
@@ -2327,16 +2327,18 @@ Http2Session::ReadSegments(nsAHttpSegmen
           this, rv));
     if (rv == NS_BASE_STREAM_WOULD_BLOCK) {
       return rv;
     }
 
     CleanupStream(stream, rv, CANCEL_ERROR);
     if (SoftStreamError(rv)) {
       LOG3(("Http2Session::ReadSegments %p soft error override\n", this));
+      *again = false;
+      SetWriteCallbacks();
       rv = NS_OK;
     }
     return rv;
   }
 
   if (*countRead > 0) {
     LOG3(("Http2Session::ReadSegments %p stream=%p countread=%d",
           this, stream, *countRead));
@@ -2357,16 +2359,24 @@ Http2Session::ReadSegments(nsAHttpSegmen
   // call readsegments again if there are other streams ready
   // to go in this session
   SetWriteCallbacks();
 
   return rv;
 }
 
 nsresult
+Http2Session::ReadSegments(nsAHttpSegmentReader *reader,
+                           uint32_t count, uint32_t *countRead)
+{
+  bool again = false;
+  return ReadSegmentsAgain(reader, count, countRead, &again);
+}
+
+nsresult
 Http2Session::ReadyToProcessDataFrame(enum internalStateType newState)
 {
   MOZ_ASSERT(newState == PROCESSING_DATA_FRAME ||
              newState == DISCARDING_DATA_FRAME_PADDING);
   ChangeDownstreamState(newState);
 
   Telemetry::Accumulate(Telemetry::SPDY_CHUNK_RECVD,
                         mInputFrameDataSize >> 10);
@@ -2423,18 +2433,19 @@ Http2Session::ReadyToProcessDataFrame(en
 
 // we call writer->OnWriteSegment via NetworkRead() to get a http2 header..
 // and decide if it is data or control.. if it is control, just deal with it.
 // if it is data, identify the stream
 // call stream->WriteSegments which can call this::OnWriteSegment to get the
 // data. It always gets full frames if they are part of the stream
 
 nsresult
-Http2Session::WriteSegments(nsAHttpSegmentWriter *writer,
-                            uint32_t count, uint32_t *countWritten)
+Http2Session::WriteSegmentsAgain(nsAHttpSegmentWriter *writer,
+                                 uint32_t count, uint32_t *countWritten,
+                                 bool *again)
 {
   MOZ_ASSERT(PR_GetCurrentThread() == gSocketThread);
 
   LOG3(("Http2Session::WriteSegments %p InternalState %X\n",
         this, mDownstreamState));
 
   *countWritten = 0;
 
@@ -2695,16 +2706,18 @@ Http2Session::WriteSegments(nsAHttpSegme
         ResetDownstreamState();
       LOG3(("Http2Session::WriteSegments session=%p id 0x%X "
             "needscleanup=%p. cleanup stream based on "
             "stream->writeSegments returning code %x\n",
             this, streamID, mNeedsCleanup, rv));
       MOZ_ASSERT(!mNeedsCleanup || mNeedsCleanup->StreamID() == streamID);
       CleanupStream(streamID, NS_OK, CANCEL_ERROR);
       mNeedsCleanup = nullptr;
+      *again = false;
+      ResumeRecv();
       return NS_OK;
     }
 
     if (mNeedsCleanup) {
       LOG3(("Http2Session::WriteSegments session=%p stream=%p 0x%X "
             "cleanup stream based on mNeedsCleanup.\n",
             this, mNeedsCleanup, mNeedsCleanup ? mNeedsCleanup->StreamID() : 0));
       CleanupStream(mNeedsCleanup, NS_OK, CANCEL_ERROR);
@@ -2809,16 +2822,24 @@ Http2Session::WriteSegments(nsAHttpSegme
              "Control Handler returned OK but did not change state");
 
   if (mShouldGoAway && !mStreamTransactionHash.Count())
     Close(NS_OK);
   return rv;
 }
 
 nsresult
+Http2Session::WriteSegments(nsAHttpSegmentWriter *writer,
+                            uint32_t count, uint32_t *countWritten)
+{
+  bool again = false;
+  return WriteSegmentsAgain(writer, count, countWritten, &again);
+}
+
+nsresult
 Http2Session::ProcessConnectedPush(Http2Stream *pushConnectedStream,
                                    nsAHttpSegmentWriter * writer,
                                    uint32_t count, uint32_t *countWritten)
 {
   LOG3(("Http2Session::ProcessConnectedPush %p 0x%X\n",
         this, pushConnectedStream->StreamID()));
   mSegmentWriter = writer;
   nsresult rv = pushConnectedStream->WriteSegments(this, count, countWritten);
--- a/netwerk/protocol/http/Http2Session.h
+++ b/netwerk/protocol/http/Http2Session.h
@@ -227,16 +227,20 @@ public:
   int64_t ServerSessionWindow() { return mServerSessionWindow; }
   void DecrementServerSessionWindow (uint32_t bytes) { mServerSessionWindow -= bytes; }
   uint32_t InitialRwin() { return mInitialRwin; }
 
   void SendPing() override;
   bool MaybeReTunnel(nsAHttpTransaction *) override;
   bool UseH2Deps() { return mUseH2Deps; }
 
+  // overload of nsAHttpTransaction
+  nsresult ReadSegmentsAgain(nsAHttpSegmentReader *, uint32_t, uint32_t *, bool *) override final;
+  nsresult WriteSegmentsAgain(nsAHttpSegmentWriter *, uint32_t , uint32_t *, bool *) override final;
+
 private:
 
   // These internal states do not correspond to the states of the HTTP/2 specification
   enum internalStateType {
     BUFFERING_OPENING_SETTINGS,
     BUFFERING_FRAME_HEADER,
     BUFFERING_CONTROL_FRAME,
     PROCESSING_DATA_FRAME_PADDING_CONTROL,
--- a/netwerk/protocol/http/SpdySession31.cpp
+++ b/netwerk/protocol/http/SpdySession31.cpp
@@ -1751,19 +1751,20 @@ SpdySession31::OnTransportStatus(nsITran
 }
 
 // ReadSegments() is used to write data to the network. Generally, HTTP
 // request data is pulled from the approriate transaction and
 // converted to SPDY data. Sometimes control data like window-update are
 // generated instead.
 
 nsresult
-SpdySession31::ReadSegments(nsAHttpSegmentReader *reader,
-                            uint32_t count,
-                            uint32_t *countRead)
+SpdySession31::ReadSegmentsAgain(nsAHttpSegmentReader *reader,
+                                 uint32_t count,
+                                 uint32_t *countRead,
+                                 bool *again)
 {
   MOZ_ASSERT(PR_GetCurrentThread() == gSocketThread);
 
   MOZ_ASSERT(!mSegmentReader || !reader || (mSegmentReader == reader),
              "Inconsistent Write Function Callback");
 
   if (reader)
     mSegmentReader = reader;
@@ -1821,16 +1822,18 @@ SpdySession31::ReadSegments(nsAHttpSegme
           this, rv));
     if (rv == NS_BASE_STREAM_WOULD_BLOCK) {
       return rv;
     }
 
     CleanupStream(stream, rv, RST_CANCEL);
     if (SoftStreamError(rv)) {
       LOG3(("SpdySession31::ReadSegments %p soft error override\n", this));
+      *again = false;
+      SetWriteCallbacks();
       rv = NS_OK;
     }
     return rv;
   }
 
   if (*countRead > 0) {
     LOG3(("SpdySession31::ReadSegments %p stream=%p countread=%d",
           this, stream, *countRead));
@@ -1850,33 +1853,42 @@ SpdySession31::ReadSegments(nsAHttpSegme
 
   // call readsegments again if there are other streams ready
   // to go in this session
   SetWriteCallbacks();
 
   return rv;
 }
 
+nsresult
+SpdySession31::ReadSegments(nsAHttpSegmentReader *reader,
+                            uint32_t count, uint32_t *countRead)
+{
+  bool again = false;
+  return ReadSegmentsAgain(reader, count, countRead, &again);
+}
+
 // WriteSegments() is used to read data off the socket. Generally this is
 // just the SPDY frame header and from there the appropriate SPDYStream
 // is identified from the Stream-ID. The http transaction associated with
 // that read then pulls in the data directly, which it will feed to
 // OnWriteSegment(). That function will gateway it into http and feed
 // it to the appropriate transaction.
 
 // we call writer->OnWriteSegment via NetworkRead() to get a spdy header..
 // and decide if it is data or control.. if it is control, just deal with it.
 // if it is data, identify the spdy stream
 // call stream->WriteSegments which can call this::OnWriteSegment to get the
 // data. It always gets full frames if they are part of the stream
 
 nsresult
-SpdySession31::WriteSegments(nsAHttpSegmentWriter *writer,
-                             uint32_t count,
-                             uint32_t *countWritten)
+SpdySession31::WriteSegmentsAgain(nsAHttpSegmentWriter *writer,
+                                  uint32_t count,
+                                  uint32_t *countWritten,
+                                  bool *again)
 {
   typedef nsresult  (*Control_FX) (SpdySession31 *self);
   static const Control_FX sControlFunctions[] =
   {
       nullptr,
       SpdySession31::HandleSynStream,
       SpdySession31::HandleSynReply,
       SpdySession31::HandleRstStream,
@@ -2127,16 +2139,18 @@ SpdySession31::WriteSegments(nsAHttpSegm
       LOG3(("SpdySession31::WriteSegments session=%p stream=%p 0x%X "
             "needscleanup=%p. cleanup stream based on "
             "stream->writeSegments returning code %X\n",
             this, stream, stream ? stream->StreamID() : 0,
             mNeedsCleanup, rv));
       CleanupStream(stream, NS_OK, RST_CANCEL);
       MOZ_ASSERT(!mNeedsCleanup || mNeedsCleanup == stream);
       mNeedsCleanup = nullptr;
+      *again = false;
+      ResumeRecv();
       return NS_OK;
     }
 
     if (mNeedsCleanup) {
       LOG3(("SpdySession31::WriteSegments session=%p stream=%p 0x%X "
             "cleanup stream based on mNeedsCleanup.\n",
             this, mNeedsCleanup, mNeedsCleanup ? mNeedsCleanup->StreamID() : 0));
       CleanupStream(mNeedsCleanup, NS_OK, RST_CANCEL);
@@ -2229,16 +2243,24 @@ SpdySession31::WriteSegments(nsAHttpSegm
              mDownstreamState != BUFFERING_CONTROL_FRAME,
              "Control Handler returned OK but did not change state");
 
   if (mShouldGoAway && !mStreamTransactionHash.Count())
     Close(NS_OK);
   return rv;
 }
 
+nsresult
+SpdySession31::WriteSegments(nsAHttpSegmentWriter *writer,
+                             uint32_t count, uint32_t *countWritten)
+{
+  bool again = false;
+  return WriteSegmentsAgain(writer, count, countWritten, &again);
+}
+
 void
 SpdySession31::UpdateLocalStreamWindow(SpdyStream31 *stream,
                                        uint32_t bytes)
 {
   if (!stream) // this is ok - it means there was a data frame for a rst stream
     return;
 
   stream->DecrementLocalWindow(bytes);
--- a/netwerk/protocol/http/SpdySession31.h
+++ b/netwerk/protocol/http/SpdySession31.h
@@ -193,16 +193,20 @@ public:
   nsISocketTransport *SocketTransport() { return mSocketTransport; }
   int64_t RemoteSessionWindow() { return mRemoteSessionWindow; }
   void DecrementRemoteSessionWindow (uint32_t bytes) { mRemoteSessionWindow -= bytes; }
 
   void SendPing() override;
 
   bool MaybeReTunnel(nsAHttpTransaction *) override;
 
+  // overload of nsAHttpTransaction
+  nsresult ReadSegmentsAgain(nsAHttpSegmentReader *, uint32_t, uint32_t *, bool *) override final;
+  nsresult WriteSegmentsAgain(nsAHttpSegmentWriter *, uint32_t , uint32_t *, bool *) override final;
+
 private:
 
   enum stateType {
     BUFFERING_FRAME_HEADER,
     BUFFERING_CONTROL_FRAME,
     PROCESSING_DATA_FRAME,
     DISCARDING_DATA_FRAME,
     PROCESSING_COMPLETE_HEADERS,
--- a/netwerk/protocol/http/nsAHttpTransaction.h
+++ b/netwerk/protocol/http/nsAHttpTransaction.h
@@ -71,16 +71,30 @@ public:
     // called to read request data from the transaction.
     virtual nsresult ReadSegments(nsAHttpSegmentReader *reader,
                                   uint32_t count, uint32_t *countRead) = 0;
 
     // called to write response data to the transaction.
     virtual nsresult WriteSegments(nsAHttpSegmentWriter *writer,
                                    uint32_t count, uint32_t *countWritten) = 0;
 
+    // These versions of the functions allow the overloader to specify whether or
+    // not it is safe to call *Segments() in a loop while they return OK.
+    // The callee should turn again to false if it is not, otherwise leave untouched
+    virtual nsresult ReadSegmentsAgain(nsAHttpSegmentReader *reader,
+                                       uint32_t count, uint32_t *countRead, bool *again)
+    {
+        return ReadSegments(reader, count, countRead);
+    }
+    virtual nsresult WriteSegmentsAgain(nsAHttpSegmentWriter *writer,
+                                   uint32_t count, uint32_t *countWritten, bool *again)
+    {
+        return WriteSegments(writer, count, countWritten);
+    }
+
     // called to close the transaction
     virtual void Close(nsresult reason) = 0;
 
     // called to indicate a failure with proxy CONNECT
     virtual void SetProxyConnectFailed() = 0;
 
     // called to retrieve the request headers of the transaction
     virtual nsHttpRequestHead *RequestHead() = 0;
--- a/netwerk/protocol/http/nsHttpConnection.cpp
+++ b/netwerk/protocol/http/nsHttpConnection.cpp
@@ -1590,18 +1590,18 @@ nsHttpConnection::OnSocketWritable()
             if (!mReportedSpdy) {
                 mReportedSpdy = true;
                 MOZ_ASSERT(!mEverUsedSpdy);
                 gHttpHandler->ConnMgr()->ReportSpdyConnection(this, false);
             }
 
             LOG(("  writing transaction request stream\n"));
             mProxyConnectInProgress = false;
-            rv = mTransaction->ReadSegments(this, nsIOService::gDefaultSegmentSize,
-                                            &transactionBytes);
+            rv = mTransaction->ReadSegmentsAgain(this, nsIOService::gDefaultSegmentSize,
+                                                 &transactionBytes, &again);
             mContentBytesWritten += transactionBytes;
         }
 
         LOG(("nsHttpConnection::OnSocketWritable %p "
              "ReadSegments returned [rv=%x read=%u sock-cond=%x]\n",
              this, rv, transactionBytes, mSocketOutCondition));
 
         // XXX some streams return NS_BASE_STREAM_CLOSED to indicate EOF.
@@ -1761,17 +1761,18 @@ nsHttpConnection::OnSocketReadable()
 
             LOG(("nsHttpConnection::OnSocketReadable %p return due to inactive "
                  "tunnel setup but incomplete NPN state\n", this));
             rv = NS_OK;
             break;
         }
 
         mSocketInCondition = NS_OK;
-        rv = mTransaction->WriteSegments(this, nsIOService::gDefaultSegmentSize, &n);
+        rv = mTransaction->
+            WriteSegmentsAgain(this, nsIOService::gDefaultSegmentSize, &n, &again);
         LOG(("nsHttpConnection::OnSocketReadable %p trans->ws rv=%x n=%d socketin=%x\n",
              this, rv, n, mSocketInCondition));
         if (NS_FAILED(rv)) {
             // if the transaction didn't want to take any more data, then
             // wait for the transaction to call ResumeRecv.
             if (rv == NS_BASE_STREAM_WOULD_BLOCK) {
                 rv = NS_OK;
             }