Add an async method to QuicChromiumClientSession::Handle for

net/quic/chromium/quic_network_transaction_unittest.cc

Index: net/quic/chromium/quic_network_transaction_unittest.cc
diff --git a/net/quic/chromium/quic_network_transaction_unittest.cc b/net/quic/chromium/quic_network_transaction_unittest.cc
index 334ae5c7ceaaef58bdab279c5af82c256ee7c8d2..cb0ccb584ec3ac60d75ae4392f42230c1aca2267 100644
--- a/net/quic/chromium/quic_network_transaction_unittest.cc
+++ b/net/quic/chromium/quic_network_transaction_unittest.cc
@@ -4300,6 +4300,77 @@ TEST_P(QuicNetworkTransactionTest, QuicServerPush) {
   EXPECT_LT(0, pos);
 }
 
+// Regression test for http://crbug.com/719461 in which a promised stream
+// is closed before the pushed headers arrive, but after the connection
+// is closed and before the callbacks are executed.
+TEST_P(QuicNetworkTransactionTest, CancelServerPushAfterConnectionClose) {
+  session_params_.origins_to_force_quic_on.insert(
+      HostPortPair::FromString("mail.example.org:443"));
+
+  MockQuicData mock_quic_data;
+  QuicStreamOffset header_stream_offset = 0;
+  // Initial SETTINGS frame.
+  mock_quic_data.AddWrite(
+      ConstructInitialSettingsPacket(1, &header_stream_offset));
+  // First request: GET https://mail.example.org/
+  mock_quic_data.AddWrite(ConstructClientRequestHeadersPacket(
+      2, GetNthClientInitiatedStreamId(0), true, true,
+      GetRequestHeaders("GET", "https", "/"), &header_stream_offset));
+  QuicStreamOffset server_header_offset = 0;
+  // Server promise for: https://mail.example.org/pushed.jpg
+  mock_quic_data.AddRead(ConstructServerPushPromisePacket(
+      1, GetNthClientInitiatedStreamId(0), GetNthServerInitiatedStreamId(0),
+      false, GetRequestHeaders("GET", "https", "/pushed.jpg"),
+      &server_header_offset, &server_maker_));
+  // Response headers for first request.
+  mock_quic_data.AddRead(ConstructServerResponseHeadersPacket(
+      2, GetNthClientInitiatedStreamId(0), false, false,
+      GetResponseHeaders("200 OK"), &server_header_offset));
+  // Client ACKs the response headers.
+  mock_quic_data.AddWrite(ConstructClientAckPacket(3, 2, 1, 1));
+  // Response body for first request.
+  mock_quic_data.AddRead(ConstructServerDataPacket(
+      3, GetNthClientInitiatedStreamId(0), false, true, 0, "hello!"));
+  // Write error for the third request.
+  mock_quic_data.AddWrite(SYNCHRONOUS, ERR_FAILED);
+  mock_quic_data.AddRead(ASYNC, ERR_IO_PENDING);  // No more data to read
+  mock_quic_data.AddRead(ASYNC, 0);               // EOF
+  mock_quic_data.AddSocketDataToFactory(&socket_factory_);
+
+  CreateSession();
+
+  // Send a request which triggers a push promise from the server.
+  SendRequestAndExpectQuicResponse("hello!");
+
+  // Start a push transaction that will be cancelled after the connection
+  // is closed, but before the callback is executed.
+  request_.url = GURL("https://mail.example.org/pushed.jpg");
+  auto trans2 = base::MakeUnique<HttpNetworkTransaction>(DEFAULT_PRIORITY,
+                                                         session_.get());
+  TestCompletionCallback callback2;
+  int rv = trans2->Start(&request_, callback2.callback(), net_log_.bound());
+  EXPECT_THAT(rv, IsError(ERR_IO_PENDING));
+  base::RunLoop().RunUntilIdle();
+
+  // Cause the connection to close on a write error.
+  HttpNetworkTransaction trans3(DEFAULT_PRIORITY, session_.get());
+  TestCompletionCallback callback3;
+  HttpRequestInfo request3;
+  request3.method = "GET";
+  request3.url = GURL("https://mail.example.org/");
+  request3.load_flags = 0;
+  EXPECT_THAT(trans3.Start(&request3, callback3.callback(), net_log_.bound()),
+              IsError(ERR_IO_PENDING));
+
+  EXPECT_FALSE(callback2.have_result());
+  base::RunLoop().RunUntilIdle();

[xunjieli, 2017/06/22 20:50:46]

Maybe remove this base::RunLoop().RunUntilIdle();

[Ryan Hamilton, 2017/06/22 21:54:35]

So I initially tried replacing that with:

EXPECT_THAT(callback3.WaitForResult(), IsError(ERR_QUIC_PROTOCOL_ERROR))

Thinking that they should both be equivalent. Namely, they spin the message loop
to cause the write to complete and invoke callback3. However they turn out not
to be equivalent in that, with the old code at least, if I use WaitForResult
here, then the test fails when the pushed stream is cancelled after the session
is already closed, which hits a DCHECK. It doesn't fail because of the crash.

Of course, if I just remove RunUntilIdle, then the connection has not yet been
closed when trans2 is destroyed and the old code does not crash.

So I think I need it.

[xunjieli, 2017/06/22 22:33:04]

Acknowledged. I see. I don't fully understand this part, but I will trust your
word for it :)

[Ryan Hamilton, 2017/06/22 22:43:29]

That makes two of us :)

+
+  // When |trans2| is destroyed, the underlying stream will be closed.
+  trans2 = nullptr;
+
+  EXPECT_THAT(callback3.WaitForResult(), IsError(ERR_QUIC_PROTOCOL_ERROR));
+}
+
 TEST_P(QuicNetworkTransactionTest, QuicForceHolBlocking) {
   session_params_.quic_force_hol_blocking = true;
   session_params_.origins_to_force_quic_on.insert(