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..f17afbc285b8fd8b49b855394ca364a73274d6d7 100644
--- a/net/quic/chromium/quic_network_transaction_unittest.cc
+++ b/net/quic/chromium/quic_network_transaction_unittest.cc
@@ -4300,6 +4300,64 @@ 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 header arrive, but after the connection

[xunjieli, 2017/06/21 20:55:28]

nit: s/arrive/arrives

[Ryan Hamilton, 2017/06/21 22:24:51]

Done. (Well, s/header/headers/)

+// 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;
+  mock_quic_data.AddWrite(
+      ConstructInitialSettingsPacket(1, &header_stream_offset));
+  mock_quic_data.AddWrite(ConstructClientRequestHeadersPacket(
+      2, GetNthClientInitiatedStreamId(0), true, true,
+      GetRequestHeaders("GET", "https", "/"), &header_stream_offset));
+  QuicStreamOffset server_header_offset = 0;
+  mock_quic_data.AddRead(ConstructServerPushPromisePacket(
+      1, GetNthClientInitiatedStreamId(0), GetNthServerInitiatedStreamId(0),
+      false, GetRequestHeaders("GET", "https", "/pushed.jpg"),
+      &server_header_offset, &server_maker_));
+  mock_quic_data.AddRead(ConstructServerResponseHeadersPacket(
+      2, GetNthClientInitiatedStreamId(0), false, false,
+      GetResponseHeaders("200 OK"), &server_header_offset));

[xunjieli, 2017/06/21 20:55:28]

Sorry I am not familiar with how push promise works. Is this the response code
for GET / or GET /pushed.jpg ?

[Ryan Hamilton, 2017/06/21 22:24:51]

It's the response headers for the first request ("/"). The response body for
/pushed.jpg never actually arrives.

+  mock_quic_data.AddWrite(ConstructClientAckPacket(3, 2, 1, 1));
+  mock_quic_data.AddRead(ConstructServerDataPacket(
+      3, GetNthClientInitiatedStreamId(0), false, true, 0, "hello!"));

[xunjieli, 2017/06/21 20:55:28]

Could you annotate these data packets? 
Is this the response body of GET / or GET /pushed.jpg ?

[Ryan Hamilton, 2017/06/21 22:24:51]

Done.
It's the response for the first request ("/"). The response for /pushed.jpg
never actually arrives.

+  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 conneciton

[xunjieli, 2017/06/21 20:55:28]

nit: typo in connection.

[Ryan Hamilton, 2017/06/21 22:24:51]

Done.

+    // is closed, but before the callback is executed.
+    request_.url = GURL("https://mail.example.org/pushed.jpg");
+    HttpNetworkTransaction trans(DEFAULT_PRIORITY, session_.get());
+    TestCompletionCallback callback;

[xunjieli, 2017/06/21 20:55:28]

|callback| should still be notified, right?
Can we check its result at the end of this test?

[Ryan Hamilton, 2017/06/21 22:24:51]

No, it will not be notified because the transaction is destroyed before the
callback runs. (Which is the particular corner case which leads to this crash)

[xunjieli, 2017/06/22 14:43:30]

Acknowledged.

+    int rv = trans.Start(&request_, callback.callback(), net_log_.bound());
+    EXPECT_THAT(rv, IsError(ERR_IO_PENDING));
+    base::RunLoop().RunUntilIdle();
+
+    // Cause the connection to close on a write error.
+    HttpNetworkTransaction trans2(DEFAULT_PRIORITY, session_.get());
+    TestCompletionCallback callback2;
+    request_.url = GURL("https://mail.example.org/");

[xunjieli, 2017/06/21 20:55:28]

nit: do not reuse |request_| for |trans2|. The Transaction.Start() takes in the
pointer address of |request_|. I am nervous about changing the HttpRequestInfo
while it is used by another transaction.

[Ryan Hamilton, 2017/06/21 22:24:51]

Done.

+    EXPECT_THAT(trans2.Start(&request_, callback2.callback(), net_log_.bound()),
+                IsError(ERR_IO_PENDING));
+
+    base::RunLoop().RunUntilIdle();
+
+    // When |trans| goes out of scope, the underlying stream will be closed
+  }

[xunjieli, 2017/06/21 20:55:28]

why do we need this extra scope {}?

[Ryan Hamilton, 2017/06/21 22:24:51]

Ah. We need this so that |trans| will be closed. There's no Cancel() method on
HttpNetworkTransaction. I could stuff it into a unique_ptr if you'd prefer?

[xunjieli, 2017/06/22 14:43:30]

|trans| is going out of scope anyway, right? This is the last line of the test.
If I remove the scope, the test still crashes at ResetStream() without the
patch.

[Ryan Hamilton, 2017/06/22 20:40:38]

Hahahhahahahahaha! Yes, you're obviously right, of course. Done.

+}
+
 TEST_P(QuicNetworkTransactionTest, QuicForceHolBlocking) {
   session_params_.quic_force_hol_blocking = true;
   session_params_.origins_to_force_quic_on.insert(