Bug 1409570 - Ensure a pushed stream knows we've reached EOS. r=mcmanus
authorNicholas Hurley <hurley@mozilla.com>
Wed, 18 Oct 2017 12:34:42 -0700
changeset 390606 140a38df3d5639e8a15e2cb5f2fd35d844e44d67
parent 390605 dd50caefe127bb9a0993236dfefabe1a2cac68de
child 390607 1924564466fe274a20ce6c15a97e5f106cb9a3a9
push id32838
push usernbeleuzu@mozilla.com
push dateWed, 08 Nov 2017 10:46:55 +0000
treeherdermozilla-central@3e95c596ad5b [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmcmanus
bugs1409570
milestone58.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 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,