Bug 1368080 - Only remove pushed streams from cache when canceling the stream if the ids match. r=mcmanus

MozReview-Commit-ID: 5y0Aj6Bgk9u

--HG--
extra : rebase_source : 015352a4b0a87734afce856fcd7b424e96c6ede8

netwerk/protocol/http/ASpdySession.cpp

@@ -123,6 +123,21 @@ SpdyPushCache::RemovePushedStreamHttp2(const nsCString& 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
 

netwerk/protocol/http/Http2Session.cpp

@@ -1105,7 +1105,9 @@ Http2Session::CleanupStream(Http2Stream *aStream, nsresult aResult,
         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));
         }

netwerk/protocol/http/PSpdyPush.h

@@ -46,6 +46,7 @@ public:
   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;
 };

netwerk/test/unit/test_http2.js

@@ -969,6 +969,45 @@ function test_complete() {
   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.
 //
@@ -1004,6 +1043,7 @@ var tests = [ test_http2_post_big
             , 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

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

@@ -807,6 +807,30 @@ function handleRequest(req, res) {
     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');