Consistently suggest ephemeral port for QUIC client

net/quic/quic_stream_factory.cc

 // Copyright (c) 2012 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/quic/quic_stream_factory.h"
 
 #include <set>
 
 #include "base/logging.h"
 #include "base/message_loop/message_loop.h"
 #include "base/message_loop/message_loop_proxy.h"
+#include "base/metrics/histogram.h"
 #include "base/rand_util.h"
 #include "base/stl_util.h"
 #include "base/strings/string_util.h"
 #include "base/values.h"
 #include "net/base/net_errors.h"
 #include "net/cert/cert_verifier.h"
 #include "net/dns/host_resolver.h"
 #include "net/dns/single_request_host_resolver.h"
 #include "net/http/http_server_properties.h"
 #include "net/quic/congestion_control/tcp_receiver.h"
 #include "net/quic/crypto/proof_verifier_chromium.h"
 #include "net/quic/crypto/quic_random.h"
+#include "net/quic/port_suggester.h"
 #include "net/quic/quic_client_session.h"
 #include "net/quic/quic_clock.h"
 #include "net/quic/quic_connection.h"
 #include "net/quic/quic_connection_helper.h"
 #include "net/quic/quic_crypto_client_stream_factory.h"
 #include "net/quic/quic_default_packet_writer.h"
 #include "net/quic/quic_http_stream.h"
 #include "net/quic/quic_protocol.h"
 #include "net/socket/client_socket_factory.h"
 
@@ skipping 215 matching lines @@
     QuicClock* clock,
     size_t max_packet_length)
     : require_confirmation_(true),
       host_resolver_(host_resolver),
       client_socket_factory_(client_socket_factory),
       http_server_properties_(http_server_properties),
       quic_crypto_client_stream_factory_(quic_crypto_client_stream_factory),
       random_generator_(random_generator),
       clock_(clock),
       max_packet_length_(max_packet_length),
-      weak_factory_(this) {
+      weak_factory_(this),
+      port_entropy_(random_generator_->RandUint64()) {
   config_.SetDefaults();
   config_.set_idle_connection_state_lifetime(
       QuicTime::Delta::FromSeconds(30),
       QuicTime::Delta::FromSeconds(30));
   cannoncial_suffixes_.push_back(string(".c.youtube.com"));
   cannoncial_suffixes_.push_back(string(".googlevideo.com"));
 }
 
 QuicStreamFactory::~QuicStreamFactory() {
   CloseAllSessions(ERR_ABORTED);
@@ skipping 170 matching lines @@
 }
 
 QuicClientSession* QuicStreamFactory::CreateSession(
     const HostPortProxyPair& host_port_proxy_pair,
     bool is_https,
     CertVerifier* cert_verifier,
     const AddressList& address_list,
     const BoundNetLog& net_log) {
   QuicGuid guid = random_generator_->RandUint64();
   IPEndPoint addr = *address_list.begin();
+  scoped_refptr<PortSuggester> port_suggester =
+      new PortSuggester(host_port_proxy_pair.first, port_entropy_);
   scoped_ptr<DatagramClientSocket> socket(
       client_socket_factory_->CreateDatagramClientSocket(
-          DatagramSocket::DEFAULT_BIND, base::Bind(&base::RandInt),
+          DatagramSocket::RANDOM_BIND,
+          base::Bind(&PortSuggester::SuggestPort, port_suggester),
           net_log.net_log(), net_log.source()));
   socket->Connect(addr);
 
+  DCHECK(port_suggester->call_count() >= 1);
+  IPEndPoint address;  // For debugging only.
+  // This DCHECK will try to validate that our port_suggester was used, and not
+  // just consulted.  Non-Mock Chromium tries using the port_suggester up to 10
+  // times before giving up, and accepting an OS selected port, in which case we
+  // *can't* validate the source port.
+  // Since the Mock tests don't really use the suggestions, we'll allow Mocks to
+  // signal this non-use by requesting 3 or more suggestions, and then we won't
+  // validate the source port.
+  // Bottom line: More than 2 suggestions, and we won't validate the port.
+  // Common use cases result in only one suggestion being requested, and it is
+  // immediately used as the source port, so this DCHECK is very meaningful.
+  DCHECK(ERR_SOCKET_NOT_CONNECTED == socket->GetLocalAddress(&address) ||
+         port_suggester->call_count() > 2 ||

[jar (doing other things), 2013/12/08 21:12:58]

The polarity of the first clause (line 474) was wrong, which mostly neutered the
DCHECK.

When I corrected the polarity, then some tests in quic_factory_stream_test.cc do
indeed fail... <yeah!>

...and the proposed technique of calling the PRNG several times in the Mock
socket, allows this DCHECK to be fine.  

[Ryan Hamilton, 2013/12/08 22:54:15]

Yay for tests :>

+         address.port() == port_suggester->previous_suggestion());
+  UMA_HISTOGRAM_COUNTS("Net.QuicEphemeralPortsSuggested",
+                       port_suggester->call_count());
+
   // We should adaptively set this buffer size, but for now, we'll use a size
   // that is more than large enough for a full receive window, and yet
   // does not consume "too much" memory.  If we see bursty packet loss, we may
   // revisit this setting and test for its impact.
   const int32 kSocketBufferSize(TcpReceiver::kReceiveWindowTCP);
   socket->SetReceiveBufferSize(kSocketBufferSize);
   // Set a buffer large enough to contain the initial CWND's worth of packet
   // to work around the problem with CHLO packets being sent out with the
   // wrong encryption level, when the send buffer is full.
   socket->SetSendBufferSize(kMaxPacketSize * 20); // Support 20 packets.
@@ skipping 94 matching lines @@
   // Copy the CachedState for the canonical server from canonical_crypto_config
   // as the initial CachedState for the server_hostname in crypto_config.
   crypto_config->InitializeFrom(server_hostname,
                                 canonical_host_port_proxy_pair.first.host(),
                                 canonical_crypto_config);
   // Update canonical version to point at the "most recent" crypto_config.
   canonical_hostname_to_origin_map_[canonical_host_port] = host_port_proxy_pair;
 }
 
 }  // namespace net