Make alternate Jobs not use HTTP/1.1 sockets.

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 <string>
 #include <vector>
@@ skipping 11953 matching lines @@
 // The alternative service host must exhibit a certificate that is valid for the
 // origin host.  Test that this is enforced when opening a new connection.
 TEST_P(AltSvcCertificateVerificationTest, NewConnectionValid) {
   Run(false, true);
 }
 
 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;
+
+  // HTTP/2 (or SPDY) is required for alternative service.
+  int rv = trans->Start(&request, callback.callback(), BoundNetLog());
+  EXPECT_EQ(ERR_NPN_NEGOTIATION_FAILED, callback.GetResult(rv));

[Ryan Hamilton, 2015/05/05 20:51:12]

It's not obvious to me that this should be user visible. I guess there's no
problem?, but it seems odd.

[Bence, 2015/05/06 13:21:55]

This test passes without landing the rest of this CL.  For an alternate job,
|want_spdy_over_npn| is set, it is plumbed through to the socket logic, and that
fails after ALPN or NPN negotiation if not HTTP/2 (or SPDY) is negotiated.

If it makes things clearer, I can land this test in a separate CL.  If you
prefer to change the behavior, that should also go in that separate CL.  Let me
know.

[Ryan Hamilton, 2015/05/06 21:32:53]

Landing this now is totally fine. I guess this all makes sense. I was just a bit
surprised to see an alternative-job failure being user visible. But I suppose
that only happens when the main job fails and it fails before the alt job?

[Bence, 2015/05/08 14:46:13]

Yes, that's my understanding.

+}
+
+// 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));

[Ryan Hamilton, 2015/05/05 20:51:12]

I don't understand why this test sets connection refused. Can you elaborate?

[Bence, 2015/05/06 13:21:55]

|request2| is to |origin|, which has an alternative.  I have an HTTP/1.1
connection warmed up to the alternative, and I want to test that |request2| does
not pool to that.  In order to do this, I fail the connection to origin.  If
|request2| pooled to the HTTP/1.1 connection, it would still go through.  I'm
checking that it does not.

Since the alternative job is expected to fail (because it should not pool to an
HTTP/1.1 connection), I cannot leave the connection to the origin hanging,
otherwise the test would just time out.

[Ryan Hamilton, 2015/05/06 21:32:53]

Mind adding a comment to this (and the previous) test which explains this. What
you say makes perfect sense. 

Should we also have tests in which the main request succeeds and which the alt
job fails? (In which we make sure that we only attempt 1 alt job even if we make
several requests, and that they all go to the origin)

[Bence, 2015/05/08 14:46:13]

Done.
I don't see why we would need them.  In this CL, I am adding tests to enforce
that an HTTP/2 alternative service does not go through HTTP/1.1.  But the Job to
the origin can use HTTP/1.1, so what exactly would we test?  Also, why would we
attempt only one alternate Job?  Because the alternative service is marked as
broken?  There is currently no test for that, I can add one, but that should be
a separate CL.

[Ryan Hamilton, 2015/05/08 15:15:04]

This would be testing that in the "normal" case, the Alt-Svc job failure is not
user visible. The current set of tests only shows Alt-Svc failures that are user
visible. These new tests are great! But some additional tests would give broader
coverage of the expected behaviors.
Separate CL is totally fine.

+  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(OK, callback1.GetResult(rv));
+  const HttpResponseInfo* response1 = trans1->GetResponseInfo();
+  ASSERT_TRUE(response1);
+  ASSERT_TRUE(response1->headers.get());
+  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_CONNECTION_REFUSED, callback2.GetResult(rv));
+
+  // Another transaction to alternative.  This is to test that the HTTP/1.1
+  // socket is still open and in the pool.
+  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(OK, callback3.GetResult(rv));
+  const HttpResponseInfo* response3 = trans3->GetResponseInfo();
+  ASSERT_TRUE(response3);
+  ASSERT_TRUE(response3->headers.get());
+  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/";
 
   // SPDY GET for HTTPS URL (through CONNECT tunnel)
   const HostPortPair host_port_pair("www.example.org", 8080);
   scoped_ptr<SpdyFrame> connect(
       spdy_util_.ConstructSpdyConnect(NULL, 0, 1, LOWEST, host_port_pair));
   scoped_ptr<SpdyFrame> req1(
       spdy_util_.ConstructSpdyGet(https_url.c_str(), false, 1, LOWEST));
@@ skipping 2022 matching lines @@
   ASSERT_TRUE(response);
   ASSERT_TRUE(response->headers.get());
 
   EXPECT_EQ(101, response->headers->response_code());
 
   trans.reset();
   session->CloseAllConnections();
 }
 
 }  // namespace net