Move the "wait for QUIC handshake confirmation" logic to QuicChromiumClientSession::StreamRequest

net/quic/chromium/bidirectional_stream_quic_impl_unittest.cc

Index: net/quic/chromium/bidirectional_stream_quic_impl_unittest.cc
diff --git a/net/quic/chromium/bidirectional_stream_quic_impl_unittest.cc b/net/quic/chromium/bidirectional_stream_quic_impl_unittest.cc
index c92c380862cb53af67e128f835495859786125b4..fe867052cfa9a8a2b26d5a42e9e7e6af256eeda4 100644
--- a/net/quic/chromium/bidirectional_stream_quic_impl_unittest.cc
+++ b/net/quic/chromium/bidirectional_stream_quic_impl_unittest.cc
@@ -915,6 +915,7 @@ TEST_P(BidirectionalStreamQuicImplTest, CoalesceDataBuffersNotHeadersFrame) {
   EXPECT_FALSE(delegate->is_ready());
   ConfirmHandshake();
   delegate->WaitUntilNextCallback();  // OnStreamReady
+  EXPECT_TRUE(delegate->is_ready());
 
   // Sends request headers separately, which causes them to be sent in a
   // separate packet.
@@ -1191,10 +1192,10 @@ TEST_P(BidirectionalStreamQuicImplTest, PostRequest) {
   SetRequest("POST", "/", DEFAULT_PRIORITY);
   size_t spdy_request_headers_frame_length;
   QuicStreamOffset header_stream_offset = 0;
+  AddWrite(ConstructInitialSettingsPacket(1, &header_stream_offset));
   AddWrite(ConstructRequestHeadersPacketInner(
-      1, kClientDataStreamId1, !kFin, DEFAULT_PRIORITY,
+      2, kClientDataStreamId1, !kFin, DEFAULT_PRIORITY,
       &spdy_request_headers_frame_length, &header_stream_offset));

[xunjieli, 2017/05/01 20:15:05]

Settings header before request headers is much more reasonable. I wonder why
these tests had the order flipped before..

[Ryan Hamilton, 2017/05/01 21:42:32]

Ah! Sorry, I meant to call this out in my mail to you.  The initial SETTINGS
frame is sent by the session from OnHandshakeConfirmed, but at the end of the
method. With the old code, a request which was waiting for confirmation would
have OnHandshakeConfirmation() invoked earlier in the method, which would send
out the request *before* the settings. To avoid reentrancy, the new code posts a
tasks to resume the waiting requests which changes the order to what we see
here.

QuicHttpStream, on the other hand, posted a task in the old code, so there is no
behavior change there.

[xunjieli, 2017/05/02 17:23:05]

Acknowledged. Got it. Thanks for walking me through the code!

-  AddWrite(ConstructInitialSettingsPacket(2, &header_stream_offset));
   AddWrite(ConstructDataPacket(3, kIncludeVersion, kFin, 0, kUploadData,
                                &client_maker_));
   AddWrite(ConstructClientAckPacket(4, 3, 1, 1));
@@ -1349,11 +1350,10 @@ TEST_P(BidirectionalStreamQuicImplTest, InterleaveReadDataAndSendData) {
   SetRequest("POST", "/", DEFAULT_PRIORITY);
   size_t spdy_request_headers_frame_length;
   QuicStreamOffset header_stream_offset = 0;
+  AddWrite(ConstructInitialSettingsPacket(1, &header_stream_offset));
   AddWrite(ConstructRequestHeadersPacketInner(
-      1, kClientDataStreamId1, !kFin, DEFAULT_PRIORITY,
+      2, kClientDataStreamId1, !kFin, DEFAULT_PRIORITY,
       &spdy_request_headers_frame_length, &header_stream_offset));
-
-  AddWrite(ConstructInitialSettingsPacket(2, &header_stream_offset));
   AddWrite(ConstructAckAndDataPacket(3, !kIncludeVersion, 2, 1, 1, !kFin, 0,
                                      kUploadData, &client_maker_));
   AddWrite(ConstructAckAndDataPacket(4, !kIncludeVersion, 3, 3, 3, kFin,
@@ -1540,10 +1540,10 @@ TEST_P(BidirectionalStreamQuicImplTest, SessionClosedBeforeReadData) {
   SetRequest("POST", "/", DEFAULT_PRIORITY);
   size_t spdy_request_headers_frame_length;
   QuicStreamOffset header_stream_offset = 0;
+  AddWrite(ConstructInitialSettingsPacket(1, &header_stream_offset));
   AddWrite(ConstructRequestHeadersPacketInner(
-      1, kClientDataStreamId1, !kFin, DEFAULT_PRIORITY,
+      2, kClientDataStreamId1, !kFin, DEFAULT_PRIORITY,
       &spdy_request_headers_frame_length, &header_stream_offset));
-  AddWrite(ConstructInitialSettingsPacket(2, &header_stream_offset));
   Initialize();
 
   BidirectionalStreamRequestInfo request;
@@ -1600,10 +1600,10 @@ TEST_P(BidirectionalStreamQuicImplTest, DeleteStreamAfterReadData) {
   SetRequest("POST", "/", DEFAULT_PRIORITY);
   size_t spdy_request_headers_frame_length;
   QuicStreamOffset header_stream_offset = 0;
+  AddWrite(ConstructInitialSettingsPacket(1, &header_stream_offset));
   AddWrite(ConstructRequestHeadersPacketInner(
-      1, kClientDataStreamId1, !kFin, DEFAULT_PRIORITY,
+      2, kClientDataStreamId1, !kFin, DEFAULT_PRIORITY,
       &spdy_request_headers_frame_length, &header_stream_offset));
-  AddWrite(ConstructInitialSettingsPacket(2, &header_stream_offset));
   AddWrite(ConstructClientAckAndRstStreamPacket(3, 2, 1, 1));
 
   Initialize();
@@ -1654,10 +1654,10 @@ TEST_P(BidirectionalStreamQuicImplTest, DeleteStreamDuringOnHeadersReceived) {
   SetRequest("POST", "/", DEFAULT_PRIORITY);
   size_t spdy_request_headers_frame_length;
   QuicStreamOffset header_stream_offset = 0;
+  AddWrite(ConstructInitialSettingsPacket(1, &header_stream_offset));
   AddWrite(ConstructRequestHeadersPacketInner(
-      1, kClientDataStreamId1, !kFin, DEFAULT_PRIORITY,
+      2, kClientDataStreamId1, !kFin, DEFAULT_PRIORITY,
       &spdy_request_headers_frame_length, &header_stream_offset));
-  AddWrite(ConstructInitialSettingsPacket(2, &header_stream_offset));
   AddWrite(ConstructClientAckAndRstStreamPacket(3, 2, 1, 1));
 
   Initialize();
@@ -1700,10 +1700,10 @@ TEST_P(BidirectionalStreamQuicImplTest, DeleteStreamDuringOnDataRead) {
   SetRequest("POST", "/", DEFAULT_PRIORITY);
   size_t spdy_request_headers_frame_length;
   QuicStreamOffset header_stream_offset = 0;
+  AddWrite(ConstructInitialSettingsPacket(1, &header_stream_offset));
   AddWrite(ConstructRequestHeadersPacketInner(
-      1, kClientDataStreamId1, !kFin, DEFAULT_PRIORITY,
+      2, kClientDataStreamId1, !kFin, DEFAULT_PRIORITY,
       &spdy_request_headers_frame_length, &header_stream_offset));
-  AddWrite(ConstructInitialSettingsPacket(2, &header_stream_offset));
   AddWrite(ConstructClientAckPacket(3, 3, 1, 1));
   AddWrite(ConstructClientRstStreamPacket(4));