Bug 1249379 - H2 Push WindowUpdate not written to network r=hurley

netwerk/protocol/http/Http2Push.cpp

@@ -152,25 +152,47 @@ Http2PushedStream::TryOnPush()
 }
 
 nsresult
-Http2PushedStream::ReadSegments(nsAHttpSegmentReader *,
+Http2PushedStream::ReadSegments(nsAHttpSegmentReader *reader,
                                 uint32_t, uint32_t *count)
 {
-  // The request headers for this has been processed, so we need to verify
-  // that :authority, :scheme, and :path MUST be present. :method MUST NOT be
-  // present
-  CreatePushHashKey(mHeaderScheme, mHeaderHost,
-                    mSession->Serial(), mHeaderPath,
-                    mOrigin, mHashKey);
-
-  LOG3(("Http2PushStream 0x%X hash key %s\n", mStreamID, mHashKey.get()));
-
-  // the write side of a pushed transaction just involves manipulating a little state
-  SetSentFin(true);
-  Http2Stream::mRequestHeadersDone = 1;
-  Http2Stream::mOpenGenerated = 1;
-  Http2Stream::ChangeState(UPSTREAM_COMPLETE);
+  nsresult rv = NS_OK;
   *count = 0;
-  return NS_OK;
+
+  switch (mUpstreamState) {
+  case GENERATING_HEADERS:
+    // The request headers for this has been processed, so we need to verify
+    // that :authority, :scheme, and :path MUST be present. :method MUST NOT be
+    // present
+    CreatePushHashKey(mHeaderScheme, mHeaderHost,
+                      mSession->Serial(), mHeaderPath,
+                      mOrigin, mHashKey);
+
+    LOG3(("Http2PushStream 0x%X hash key %s\n", mStreamID, mHashKey.get()));
+
+    // the write side of a pushed transaction just involves manipulating a little state
+    SetSentFin(true);
+    Http2Stream::mRequestHeadersDone = 1;
+    Http2Stream::mOpenGenerated = 1;
+    Http2Stream::ChangeState(UPSTREAM_COMPLETE);
+    break;
+
+  case UPSTREAM_COMPLETE:
+    // Let's just clear the stream's transmit buffer by pushing it into
+    // the session. This is probably a window adjustment.
+    LOG3(("Http2Push::ReadSegments 0x%X \n", mStreamID));
+    mSegmentReader = reader;
+    rv = TransmitFrame(nullptr, nullptr, true);
+    mSegmentReader = nullptr;
+    break;
+
+  case GENERATING_BODY:
+  case SENDING_BODY:
+  case SENDING_FIN_STREAM:
+  default:
+    break;
+  }
+
+  return rv;
 }
 
 void
@@ -178,12 +200,13 @@ Http2PushedStream::AdjustInitialWindow()
 {
   LOG3(("Http2PushStream %p 0x%X AdjustInitialWindow", this, mStreamID));
   if (mConsumerStream) {
-    LOG3(("Http2PushStream::AdjustInitialWindow %p 0x%X calling super %p", this,
-          mStreamID, mConsumerStream));
+    LOG3(("Http2PushStream::AdjustInitialWindow %p 0x%X "
+          "calling super consumer %p 0x%X\n", this,
+          mStreamID, mConsumerStream, mConsumerStream->StreamID()));
     Http2Stream::AdjustInitialWindow();
-    // We have to use mConsumerStream here because our ReadSegments is a nop for
-    // actually sending data.
-    mSession->TransactionHasDataToWrite(mConsumerStream);
+    // Http2PushedStream::ReadSegments is needed to call TransmitFrame()
+    // and actually get this information into the session bytestream
+    mSession->TransactionHasDataToWrite(this);
   }
   // Otherwise, when we get hooked up, the initial window will get bumped
   // anyway, so we're good to go.

netwerk/protocol/http/Http2Stream.cpp

@@ -40,6 +40,8 @@ Http2Stream::Http2Stream(nsAHttpTransaction *httpTransaction,
                          int32_t priority)
   : mStreamID(0)
   , mSession(session)
+  , mSegmentReader(nullptr)
+  , mSegmentWriter(nullptr)
   , mUpstreamState(GENERATING_HEADERS)
   , mState(IDLE)
   , mRequestHeadersDone(0)
@@ -48,8 +50,6 @@ Http2Stream::Http2Stream(nsAHttpTransaction *httpTransaction,
   , mQueued(0)
   , mTransaction(httpTransaction)
   , mSocketTransport(session->SocketTransport())
-  , mSegmentReader(nullptr)
-  , mSegmentWriter(nullptr)
   , mChunkSize(session->SendingChunkSize())
   , mRequestBlockedOnRead(0)
   , mRecvdFin(0)
@@ -67,7 +67,6 @@ Http2Stream::Http2Stream(nsAHttpTransaction *httpTransaction,
   , mRequestBodyLenRemaining(0)
   , mLocalUnacked(0)
   , mBlockedOnRwin(false)
-  , mSentPushWindowBump(false)
   , mTotalSent(0)
   , mTotalRead(0)
   , mPushSource(nullptr)
@@ -1367,11 +1366,8 @@ Http2Stream::OnReadSegment(const char *buf,
     break;
 
   case UPSTREAM_COMPLETE:
-    MOZ_ASSERT(mPushSource && !mSentPushWindowBump, "unexpected upstream_complete");
+    MOZ_ASSERT(mPushSource);
     rv = TransmitFrame(nullptr, nullptr, true);
-    if (NS_SUCCEEDED(rv)) {
-      mSentPushWindowBump = true;
-    }
     break;
 
   default:

netwerk/protocol/http/Http2Stream.h

@@ -175,6 +175,12 @@ protected:
   // The session that this stream is a subset of
   Http2Session *mSession;
 
+  // These are temporary state variables to hold the argument to
+  // Read/WriteSegments so it can be accessed by On(read/write)segment
+  // further up the stack.
+  nsAHttpSegmentReader        *mSegmentReader;
+  nsAHttpSegmentWriter        *mSegmentWriter;
+
   nsCString     mOrigin;
   nsCString     mHeaderHost;
   nsCString     mHeaderScheme;
@@ -204,6 +210,7 @@ protected:
   void     ChangeState(enum upstreamStateType);
 
   virtual void AdjustInitialWindow();
+  nsresult TransmitFrame(const char *, uint32_t *, bool forceCommitment);
 
 private:
   friend class nsAutoPtr<Http2Stream>;
@@ -212,7 +219,6 @@ private:
   nsresult GenerateOpen();
 
   void     AdjustPushedPriority();
-  nsresult TransmitFrame(const char *, uint32_t *, bool forceCommitment);
   void     GenerateDataFrameHeader(uint32_t, bool);
 
   nsresult BufferInput(uint32_t , uint32_t *);
@@ -226,12 +232,6 @@ private:
   // The underlying socket transport object is needed to propogate some events
   nsISocketTransport         *mSocketTransport;
 
-  // These are temporary state variables to hold the argument to
-  // Read/WriteSegments so it can be accessed by On(read/write)segment
-  // further up the stack.
-  nsAHttpSegmentReader        *mSegmentReader;
-  nsAHttpSegmentWriter        *mSegmentWriter;
-
   // The quanta upstream data frames are chopped into
   uint32_t                    mChunkSize;
 
@@ -316,9 +316,6 @@ private:
   //   <= 0
   bool                         mBlockedOnRwin;
 
-  // For properly adjusting push stream windows - see bug 1228822
-  bool                         mSentPushWindowBump;
-
   // For Progress Events
   uint64_t                     mTotalSent;
   uint64_t                     mTotalRead;

netwerk/test/unit/test_http2.js

@@ -155,8 +155,9 @@ Http2PushListener.prototype = new Http2CheckListener();
 Http2PushListener.prototype.onDataAvailable = function(request, ctx, stream, off, cnt) {
   this.onDataAvailableFired = true;
   this.isHttp2Connection = checkIsHttp2(request);
-  if (request.originalURI.spec == "https://localhost:" + serverPort + "/push.js" ||
-      request.originalURI.spec == "https://localhost:" + serverPort + "/push2.js") {
+  if (request.originalURI.spec == "https://localhost:" + serverPort + "/push.js"  ||
+      request.originalURI.spec == "https://localhost:" + serverPort + "/push2.js" ||
+      request.originalURI.spec == "https://localhost:" + serverPort + "/push5.js") {
     do_check_eq(request.getResponseHeader("pushed"), "yes");
   }
   read_stream(stream, cnt);
@@ -517,6 +518,20 @@ function test_http2_push4() {
   chan.asyncOpen2(listener);
 }
 
+function test_http2_push5() {
+  var chan = makeChan("https://localhost:" + serverPort + "/push5");
+  chan.loadGroup = loadGroup;
+  var listener = new Http2PushListener();
+  chan.asyncOpen2(listener);
+}
+
+function test_http2_push6() {
+  var chan = makeChan("https://localhost:" + serverPort + "/push5.js");
+  chan.loadGroup = loadGroup;
+  var listener = new Http2PushListener();
+  chan.asyncOpen2(listener);
+}
+
 // this is a basic test where the server sends a simple document with 2 header
 // blocks. bug 1027364
 function test_http2_doubleheader() {
@@ -882,6 +897,8 @@ var tests = [ test_http2_post_big
             , test_http2_push2
             , test_http2_push3
             , test_http2_push4
+            , test_http2_push5
+            , test_http2_push6
             , test_http2_altsvc
             , test_http2_doubleheader
             , test_http2_xhr
@@ -984,8 +1001,10 @@ var altsvcpref1;
 var altsvcpref2;
 var loadGroup;
 var serverPort;
+var speculativeLimit;
 
 function resetPrefs() {
+  prefs.setIntPref("network.http.speculative-parallel-limit", speculativeLimit);
   prefs.setBoolPref("network.http.spdy.enabled", spdypref);
   prefs.setBoolPref("network.http.spdy.enabled.v3-1", spdy3pref);
   prefs.setBoolPref("network.http.spdy.allow-push", spdypush);
@@ -1004,7 +1023,7 @@ function run_test() {
   // Set to allow the cert presented by our H2 server
   do_get_profile();
   prefs = Cc["@mozilla.org/preferences-service;1"].getService(Ci.nsIPrefBranch);
-  var oldPref = prefs.getIntPref("network.http.speculative-parallel-limit");
+  speculativeLimit = prefs.getIntPref("network.http.speculative-parallel-limit");
   prefs.setIntPref("network.http.speculative-parallel-limit", 0);
 
   addCertOverride("localhost", serverPort,
@@ -1012,8 +1031,6 @@ function run_test() {
                   Ci.nsICertOverrideService.ERROR_MISMATCH |
                   Ci.nsICertOverrideService.ERROR_TIME);
 
-  prefs.setIntPref("network.http.speculative-parallel-limit", oldPref);
-
   // Enable all versions of spdy to see that we auto negotiate http/2
   spdypref = prefs.getBoolPref("network.http.spdy.enabled");
   spdy3pref = prefs.getBoolPref("network.http.spdy.enabled.v3-1");

testing/xpcshell/moz-http2/moz-http2.js

@@ -264,6 +264,20 @@ function handleRequest(req, res) {
     content = '<head> <script src="push2.js"/></head>body text';
   }
 
+  else if (u.pathname === "/push5") {
+    push = res.push('/push5.js');
+    push.writeHead(200, {
+      'content-type': 'application/javascript',
+      'pushed' : 'yes',
+      // no content-length
+      'X-Connection-Http2': 'yes'
+    });
+    content = generateContent(1024 * 150);
+    push.write(content);
+    push.end();
+    content = '<head> <script src="push5.js"/></head>body text';
+  }
+
   else if (u.pathname === "/pushapi1") {
     push1 = res.push(
         { hostname: 'localhost:' + serverPort, port: serverPort, path : '/pushapi1/1', method : 'GET',