Make alternate Jobs not use HTTP/1.1 sockets.

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 aba2828919941d86b684290b57785fc5e65d5e6f..49a3457abbfd21200d7a1b3e275b8d0d1c1f9527 100644
--- a/net/http/http_network_transaction_unittest.cc
+++ b/net/http/http_network_transaction_unittest.cc
@@ -11971,6 +11971,173 @@ TEST_P(AltSvcCertificateVerificationTest, NewConnectionInvalid) {
   Run(false, false);
 }
 
+// Alternative service requires HTTP/2 (or SPDY), but HTTP/1.1 is negotiated
+// with the alternative server.  That connection should not be used.
+TEST_P(HttpNetworkTransactionTest, AlternativeServiceNotOnHttp11) {
+  HostPortPair origin("origin.example.org", 443);
+  HostPortPair alternative("alternative.example.org", 443);
+
+  // Negotiate HTTP/1.1 with alternative.example.org.
+  SSLSocketDataProvider ssl(ASYNC, OK);
+  ssl.SetNextProto(kProtoHTTP11);
+  session_deps_.socket_factory->AddSSLSocketDataProvider(&ssl);
+
+  // No data should be read from the alternative, because HTTP/1.1 is
+  // negotiated.
+  StaticSocketDataProvider data;
+  session_deps_.socket_factory->AddSocketDataProvider(&data);
+
+  // Connection to the origin fails.
+  StaticSocketDataProvider data_refused;
+  data_refused.set_connect_data(MockConnect(ASYNC, ERR_CONNECTION_REFUSED));
+  session_deps_.socket_factory->AddSocketDataProvider(&data_refused);
+
+  // Set up alternative service for origin.
+  session_deps_.use_alternate_protocols = true;
+  scoped_refptr<HttpNetworkSession> session(CreateSession(&session_deps_));
+  base::WeakPtr<HttpServerProperties> http_server_properties =
+      session->http_server_properties();
+  AlternativeService alternative_service(
+      AlternateProtocolFromNextProto(GetParam()), alternative);
+  http_server_properties->SetAlternativeService(origin, alternative_service,
+                                                1.0);
+
+  scoped_ptr<HttpTransaction> trans(
+      new HttpNetworkTransaction(DEFAULT_PRIORITY, session.get()));
+  HttpRequestInfo request;
+  request.method = "GET";
+  request.url = GURL("https://origin.example.org:443");
+  request.load_flags = 0;
+  TestCompletionCallback callback;
+
+  int rv = trans->Start(&request, callback.callback(), BoundNetLog());
+  EXPECT_EQ(ERR_IO_PENDING, rv);
+  rv = callback.WaitForResult();

[davidben, 2015/05/01 23:21:46]

Since we don't actually care whether Start completes synchronously or no, I
would write:

EXPECT_EQ(ERR_NPN_NEGOTIATED_FAILED, callback.GetResult(trans->Start(...,
callback.callback(), ...));

Ditto elsewhere.

[Bence, 2015/05/04 12:05:07]

I'm not a big fan multiply nested function calls or very long lines.  How about
this instead?  Also note that because of the way git cl format breaks line, what
I propose is two fewer lines in total.

[davidben, 2015/05/04 16:11:51]

That'll silently break if things, for some reason, start completing
synchronously. I think the old version is preferable there since the EXPECT_EQ
(probably better an ASSERT_EQ) will fire. Perhaps:

  int rv = ....
  rv = callback.GetResult(rv);
  EXPECT_EQ(ERR_NPN_NEGOTIATION_FAILED, rv);

+  // HTTP/2 (or SPDY) is required for alternative service.
+  EXPECT_EQ(ERR_NPN_NEGOTIATION_FAILED, rv);
+}
+
+// Alternative service requires HTTP/2 (or SPDY), but there is already a
+// HTTP/1.1 socket open to the alternative server.  That socket should not be
+// used.
+TEST_P(HttpNetworkTransactionTest, AlternativeServiceShouldNotPoolToHttp11) {
+  HostPortPair origin("origin.example.org", 443);
+  HostPortPair alternative("alternative.example.org", 443);
+  std::string origin_url = "https://origin.example.org:443";
+  std::string alternative_url = "https://alternative.example.org:443";
+
+  // Negotiate HTTP/1.1 with alternative.example.org.
+  SSLSocketDataProvider ssl(ASYNC, OK);
+  ssl.SetNextProto(kProtoHTTP11);
+  session_deps_.socket_factory->AddSSLSocketDataProvider(&ssl);
+
+  MockWrite http_writes[] = {
+      MockWrite(
+          "GET / HTTP/1.1\r\n"
+          "Host: alternative.example.org\r\n"
+          "Connection: keep-alive\r\n\r\n"),
+      MockWrite(
+          "GET / HTTP/1.1\r\n"
+          "Host: alternative.example.org\r\n"
+          "Connection: keep-alive\r\n\r\n"),
+  };
+
+  MockRead http_reads[] = {
+      MockRead(
+          "HTTP/1.1 200 OK\r\n"
+          "Content-Type: text/html; charset=iso-8859-1\r\n"
+          "Content-Length: 40\r\n\r\n"
+          "first HTTP/1.1 response from alternative"),
+      MockRead(
+          "HTTP/1.1 200 OK\r\n"
+          "Content-Type: text/html; charset=iso-8859-1\r\n"
+          "Content-Length: 41\r\n\r\n"
+          "second HTTP/1.1 response from alternative"),
+  };
+  StaticSocketDataProvider http_data(http_reads, arraysize(http_reads),
+                                     http_writes, arraysize(http_writes));
+  session_deps_.socket_factory->AddSocketDataProvider(&http_data);
+
+  // Connection to the origin fails.
+  StaticSocketDataProvider data_refused;
+  data_refused.set_connect_data(MockConnect(ASYNC, ERR_CONNECTION_REFUSED));
+  session_deps_.socket_factory->AddSocketDataProvider(&data_refused);
+
+  // Set up alternative service for origin.
+  session_deps_.use_alternate_protocols = true;
+  scoped_refptr<HttpNetworkSession> session(CreateSession(&session_deps_));
+  base::WeakPtr<HttpServerProperties> http_server_properties =
+      session->http_server_properties();
+  AlternativeService alternative_service(
+      AlternateProtocolFromNextProto(GetParam()), alternative);
+  http_server_properties->SetAlternativeService(origin, alternative_service,
+                                                1.0);
+
+  // First transaction to alternative to open an HTTP/1.1 socket.
+  scoped_ptr<HttpTransaction> trans1(
+      new HttpNetworkTransaction(DEFAULT_PRIORITY, session.get()));
+  HttpRequestInfo request1;
+  request1.method = "GET";
+  request1.url = GURL(alternative_url);
+  request1.load_flags = 0;
+  TestCompletionCallback callback1;
+
+  int rv = trans1->Start(&request1, callback1.callback(), BoundNetLog());
+  EXPECT_EQ(ERR_IO_PENDING, rv);
+  rv = callback1.WaitForResult();
+  EXPECT_EQ(OK, rv);
+  const HttpResponseInfo* response1 = trans1->GetResponseInfo();
+  ASSERT_TRUE(response1 != nullptr);

[davidben, 2015/05/01 23:21:46]

Nit: I think this can just be ASSERT_TRUE(response1)

[Bence, 2015/05/04 12:05:07]

Done.

+  ASSERT_TRUE(response1->headers.get() != nullptr);

[davidben, 2015/05/01 23:21:46]

Nit: Ditto.

[Bence, 2015/05/04 12:05:06]

Done.

+  EXPECT_EQ("HTTP/1.1 200 OK", response1->headers->GetStatusLine());
+  EXPECT_TRUE(response1->was_npn_negotiated);
+  EXPECT_FALSE(response1->was_fetched_via_spdy);
+  std::string response_data1;
+  ASSERT_EQ(OK, ReadTransaction(trans1.get(), &response_data1));
+  EXPECT_EQ("first HTTP/1.1 response from alternative", response_data1);
+
+  // Request for origin.example.org, which has an alternative service.  This
+  // will start two Jobs: the alternative should find the HTTP/1.1, ignore it,
+  // but not open other connections to alternative.  The Job to origin fails,
+  // so this request fails.
+  scoped_ptr<HttpTransaction> trans2(
+      new HttpNetworkTransaction(DEFAULT_PRIORITY, session.get()));
+  HttpRequestInfo request2;
+  request2.method = "GET";
+  request2.url = GURL(origin_url);
+  request2.load_flags = 0;
+  TestCompletionCallback callback2;
+
+  rv = trans2->Start(&request2, callback2.callback(), BoundNetLog());
+  EXPECT_EQ(ERR_IO_PENDING, rv);
+  rv = callback2.WaitForResult();
+  EXPECT_EQ(ERR_CONNECTION_REFUSED, rv);
+
+  // Another transaction to alternative.  This is to test that the HTTP/1.1
+  // socket is still open and in the pool.

[davidben, 2015/05/01 23:21:46]

Huh, interesting. I guess this works because we don't explicitly Disconnect()
the socket and the default behavior must be to return it. So then the thing
about draining the socket pool we were talking about doesn't really happen.

[Bence, 2015/05/04 12:05:07]

Yup.  I think this is really essential for this test here, otherwise there is no
way to know if request2 didn't pool to the first socket just because that had
gotten disconnected already.

+  scoped_ptr<HttpTransaction> trans3(
+      new HttpNetworkTransaction(DEFAULT_PRIORITY, session.get()));
+  HttpRequestInfo request3;
+  request3.method = "GET";
+  request3.url = GURL(alternative_url);
+  request3.load_flags = 0;
+  TestCompletionCallback callback3;
+
+  rv = trans3->Start(&request3, callback3.callback(), BoundNetLog());
+  EXPECT_EQ(ERR_IO_PENDING, rv);
+  rv = callback3.WaitForResult();
+  EXPECT_EQ(OK, rv);
+  const HttpResponseInfo* response3 = trans3->GetResponseInfo();
+  ASSERT_TRUE(response3 != nullptr);
+  ASSERT_TRUE(response3->headers.get() != nullptr);

[davidben, 2015/05/01 23:21:46]

Ditto.

[Bence, 2015/05/04 12:05:07]

Done.

+  EXPECT_EQ("HTTP/1.1 200 OK", response3->headers->GetStatusLine());
+  EXPECT_TRUE(response3->was_npn_negotiated);
+  EXPECT_FALSE(response3->was_fetched_via_spdy);
+  std::string response_data3;
+  ASSERT_EQ(OK, ReadTransaction(trans3.get(), &response_data3));
+  EXPECT_EQ("second HTTP/1.1 response from alternative", response_data3);
+}
+
 TEST_P(HttpNetworkTransactionTest, DoNotUseSpdySessionForHttpOverTunnel) {
   const std::string https_url = "https://www.example.org:8080/";
   const std::string http_url = "http://www.example.org:8080/";