Retry request if server resets HTTP/2 pushed stream

When the server sends down a pushed stream and later on cancels it,
there's currently a race condition. If the pushed stream was not
adopted a request is sent for it, but if it was, it currently fails
in a user-visible way.
This CL fixes that by retrying the request if a pushed stream was
adopted and later cancelled.

Bug: 798508
Change-Id: Ia580cd4bad355cf4c5738e03db8dbfcf80716ae9
Reviewed-on: https://chromium-review.googlesource.com/848776Reviewed-by: Bence Béky <bnc@chromium.org>
Commit-Queue: Yoav Weiss <yoav@yoav.ws>
Cr-Commit-Position: refs/heads/master@{#527000}

net/base/net_error_list.h

@@ -732,6 +732,10 @@ NET_ERROR(SPDY_RST_STREAM_NO_ERROR_RECEIVED, -372)
 // The pushed stream claimed by the request is no longer available.
 NET_ERROR(SPDY_PUSHED_STREAM_NOT_AVAILABLE, -373)
 
+// A pushed stream was claimed and later reset by the server. When this happens,
+// the request should be retried.
+NET_ERROR(SPDY_CLAIMED_PUSHED_STREAM_RESET_BY_SERVER, -374)
+
 // The cache does not have the requested entry.
 NET_ERROR(CACHE_MISS, -400)
 

net/http/http_network_transaction.cc

@@ -1562,6 +1562,7 @@ int HttpNetworkTransaction::HandleIOError(int error) {
       break;
     case ERR_SPDY_PING_FAILED:
     case ERR_SPDY_SERVER_REFUSED_STREAM:
+    case ERR_SPDY_CLAIMED_PUSHED_STREAM_RESET_BY_SERVER:
     case ERR_QUIC_HANDSHAKE_FAILED:
       if (HasExceededMaxRetries())
         break;

net/spdy/chromium/spdy_network_transaction_unittest.cc

@@ -535,6 +535,11 @@ class SpdyNetworkTransactionTest : public ::testing::Test {
                url, session.get()) != kNoPushedStreamFound;
   }
 
+  static SpdyStreamId spdy_stream_hi_water_mark(
+      base::WeakPtr<SpdySession> session) {
+    return session->stream_hi_water_mark_;
+  }
+
   const GURL default_url_;
   const HostPortPair host_port_pair_;
   HttpRequestInfo request_;
@@ -6633,4 +6638,101 @@ TEST_F(SpdyNetworkTransactionTest, RequestHeadersCallback) {
   EXPECT_TRUE(raw_headers.request_line().empty());
 }
 
+// A request that has adopted a push promise and later got reset by the server
+// should be retried on a new stream.
+// Regression test for https://crbug.com/798508.
+TEST_F(SpdyNetworkTransactionTest, PushCanceledByServerAfterClaimed) {
+  const char pushed_url[] = "https://www.example.org/a.dat";
+  // Construct a request to the default URL on stream 1.
+  SpdySerializedFrame req(
+      spdy_util_.ConstructSpdyGet(nullptr, 0, 1, LOWEST, true));
+  SpdySerializedFrame req2(spdy_util_.ConstructSpdyGet(pushed_url, 3, LOWEST));
+  // Construct a priority frame for stream 2.
+  SpdySerializedFrame priority(
+      spdy_util_.ConstructSpdyPriority(2, 1, IDLE, true));
+  MockWrite writes[] = {CreateMockWrite(req, 0), CreateMockWrite(priority, 3),
+                        CreateMockWrite(req2, 6)};
+
+  // Construct a Push Promise frame, with no response.
+  SpdySerializedFrame push_promise(spdy_util_.ConstructInitialSpdyPushFrame(
+      spdy_util_.ConstructGetHeaderBlock(pushed_url), 2, 1));
+  // Construct a RST frame, canceling stream 2.
+  SpdySerializedFrame rst_server(
+      spdy_util_.ConstructSpdyRstStream(2, ERROR_CODE_CANCEL));
+  // Construct response headers and bodies.
+  SpdySerializedFrame resp1(spdy_util_.ConstructSpdyGetReply(nullptr, 0, 1));
+  SpdySerializedFrame body1(spdy_util_.ConstructSpdyDataFrame(1, true));
+  SpdySerializedFrame resp2(spdy_util_.ConstructSpdyGetReply(nullptr, 0, 3));
+  SpdySerializedFrame body2(spdy_util_.ConstructSpdyDataFrame(3, true));
+  MockRead reads[] = {
+      CreateMockRead(push_promise, 1), MockRead(ASYNC, ERR_IO_PENDING, 2),
+      CreateMockRead(rst_server, 4),   MockRead(ASYNC, ERR_IO_PENDING, 5),
+      CreateMockRead(resp1, 7),        CreateMockRead(body1, 8),
+      CreateMockRead(resp2, 9),        CreateMockRead(body2, 10),
+      MockRead(ASYNC, 0, 11)};
+
+  SequencedSocketData data(reads, arraysize(reads), writes, arraysize(writes));
+
+  NormalSpdyTransactionHelper helper(request_, DEFAULT_PRIORITY, log_, nullptr);
+
+  helper.RunPreTestSetup();
+  helper.AddData(&data);
+
+  HttpNetworkTransaction* trans = helper.trans();
+
+  // First request to start the connection.
+  TestCompletionCallback callback1;
+  int rv = trans->Start(&request_, callback1.callback(), log_);
+  EXPECT_THAT(rv, IsError(ERR_IO_PENDING));
+
+  data.RunUntilPaused();
+
+  // Get a SpdySession.
+  SpdySessionKey key(HostPortPair::FromURL(request_.url), ProxyServer::Direct(),
+                     PRIVACY_MODE_DISABLED);
+  HttpNetworkSession* session = helper.session();
+  base::WeakPtr<SpdySession> spdy_session =
+      session->spdy_session_pool()->FindAvailableSession(
+          key, /* enable_ip_based_pooling = */ true, log_);
+
+  // Verify that there is one unclaimed push stream.
+  EXPECT_EQ(1u, num_unclaimed_pushed_streams(spdy_session));
+
+  // Claim the pushed stream.
+  HttpNetworkTransaction transaction2(DEFAULT_PRIORITY, session);
+  TestCompletionCallback callback2;
+  HttpRequestInfo request2;
+  request2.method = "GET";
+  request2.url = GURL(pushed_url);
+  transaction2.Start(&request2, callback2.callback(), log_);
+  base::RunLoop().RunUntilIdle();
+  EXPECT_EQ(3u, spdy_stream_hi_water_mark(spdy_session));
+
+  EXPECT_EQ(0u, num_unclaimed_pushed_streams(spdy_session));
+
+  // Continue reading and get the RST.
+  data.Resume();
+  base::RunLoop().RunUntilIdle();
+
+  // Make sure we got the RST and retried the request.
+  EXPECT_EQ(2u, num_active_streams(spdy_session));
+  EXPECT_EQ(0u, num_unclaimed_pushed_streams(spdy_session));
+  EXPECT_EQ(5u, spdy_stream_hi_water_mark(spdy_session));
+
+  data.Resume();
+
+  // Test that transactions succeeded.
+  rv = callback1.WaitForResult();
+  ASSERT_THAT(rv, IsOk());
+
+  rv = callback2.WaitForResult();
+  ASSERT_THAT(rv, IsOk());
+
+  // Read EOF.
+  base::RunLoop().RunUntilIdle();
+
+  // Verify that all data was read and written.
+  helper.VerifyDataConsumed();
+}
+
 }  // namespace net

