Retry upon 421 status code without IP pooling.

net/http/http_network_transaction_unittest.cc

Index: net/http/http_network_transaction_unittest.cc
diff --git a/net/http/http_network_transaction_unittest.cc b/net/http/http_network_transaction_unittest.cc
index 178ad60529e02259c68b37733fec80e05b252d82..cb248742739c5325bf472b06dbd556e3ab995e71 100644
--- a/net/http/http_network_transaction_unittest.cc
+++ b/net/http/http_network_transaction_unittest.cc
@@ -13594,6 +13594,128 @@ TEST_F(HttpNetworkTransactionTest, UseIPConnectionPoolingAfterResolution) {
   EXPECT_EQ("hello!", response_data);
 }
 
+// Regression test for https://crbug.com/546991.
+// The server might not be able to serve an IP pooled request, and might send a
+// 421 Misdirected Request response status to indicate this.
+// HttpNetworkTransaction should reset the request and retry without IP pooling.
+TEST_F(HttpNetworkTransactionTest, RetryWithoutConnectionPooling) {
+  // Two hosts resolve to the same IP address.
+  const std::string ip_addr = "1.2.3.4";
+  IPAddress ip;
+  ASSERT_TRUE(ip.AssignFromIPLiteral(ip_addr));
+  IPEndPoint peer_addr = IPEndPoint(ip, 443);
+
+  session_deps_.host_resolver.reset(new MockCachingHostResolver());
+  session_deps_.host_resolver->rules()->AddRule("www.example.org", ip_addr);
+  session_deps_.host_resolver->rules()->AddRule("mail.example.com", ip_addr);

[Zhongyi Shi, 2017/03/29 03:05:06]

optional nit: s/mail.example.com/mail.example.org, totally optional. 
I think we might need a clean-up in net unittests.

[Bence, 2017/03/29 16:46:08]

Done.

BTW RFC2606 Section 3 allows both example.com and example.org, and
spdy_pooling.pem covers www.example.org, mail.example.org, and mail.example.com,
so it does not matter much, but if you want to clean up net unittests, I'm happy
to approve that change.

[Zhongyi Shi, 2017/03/30 22:33:02]

Acknowledged.

+
+  std::unique_ptr<HttpNetworkSession> session(CreateSession(&session_deps_));
+
+  // Two requests on the first connection.
+  SpdySerializedFrame req1(
+      spdy_util_.ConstructSpdyGet("https://www.example.org", 1, LOWEST));
+  spdy_util_.UpdateWithStreamDestruction(1);
+  SpdySerializedFrame req2(
+      spdy_util_.ConstructSpdyGet("https://mail.example.com", 3, LOWEST));
+  SpdySerializedFrame rst(
+      spdy_util_.ConstructSpdyRstStream(3, ERROR_CODE_CANCEL));
+  MockWrite writes1[] = {
+      CreateMockWrite(req1, 0), CreateMockWrite(req2, 3),
+      CreateMockWrite(rst, 6),
+  };
+
+  // The first one succeeds, the second gets error 421 Misdirected Request.
+  SpdySerializedFrame resp1(spdy_util_.ConstructSpdyGetReply(nullptr, 0, 1));
+  SpdySerializedFrame body1(spdy_util_.ConstructSpdyDataFrame(1, true));
+  SpdyHeaderBlock response_headers;
+  response_headers[SpdyTestUtil::GetStatusKey()] = "421";
+  SpdySerializedFrame resp2(
+      spdy_util_.ConstructSpdyReply(3, std::move(response_headers)));
+  MockRead reads1[] = {CreateMockRead(resp1, 1), CreateMockRead(body1, 2),
+                       CreateMockRead(resp2, 4), MockRead(ASYNC, 0, 5)};
+
+  MockConnect connect1(ASYNC, OK, peer_addr);
+  SequencedSocketData data1(connect1, reads1, arraysize(reads1), writes1,
+                            arraysize(writes1));
+  session_deps_.socket_factory->AddSocketDataProvider(&data1);
+
+  AddSSLSocketData();
+
+  // Retry the second request on a second connection.
+  SpdyTestUtil spdy_util2;
+  SpdySerializedFrame req3(
+      spdy_util2.ConstructSpdyGet("https://mail.example.com", 1, LOWEST));
+  MockWrite writes2[] = {
+      CreateMockWrite(req3, 0),
+  };
+
+  SpdySerializedFrame resp3(spdy_util2.ConstructSpdyGetReply(nullptr, 0, 1));
+  SpdySerializedFrame body3(spdy_util2.ConstructSpdyDataFrame(1, true));
+  MockRead reads2[] = {CreateMockRead(resp3, 1), CreateMockRead(body3, 2),
+                       MockRead(ASYNC, 0, 3)};
+
+  MockConnect connect2(ASYNC, OK, peer_addr);
+  SequencedSocketData data2(connect2, reads2, arraysize(reads2), writes2,
+                            arraysize(writes2));
+  session_deps_.socket_factory->AddSocketDataProvider(&data2);
+
+  AddSSLSocketData();
+
+  TestCompletionCallback callback;
+  HttpRequestInfo request1;
+  request1.method = "GET";
+  request1.url = GURL("https://www.example.org/");
+  request1.load_flags = 0;
+  HttpNetworkTransaction trans1(DEFAULT_PRIORITY, session.get());
+
+  int rv = trans1.Start(&request1, callback.callback(), NetLogWithSource());
+  EXPECT_THAT(rv, IsError(ERR_IO_PENDING));
+  rv = callback.WaitForResult();
+  EXPECT_THAT(rv, IsOk());
+
+  const HttpResponseInfo* response = trans1.GetResponseInfo();
+  ASSERT_TRUE(response);
+  ASSERT_TRUE(response->headers);
+  EXPECT_EQ("HTTP/1.1 200", response->headers->GetStatusLine());
+  EXPECT_TRUE(response->was_fetched_via_spdy);
+  EXPECT_TRUE(response->was_alpn_negotiated);
+  std::string response_data;
+  ASSERT_THAT(ReadTransaction(&trans1, &response_data), IsOk());
+  EXPECT_EQ("hello!", response_data);
+
+  // Preload mail.example.com into HostCache.

[Zhongyi Shi, 2017/03/29 03:05:06]

nit: This seemed to be part of the test setting up, which doesn't have to happen
after the first request complete? If so, it might be cleaner to move this
upfront before we start the first transaction. 

[Bence, 2017/03/29 16:46:08]

Done.

+  HostPortPair host_port("mail.example.com", 443);
+  HostResolver::RequestInfo resolve_info(host_port);
+  AddressList ignored;
+  std::unique_ptr<HostResolver::Request> request;
+  rv = session_deps_.host_resolver->Resolve(resolve_info, DEFAULT_PRIORITY,
+                                            &ignored, callback.callback(),
+                                            &request, NetLogWithSource());
+  EXPECT_THAT(rv, IsError(ERR_IO_PENDING));
+  rv = callback.WaitForResult();
+  EXPECT_THAT(rv, IsOk());
+
+  HttpRequestInfo request2;
+  request2.method = "GET";
+  request2.url = GURL("https://mail.example.com/");
+  request2.load_flags = 0;
+  HttpNetworkTransaction trans2(DEFAULT_PRIORITY, session.get());
+
+  rv = trans2.Start(&request2, callback.callback(), NetLogWithSource());

[Zhongyi Shi, 2017/03/29 03:05:06]

Seems like we are only verifying the second request succeeds in this test. It
might be better to replace the NetLogWithSource() by TestNetLog so that you
could verify that MISDIRECTED_RESPONSE is processed.

[Bence, 2017/03/29 16:46:08]

Excellent idea, thank you!  Done.

+  EXPECT_THAT(rv, IsError(ERR_IO_PENDING));
+  rv = callback.WaitForResult();
+  EXPECT_THAT(rv, IsOk());
+
+  response = trans2.GetResponseInfo();
+  ASSERT_TRUE(response);
+  ASSERT_TRUE(response->headers);
+  EXPECT_EQ("HTTP/1.1 200", response->headers->GetStatusLine());
+  EXPECT_TRUE(response->was_fetched_via_spdy);
+  EXPECT_TRUE(response->was_alpn_negotiated);
+  ASSERT_THAT(ReadTransaction(&trans2, &response_data), IsOk());
+  EXPECT_EQ("hello!", response_data);
+}
+
 class OneTimeCachingHostResolver : public HostResolver {
  public:
   explicit OneTimeCachingHostResolver(const HostPortPair& host_port)
@@ -14953,6 +15075,7 @@ class FakeStreamFactory : public HttpStreamFactory {
                                    const SSLConfig& server_ssl_config,
                                    const SSLConfig& proxy_ssl_config,
                                    HttpStreamRequest::Delegate* delegate,
+                                   bool enable_ip_based_pooling,
                                    const NetLogWithSource& net_log) override {
     FakeStreamRequest* fake_request = new FakeStreamRequest(priority, delegate);
     last_stream_request_ = fake_request->AsWeakPtr();
@@ -14965,6 +15088,7 @@ class FakeStreamFactory : public HttpStreamFactory {
       const SSLConfig& server_ssl_config,
       const SSLConfig& proxy_ssl_config,
       HttpStreamRequest::Delegate* delegate,
+      bool enable_ip_based_pooling,
       const NetLogWithSource& net_log) override {
     NOTREACHED();
     return nullptr;
@@ -14977,6 +15101,7 @@ class FakeStreamFactory : public HttpStreamFactory {
       const SSLConfig& proxy_ssl_config,
       HttpStreamRequest::Delegate* delegate,
       WebSocketHandshakeStreamBase::CreateHelper* create_helper,
+      bool enable_ip_based_pooling,
       const NetLogWithSource& net_log) override {
     FakeStreamRequest* fake_request =
         new FakeStreamRequest(priority, delegate, create_helper);