Bug 1368080 - Only remove pushed streams from cache when canceling the stream if the ids match. r?mcmanus draft
authorNicholas Hurley <hurley@todesschaf.org>
Wed, 28 Jun 2017 10:29:34 -0700
changeset 602106 52a26f9e0b4883ad2605120b6f2cf5e3a1269aaf
parent 602105 0a91a5f772af3be0540794e44c94c7ff44974cbd
child 635463 231d9b6d2de473dc4def9beded86fc287635a204
push id66279
push userbmo:hurley@mozilla.com
push dateThu, 29 Jun 2017 17:21:43 +0000
reviewersmcmanus
bugs1368080
milestone56.0a1
Bug 1368080 - Only remove pushed streams from cache when canceling the stream if the ids match. r?mcmanus MozReview-Commit-ID: 5y0Aj6Bgk9u
netwerk/protocol/http/ASpdySession.cpp
netwerk/protocol/http/Http2Session.cpp
netwerk/protocol/http/PSpdyPush.h
netwerk/test/unit/test_http2.js
testing/xpcshell/moz-http2/moz-http2.js
--- a/netwerk/protocol/http/ASpdySession.cpp
+++ b/netwerk/protocol/http/ASpdySession.cpp
@@ -118,11 +118,26 @@ SpdyPushCache::RemovePushedStreamHttp2(c
   Http2PushedStream *rv = mHashHttp2.Get(key);
   LOG3(("SpdyPushCache::RemovePushedStreamHttp2 %s 0x%X\n",
         key.get(), rv ? rv->StreamID() : 0));
   if (rv)
     mHashHttp2.Remove(key);
   return rv;
 }
 
+Http2PushedStream *
+SpdyPushCache::RemovePushedStreamHttp2ByID(const nsCString& key, const uint32_t& streamID)
+{
+  Http2PushedStream *rv = mHashHttp2.Get(key);
+  LOG3(("SpdyPushCache::RemovePushedStreamHttp2ByID %s 0x%X 0x%X",
+        key.get(), rv ? rv->StreamID() : 0, streamID));
+  if (rv && streamID == rv->StreamID()) {
+    mHashHttp2.Remove(key);
+  } else {
+    // Ensure we overwrite our rv with null in case the stream IDs don't match
+    rv = nullptr;
+  }
+  return rv;
+}
+
 } // namespace net
 } // namespace mozilla
 
--- a/netwerk/protocol/http/Http2Session.cpp
+++ b/netwerk/protocol/http/Http2Session.cpp
@@ -1100,17 +1100,19 @@ Http2Session::CleanupStream(Http2Stream 
       nsAutoCString hashKey;
       DebugOnly<bool> rv = pushStream->GetHashKey(hashKey);
       MOZ_ASSERT(rv);
       nsIRequestContext *requestContext = aStream->RequestContext();
       if (requestContext) {
         SpdyPushCache *cache = nullptr;
         requestContext->GetSpdyPushCache(&cache);
         if (cache) {
-          Http2PushedStream *trash = cache->RemovePushedStreamHttp2(hashKey);
+          // Make sure the id of the stream in the push cache is the same
+          // as the id of the stream we're cleaning up! See bug 1368080.
+          Http2PushedStream *trash = cache->RemovePushedStreamHttp2ByID(hashKey, aStream->StreamID());
           LOG3(("Http2Session::CleanupStream %p aStream=%p pushStream=%p trash=%p",
                 this, aStream, pushStream, trash));
         }
       }
     }
   }
 
   RemoveStreamFromQueues(aStream);
--- a/netwerk/protocol/http/PSpdyPush.h
+++ b/netwerk/protocol/http/PSpdyPush.h
@@ -41,16 +41,17 @@ class SpdyPushCache
 {
 public:
   // The cache holds only weak pointers - no references
   SpdyPushCache();
   virtual ~SpdyPushCache();
   MOZ_MUST_USE bool  RegisterPushedStreamHttp2(const nsCString& key,
                                                Http2PushedStream *stream);
   Http2PushedStream *RemovePushedStreamHttp2(const nsCString& key);
+  Http2PushedStream *RemovePushedStreamHttp2ByID(const nsCString& key, const uint32_t& streamID);
 private:
   nsDataHashtable<nsCStringHashKey, Http2PushedStream *> mHashHttp2;
 };
 
 } // namespace net
 } // namespace mozilla
 
 #endif // mozilla_net_SpdyPush_Public_h