net/spdy/chromium/spdy_session.cc

@@ -2639,9 +2639,13 @@ void SpdySession::OnRstStream(SpdyStreamId stream_id,
     return;
   }
 
+  DCHECK(it->second);
   CHECK_EQ(it->second->stream_id(), stream_id);
 
-  if (error_code == ERROR_CODE_NO_ERROR) {
+  if (it->second->ShouldRetryRSTPushStream()) {
+    CloseActiveStreamIterator(it,
+                              ERR_SPDY_CLAIMED_PUSHED_STREAM_RESET_BY_SERVER);
+  } else if (error_code == ERROR_CODE_NO_ERROR) {
     CloseActiveStreamIterator(it, ERR_SPDY_RST_STREAM_NO_ERROR_RECEIVED);
   } else if (error_code == ERROR_CODE_REFUSED_STREAM) {
     CloseActiveStreamIterator(it, ERR_SPDY_SERVER_REFUSED_STREAM);

net/spdy/chromium/spdy_stream.cc

@@ -455,6 +455,12 @@ void SpdyStream::OnHeadersReceived(const SpdyHeaderBlock& response_headers,
   }
 }
 
+bool SpdyStream::ShouldRetryRSTPushStream() {
+  // Retry if the stream is a pushed stream, has been claimed, but did not yet
+  // receive response headers
+  return (response_headers_.empty() && type_ == SPDY_PUSH_STREAM && delegate_);
+}
+
 void SpdyStream::OnPushPromiseHeadersReceived(SpdyHeaderBlock headers) {
   CHECK(!request_headers_valid_);
   CHECK_EQ(io_state_, STATE_IDLE);

net/spdy/chromium/spdy_stream.h

@@ -370,6 +370,7 @@ class NET_EXPORT_PRIVATE SpdyStream {
   int64_t raw_received_bytes() const { return raw_received_bytes_; }
   int64_t raw_sent_bytes() const { return raw_sent_bytes_; }
   int recv_bytes() const { return recv_bytes_; }
+  bool ShouldRetryRSTPushStream();
 
   bool GetLoadTimingInfo(LoadTimingInfo* load_timing_info) const;