Retry upon 421 status code without IP pooling.

net/http/http_network_transaction_unittest.cc

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "net/http/http_network_transaction.h"
 
 #include <math.h>  // ceil
 #include <stdarg.h>
 #include <stdint.h>
 
@@ skipping 13576 matching lines @@
   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);
 }
 
+// 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)
       : host_port_(host_port) {}
   ~OneTimeCachingHostResolver() override {}
 
   RuleBasedHostResolverProc* rules() { return host_resolver_.rules(); }
 
   // HostResolver methods:
   int Resolve(const RequestInfo& info,
@@ skipping 1339 matching lines @@
   // RequestStream() (which may be NULL if it was destroyed already).
   base::WeakPtr<FakeStreamRequest> last_stream_request() {
     return last_stream_request_;
   }
 
   HttpStreamRequest* RequestStream(const HttpRequestInfo& info,
                                    RequestPriority priority,
                                    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();
     return fake_request;
   }
 
   HttpStreamRequest* RequestBidirectionalStreamImpl(
       const HttpRequestInfo& info,
       RequestPriority priority,
       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;
   }
 
   HttpStreamRequest* RequestWebSocketHandshakeStream(
       const HttpRequestInfo& info,
       RequestPriority priority,
       const SSLConfig& server_ssl_config,
       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);
     last_stream_request_ = fake_request->AsWeakPtr();
     return fake_request;
   }
 
   void PreconnectStreams(int num_streams,
                          const HttpRequestInfo& info) override {
     ADD_FAILURE();
@@ skipping 1529 matching lines @@
   base::RunLoop().RunUntilIdle();
 
   EXPECT_TRUE(trans.GetResponseInfo()->was_fetched_via_spdy);
   HttpRequestHeaders headers;
   ASSERT_TRUE(trans.GetFullRequestHeaders(&headers));
   EXPECT_TRUE(headers.HasHeader(HttpRequestHeaders::kTokenBinding));
 }
 #endif  // !defined(OS_IOS)
 
 }  // namespace net