Move the logic for delaying 0-RTT QUIC POST from the QuicStreamFactory

net/quic/bidirectional_stream_quic_impl_unittest.cc

Index: net/quic/bidirectional_stream_quic_impl_unittest.cc
diff --git a/net/quic/bidirectional_stream_quic_impl_unittest.cc b/net/quic/bidirectional_stream_quic_impl_unittest.cc
index 22dba77f7459d9490ff900fdbf3f04e9b20ade90..e3c4d399bd8d4e54ede2b0872209d707dbf256f9 100644
--- a/net/quic/bidirectional_stream_quic_impl_unittest.cc
+++ b/net/quic/bidirectional_stream_quic_impl_unittest.cc
@@ -68,17 +68,20 @@ class TestDelegateBase : public BidirectionalStreamImpl::Delegate {
         on_data_sent_count_(0),
         not_expect_callback_(false),
         on_failed_called_(false),
-        send_request_headers_automatically_(true) {
+        send_request_headers_automatically_(true),
+        is_ready_(false) {
     loop_.reset(new base::RunLoop);
   }
 
   ~TestDelegateBase() override {}
 
   void OnStreamReady(bool request_headers_sent) override {
+    CHECK(!is_ready_);
     CHECK(!on_failed_called_);
     EXPECT_EQ(send_request_headers_automatically_, request_headers_sent);
     CHECK(!not_expect_callback_);
     loop_->Quit();
+    is_ready_ = true;

[xunjieli, 2016/06/17 18:05:42]

nit: maybe set |is_ready_| before exiting the |loop_|? 

is_ready_ = true;
loop_->Quit();

[Ryan Hamilton, 2016/06/17 18:20:35]

Done.

   }
 
   void OnHeadersReceived(const SpdyHeaderBlock& response_headers) override {
@@ -194,6 +197,7 @@ class TestDelegateBase : public BidirectionalStreamImpl::Delegate {
   int on_data_read_count() const { return on_data_read_count_; }
   int on_data_sent_count() const { return on_data_sent_count_; }
   bool on_failed_called() const { return on_failed_called_; }
+  bool is_ready() const { return is_ready_; }
 
  protected:
   // Quits |loop_|.
@@ -220,6 +224,7 @@ class TestDelegateBase : public BidirectionalStreamImpl::Delegate {
   bool on_failed_called_;
   CompletionCallback callback_;
   bool send_request_headers_automatically_;
+  bool is_ready_;
 
   DISALLOW_COPY_AND_ASSIGN(TestDelegateBase);
 };
@@ -342,6 +347,8 @@ class BidirectionalStreamQuicImplTest
 
   // Configures the test fixture to use the list of expected writes.
   void Initialize() {
+    crypto_client_stream_factory_.set_handshake_mode(
+        MockCryptoClientStream::ZERO_RTT);
     mock_writes_.reset(new MockWrite[writes_.size()]);
     for (size_t i = 0; i < writes_.size(); i++) {
       if (writes_[i].packet == nullptr) {
@@ -382,7 +389,12 @@ class BidirectionalStreamQuicImplTest
         /*socket_performance_watcher=*/nullptr, net_log().bound().net_log()));
     session_->Initialize();
     session_->GetCryptoStream()->CryptoConnect();
-    EXPECT_TRUE(session_->IsCryptoHandshakeConfirmed());
+    EXPECT_TRUE(session_->IsEncryptionEstablished());
+  }
+
+  void ConfirmHandshake() {
+    crypto_client_stream_factory_.last_stream()->SendOnCryptoHandshakeEvent(
+        QuicSession::HANDSHAKE_CONFIRMED);
   }
 
   void SetRequest(const std::string& method,
@@ -716,6 +728,8 @@ TEST_P(BidirectionalStreamQuicImplTest, CoalesceDataBuffersNotHeadersFrame) {
       new TestDelegateBase(read_buffer.get(), kReadBufferSize));
   delegate->DoNotSendRequestHeadersAutomatically();
   delegate->Start(&request, net_log().bound(), session()->GetWeakPtr());

[xunjieli, 2016/06/17 18:05:42]

Does QuicStreamRequest always finish synchronously? If it does, maybe worth
adding a comment here. If it doesn't, maybe add a 
base::RunLoop().RunUntilIdle() to flush posted tasks from the message loop?

[Ryan Hamilton, 2016/06/17 18:20:35]

No, it's not guaranteed to complete synchronously. For example, if there are max
streams open, it will not complete until the next stream closes. Or if handshake
confirmation is required, it won't return until the SHLO is received. I'm not
sure that adding RunUntilIdle() is a good idea though.

+  EXPECT_FALSE(delegate->is_ready());
+  ConfirmHandshake();
   delegate->WaitUntilNextCallback();  // OnStreamReady
 
   // Sends request headers separately, which causes them to be sent in a
@@ -819,6 +833,7 @@ TEST_P(BidirectionalStreamQuicImplTest,
       new TestDelegateBase(read_buffer.get(), kReadBufferSize));
   delegate->DoNotSendRequestHeadersAutomatically();
   delegate->Start(&request, net_log().bound(), session()->GetWeakPtr());
+  ConfirmHandshake();
   delegate->WaitUntilNextCallback();  // OnStreamReady
 
   // Send a Data packet.
@@ -916,6 +931,7 @@ TEST_P(BidirectionalStreamQuicImplTest,
       new TestDelegateBase(read_buffer.get(), kReadBufferSize));
   delegate->DoNotSendRequestHeadersAutomatically();
   delegate->Start(&request, net_log().bound(), session()->GetWeakPtr());
+  ConfirmHandshake();
   delegate->WaitUntilNextCallback();  // OnStreamReady
 
   // Send a Data packet.
@@ -1006,6 +1022,7 @@ TEST_P(BidirectionalStreamQuicImplTest, PostRequest) {
   std::unique_ptr<TestDelegateBase> delegate(
       new TestDelegateBase(read_buffer.get(), kReadBufferSize));
   delegate->Start(&request, net_log().bound(), session()->GetWeakPtr());
+  ConfirmHandshake();
   delegate->WaitUntilNextCallback();  // OnStreamReady
 
   // Send a DATA frame.
@@ -1084,6 +1101,7 @@ TEST_P(BidirectionalStreamQuicImplTest, InterleaveReadDataAndSendData) {
   std::unique_ptr<TestDelegateBase> delegate(
       new TestDelegateBase(read_buffer.get(), kReadBufferSize));
   delegate->Start(&request, net_log().bound(), session()->GetWeakPtr());
+  ConfirmHandshake();
   delegate->WaitUntilNextCallback();  // OnStreamReady
 
   // Server acks the request.
@@ -1163,6 +1181,7 @@ TEST_P(BidirectionalStreamQuicImplTest, ServerSendsRstAfterHeaders) {
       new TestDelegateBase(read_buffer.get(), kReadBufferSize));
   delegate->Start(&request, net_log().bound(), session()->GetWeakPtr());
   delegate->WaitUntilNextCallback();  // OnStreamReady
+  ConfirmHandshake();
 
   // Server sends a Rst.
   ProcessPacket(ConstructServerRstStreamPacket(1));
@@ -1202,6 +1221,7 @@ TEST_P(BidirectionalStreamQuicImplTest, ServerSendsRstAfterReadData) {
       new TestDelegateBase(read_buffer.get(), kReadBufferSize));
   delegate->Start(&request, net_log().bound(), session()->GetWeakPtr());
   delegate->WaitUntilNextCallback();  // OnStreamReady
+  ConfirmHandshake();
 
   // Server acks the request.
   ProcessPacket(ConstructServerAckPacket(1, 0, 0));
@@ -1259,6 +1279,7 @@ TEST_P(BidirectionalStreamQuicImplTest, CancelStreamAfterSendData) {
   std::unique_ptr<TestDelegateBase> delegate(
       new TestDelegateBase(read_buffer.get(), kReadBufferSize));
   delegate->Start(&request, net_log().bound(), session()->GetWeakPtr());
+  ConfirmHandshake();
   delegate->WaitUntilNextCallback();  // OnStreamReady
 
   // Server acks the request.
@@ -1314,6 +1335,7 @@ TEST_P(BidirectionalStreamQuicImplTest, SessionClosedBeforeReadData) {
   std::unique_ptr<TestDelegateBase> delegate(
       new TestDelegateBase(read_buffer.get(), kReadBufferSize));
   delegate->Start(&request, net_log().bound(), session()->GetWeakPtr());
+  ConfirmHandshake();
   delegate->WaitUntilNextCallback();  // OnStreamReady
 
   // Server acks the request.
@@ -1372,6 +1394,7 @@ TEST_P(BidirectionalStreamQuicImplTest, CancelStreamAfterReadData) {
   std::unique_ptr<TestDelegateBase> delegate(
       new TestDelegateBase(read_buffer.get(), kReadBufferSize));
   delegate->Start(&request, net_log().bound(), session()->GetWeakPtr());
+  ConfirmHandshake();
   delegate->WaitUntilNextCallback();  // OnStreamReady
 
   // Server acks the request.
@@ -1422,6 +1445,7 @@ TEST_P(BidirectionalStreamQuicImplTest, DeleteStreamDuringOnHeadersReceived) {
       new DeleteStreamDelegate(read_buffer.get(), kReadBufferSize,
                                DeleteStreamDelegate::ON_HEADERS_RECEIVED));
   delegate->Start(&request, net_log().bound(), session()->GetWeakPtr());
+  ConfirmHandshake();
   delegate->WaitUntilNextCallback();  // OnStreamReady
 
   // Server acks the request.
@@ -1464,6 +1488,7 @@ TEST_P(BidirectionalStreamQuicImplTest, DeleteStreamDuringOnDataRead) {
   std::unique_ptr<DeleteStreamDelegate> delegate(new DeleteStreamDelegate(
       read_buffer.get(), kReadBufferSize, DeleteStreamDelegate::ON_DATA_READ));
   delegate->Start(&request, net_log().bound(), session()->GetWeakPtr());
+  ConfirmHandshake();
   delegate->WaitUntilNextCallback();  // OnStreamReady
 
   // Server acks the request.
@@ -1517,6 +1542,7 @@ TEST_P(BidirectionalStreamQuicImplTest, DeleteStreamDuringOnTrailersReceived) {
                                DeleteStreamDelegate::ON_TRAILERS_RECEIVED));
   delegate->Start(&request, net_log().bound(), session()->GetWeakPtr());
   delegate->WaitUntilNextCallback();  // OnStreamReady
+  ConfirmHandshake();

[xunjieli, 2016/06/17 18:05:43]

You have examples here to demonstrate that GET doesn't require handshake
confirmation before sending the headers. 
Could you make a new unit test where we exercise PUT? You can duplicate the code
from BidirectionalStreamQuicImplTest.PostRequest, and change the method to PUT.
We won't gain much from having that test, but it serves as an example that PUT
also does not require handshake confirmation in 0RTT.

[Ryan Hamilton, 2016/06/17 18:20:35]

Done.

 
   // Server acks the request.
   ProcessPacket(ConstructServerAckPacket(1, 0, 0));