Consistently suggest ephemeral port for QUIC client

net/quic/quic_stream_factory.cc

Index: net/quic/quic_stream_factory.cc
diff --git a/net/quic/quic_stream_factory.cc b/net/quic/quic_stream_factory.cc
index 40f294821850e0a6fbffa5c9dc5a1d0fbb58d13c..839b01be82edd0e16cc2ccf696f7668effe61b8c 100644
--- a/net/quic/quic_stream_factory.cc
+++ b/net/quic/quic_stream_factory.cc
@@ -9,6 +9,7 @@
 #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"
@@ -21,6 +22,7 @@
 #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"
@@ -256,7 +258,8 @@ QuicStreamFactory::QuicStreamFactory(
       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),
@@ -447,12 +450,21 @@ QuicClientSession* QuicStreamFactory::CreateSession(
     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);
 
+  IPEndPoint address;
+  DCHECK(ERR_SOCKET_NOT_CONNECTED != socket->GetLocalAddress(&address) ||
+         address.port() == port_suggester->previous_suggestion());

[Ryan Hamilton, 2013/12/07 23:21:55]

I don't think that this DCHECK is guaranteed to succeed, is it?  If we get
*really* unlikely the kBindRetries ports suggested by the suggester may all be
taken.  As I understand it, at that point we do a default bind (with the
suggestion of 0).

  for (int i = 0; i < kBindRetries; ++i) {
    int rv = DoBind(IPEndPoint(ip, rand_int_cb_.Run(kPortStart, kPortEnd)));
    if (rv == OK || rv != ERR_ADDRESS_IN_USE)
      return rv;
  }
  return DoBind(IPEndPoint(ip, 0));

Given this, I think you should remove this DCHECK (and possibly
previous_suggestion(), since it would then become unused)

[jar (doing other things), 2013/12/08 01:38:36]

I thought the chance of exhausting the 10 attempts was pretty unlikely (in a
debug build).

I added an extra test that assures it is correct (it only validates the result
when the port_suggester was not called many times.

The reason why I especially liked this DCHECK was that I wanted to be sure that
the PRNG was getting called on each platform.  The previous cade had the bug
that it was never calling the PRNG anywhere.. and I noticed that each platform
(win vs posix vs other?) makes separate use of the PRNG... and I guessed it
would be easy for some platform to not bother getting this right (even now that
*we* called with the right argument).

[Ryan Hamilton, 2013/12/08 04:00:29]

Hm.  I'm totally down with a DCHECK to make sure this is working, since a
bona-fide test is highly non-trivial.  (Perhaps I'll work on that in my copious
spare time :>).  However, I'm very uncomfortable with landing DCHECKs that we
know can fail (albeit under unlikely circumstances).  In particular, I'm
extremely reluctant to do have code that might occasionally break the tree.  As
a sheriff, those kinds of failures drive me nuts!  You could consider augmenting
the check to make sure that fewer than kBindRetries have been attempted?  But at
that point, we're starting to get a really convoluted set of conditions.

By the way, when we use MockSockets, the rand_in_cb is not invoked at all.  Does
this DCHECK fail in those situations?

[jar (doing other things), 2013/12/08 04:57:43]

Please take a look at the DCHECK that was in the most recent version.  I think
it is very conservative, and I don't see a way in which it can break bots.  If
bots make it "hard to connect" then the suggest will be called 10 times, and the
DCHECK will have long since passed.

Currently, this DCHECK does not break any tests that I'm aware of (as can be
seen by the try bots).

Can you tell me how I should run such a test?

Note that IF we have mocked out tests, and they start to trigger this DCHECK,
all we'd have to do is call the cb more than 2 times... and the DCHECK would be
"satisfied."

[Ryan Hamilton, 2013/12/08 05:24:08]

I totally believe the DCHECK is passing on the bots, I'm just not sure what it
does :>  Can you help me understand?  I feel like I'm screwing up the polarity
of the conditions.

 467   DCHECK(ERR_SOCKET_NOT_CONNECTED != socket->GetLocalAddress(&address) ||

I'm not sure I'm reading this first clause correctly, but this seems to say that
if the socket is actually connected, then we don't do the rest of the checks. 
Am I misreading it?  If not, is this the right behavior?

The test I'm thinking of is QuicStreamFactoryTest.  I patched in your CL and
notice that when I run that test, the suggester's call_count is 0.  However, the
DCHECK does not trigger.  This seems to be because the first condition is
true... the socket is actually connected.  Is this doing what you expect?

[Ryan Hamilton, 2013/12/08 06:06:23]

Something that might make the DCHECK a bit more clear would be to structure it
like:

if (socket->GetLocalAddress(&address) == OK &&  // (or whatever this clause is
expected to be)
    port_suggester->call_count() <= 2) {
  DCHECK_EQ(port_suggester->previous_suggestion(), address.port());
}

It's a bit easier to read the intend, and in the event that it fires, you'll get
to see both values.  For example, if suggestions is -1, that means that it
wasn't called at all..

+  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