--- a/netwerk/test/unit/test_http2.js
+++ b/netwerk/test/unit/test_http2.js
@@ -964,16 +964,55 @@ function test_complete() {
   httpserv.stop(do_test_finished);
   do_test_pending();
   httpserv2.stop(do_test_finished);
 
   do_test_finished();
   do_timeout(0,run_next_test);
 }
 
+var Http2DoublepushListener = function () {};
+Http2DoublepushListener.prototype = new Http2CheckListener();
+Http2DoublepushListener.prototype.onStopRequest = function (request, ctx, status) {
+  do_check_true(this.onStartRequestFired);
+  do_check_true(Components.isSuccessCode(status));
+  do_check_true(this.onDataAvailableFired);
+  do_check_true(this.isHttp2Connection == this.shouldBeHttp2);
+
+  var chan = makeChan("https://localhost:" + serverPort + "/doublypushed");
+  var listener = new Http2DoublypushedListener();
+  chan.loadGroup = loadGroup;
+  chan.asyncOpen2(listener);
+};
+
+var Http2DoublypushedListener = function () {};
+Http2DoublypushedListener.prototype = new Http2CheckListener();
+Http2DoublypushedListener.prototype.readData = "";
+Http2DoublypushedListener.prototype.onDataAvailable = function (request, ctx, stream, off, cnt) {
+  this.onDataAvailableFired = true;
+  this.accum += cnt;
+  this.readData += read_stream(stream, cnt);
+};
+Http2DoublypushedListener.prototype.onStopRequest = function (request, ctx, status) {
+  do_check_true(this.onStartRequestFired);
+  do_check_true(Components.isSuccessCode(status));
+  do_check_true(this.onDataAvailableFired);
+  do_check_eq(this.readData, "pushed");
+
+  run_next_test();
+  do_test_finished();
+};
+
+function test_http2_doublepush() {
+  var chan = makeChan("https://localhost:" + serverPort + "/doublepush");
+  var listener = new Http2DoublepushListener();
+  chan.loadGroup = loadGroup;
+  chan.asyncOpen2(listener);
+}
+
 // hack - the header test resets the multiplex object on the server,
 // so make sure header is always run before the multiplex test.
 //
 // make sure post_big runs first to test race condition in restarting
 // a stalled stream when a SETTINGS frame arrives
 var tests = [ test_http2_post_big
             , test_http2_basic
             , test_http2_concurrent
@@ -999,16 +1038,17 @@ var tests = [ test_http2_post_big
             , test_http2_pushapi_1
             , test_http2_continuations
             , test_http2_blocking_download
             , test_http2_illegalhpacksoft
             , test_http2_illegalhpackhard
             , test_http2_folded_header
             , test_http2_empty_data
             , test_http2_status_phrase
+            , test_http2_doublepush
             // Add new tests above here - best to add new tests before h1
             // streams get too involved
             // These next two must always come in this order
             , test_http2_h11required_stream
             , test_http2_h11required_session
             , test_http2_retry_rst
             , test_http2_wrongsuite
             , test_http2_push_firstparty1
--- a/testing/xpcshell/moz-http2/moz-http2.js
+++ b/testing/xpcshell/moz-http2/moz-http2.js
@@ -802,16 +802,40 @@ function handleRequest(req, res) {
   else if (u.pathname === "/statusphrase") {
     // Fortunately, the node-http2 API is dumb enough to allow this right on
     // through, so we can easily test rejecting this on gecko's end.
     res.writeHead("200 OK");
     res.end(content);
     return;
   }
 
+  else if (u.pathname === "/doublepush") {
+    push1 = res.push('/doublypushed');
+    push1.writeHead(200, {
+      'content-type': 'text/plain',
+      'pushed' : 'yes',
+      'content-length' : 6,
+      'X-Connection-Http2': 'yes'
+    });
+    push1.end('pushed');
+
+    push2 = res.push('/doublypushed');
+    push2.writeHead(200, {
+      'content-type': 'text/plain',
+      'pushed' : 'yes',
+      'content-length' : 6,
+      'X-Connection-Http2': 'yes'
+    });
+    push2.end('pushed');
+  }
+
+  else if (u.pathname === "/doublypushed") {
+    content = 'not pushed';
+  }
+
   res.setHeader('Content-Type', 'text/html');
   if (req.httpVersionMajor != 2) {
     res.setHeader('Connection', 'close');
   }
   res.writeHead(200);
   res.end(content);
 }