Don't call GetOrCreateGroup until we need a group

Bug: 830917
Change-Id: I52deae4c68054b0dcef62b18e7b263830b438162
Reviewed-on: https://chromium-review.googlesource.com/1010507
Commit-Queue: Brad Lassey <lassey@chromium.org>
Reviewed-by: Matt Menke <mmenke@chromium.org>
Cr-Commit-Position: refs/heads/master@{#562448}

net/BUILD.gn

@@ -5559,6 +5559,10 @@ test("net_unittests") {
     ]
   }
 
+  if (enable_built_in_dns) {
+    sources += [ "url_request/http_with_dns_over_https_unittest.cc" ]
+  }
+
   if (use_v8_in_net) {
     deps += [ ":net_with_v8" ]
   } else {

net/socket/client_socket_pool_base.cc

@@ -278,7 +278,6 @@ int ClientSocketPoolBaseHelper::RequestSocket(
   CleanupIdleSockets(false);
 
   request->net_log().BeginEvent(NetLogEventType::SOCKET_POOL);
-  Group* group = GetOrCreateGroup(group_name);
 
   int rv = RequestSocketInternal(group_name, *request);
   if (rv != ERR_IO_PENDING) {
@@ -290,6 +289,7 @@ int ClientSocketPoolBaseHelper::RequestSocket(
     CHECK(!request->handle()->is_initialized());
     request.reset();
   } else {
+    Group* group = GetOrCreateGroup(group_name);
     group->InsertPendingRequest(std::move(request));
     // Have to do this asynchronously, as closing sockets in higher level pools
     // call back in to |this|, which will cause all sorts of fun and exciting
@@ -362,30 +362,35 @@ int ClientSocketPoolBaseHelper::RequestSocketInternal(
     const Request& request) {
   ClientSocketHandle* const handle = request.handle();
   const bool preconnecting = !handle;
-  Group* group = GetOrCreateGroup(group_name);
 
-  if (!(request.flags() & NO_IDLE_SOCKETS)) {
-    // Try to reuse a socket.
-    if (AssignIdleSocketToRequest(request, group))
-      return OK;
-  }
+  Group* group = nullptr;
+  GroupMap::iterator group_it = group_map_.find(group_name);
+  if (group_it != group_map_.end()) {
+    group = group_it->second;
 
-  // If there are more ConnectJobs than pending requests, don't need to do
-  // anything.  Can just wait for the extra job to connect, and then assign it
-  // to the request.
-  if (!preconnecting && group->TryToUseUnassignedConnectJob())
-    return ERR_IO_PENDING;
+    if (!(request.flags() & NO_IDLE_SOCKETS)) {
+      // Try to reuse a socket.
+      if (AssignIdleSocketToRequest(request, group))
+        return OK;
+    }
 
-  // Can we make another active socket now?
-  if (!group->HasAvailableSocketSlot(max_sockets_per_group_) &&
-      request.respect_limits() == ClientSocketPool::RespectLimits::ENABLED) {
-    // TODO(willchan): Consider whether or not we need to close a socket in a
-    // higher layered group. I don't think this makes sense since we would just
-    // reuse that socket then if we needed one and wouldn't make it down to this
-    // layer.
-    request.net_log().AddEvent(
-        NetLogEventType::SOCKET_POOL_STALLED_MAX_SOCKETS_PER_GROUP);
-    return ERR_IO_PENDING;
+    // If there are more ConnectJobs than pending requests, don't need to do
+    // anything.  Can just wait for the extra job to connect, and then assign it
+    // to the request.
+    if (!preconnecting && group->TryToUseUnassignedConnectJob())
+      return ERR_IO_PENDING;
+
+    // Can we make another active socket now?
+    if (!group->HasAvailableSocketSlot(max_sockets_per_group_) &&
+        request.respect_limits() == ClientSocketPool::RespectLimits::ENABLED) {
+      // TODO(willchan): Consider whether or not we need to close a socket in a
+      // higher layered group. I don't think this makes sense since we would
+      // just reuse that socket then if we needed one and wouldn't make it down
+      // to this layer.
+      request.net_log().AddEvent(
+          NetLogEventType::SOCKET_POOL_STALLED_MAX_SOCKETS_PER_GROUP);
+      return ERR_IO_PENDING;
+    }
   }
 
   if (ReachedMaxSocketsLimit() &&
@@ -394,14 +399,16 @@ int ClientSocketPoolBaseHelper::RequestSocketInternal(
     // here.  Only reason for them now seems to be preconnects.
     if (idle_socket_count() > 0) {
       // There's an idle socket in this pool. Either that's because there's
-      // still one in this group, but we got here due to preconnecting bypassing
-      // idle sockets, or because there's an idle socket in another group.
+      // still one in this group, but we got here due to preconnecting
+      // bypassing idle sockets, or because there's an idle socket in another
+      // group.
       bool closed = CloseOneIdleSocketExceptInGroup(group);
       if (preconnecting && !closed)
         return ERR_PRECONNECT_MAX_SOCKET_LIMIT;
     } else {
-      // We could check if we really have a stalled group here, but it requires
-      // a scan of all groups, so just flip a flag here, and do the check later.
+      // We could check if we really have a stalled group here, but it
+      // requires a scan of all groups, so just flip a flag here, and do the
+      // check later.
       request.net_log().AddEvent(
           NetLogEventType::SOCKET_POOL_STALLED_MAX_SOCKETS);
       return ERR_IO_PENDING;
@@ -419,14 +426,15 @@ int ClientSocketPoolBaseHelper::RequestSocketInternal(
     if (!preconnecting) {
       HandOutSocket(connect_job->PassSocket(), ClientSocketHandle::UNUSED,
                     connect_job->connect_timing(), handle, base::TimeDelta(),
-                    group, request.net_log());
+                    GetOrCreateGroup(group_name), request.net_log());
     } else {
-      AddIdleSocket(connect_job->PassSocket(), group);
+      AddIdleSocket(connect_job->PassSocket(), GetOrCreateGroup(group_name));
     }
   } else if (rv == ERR_IO_PENDING) {
     // If we don't have any sockets in this group, set a timer for potentially
     // creating a new one.  If the SYN is lost, this backup socket may complete
     // before the slow socket, improving end user latency.
+    Group* group = GetOrCreateGroup(group_name);
     if (connect_backup_jobs_enabled_ && group->IsEmpty()) {
       group->StartBackupJobTimer(group_name, this);
     }
@@ -442,6 +450,7 @@ int ClientSocketPoolBaseHelper::RequestSocketInternal(
       connect_job->GetAdditionalErrorState(handle);
       error_socket = connect_job->PassSocket();
     }
+    Group* group = GetOrCreateGroup(group_name);
     if (error_socket) {
       HandOutSocket(std::move(error_socket), ClientSocketHandle::UNUSED,
                     connect_job->connect_timing(), handle, base::TimeDelta(),

net/url_request/http_with_dns_over_https_unittest.cc

+// Copyright 2018 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/dns/dns_client.h"
+#include "net/dns/dns_transaction.h"
+#include "net/dns/host_resolver_impl.h"
+#include "net/http/http_stream_factory_test_util.h"
+#include "net/log/net_log.h"
+#include "net/socket/transport_client_socket_pool.h"
+#include "net/test/embedded_test_server/embedded_test_server.h"
+#include "net/test/embedded_test_server/http_request.h"
+#include "net/test/embedded_test_server/http_response.h"
+#include "net/test/test_with_scoped_task_environment.h"
+#include "net/traffic_annotation/network_traffic_annotation_test_helper.h"
+#include "net/url_request/url_request.h"
+#include "net/url_request/url_request_context.h"
+#include "net/url_request/url_request_test_util.h"
+#include "testing/gtest/include/gtest/gtest.h"
+#include "testing/platform_test.h"
+
+namespace net {
+namespace {
+
+const size_t kHeaderSize = sizeof(dns_protocol::Header);
+const char kTestBody[] = "<html><body>TEST RESPONSE</body></html>";
+
+class TestHostResolverProc : public HostResolverProc {
+ public:
+  TestHostResolverProc() : HostResolverProc(nullptr) {}
+
+  int Resolve(const std::string& hostname,
+              AddressFamily address_family,
+              HostResolverFlags host_resolver_flags,
+              AddressList* addrlist,
+              int* os_error) override {
+    return ERR_NAME_NOT_RESOLVED;
+  }
+
+ private:
+  ~TestHostResolverProc() override {}
+};
+
+class HttpWithDnsOverHttpsTest : public TestWithScopedTaskEnvironment {
+ public:
+  HttpWithDnsOverHttpsTest()
+      : resolver_(HostResolver::Options(), nullptr),
+        request_context_(true),
+        doh_server_(EmbeddedTestServer::Type::TYPE_HTTPS),
+        test_server_(EmbeddedTestServer::Type::TYPE_HTTPS),
+        doh_queries_served_(0),
+        test_requests_served_(0) {
+    doh_server_.RegisterRequestHandler(
+        base::BindRepeating(&HttpWithDnsOverHttpsTest::HandleDefaultConnect,
+                            base::Unretained(this)));
+    test_server_.RegisterRequestHandler(
+        base::BindRepeating(&HttpWithDnsOverHttpsTest::HandleDefaultConnect,
+                            base::Unretained(this)));
+    EXPECT_TRUE(doh_server_.Start());
+    EXPECT_TRUE(test_server_.Start());
+    GURL url(doh_server_.GetURL("/dns_query"));
+    std::unique_ptr<DnsClient> dns_client(DnsClient::CreateClient(nullptr));
+    DnsConfig config;
+    config.nameservers.push_back(IPEndPoint());
+    config.dns_over_https_servers.emplace_back(url, true);
+    dns_client->SetConfig(config);
+    resolver_.SetRequestContext(&request_context_);
+    resolver_.set_proc_params_for_test(
+        HostResolverImpl::ProcTaskParams(new TestHostResolverProc(), 1));
+    resolver_.SetDnsClient(std::move(dns_client));
+    request_context_.set_host_resolver(&resolver_);
+    request_context_.Init();
+  }
+
+  URLRequestContext* context() { return &request_context_; }
+
+  std::unique_ptr<test_server::HttpResponse> HandleDefaultConnect(
+      const test_server::HttpRequest& request) {
+    if (request.relative_url.compare("/dns_query") == 0) {
+      doh_queries_served_++;
+      uint8_t id1 = request.content[0];
+      uint8_t id2 = request.content[1];
+      std::unique_ptr<test_server::BasicHttpResponse> http_response(
+          new test_server::BasicHttpResponse);
+      const uint8_t header_data[] = {
+          id1,  id2,   // - Same ID as before
+          0x81, 0x80,  // - Different flags, we'll look at this below
+          0x00, 0x01,  // - 1 question
+          0x00, 0x01,  // - 1 answer
+          0x00, 0x00,  // - No authority records
+          0x00, 0x00,  // - No additional records
+      };
+      std::string question = request.content.substr(kHeaderSize);
+
+      const uint8_t answer_data[]{0xC0, 0x0C,  // - NAME
+                                  0x00, 0x01,  // - TYPE
+                                  0x00, 0x01,  // - CLASS
+                                  0x00, 0x00,  //
+                                  0x18, 0x4C,  // - TTL
+                                  0x00, 0x04,  // - RDLENGTH = 4 bytes
+                                  0x7f, 0x00,  // - RDDATA, IP is 127.0.0.1
+                                  0x00, 0x01};
+      http_response->set_content(
+          std::string((char*)header_data, sizeof(header_data)) + question +
+          std::string((char*)answer_data, sizeof(answer_data)));
+      http_response->set_content_type("application/dns-udpwireformat");
+      return std::move(http_response);
+    } else {
+      test_requests_served_++;
+      std::unique_ptr<test_server::BasicHttpResponse> http_response(
+          new test_server::BasicHttpResponse);
+      http_response->set_content(kTestBody);
+      http_response->set_content_type("text/html");
+      return std::move(http_response);
+    }
+  }
+
+ protected:
+  HostResolverImpl resolver_;
+  TestURLRequestContext request_context_;
+  EmbeddedTestServer doh_server_;
+  EmbeddedTestServer test_server_;
+  uint32_t doh_queries_served_;
+  uint32_t test_requests_served_;
+};
+
+class TestHttpDelegate : public HttpStreamRequest::Delegate {
+ public:
+  TestHttpDelegate(base::RunLoop* loop) : loop_(loop) {}
+  ~TestHttpDelegate() override {}
+  void OnStreamReady(const SSLConfig& used_ssl_config,
+                     const ProxyInfo& used_proxy_info,
+                     std::unique_ptr<HttpStream> stream) override {
+    stream->Close(false);
+    loop_->Quit();
+  }
+
+  void OnWebSocketHandshakeStreamReady(
+      const SSLConfig& used_ssl_config,
+      const ProxyInfo& used_proxy_info,
+      std::unique_ptr<WebSocketHandshakeStreamBase> stream) override {}
+
+  void OnBidirectionalStreamImplReady(
+      const SSLConfig& used_ssl_config,
+      const ProxyInfo& used_proxy_info,
+      std::unique_ptr<BidirectionalStreamImpl> stream) override {}
+
+  void OnStreamFailed(int status,
+                      const NetErrorDetails& net_error_details,
+                      const SSLConfig& used_ssl_config) override {}
+
+  void OnCertificateError(int status,
+                          const SSLConfig& used_ssl_config,
+                          const SSLInfo& ssl_info) override {}
+
+  void OnNeedsProxyAuth(const HttpResponseInfo& proxy_response,
+                        const SSLConfig& used_ssl_config,
+                        const ProxyInfo& used_proxy_info,
+                        HttpAuthController* auth_controller) override {}
+
+  void OnNeedsClientAuth(const SSLConfig& used_ssl_config,
+                         SSLCertRequestInfo* cert_info) override {}
+
+  void OnHttpsProxyTunnelResponse(const HttpResponseInfo& response_info,
+                                  const SSLConfig& used_ssl_config,
+                                  const ProxyInfo& used_proxy_info,
+                                  std::unique_ptr<HttpStream> stream) override {
+  }
+
+  void OnQuicBroken() override {}
+
+ private:
+  base::RunLoop* loop_;
+};
+
+// This test sets up a request which will reenter the connection pools by
+// triggering a DNS over HTTPS request. It also sets up an idle socket
+// which was a precondition for the crash we saw in  https://crbug.com/830917.
+TEST_F(HttpWithDnsOverHttpsTest, EndToEnd) {
+  // Create and start http server.
+  EmbeddedTestServer http_server(EmbeddedTestServer::Type::TYPE_HTTP);
+  http_server.RegisterRequestHandler(base::BindRepeating(
+      &HttpWithDnsOverHttpsTest::HandleDefaultConnect, base::Unretained(this)));
+  EXPECT_TRUE(http_server.Start());
+
+  // Set up an idle socket.
+  HttpTransactionFactory* transaction_factory =
+      request_context_.http_transaction_factory();
+  HttpStreamFactory::JobFactory default_job_factory;
+  HttpNetworkSession* network_session = transaction_factory->GetSession();
+  base::RunLoop loop;
+  TestHttpDelegate request_delegate(&loop);
+
+  HttpStreamFactory* factory = network_session->http_stream_factory();
+  HttpRequestInfo request_info;
+  request_info.method = "GET";
+  request_info.url = http_server.GetURL("localhost", "/preconnect");
+
+  std::unique_ptr<HttpStreamRequest> request(factory->RequestStream(
+      request_info, DEFAULT_PRIORITY, SSLConfig(), SSLConfig(),
+      &request_delegate, false, false, NetLogWithSource()));
+  loop.Run();
+
+  std::string group_name(request_info.url.host() + ":" +
+                         request_info.url.port());
+  EXPECT_EQ(network_session
+                ->GetTransportSocketPool(HttpNetworkSession::NORMAL_SOCKET_POOL)
+                ->IdleSocketCountInGroup(group_name),
+            1);
+
+  // Make a request that will trigger a DoH query as well.
+  TestDelegate d;
+  d.set_allow_certificate_errors(true);
+  GURL main_url = test_server_.GetURL("bar.example.com", "/test");
+  std::unique_ptr<URLRequest> req(context()->CreateRequest(
+      main_url, DEFAULT_PRIORITY, &d, TRAFFIC_ANNOTATION_FOR_TESTS));
+  req->Start();
+  base::RunLoop().Run();
+  EXPECT_TRUE(test_server_.ShutdownAndWaitUntilComplete());
+  EXPECT_TRUE(http_server.ShutdownAndWaitUntilComplete());
+  EXPECT_TRUE(doh_server_.ShutdownAndWaitUntilComplete());
+  EXPECT_EQ(doh_queries_served_, 2u);
+  EXPECT_EQ(test_requests_served_, 1u);
+  EXPECT_TRUE(d.response_completed());
+  EXPECT_EQ(d.request_status(), 0);
+  EXPECT_EQ(d.data_received(), kTestBody);
+}
+
+}  // namespace
+}  // namespace net