Bug 1409570 - Ensure a pushed stream knows we've reached EOS. r?mcmanus draft
authorNicholas Hurley <hurley@mozilla.com>
Wed, 18 Oct 2017 12:34:42 -0700
changeset 693880 c2cdc814a1aab28b21e0eb27f5b10fac5086f3a7
parent 692714 4ada8f0d5cc011af4bc0f4fadbb25ea3e1e62bfc
child 739171 900d1d7cebf11a5553969d6a7ec9f2858a0a4aa8
push id87952
push userbmo:hurley@mozilla.com
push dateTue, 07 Nov 2017 00:20:21 +0000
reviewersmcmanus
bugs1409570
milestone58.0a1
Bug 1409570 - Ensure a pushed stream knows we've reached EOS. r?mcmanus Previously, if a pushed stream was ended with a padding-only DATA frame with the FIN bit set, and that stream didn't have a Content-Length, there would be no way of knowing that the stream was finished. Now we force-mark the push as complete if we hit a padding-only DATA frame with the FIN bit set. MozReview-Commit-ID: 7tk8x2FNgSj
netwerk/protocol/http/Http2Push.h
netwerk/protocol/http/Http2Session.cpp
netwerk/protocol/http/Http2Stream.h
--- a/netwerk/protocol/http/Http2Push.h
+++ b/netwerk/protocol/http/Http2Push.h
@@ -63,16 +63,17 @@ public:
   bool IsOrphaned(TimeStamp now);
   void OnPushFailed() { mDeferCleanupOnPush = false; mOnPushFailed = true; }
 
   MOZ_MUST_USE nsresult GetBufferedData(char *buf, uint32_t count,
                                         uint32_t *countWritten);
 
   // overload of Http2Stream
   virtual bool HasSink() override { return !!mConsumerStream; }
+  virtual void SetPushComplete() override { mPushCompleted = true; }
 
   nsCString &GetRequestString() { return mRequestString; }
 
 private:
 
   Http2Stream *mConsumerStream; // paired request stream that consumes from
                                 // real http/2 one.. null until a match is made.
 
--- a/netwerk/protocol/http/Http2Session.cpp
+++ b/netwerk/protocol/http/Http2Session.cpp
@@ -3279,18 +3279,19 @@ Http2Session::WriteSegmentsAgain(nsAHttp
     return rv;
   }
 
   if (mDownstreamState == DISCARDING_DATA_FRAME ||
       mDownstreamState == DISCARDING_DATA_FRAME_PADDING) {
     char trash[4096];
     uint32_t discardCount = std::min(mInputFrameDataSize - mInputFrameDataRead,
                                      4096U);
-    LOG3(("Http2Session::WriteSegments %p trying to discard %d bytes of data",
-          this, discardCount));
+    LOG3(("Http2Session::WriteSegments %p trying to discard %d bytes of %s",
+          this, discardCount,
+          mDownstreamState == DISCARDING_DATA_FRAME ? "data" : "padding"));
 
     if (!discardCount && mDownstreamState == DISCARDING_DATA_FRAME) {
       // Only do this short-cirtuit if we're not discarding a pure padding
       // frame, as we need to potentially handle the stream FIN in those cases.
       // See bug 1381016 comment 36 for more details.
       ResetDownstreamState();
       Unused << ResumeRecv();
       return NS_BASE_STREAM_WOULD_BLOCK;
@@ -3312,19 +3313,25 @@ Http2Session::WriteSegmentsAgain(nsAHttp
     mInputFrameDataRead += *countWritten;
 
     if (mInputFrameDataRead == mInputFrameDataSize) {
       Http2Stream *streamToCleanup = nullptr;
       if (mInputFrameFinal) {
         streamToCleanup = mInputFrameDataStream;
       }
 
+      bool discardedPadding = (mDownstreamState == DISCARDING_DATA_FRAME_PADDING);
       ResetDownstreamState();
 
       if (streamToCleanup) {
+        if (discardedPadding && !(streamToCleanup->StreamID() & 1)) {
+          // Pushed streams are special on padding-only final data frames.
+          // See bug 1409570 comments 6-8 for details.
+          streamToCleanup->SetPushComplete();
+        }
         CleanupStream(streamToCleanup, NS_OK, CANCEL_ERROR);
       }
     }
     return rv;
   }
 
   if (mDownstreamState != BUFFERING_CONTROL_FRAME) {
     MOZ_ASSERT(false); // this cannot happen
--- a/netwerk/protocol/http/Http2Stream.h
+++ b/netwerk/protocol/http/Http2Stream.h
@@ -148,16 +148,19 @@ public:
   void SetPriority(uint32_t);
   void SetPriorityDependency(uint32_t, uint8_t, bool);
   void UpdatePriorityDependency();
 
   // A pull stream has an implicit sink, a pushed stream has a sink
   // once it is matched to a pull stream.
   virtual bool HasSink() { return true; }
 
+  // This is a no-op on pull streams. Pushed streams override this.
+  virtual void SetPushComplete() { };
+
   virtual ~Http2Stream();
 
   Http2Session *Session() { return mSession; }
 
   static MOZ_MUST_USE nsresult MakeOriginURL(const nsACString &origin,
                                              RefPtr<nsStandardURL> &url);
 
   static MOZ_MUST_USE nsresult MakeOriginURL(const nsACString &scheme,