Migrates QUIC sessions to a new network when old network is (about to be) disconnected.

Adds connection migration capability to QUIC. This CL causes QUIC connections to migrate to a new network when an old network is disconnected or about to be disconnected. These notifications are available for Android >= L, so QUIC connection migration will only work for Android >= L.

BUG=576998
Committed: https://crrev.com/7e636640ca1dc9f71ebe8a746d1c9bf53c68a2f3
Cr-Commit-Position: refs/heads/master@{#369356}

[Jana, 2015-09-03 06:27:49]

jri@chromium.org changed reviewers:
+ rch@chromium.org

[Jana, 2015-09-03 06:28:13]

Sending for initial review

[Ryan Hamilton, 2015-09-08 04:09:38]

Exciting stuff! A few, mostly cosmetic, comments.

https://codereview.chromium.org/1327923002/diff/20001/net/quic/quic_chromium_...
File net/quic/quic_chromium_client_session.cc (right):

https://codereview.chromium.org/1327923002/diff/20001/net/quic/quic_chromium_...
net/quic/quic_chromium_client_session.cc:193: sockets_[0] = socket.Pass();
nit: sockets_.push_back(socket.Pass())

https://codereview.chromium.org/1327923002/diff/20001/net/quic/quic_chromium_...
net/quic/quic_chromium_client_session.cc:195: new
QuicPacketReader(sockets_[0].get(), this, net_log_));
ditto

https://codereview.chromium.org/1327923002/diff/20001/net/quic/quic_chromium_...
net/quic/quic_chromium_client_session.cc:754: for (size_t i = 0; i <
sockets_.size(); ++i) {
nit: for (Socket socket : sockets) { ... }

https://codereview.chromium.org/1327923002/diff/20001/net/quic/quic_chromium_...
net/quic/quic_chromium_client_session.cc:800: for (size_t i = 0; i <
packet_readers_.size(); ++i) {
nit: c++11 foreach loop

https://codereview.chromium.org/1327923002/diff/20001/net/quic/quic_chromium_...
net/quic/quic_chromium_client_session.cc:971: sockets_[num_sockets - 1] =
socket.Pass();
I think you can just do sockets_.push_back(socket.Pass());

https://codereview.chromium.org/1327923002/diff/20001/net/quic/quic_chromium_...
net/quic/quic_chromium_client_session.cc:982: }
It's a bit surprising that the AddSocket method returns void, but the AddReader
returns bool. Perhaps a single method which takes the Socket, Reader and Writer?

Should the size of readers_ and sockets_ be the same? If so, perhaps some
DCHECKs.

https://codereview.chromium.org/1327923002/diff/20001/net/quic/quic_chromium_...
File net/quic/quic_chromium_client_session.h (right):

https://codereview.chromium.org/1327923002/diff/20001/net/quic/quic_chromium_...
net/quic/quic_chromium_client_session.h:214: void
AddSocket(scoped_ptr<DatagramClientSocket> socket);
Do we ever remove sockets? How shall we handle read/write errors?

https://codereview.chromium.org/1327923002/diff/20001/net/quic/quic_chromium_...
net/quic/quic_chromium_client_session.h:218: bool
AddPacketReader(QuicPacketReader* reader);
Should this be a scoped_ptr?

https://codereview.chromium.org/1327923002/diff/20001/net/quic/quic_chromium_...
net/quic/quic_chromium_client_session.h:219: 
It's a bit odd that we need a socket, reader and writer but only need to pass in
a reader and a socket. This is probably out of scope for this CL...

https://codereview.chromium.org/1327923002/diff/20001/net/quic/quic_stream_fa...
File net/quic/quic_stream_factory.cc (right):

https://codereview.chromium.org/1327923002/diff/20001/net/quic/quic_stream_fa...
net/quic/quic_stream_factory.cc:1117: it != all_sessions_.end(); ++it) {
Can you do a C++11 range loop here?

https://codereview.chromium.org/1327923002/diff/20001/net/quic/quic_stream_fa...
net/quic/quic_stream_factory.cc:1123:
session->CloseSessionOnError(ERR_NETWORK_CHANGED, QUIC_INTERNAL_ERROR);
not sure about INTERNAL_ERROR

nit: add a continue, to remove the "} else {" and outdent.

https://codereview.chromium.org/1327923002/diff/20001/net/quic/quic_stream_fa...
net/quic/quic_stream_factory.cc:1125: // If session is active, (i) create a new
socket (which should be
nit: s/is active/has active streams/

https://codereview.chromium.org/1327923002/diff/20001/net/quic/quic_stream_fa...
net/quic/quic_stream_factory.cc:1138: set_require_confirmation(true);
Is this required?

https://codereview.chromium.org/1327923002/diff/20001/net/quic/quic_stream_fa...
net/quic/quic_stream_factory.cc:1148: session->AddSocket(socket.Pass());
Yeah, probably a single method which takes Socket, Reader and Writer would be
best. That method could also call StartReading().

https://codereview.chromium.org/1327923002/diff/20001/net/quic/quic_stream_fa...
net/quic/quic_stream_factory.cc:1152: set_require_confirmation(true);
Might as well do this before the for loop since it can be easier to read if
there's nothing after the big loop

[pauljensen, 2015-10-14 11:51:33]

pauljensen@chromium.org changed reviewers:
+ pauljensen@chromium.org

[pauljensen, 2015-10-14 11:51:34]

https://codereview.chromium.org/1327923002/diff/30001/net/quic/quic_stream_fa...
File net/quic/quic_stream_factory.cc (right):

https://codereview.chromium.org/1327923002/diff/30001/net/quic/quic_stream_fa...
net/quic/quic_stream_factory.cc:83: const NetworkHandle kDefaultNetworkHandle =
-2;
This is a little sketchy for my liking.  I'd suggest either (listed in order of
increasing sketchyness):
1. add "bool bind_to_default_network" argument to
CreateAndConfigureDatagramSocket
2. overload kInvalidNetworkHandle to mean bind-to-default-network when passed to
CreateAndConfigureDatagramSocket
3. define this to kInvalidNetworkHandle-1

https://codereview.chromium.org/1327923002/diff/30001/net/quic/quic_stream_fa...
net/quic/quic_stream_factory.cc:1145: // pauljensen: How do we do this?
Where do you register for events? (i.e. call
NetworkChangeNotifier::AddIPAddressObserver)?

I'd add a flag controlling all this new behavior and in the constructor I'd add
some:
   migrate_connections(some_quic_connection_migration_flag_thingy
#if defined(OS_ANDROID)
      && base::android::BuildInfo::GetInstance()->sdk_int() >=
         base::android::SDK_VERSION_LOLLIPOP
#endif

Where ever you call AddIPAddressObserver I'd change the code to:
  if (migrate_connections) {
    NetworkChangeNotifier::AddNetworkObserver(this);
  } else {
    NetworkChangeNotifier::AddIPAddressObserver(this);
  }

https://codereview.chromium.org/1327923002/diff/30001/net/quic/quic_stream_fa...
net/quic/quic_stream_factory.cc:1168: // pauljensen: do we need the following
"== network" check?
seems like a good idea

https://codereview.chromium.org/1327923002/diff/30001/net/quic/quic_stream_fa...
net/quic/quic_stream_factory.cc:1179: // pauljensen: do we need the following
"== network" check?
seems like a good idea

https://codereview.chromium.org/1327923002/diff/30001/net/quic/quic_stream_fa...
net/quic/quic_stream_factory.cc:1277: scoped_ptr<DatagramClientSocket>*
new_socket,
http://dev.chromium.org/developers/smart-pointer-guidelines
says:
"You should especially never see scoped_ptr<>*"

https://codereview.chromium.org/1327923002/diff/30001/net/quic/quic_stream_fa...
net/quic/quic_stream_factory.cc:1312: network =
NetworkChangeNotifier::GetDefaultNetwork();
We may want to loop here in cases where the default changes quickly.  I thought
we were going to add DatagramClientSocket::BindToDefaultNetwork() that included
the looping?  So here we'd have something like:
if (migrate_connections) {
  int rv;
  if (network == kDefaultNetworkHandle) {
    rv = socket->BindToDefaultNetwork();
  } else {
    rv = socket->BindToNetwork(network);
  }
  if (rv != OK)
    return rv;
}

[Jana, 2015-10-16 23:11:00]

Still need to add verification of new network, but addressing paul's comments.

https://codereview.chromium.org/1327923002/diff/30001/net/quic/quic_stream_fa...
File net/quic/quic_stream_factory.cc (right):

https://codereview.chromium.org/1327923002/diff/30001/net/quic/quic_stream_fa...
net/quic/quic_stream_factory.cc:83: const NetworkHandle kDefaultNetworkHandle =
-2;
On 2015/10/14 11:51:34, pauljensen wrote:
> This is a little sketchy for my liking.  I'd suggest either (listed in order
of
> increasing sketchyness):
> 1. add "bool bind_to_default_network" argument to
> CreateAndConfigureDatagramSocket
> 2. overload kInvalidNetworkHandle to mean bind-to-default-network when passed
to
> CreateAndConfigureDatagramSocket
> 3. define this to kInvalidNetworkHandle-1

I think this is all sketchy :-) But, yeah, it seems reasonable to overload
kInvalid instead of creating a new value, so using option 2.

https://codereview.chromium.org/1327923002/diff/30001/net/quic/quic_stream_fa...
net/quic/quic_stream_factory.cc:1145: // pauljensen: How do we do this?
On 2015/10/14 11:51:34, pauljensen wrote:
> Where do you register for events? (i.e. call
> NetworkChangeNotifier::AddIPAddressObserver)?
> 
> I'd add a flag controlling all this new behavior and in the constructor I'd
add
> some:
>    migrate_connections(some_quic_connection_migration_flag_thingy
> #if defined(OS_ANDROID)
>       && base::android::BuildInfo::GetInstance()->sdk_int() >=
>          base::android::SDK_VERSION_LOLLIPOP
> #endif
> 
> Where ever you call AddIPAddressObserver I'd change the code to:
>   if (migrate_connections) {
>     NetworkChangeNotifier::AddNetworkObserver(this);
>   } else {
>     NetworkChangeNotifier::AddIPAddressObserver(this);
>   }

*Facepalm*. So I discovered that we do not actually register for events
currently. Fixing now.
Also, added code here to add NetworkObserver.

https://codereview.chromium.org/1327923002/diff/30001/net/quic/quic_stream_fa...
net/quic/quic_stream_factory.cc:1168: // pauljensen: do we need the following
"== network" check?
On 2015/10/14 11:51:34, pauljensen wrote:
> seems like a good idea

Acknowledged.

https://codereview.chromium.org/1327923002/diff/30001/net/quic/quic_stream_fa...
net/quic/quic_stream_factory.cc:1179: // pauljensen: do we need the following
"== network" check?
On 2015/10/14 11:51:34, pauljensen wrote:
> seems like a good idea

Acknowledged.

https://codereview.chromium.org/1327923002/diff/30001/net/quic/quic_stream_fa...
net/quic/quic_stream_factory.cc:1277: scoped_ptr<DatagramClientSocket>*
new_socket,
On 2015/10/14 11:51:34, pauljensen wrote:
> http://dev.chromium.org/developers/smart-pointer-guidelines
> says:
> "You should especially never see scoped_ptr<>*"

Done.

https://codereview.chromium.org/1327923002/diff/30001/net/quic/quic_stream_fa...
net/quic/quic_stream_factory.cc:1312: network =
NetworkChangeNotifier::GetDefaultNetwork();
On 2015/10/14 11:51:34, pauljensen wrote:
> We may want to loop here in cases where the default changes quickly.  I
thought
> we were going to add DatagramClientSocket::BindToDefaultNetwork() that
included
> the looping?  So here we'd have something like:
> if (migrate_connections) {
>   int rv;
>   if (network == kDefaultNetworkHandle) {
>     rv = socket->BindToDefaultNetwork();
>   } else {
>     rv = socket->BindToNetwork(network);
>   }
>   if (rv != OK)
>     return rv;
> }

I've added a BindToDefaultNetwork method to DatagramClientSocket. I don't think
I understand how you see the looping work -- what if there's no default network
for a long time? Does it make sense to consider using a timer to wait "a short
while" and then give up?

[pauljensen, 2015-10-19 19:01:39]

https://codereview.chromium.org/1327923002/diff/70001/net/udp/udp_client_sock...
File net/udp/udp_client_socket.cc (right):

https://codereview.chromium.org/1327923002/diff/70001/net/udp/udp_client_sock...
net/udp/udp_client_socket.cc:30: return BindToNetwork(network);
The reason this needs to loop is that the default network can change, so this is
racy.  The idea is to do something like:
int ret;
for (int attempt = 0; attempt < 3; attempt++) {
  NetworkChangeNotifier::NetworkHandle network =
      NetworkChangeNotifier::GetDefaultNetwork();
  if (network == kInvalidNetworkHandle)
    return ERR_INTERNET_DISCONNECTED;
  ret = BindToNetwork(network);
  if (ret != /*whatever ENONET turns into*/)
    return ret;
}
return ret;

You can see we return ENONET when we try to bind a socket to a network that no
longer exists here in Android:
http://androidxref.com/6.0.0_r1/xref/system/netd/server/NetworkController.cpp...

[pauljensen, 2015-10-19 19:49:04]

https://codereview.chromium.org/1327923002/diff/70001/net/udp/udp_client_sock...
File net/udp/udp_client_socket.cc (right):

https://codereview.chromium.org/1327923002/diff/70001/net/udp/udp_client_sock...
net/udp/udp_client_socket.cc:30: return BindToNetwork(network);
On 2015/10/19 19:01:39, pauljensen wrote:
> The reason this needs to loop is that the default network can change, so this
is
> racy.  The idea is to do something like:
> int ret;
> for (int attempt = 0; attempt < 3; attempt++) {
>   NetworkChangeNotifier::NetworkHandle network =
>       NetworkChangeNotifier::GetDefaultNetwork();
>   if (network == kInvalidNetworkHandle)
>     return ERR_INTERNET_DISCONNECTED;
>   ret = BindToNetwork(network);
>   if (ret != /*whatever ENONET turns into*/)
>     return ret;
> }
> return ret;
> 
> You can see we return ENONET when we try to bind a socket to a network that no
> longer exists here in Android:
>
http://androidxref.com/6.0.0_r1/xref/system/netd/server/NetworkController.cpp...

This incorrectly returns ERR_INTERNET_DISCONNECTED however when
GetDefaultNetwork isn't implemented however...hmm

[pauljensen, 2015-10-22 03:43:04]

On 2015/10/19 19:49:04, pauljensen wrote:
>
https://codereview.chromium.org/1327923002/diff/70001/net/udp/udp_client_sock...
> File net/udp/udp_client_socket.cc (right):
> 
>
https://codereview.chromium.org/1327923002/diff/70001/net/udp/udp_client_sock...
> net/udp/udp_client_socket.cc:30: return BindToNetwork(network);
> On 2015/10/19 19:01:39, pauljensen wrote:
> > The reason this needs to loop is that the default network can change, so
this
> is
> > racy.  The idea is to do something like:
> > int ret;
> > for (int attempt = 0; attempt < 3; attempt++) {
> >   NetworkChangeNotifier::NetworkHandle network =
> >       NetworkChangeNotifier::GetDefaultNetwork();
> >   if (network == kInvalidNetworkHandle)
> >     return ERR_INTERNET_DISCONNECTED;
> >   ret = BindToNetwork(network);
> >   if (ret != /*whatever ENONET turns into*/)
> >     return ret;
> > }
> > return ret;
> > 
> > You can see we return ENONET when we try to bind a socket to a network that
no
> > longer exists here in Android:
> >
>
http://androidxref.com/6.0.0_r1/xref/system/netd/server/NetworkController.cpp...
> 
> This incorrectly returns ERR_INTERNET_DISCONNECTED however when
> GetDefaultNetwork isn't implemented however...hmm

I took a stab at addressing some of this:
https://codereview.chromium.org/1416213003
I still need to write tests.

[Jana, 2015-11-17 01:50:09]

Thanks, Ryan, Paul. I'm sending along some fixes and responses (most of them to
Ryan's early review). There are still a few things outstanding:
(i) Verification of the new network needs to be done correctly. There's
currently one flag that can be set twice as OnNetworkSoonToDisconnect and
OnNetworkDisconnected are called.
(ii) Socket read errors need to be handled, as dead sockets may result in read
errors.
(iii) Tests. Blech.

https://codereview.chromium.org/1327923002/diff/20001/net/quic/quic_chromium_...
File net/quic/quic_chromium_client_session.cc (right):

https://codereview.chromium.org/1327923002/diff/20001/net/quic/quic_chromium_...
net/quic/quic_chromium_client_session.cc:193: sockets_[0] = socket.Pass();
On 2015/09/08 04:09:37, Ryan Hamilton wrote:
> nit: sockets_.push_back(socket.Pass())

Done.

https://codereview.chromium.org/1327923002/diff/20001/net/quic/quic_chromium_...
net/quic/quic_chromium_client_session.cc:195: new
QuicPacketReader(sockets_[0].get(), this, net_log_));
On 2015/09/08 04:09:37, Ryan Hamilton wrote:
> ditto

Done.

https://codereview.chromium.org/1327923002/diff/20001/net/quic/quic_chromium_...
net/quic/quic_chromium_client_session.cc:754: for (size_t i = 0; i <
sockets_.size(); ++i) {
On 2015/09/08 04:09:37, Ryan Hamilton wrote:
> nit: for (Socket socket : sockets) { ... }

Done.

https://codereview.chromium.org/1327923002/diff/20001/net/quic/quic_chromium_...
net/quic/quic_chromium_client_session.cc:800: for (size_t i = 0; i <
packet_readers_.size(); ++i) {
On 2015/09/08 04:09:37, Ryan Hamilton wrote:
> nit: c++11 foreach loop

Done.

https://codereview.chromium.org/1327923002/diff/20001/net/quic/quic_chromium_...
net/quic/quic_chromium_client_session.cc:971: sockets_[num_sockets - 1] =
socket.Pass();
On 2015/09/08 04:09:37, Ryan Hamilton wrote:
> I think you can just do sockets_.push_back(socket.Pass());

Done.

https://codereview.chromium.org/1327923002/diff/20001/net/quic/quic_chromium_...
net/quic/quic_chromium_client_session.cc:982: }
On 2015/09/08 04:09:37, Ryan Hamilton wrote:
> It's a bit surprising that the AddSocket method returns void, but the
AddReader
> returns bool. Perhaps a single method which takes the Socket, Reader and
Writer?
> 
> Should the size of readers_ and sockets_ be the same? If so, perhaps some
> DCHECKs.

Yeah, this wasn't great, so I cleaned it up now, PTAL.

https://codereview.chromium.org/1327923002/diff/20001/net/quic/quic_chromium_...
File net/quic/quic_chromium_client_session.h (right):

https://codereview.chromium.org/1327923002/diff/20001/net/quic/quic_chromium_...
net/quic/quic_chromium_client_session.h:214: void
AddSocket(scoped_ptr<DatagramClientSocket> socket);
On 2015/09/08 04:09:38, Ryan Hamilton wrote:
> Do we ever remove sockets? How shall we handle read/write errors?

We don't remove sockets at this point. This is why there's a max number of
sockets that we create. Since there's only one Writer at the moment, that error
processing shouldn't change. We maintain multiple readers however, and we need
to catch read errors. I'll do some work on figuring out how to catch
OnReadError.

https://codereview.chromium.org/1327923002/diff/20001/net/quic/quic_chromium_...
net/quic/quic_chromium_client_session.h:218: bool
AddPacketReader(QuicPacketReader* reader);
On 2015/09/08 04:09:37, Ryan Hamilton wrote:
> Should this be a scoped_ptr?

Done.

https://codereview.chromium.org/1327923002/diff/20001/net/quic/quic_chromium_...
net/quic/quic_chromium_client_session.h:219: 
On 2015/09/08 04:09:38, Ryan Hamilton wrote:
> It's a bit odd that we need a socket, reader and writer but only need to pass
in
> a reader and a socket. This is probably out of scope for this CL...

Fixed in the new version, PTAL.

https://codereview.chromium.org/1327923002/diff/20001/net/quic/quic_stream_fa...
File net/quic/quic_stream_factory.cc (right):

https://codereview.chromium.org/1327923002/diff/20001/net/quic/quic_stream_fa...
net/quic/quic_stream_factory.cc:1117: it != all_sessions_.end(); ++it) {
On 2015/09/08 04:09:38, Ryan Hamilton wrote:
> Can you do a C++11 range loop here?

Done.

https://codereview.chromium.org/1327923002/diff/20001/net/quic/quic_stream_fa...
net/quic/quic_stream_factory.cc:1123:
session->CloseSessionOnError(ERR_NETWORK_CHANGED, QUIC_INTERNAL_ERROR);
On 2015/09/08 04:09:38, Ryan Hamilton wrote:
> not sure about INTERNAL_ERROR
> 
> nit: add a continue, to remove the "} else {" and outdent.

Done.

https://codereview.chromium.org/1327923002/diff/20001/net/quic/quic_stream_fa...
net/quic/quic_stream_factory.cc:1125: // If session is active, (i) create a new
socket (which should be
On 2015/09/08 04:09:38, Ryan Hamilton wrote:
> nit: s/is active/has active streams/

Done.

https://codereview.chromium.org/1327923002/diff/20001/net/quic/quic_stream_fa...
net/quic/quic_stream_factory.cc:1138: set_require_confirmation(true);
On 2015/09/08 04:09:38, Ryan Hamilton wrote:
> Is this required?

We've not verified this new network, and while it doesn't matter for this
session, since we'll move all packet writes to this new network anyways, a new
session should verify before using this network.

That said, we need something, and simple setting this flag isn't enough... I
think we need a "verified" flag that is set on a network when the network is
verified. I'll work on getting that in in this CL.

https://codereview.chromium.org/1327923002/diff/20001/net/quic/quic_stream_fa...
net/quic/quic_stream_factory.cc:1148: session->AddSocket(socket.Pass());
On 2015/09/08 04:09:38, Ryan Hamilton wrote:
> Yeah, probably a single method which takes Socket, Reader and Writer would be
> best. That method could also call StartReading().

Done. PTAL.

https://codereview.chromium.org/1327923002/diff/20001/net/quic/quic_stream_fa...
net/quic/quic_stream_factory.cc:1152: set_require_confirmation(true);
On 2015/09/08 04:09:38, Ryan Hamilton wrote:
> Might as well do this before the for loop since it can be easier to read if
> there's nothing after the big loop

Done.

https://codereview.chromium.org/1327923002/diff/70001/net/udp/udp_client_sock...
File net/udp/udp_client_socket.cc (right):

https://codereview.chromium.org/1327923002/diff/70001/net/udp/udp_client_sock...
net/udp/udp_client_socket.cc:30: return BindToNetwork(network);
On 2015/10/19 19:49:04, pauljensen wrote:
> On 2015/10/19 19:01:39, pauljensen wrote:
> > The reason this needs to loop is that the default network can change, so
this
> is
> > racy.  The idea is to do something like:
> > int ret;
> > for (int attempt = 0; attempt < 3; attempt++) {
> >   NetworkChangeNotifier::NetworkHandle network =
> >       NetworkChangeNotifier::GetDefaultNetwork();
> >   if (network == kInvalidNetworkHandle)
> >     return ERR_INTERNET_DISCONNECTED;
> >   ret = BindToNetwork(network);
> >   if (ret != /*whatever ENONET turns into*/)
> >     return ret;
> > }
> > return ret;
> > 
> > You can see we return ENONET when we try to bind a socket to a network that
no
> > longer exists here in Android:
> >
>
http://androidxref.com/6.0.0_r1/xref/system/netd/server/NetworkController.cpp...
> 
> This incorrectly returns ERR_INTERNET_DISCONNECTED however when
> GetDefaultNetwork isn't implemented however...hmm

Let's continue this conversation on your CL to implement BindToDefaultNetwork.
I'm leaving this simple implementation as is, to be replaced by your
implementation. SG?

[Ryan Hamilton, 2015-11-17 04:57:28]

https://codereview.chromium.org/1327923002/diff/90001/chrome/browser/io_threa...
File chrome/browser/io_thread.cc (right):

https://codereview.chromium.org/1327923002/diff/90001/chrome/browser/io_threa...
chrome/browser/io_thread.cc:1501: }
Since this code is conditional, I'm surprised the tests are not conditional on
#if android.

https://codereview.chromium.org/1327923002/diff/90001/chrome/browser/io_thread.h
File chrome/browser/io_thread.h (right):

https://codereview.chromium.org/1327923002/diff/90001/chrome/browser/io_threa...
chrome/browser/io_thread.h:221: Optional<bool>
quic_migrate_sessions_on_net_change;
here and elsewhere, I think "net" => "network" since the style guide eschews
abbreviation.

https://codereview.chromium.org/1327923002/diff/90001/net/quic/quic_chromium_...
File net/quic/quic_chromium_client_session.h (right):

https://codereview.chromium.org/1327923002/diff/90001/net/quic/quic_chromium_...
net/quic/quic_chromium_client_session.h:228: // Takes ownership of |socket|,
|reader|, and |writer|.
Can you explain why this method would return false?

https://codereview.chromium.org/1327923002/diff/90001/net/quic/quic_chromium_...
net/quic/quic_chromium_client_session.h:230: QuicPacketReader* reader,
Since |reader| and writer are not scoped_ptrs, are they owned by the caller? Oh,
the comment says that they're all owned by the session. Sounds like they should
be scoped_ptr?

https://codereview.chromium.org/1327923002/diff/90001/net/quic/quic_stream_fa...
File net/quic/quic_stream_factory.cc (right):

https://codereview.chromium.org/1327923002/diff/90001/net/quic/quic_stream_fa...
net/quic/quic_stream_factory.cc:659:
NetworkChangeNotifier::AddIPAddressObserver(this);
So if migrate_ is true, then is close_ effectively ignored?

https://codereview.chromium.org/1327923002/diff/90001/net/quic/quic_stream_fa...
net/quic/quic_stream_factory.cc:1229: if
(!session->MigrateToSocket(socket.Pass(), new_reader, new_writer)) {
I think new_reader and new_writer are leaked here if this method returns false. 

In any case, why close the connection when we're on our 6th socket instead of
throwing away the 1st socket and adding the new one?

[Ryan Hamilton, 2015-11-17 14:37:10]

https://codereview.chromium.org/1327923002/diff/90001/chrome/browser/io_threa...
File chrome/browser/io_thread.cc (right):

https://codereview.chromium.org/1327923002/diff/90001/chrome/browser/io_threa...
chrome/browser/io_thread.cc:1494: // Migration only implemented for Android
versions >= L.
I was thinking about this. This code is here because NetworkChangeNotifier only
implements the high-resolution events on Lollipop, right? In that case, I think
NetworkChangeNotifier should have a method like SupportsHighResolutinEvents (or
whatever) and QuicStreamFactory should query that...

[Jana, 2015-11-18 04:31:41]

Thanks, Ryan. Addressing your comments.

https://codereview.chromium.org/1327923002/diff/90001/chrome/browser/io_threa...
File chrome/browser/io_thread.cc (right):

https://codereview.chromium.org/1327923002/diff/90001/chrome/browser/io_threa...
chrome/browser/io_thread.cc:1494: // Migration only implemented for Android
versions >= L.
On 2015/11/17 14:37:10, Ryan Hamilton wrote:
> I was thinking about this. This code is here because NetworkChangeNotifier
only
> implements the high-resolution events on Lollipop, right? In that case, I
think
> NetworkChangeNotifier should have a method like SupportsHighResolutinEvents
(or
> whatever) and QuicStreamFactory should query that...

Yeah -- I hadn't done that yet. But you're right that if we do this in the
stream_factory, we don't need to do it here. I like your API, but I'm doing this
in the quic_stream_factory for now. Paul, I can add this API; do you think this
is a reasonable change to NetworkChangeNotifier?

https://codereview.chromium.org/1327923002/diff/90001/chrome/browser/io_threa...
chrome/browser/io_thread.cc:1501: }
On 2015/11/17 04:57:27, Ryan Hamilton wrote:
> Since this code is conditional, I'm surprised the tests are not conditional on
> #if android.

code's not conditional anymore, so tests should be fine.

https://codereview.chromium.org/1327923002/diff/90001/chrome/browser/io_thread.h
File chrome/browser/io_thread.h (right):

https://codereview.chromium.org/1327923002/diff/90001/chrome/browser/io_threa...
chrome/browser/io_thread.h:221: Optional<bool>
quic_migrate_sessions_on_net_change;
On 2015/11/17 04:57:27, Ryan Hamilton wrote:
> here and elsewhere, I think "net" => "network" since the style guide eschews
> abbreviation.

Done.

https://codereview.chromium.org/1327923002/diff/90001/net/quic/quic_chromium_...
File net/quic/quic_chromium_client_session.h (right):

https://codereview.chromium.org/1327923002/diff/90001/net/quic/quic_chromium_...
net/quic/quic_chromium_client_session.h:228: // Takes ownership of |socket|,
|reader|, and |writer|.
On 2015/11/17 04:57:27, Ryan Hamilton wrote:
> Can you explain why this method would return false?

Done.

https://codereview.chromium.org/1327923002/diff/90001/net/quic/quic_chromium_...
net/quic/quic_chromium_client_session.h:230: QuicPacketReader* reader,
On 2015/11/17 04:57:28, Ryan Hamilton wrote:
> Since |reader| and writer are not scoped_ptrs, are they owned by the caller?
Oh,
> the comment says that they're all owned by the session. Sounds like they
should
> be scoped_ptr?

Good suggestion. Done.

https://codereview.chromium.org/1327923002/diff/90001/net/quic/quic_stream_fa...
File net/quic/quic_stream_factory.cc (right):

https://codereview.chromium.org/1327923002/diff/90001/net/quic/quic_stream_fa...
net/quic/quic_stream_factory.cc:659:
NetworkChangeNotifier::AddIPAddressObserver(this);
On 2015/11/17 04:57:28, Ryan Hamilton wrote:
> So if migrate_ is true, then is close_ effectively ignored?

Yes, since migrate_ and close_ are both experimental behaviors at the moment,
and since setting close_ to true effectively disables any migration.

https://codereview.chromium.org/1327923002/diff/90001/net/quic/quic_stream_fa...
net/quic/quic_stream_factory.cc:1229: if
(!session->MigrateToSocket(socket.Pass(), new_reader, new_writer)) {
On 2015/11/17 04:57:28, Ryan Hamilton wrote:
> I think new_reader and new_writer are leaked here if this method returns
false. 

Doh! They're scoped_ptrs now, so the leak is fixed :-)

> In any case, why close the connection when we're on our 6th socket instead of
> throwing away the 1st socket and adding the new one?

Easier to reason about. I don't expect connections to be migrating across > 5
sockets. If this turns out to be important for some reason, I figured we could
worry about it then.

[pauljensen, 2015-11-18 17:24:23]

So one issue with binding UDP sockets to particular networks:  on Android if a
VPN is in place apps aren't allowed to do anything but use the VPN.  Attempting
to bind to a network (other than the VPN) will fail if a VPN is present.
We need to disable connection migration when a VPN is in place.  Unfortunately
VPNs can come and go as Chrome is running...
I did a fair bit of work to get the NetworkChangeNotifierAndroid to track VPNs
(using AddressTrackerLinux) so network change signals are sent when VPNs come
and go.  I imagine it wouldn't be too hard to expose a
NetworkChangeNotifier::IsVpnPresent() API.
The question is: is it possible to disable QUIC connection migration when a VPN
shows up randomly?

https://codereview.chromium.org/1327923002/diff/90001/chrome/browser/io_threa...
File chrome/browser/io_thread.cc (right):

https://codereview.chromium.org/1327923002/diff/90001/chrome/browser/io_threa...
chrome/browser/io_thread.cc:1494: // Migration only implemented for Android
versions >= L.
On 2015/11/18 04:31:41, Jana wrote:
> On 2015/11/17 14:37:10, Ryan Hamilton wrote:
> > I was thinking about this. This code is here because NetworkChangeNotifier
> only
> > implements the high-resolution events on Lollipop, right? In that case, I
> think
> > NetworkChangeNotifier should have a method like SupportsHighResolutinEvents
> (or
> > whatever) and QuicStreamFactory should query that...
> 
> Yeah -- I hadn't done that yet. But you're right that if we do this in the
> stream_factory, we don't need to do it here. I like your API, but I'm doing
this
> in the quic_stream_factory for now. Paul, I can add this API; do you think
this
> is a reasonable change to NetworkChangeNotifier?

ya, adding a NetworkChangeNotifier API for this SGTM.  I was going to propose
adding a net error code return value from the NCN::GetDefaultNetwork API: 
https://codereview.chromium.org/1416213003/diff/1/net/base/network_change_not...
but having a more general API seems like a good idea.

[Jana, 2015-11-18 21:44:17]

About the VPN issue -- if a non-VPN network is not be exposed by
GetConnectedNetworks, it will not be used by the migration code here, right?
Isn't that enough?

https://codereview.chromium.org/1327923002/diff/90001/chrome/browser/io_threa...
File chrome/browser/io_thread.cc (right):

https://codereview.chromium.org/1327923002/diff/90001/chrome/browser/io_threa...
chrome/browser/io_thread.cc:1494: // Migration only implemented for Android
versions >= L.
On 2015/11/18 17:24:23, pauljensen wrote:
> On 2015/11/18 04:31:41, Jana wrote:
> > On 2015/11/17 14:37:10, Ryan Hamilton wrote:
> > > I was thinking about this. This code is here because NetworkChangeNotifier
> > only
> > > implements the high-resolution events on Lollipop, right? In that case, I
> > think
> > > NetworkChangeNotifier should have a method like
SupportsHighResolutinEvents
> > (or
> > > whatever) and QuicStreamFactory should query that...
> > 
> > Yeah -- I hadn't done that yet. But you're right that if we do this in the
> > stream_factory, we don't need to do it here. I like your API, but I'm doing
> this
> > in the quic_stream_factory for now. Paul, I can add this API; do you think
> this
> > is a reasonable change to NetworkChangeNotifier?
> 
> ya, adding a NetworkChangeNotifier API for this SGTM.  I was going to propose
> adding a net error code return value from the NCN::GetDefaultNetwork API: 
>
https://codereview.chromium.org/1416213003/diff/1/net/base/network_change_not...
> but having a more general API seems like a good idea.

Yeah, I think this belongs in the NetworkChangeNotifier, because we'd want to
make a decision about which notifier to register for based on this query. I'll
get this in.

[Ryan Hamilton, 2015-11-18 22:23:36]

Good progress! Would like to see if #if Android move into NCN somewhere since
it's not a good idea for that logic to be duplicated.

https://codereview.chromium.org/1327923002/diff/90001/chrome/browser/io_threa...
File chrome/browser/io_thread.cc (right):

https://codereview.chromium.org/1327923002/diff/90001/chrome/browser/io_threa...
chrome/browser/io_thread.cc:1494: // Migration only implemented for Android
versions >= L.
On 2015/11/18 17:24:23, pauljensen wrote:
> On 2015/11/18 04:31:41, Jana wrote:
> > On 2015/11/17 14:37:10, Ryan Hamilton wrote:
> > > I was thinking about this. This code is here because NetworkChangeNotifier
> > only
> > > implements the high-resolution events on Lollipop, right? In that case, I
> > think
> > > NetworkChangeNotifier should have a method like
SupportsHighResolutinEvents
> > (or
> > > whatever) and QuicStreamFactory should query that...
> > 
> > Yeah -- I hadn't done that yet. But you're right that if we do this in the
> > stream_factory, we don't need to do it here. I like your API, but I'm doing
> this
> > in the quic_stream_factory for now. Paul, I can add this API; do you think
> this
> > is a reasonable change to NetworkChangeNotifier?
> 
> ya, adding a NetworkChangeNotifier API for this SGTM.  I was going to propose
> adding a net error code return value from the NCN::GetDefaultNetwork API: 
>
https://codereview.chromium.org/1416213003/diff/1/net/base/network_change_not...
> but having a more general API seems like a good idea.

One idea might be to have NetworkChangeNotifier::AddNetworkObserver() return a
bool, which is false if the NetworkObserver is not supported?

https://codereview.chromium.org/1327923002/diff/130001/chrome/browser/io_thre...
File chrome/browser/io_thread.cc (right):

https://codereview.chromium.org/1327923002/diff/130001/chrome/browser/io_thre...
chrome/browser/io_thread.cc:1495: #endif
Does this build? Remove this?

https://codereview.chromium.org/1327923002/diff/130001/net/http/http_network_...
File net/http/http_network_session.h (right):

https://codereview.chromium.org/1327923002/diff/130001/net/http/http_network_...
net/http/http_network_session.h:176: // If true, active QUIC sessions may be
migrated onto new IPs on network
nit: onto new IPs when the network changes?

https://codereview.chromium.org/1327923002/diff/130001/net/quic/quic_chromium...
File net/quic/quic_chromium_client_session.cc (right):

https://codereview.chromium.org/1327923002/diff/130001/net/quic/quic_chromium...
net/quic/quic_chromium_client_session.cc:987:
connection()->SetQuicPacketWriter(writer.release(), /*owns_writer=*/true);
Can you add a todo here that SetQuicPAcketWriter should take a scoped_ptr?

https://codereview.chromium.org/1327923002/diff/130001/net/quic/quic_stream_f...
File net/quic/quic_stream_factory.cc (right):

https://codereview.chromium.org/1327923002/diff/130001/net/quic/quic_stream_f...
net/quic/quic_stream_factory.cc:1158: // Nothing to do.
I'm not sure that this comment adds any value.

https://codereview.chromium.org/1327923002/diff/130001/net/quic/quic_stream_f...
net/quic/quic_stream_factory.cc:1203: DCHECK_NE(network, new_network);
It looks like these DCHECKs are needed because of the code on 1182-1189. How
about doing:

   // Find a new network that old sockets can be migrated to.
   // pauljensen: do we need the following "== network" check?
   NetworkChangeNotifier::NetworkHandle new_network =
       NetworkChangeNotifier::kInvalidNetworkHandle;
   for (NetworkChangeNotifier::NetworkHandle n : network_list) {
    if (n == network) {
      continue;
     }
     new_network = n;
   }

   if (new_network == NetworkChangeNotifier::kInvalidNetworkHandle) {
     CloseAllSessions(ERR_NETWORK_CHANGED);
     return;
   }

This handles the empty case and the single element that is the current network
case.

https://codereview.chromium.org/1327923002/diff/130001/net/quic/quic_stream_f...
net/quic/quic_stream_factory.cc:1213:
session->CloseSessionOnError(ERR_NETWORK_CHANGED, QUIC_INTERNAL_ERROR);
CloseSessionOnError causes the session to be removed from active_sessions_ so I
don't think you need to explicitly erase it here. If you were paranoid about
this, you could do

  DCHECK(!ContainsKey(active_sessions_, server_id));

After calling CloseSessionOnError

https://codereview.chromium.org/1327923002/diff/130001/net/quic/quic_stream_f...
net/quic/quic_stream_factory.cc:1218: //      continue;
What's up with this code?

https://codereview.chromium.org/1327923002/diff/130001/net/quic/quic_stream_f...
net/quic/quic_stream_factory.cc:1225: OnSessionGoingAway(session);
I wonder if you want to send a GOAWAY as well?

https://codereview.chromium.org/1327923002/diff/130001/net/quic/quic_stream_f...
net/quic/quic_stream_factory.cc:1228:
CreateDatagramClientSocket(all_sessions_[session], session->net_log());
instead of all_sessions_[session] can you call session_-server_id()?

https://codereview.chromium.org/1327923002/diff/130001/net/quic/quic_stream_f...
net/quic/quic_stream_factory.cc:1230: connection->peer_address(), network);
It looks like both places this method is called is immediately after
CreateDatagramClientSocket(). How about folding the configuration into Create?

https://codereview.chromium.org/1327923002/diff/130001/net/quic/quic_stream_f...
net/quic/quic_stream_factory.cc:1231: if (rv != OK) {
nit: no need to store rv in a variable if it's not used after this:

if (Configure() != OK) { ...}

https://codereview.chromium.org/1327923002/diff/130001/net/quic/quic_stream_f...
net/quic/quic_stream_factory.cc:1387: socket.get(), addr,
NetworkChangeNotifier::kInvalidNetworkHandle);
PLease add a comment here about using an "invalid" network handle since that
sounds ... bad :>

https://codereview.chromium.org/1327923002/diff/130001/net/udp/datagram_clien...
File net/udp/datagram_client_socket.h (right):

https://codereview.chromium.org/1327923002/diff/130001/net/udp/datagram_clien...
net/udp/datagram_client_socket.h:30: virtual int BindToDefaultNetwork() = 0;
Are there other subclasses of this interface which you need to add this method
to?

https://codereview.chromium.org/1327923002/diff/130001/net/udp/udp_client_soc...
File net/udp/udp_client_socket.cc (right):

https://codereview.chromium.org/1327923002/diff/130001/net/udp/udp_client_soc...
net/udp/udp_client_socket.cc:30: return BindToNetwork(network);
I'm not sure there is much benefit to this method since it seems to be
equivalent to:

socket->BindToNetwork(NetworkChangeNotifier::GetDefaultNetwork());

I think it would be cleaner to just use the method. It also means that in
QuicStreamFactory instead of passing in kInvalid and then checking to see if
network = kInvalue and then possibly calling this method, you could just pass
NetworkChangeNotifier::GetDefaultNetwork() into the Create socket method.

[pauljensen, 2015-11-19 15:25:43]

On 2015/11/18 21:44:17, Jana wrote:
> About the VPN issue -- if a non-VPN network is not be exposed by
> GetConnectedNetworks, it will not be used by the migration code here, right?
> Isn't that enough?

GetConnectedNetworks() will return the VPN network and the underlying networks
(e.g. Cellular and/or WiFi).  Attempting to bind to the underlying networks will
be futile as it's disallowed.  It's complicated further by the fact that there
may be multiple VPNs for multiple users (yes, Android does allow this), and/or
the VPN may not apply to the current user in which case they can in fact bind to
the underlying networks.  Anyhow, I don't think we can do anything useful in
terms of connection migration when a VPN shows up so I think bailing out may be
our best option.

https://codereview.chromium.org/1327923002/diff/130001/net/udp/udp_client_soc...
File net/udp/udp_client_socket.cc (right):

https://codereview.chromium.org/1327923002/diff/130001/net/udp/udp_client_soc...
net/udp/udp_client_socket.cc:30: return BindToNetwork(network);
On 2015/11/18 22:23:36, Ryan Hamilton wrote:
> I'm not sure there is much benefit to this method since it seems to be
> equivalent to:
> 
> socket->BindToNetwork(NetworkChangeNotifier::GetDefaultNetwork());
> 
> I think it would be cleaner to just use the method. It also means that in
> QuicStreamFactory instead of passing in kInvalid and then checking to see if
> network = kInvalue and then possibly calling this method, you could just pass
> NetworkChangeNotifier::GetDefaultNetwork() into the Create socket method.

Jana's implementation and Ryan's suggestion both suffer from a race between
getting the default and binding to it, and the default changing.  This was the
point of my
https://codereview.chromium.org/1416213003/diff/1/net/udp/udp_client_socket.c...

[pauljensen, 2015-11-19 19:46:18]

On 2015/11/19 15:25:43, pauljensen wrote:
> On 2015/11/18 21:44:17, Jana wrote:
> > About the VPN issue -- if a non-VPN network is not be exposed by
> > GetConnectedNetworks, it will not be used by the migration code here, right?
> > Isn't that enough?
> 
> GetConnectedNetworks() will return the VPN network and the underlying networks
> (e.g. Cellular and/or WiFi).  Attempting to bind to the underlying networks
will
> be futile as it's disallowed.  It's complicated further by the fact that there
> may be multiple VPNs for multiple users (yes, Android does allow this), and/or
> the VPN may not apply to the current user in which case they can in fact bind
to
> the underlying networks.  Anyhow, I don't think we can do anything useful in
> terms of connection migration when a VPN shows up so I think bailing out may
be
> our best option.

Jana and I just VC'd.  On the issue of VPNs here was our conclusion:
1. Having VPNs in place could totally break BindToDefaultNetwork() because
GetDefaultNetwork() may say WiFi or Cell is the default but the VPN may be the
only usable network, so we'll fail to bind to the default network and every QUIC
socket won't work.
2. The simplest solution we could think of was to project a realistic
interpretation in GetConnectedNetworks() and GetDefaultNetwork():
     a. if a VPN is in place that applies to the current user, *only* return the
VPN from GetConnectedNetworks() and GetDefaultNetwork().
     b. if a VPN is in place that does not apply to the current user, do not
include it in either GetConnectedNetworks() or GetDefaultNetwork().
   I'll work on this modification to NetworkChangeNotifierAndroid when I get
back.  I'm not immediately sure how to determine if a VPN applies to the current
user.  I imagine there are various ways, like:
     a. try binding a UDP socket to it
     b. call GetNetworkInfo() and see if it gets blocked (Android's
ConnectivityService like to give modified views of the world when VPNs are in
place).

[Jana, 2015-11-21 02:21:22]

Thanks, Ryan. This is still quite a WIP CL, but some responses to your comments
below.

https://codereview.chromium.org/1327923002/diff/90001/chrome/browser/io_threa...
File chrome/browser/io_thread.cc (right):

https://codereview.chromium.org/1327923002/diff/90001/chrome/browser/io_threa...
chrome/browser/io_thread.cc:1494: // Migration only implemented for Android
versions >= L.
On 2015/11/18 22:23:35, Ryan Hamilton wrote:
> On 2015/11/18 17:24:23, pauljensen wrote:
> > On 2015/11/18 04:31:41, Jana wrote:
> > > On 2015/11/17 14:37:10, Ryan Hamilton wrote:
> > > > I was thinking about this. This code is here because
NetworkChangeNotifier
> > > only
> > > > implements the high-resolution events on Lollipop, right? In that case,
I
> > > think
> > > > NetworkChangeNotifier should have a method like
> SupportsHighResolutinEvents
> > > (or
> > > > whatever) and QuicStreamFactory should query that...
> > > 
> > > Yeah -- I hadn't done that yet. But you're right that if we do this in the
> > > stream_factory, we don't need to do it here. I like your API, but I'm
doing
> > this
> > > in the quic_stream_factory for now. Paul, I can add this API; do you think
> > this
> > > is a reasonable change to NetworkChangeNotifier?
> > 
> > ya, adding a NetworkChangeNotifier API for this SGTM.  I was going to
propose
> > adding a net error code return value from the NCN::GetDefaultNetwork API: 
> >
>
https://codereview.chromium.org/1416213003/diff/1/net/base/network_change_not...
> > but having a more general API seems like a good idea.
> 
> One idea might be to have NetworkChangeNotifier::AddNetworkObserver() return a
> bool, which is false if the NetworkObserver is not supported?

Ongoing in https://codereview.chromium.org/1454313002/

https://codereview.chromium.org/1327923002/diff/130001/chrome/browser/io_thre...
File chrome/browser/io_thread.cc (right):

https://codereview.chromium.org/1327923002/diff/130001/chrome/browser/io_thre...
chrome/browser/io_thread.cc:1495: #endif
On 2015/11/18 22:23:35, Ryan Hamilton wrote:
> Does this build? Remove this?

Yeah -- I fixed it later. I had tried just building net, which worked fine :-)

https://codereview.chromium.org/1327923002/diff/130001/net/http/http_network_...
File net/http/http_network_session.h (right):

https://codereview.chromium.org/1327923002/diff/130001/net/http/http_network_...
net/http/http_network_session.h:176: // If true, active QUIC sessions may be
migrated onto new IPs on network
On 2015/11/18 22:23:35, Ryan Hamilton wrote:
> nit: onto new IPs when the network changes?

Done.

https://codereview.chromium.org/1327923002/diff/130001/net/quic/quic_chromium...
File net/quic/quic_chromium_client_session.cc (right):

https://codereview.chromium.org/1327923002/diff/130001/net/quic/quic_chromium...
net/quic/quic_chromium_client_session.cc:987:
connection()->SetQuicPacketWriter(writer.release(), /*owns_writer=*/true);
On 2015/11/18 22:23:35, Ryan Hamilton wrote:
> Can you add a todo here that SetQuicPAcketWriter should take a scoped_ptr?

Done.

https://codereview.chromium.org/1327923002/diff/130001/net/quic/quic_stream_f...
File net/quic/quic_stream_factory.cc (right):

https://codereview.chromium.org/1327923002/diff/130001/net/quic/quic_stream_f...
net/quic/quic_stream_factory.cc:1203: DCHECK_NE(network, new_network);
On 2015/11/18 22:23:36, Ryan Hamilton wrote:
> It looks like these DCHECKs are needed because of the code on 1182-1189. How
> about doing:
> 
>    // Find a new network that old sockets can be migrated to.
>    // pauljensen: do we need the following "== network" check?
>    NetworkChangeNotifier::NetworkHandle new_network =
>        NetworkChangeNotifier::kInvalidNetworkHandle;
>    for (NetworkChangeNotifier::NetworkHandle n : network_list) {
>     if (n == network) {
>       continue;
>      }
>      new_network = n;
>    }
> 
>    if (new_network == NetworkChangeNotifier::kInvalidNetworkHandle) {
>      CloseAllSessions(ERR_NETWORK_CHANGED);
>      return;
>    }
> 
> This handles the empty case and the single element that is the current network
> case.

Good suggestion -- there was definitely redundant code there. Done.

https://codereview.chromium.org/1327923002/diff/130001/net/quic/quic_stream_f...
net/quic/quic_stream_factory.cc:1213:
session->CloseSessionOnError(ERR_NETWORK_CHANGED, QUIC_INTERNAL_ERROR);
On 2015/11/18 22:23:36, Ryan Hamilton wrote:
> CloseSessionOnError causes the session to be removed from active_sessions_ so
I
> don't think you need to explicitly erase it here. If you were paranoid about
> this, you could do
> 
>   DCHECK(!ContainsKey(active_sessions_, server_id));
> 
> After calling CloseSessionOnError

Ah, ok, I see that now. Removed.

https://codereview.chromium.org/1327923002/diff/130001/net/quic/quic_stream_f...
net/quic/quic_stream_factory.cc:1218: //      continue;
On 2015/11/18 22:23:36, Ryan Hamilton wrote:
> What's up with this code?

Cleaned it up some and added a method in session to help... this is waiting for
Paul's CL to land.

https://codereview.chromium.org/1327923002/diff/130001/net/quic/quic_stream_f...
net/quic/quic_stream_factory.cc:1225: OnSessionGoingAway(session);
On 2015/11/18 22:23:36, Ryan Hamilton wrote:
> I wonder if you want to send a GOAWAY as well?

Why? We won't sent any more requests to the server anyways, does the server
learn anything from the client sending it a GOAWAY?

https://codereview.chromium.org/1327923002/diff/130001/net/quic/quic_stream_f...
net/quic/quic_stream_factory.cc:1228:
CreateDatagramClientSocket(all_sessions_[session], session->net_log());
On 2015/11/18 22:23:36, Ryan Hamilton wrote:
> instead of all_sessions_[session] can you call session_-server_id()?

Ah! Didn't know that existed. Done.

https://codereview.chromium.org/1327923002/diff/130001/net/quic/quic_stream_f...
net/quic/quic_stream_factory.cc:1230: connection->peer_address(), network);
On 2015/11/18 22:23:36, Ryan Hamilton wrote:
> It looks like both places this method is called is immediately after
> CreateDatagramClientSocket(). How about folding the configuration into Create?

I had it together first (some earlier rev) but I needed an "out" param in
addition to the return value for either the scoped socket ptr or for rv. Since
it's unintuitive to have rv as an out param, and chrome style guide explicitly
calls out use of scoped_ptr*, I split this call up.

https://codereview.chromium.org/1327923002/diff/130001/net/quic/quic_stream_f...
net/quic/quic_stream_factory.cc:1231: if (rv != OK) {
On 2015/11/18 22:23:36, Ryan Hamilton wrote:
> nit: no need to store rv in a variable if it's not used after this:
> 
> if (Configure() != OK) { ...}

Done.

https://codereview.chromium.org/1327923002/diff/130001/net/quic/quic_stream_f...
net/quic/quic_stream_factory.cc:1387: socket.get(), addr,
NetworkChangeNotifier::kInvalidNetworkHandle);
On 2015/11/18 22:23:36, Ryan Hamilton wrote:
> PLease add a comment here about using an "invalid" network handle since that
> sounds ... bad :>

added a comment here and in the .h declaration of ConfigureDatagramClientSocket.

https://codereview.chromium.org/1327923002/diff/130001/net/udp/datagram_clien...
File net/udp/datagram_client_socket.h (right):

https://codereview.chromium.org/1327923002/diff/130001/net/udp/datagram_clien...
net/udp/datagram_client_socket.h:30: virtual int BindToDefaultNetwork() = 0;
On 2015/11/18 22:23:36, Ryan Hamilton wrote:
> Are there other subclasses of this interface which you need to add this method
> to?

There are two test classes that do need impls, and I'll need them when I start
writing tests. If Paul's CL
(https://codereview.chromium.org/1416213003/) doesn't land, I'll add those
temporarily in this CL to make progress (sorry for the mish-mash of CLs)

https://codereview.chromium.org/1327923002/diff/130001/net/udp/udp_client_soc...
File net/udp/udp_client_socket.cc (right):

https://codereview.chromium.org/1327923002/diff/130001/net/udp/udp_client_soc...
net/udp/udp_client_socket.cc:30: return BindToNetwork(network);
On 2015/11/19 15:25:43, pauljensen wrote:
> On 2015/11/18 22:23:36, Ryan Hamilton wrote:
> > I'm not sure there is much benefit to this method since it seems to be
> > equivalent to:
> > 
> > socket->BindToNetwork(NetworkChangeNotifier::GetDefaultNetwork());
> > 
> > I think it would be cleaner to just use the method. It also means that in
> > QuicStreamFactory instead of passing in kInvalid and then checking to see if
> > network = kInvalue and then possibly calling this method, you could just
pass
> > NetworkChangeNotifier::GetDefaultNetwork() into the Create socket method.
> 
> Jana's implementation and Ryan's suggestion both suffer from a race between
> getting the default and binding to it, and the default changing.  This was the
> point of my
>
https://codereview.chromium.org/1416213003/diff/1/net/udp/udp_client_socket.c...

What Paul said. Paul CL will be submitted before mine, so I will use whatever
that CL has.

[Jana, 2015-11-26 02:01:59]

Read errors are now caught correctly, and require_confirmation is set when the
network is disconnected. Still left to do: (i) Tests, (ii) connection migration
error codes, and (iii) histograms.

[Ryan Hamilton, 2015-12-18 19:45:48]

On 2015/11/26 02:01:59, Jana wrote:
> Read errors are now caught correctly, and require_confirmation is set when the
> network is disconnected. Still left to do: (i) Tests, (ii) connection
migration
> error codes, and (iii) histograms.

ping

[Jana, 2015-12-18 20:46:40]

This CL is ready for review. There are a lot of small pieces, and I am happy to
chunk this up into pieces if it helps, esp. NetworkChangeNotifier-related
changes separated from QUIC changes and histogram additions.

Thanks so much for your patience and for working through this tangled web.

[Ryan Hamilton, 2015-12-18 21:51:24]

I think this looks quite plausible. I think I'd break it into a few changes:

1) NCN
2) DatagramClientSocket
3) QuicPacketReader
4) Everything else which is basically the QuicStreamFactory.

https://codereview.chromium.org/1327923002/diff/270001/net/quic/quic_packet_r...
File net/quic/quic_packet_reader.h (right):

https://codereview.chromium.org/1327923002/diff/270001/net/quic/quic_packet_r...
net/quic/quic_packet_reader.h:33: const DatagramClientSocket* socket) = 0;
I'd probably break this out, since it should be easy to do so.

https://codereview.chromium.org/1327923002/diff/270001/net/quic/quic_protocol.h
File net/quic/quic_protocol.h (right):

https://codereview.chromium.org/1327923002/diff/270001/net/quic/quic_protocol...
net/quic/quic_protocol.h:638: QUIC_IP_ADDRESS_CHANGED = 77,
Do we expect this to go away in favor of the MIGRATION ones?

https://codereview.chromium.org/1327923002/diff/270001/net/quic/quic_protocol...
net/quic/quic_protocol.h:642: QUIC_CONNECTION_MIGRATION_NO_MIGRATABLE_STREAMS =
78,
How different is this from IP_ADDRESS_CHANGED?

https://codereview.chromium.org/1327923002/diff/270001/net/quic/quic_stream_f...
File net/quic/quic_stream_factory.cc (right):

https://codereview.chromium.org/1327923002/diff/270001/net/quic/quic_stream_f...
net/quic/quic_stream_factory.cc:631:
migrate_sessions_on_network_change_(migrate_sessions_on_network_change),
Presumably we don't want both close_ and migrate_ to be true at the same time?
We could use a tri-state enum for this. Or in the worst case DCHECK(!(close_ &&
migrate_));

https://codereview.chromium.org/1327923002/diff/270001/net/quic/quic_stream_f...
net/quic/quic_stream_factory.cc:671:
NetworkChangeNotifier::AreNetworkHandlesSupported()) {
This is basically Android only currently?

https://codereview.chromium.org/1327923002/diff/270001/net/quic/quic_stream_f...
net/quic/quic_stream_factory.cc:1118: void
QuicStreamFactory::CloseAllSessions(int error) {
I'd be inclined to remove this method and just use your new one instead.

https://codereview.chromium.org/1327923002/diff/270001/net/quic/quic_stream_f...
net/quic/quic_stream_factory.cc:1188: // Find a new network that old sockets can
be migrated to.
nit: s/sockets/connections/?

https://codereview.chromium.org/1327923002/diff/270001/net/quic/quic_stream_f...
net/quic/quic_stream_factory.cc:1197: new_network = n;
Can you break; here?

for (...) {
  if (n != network) {
    new_newtwork = n;
    break;
  }
}

https://codereview.chromium.org/1327923002/diff/270001/net/quic/quic_stream_f...
net/quic/quic_stream_factory.cc:1202: QUIC_CONNECTION_MIGRATION_NO_NEW_NETWORK);
It seems sad to do this in the "SoonToDisconnect" case, doesn't it?

https://codereview.chromium.org/1327923002/diff/270001/net/quic/quic_stream_f...
net/quic/quic_stream_factory.cc:1212: if (session->GetNumActiveStreams() == 0) {
Ah, that's why you were needing this method.

https://codereview.chromium.org/1327923002/diff/270001/net/quic/quic_stream_f...
net/quic/quic_stream_factory.cc:1218: MIGRATION_STATUS_MAX);
Please factor this out into a helper method which simply takes a single enum as
an argument.

https://codereview.chromium.org/1327923002/diff/270001/net/quic/quic_stream_f...
net/quic/quic_stream_factory.cc:1235: // being migrated and not being newly
created.
I don't understand this comment.

https://codereview.chromium.org/1327923002/diff/270001/net/quic/quic_stream_f...
net/quic/quic_stream_factory.cc:1317: rv = socket->BindToDefaultNetwork();
Does this represent a behavior change? Is "BindToDefault" different from not
doing BindToDefault()?

https://codereview.chromium.org/1327923002/diff/270001/net/quic/quic_stream_f...
net/quic/quic_stream_factory.cc:1323: }
nit: no {}s

https://codereview.chromium.org/1327923002/diff/270001/net/udp/datagram_clien...
File net/udp/datagram_client_socket.h (right):

https://codereview.chromium.org/1327923002/diff/270001/net/udp/datagram_clien...
net/udp/datagram_client_socket.h:35: virtual
NetworkChangeNotifier::NetworkHandle GetBoundNetwork() const = 0;
I'd definitely break this out, if possible.

[Ryan Hamilton, 2015-12-18 22:00:19]

https://codereview.chromium.org/1327923002/diff/270001/net/quic/quic_chromium...
File net/quic/quic_chromium_client_session.cc (right):

https://codereview.chromium.org/1327923002/diff/270001/net/quic/quic_chromium...
net/quic/quic_chromium_client_session.cc:776: for (auto& socket : sockets_) {
nit: I'd use the full type here. (no need for {}s)

https://codereview.chromium.org/1327923002/diff/270001/net/quic/quic_chromium...
net/quic/quic_chromium_client_session.cc:826: }
nit: no need for {}s

https://codereview.chromium.org/1327923002/diff/270001/net/quic/quic_chromium...
net/quic/quic_chromium_client_session.cc:928: // TODO(jri): Maybe clean up old
sockets on error.
Do we stop reading on the socket in this case?

https://codereview.chromium.org/1327923002/diff/270001/net/quic/quic_chromium...
net/quic/quic_chromium_client_session.cc:999: bool
QuicChromiumClientSession::MigrateToSocket(
I think we should probably have tests for this new method.

https://codereview.chromium.org/1327923002/diff/270001/net/quic/quic_chromium...
net/quic/quic_chromium_client_session.cc:1007: // TODO(jri): Make
SetQuicPacketWriter take a scoped_ptr.
I don't think that's possible because the connection does not always own the
writer?

https://codereview.chromium.org/1327923002/diff/270001/net/quic/quic_chromium...
net/quic/quic_chromium_client_session.cc:1008:
connection()->SetQuicPacketWriter(writer.release(), /*owns_writer=*/true);
So all new data will go out over this new writer, right?

https://codereview.chromium.org/1327923002/diff/270001/net/quic/quic_stream_f...
File net/quic/quic_stream_factory_test.cc (right):

https://codereview.chromium.org/1327923002/diff/270001/net/quic/quic_stream_f...
net/quic/quic_stream_factory_test.cc:160: mock_network_change_notifier_ = new
MockNetworkChangeNotifier();
I think you can initialize these in the initializer list instead of the body.

https://codereview.chromium.org/1327923002/diff/270001/net/quic/quic_stream_f...
net/quic/quic_stream_factory_test.cc:165: delete
disable_network_change_notifier_for_tests_;
Can you use a scoped_ptr<> instead of needing to explicitly delete them?

https://codereview.chromium.org/1327923002/diff/270001/net/quic/quic_stream_f...
net/quic/quic_stream_factory_test.cc:342: // For thread safety, the NCN queues
tasks to do the actual notifications,
I'd remove the first part of this (about thread safety) and just say:

// Spin the message loop so notifications are delivered.

Or actually, that method you're calling is ForTests(). Can it just spin the
message loop for you?

https://codereview.chromium.org/1327923002/diff/270001/net/quic/quic_stream_f...
net/quic/quic_stream_factory_test.cc:1367: TEST_P(QuicStreamFactoryTest,
OnNetworkChange) {
Does this test all of the various combinations? Active streams vs no active
streams. New network vs no new network, etc...

[Jana, 2015-12-21 23:10:20]

Thanks, Ryan. I've addressed most comments below, and I will add tests to the
split out CL. I'll start splitting the code out now.

https://codereview.chromium.org/1327923002/diff/270001/net/quic/quic_packet_r...
File net/quic/quic_packet_reader.h (right):

https://codereview.chromium.org/1327923002/diff/270001/net/quic/quic_packet_r...
net/quic/quic_packet_reader.h:33: const DatagramClientSocket* socket) = 0;
On 2015/12/18 21:51:23, Ryan Hamilton wrote:
> I'd probably break this out, since it should be easy to do so.

Acknowledged.

https://codereview.chromium.org/1327923002/diff/270001/net/quic/quic_protocol.h
File net/quic/quic_protocol.h (right):

https://codereview.chromium.org/1327923002/diff/270001/net/quic/quic_protocol...
net/quic/quic_protocol.h:638: QUIC_IP_ADDRESS_CHANGED = 77,
On 2015/12/18 21:51:23, Ryan Hamilton wrote:
> Do we expect this to go away in favor of the MIGRATION ones?

Eventually, on Android devices. But we don't yet have the MIGRATION
notifications for non-Android devices, so this will remain around for the
platforms where IPAddressChanged exists. I know that set is larger than
{Android}, but I don't know what else it contains.

https://codereview.chromium.org/1327923002/diff/270001/net/quic/quic_protocol...
net/quic/quic_protocol.h:642: QUIC_CONNECTION_MIGRATION_NO_MIGRATABLE_STREAMS =
78,
On 2015/12/18 21:51:23, Ryan Hamilton wrote:
> How different is this from IP_ADDRESS_CHANGED?

This is when migration could have happened, but didn't because there were no
"active" streams. It's different in that IP_ADDRESS_CHANGED means active streams
may have been reset, but here, there were no active streams. Make sense?

https://codereview.chromium.org/1327923002/diff/270001/net/quic/quic_stream_f...
File net/quic/quic_stream_factory.cc (right):

https://codereview.chromium.org/1327923002/diff/270001/net/quic/quic_stream_f...
net/quic/quic_stream_factory.cc:631:
migrate_sessions_on_network_change_(migrate_sessions_on_network_change),
On 2015/12/18 21:51:23, Ryan Hamilton wrote:
> Presumably we don't want both close_ and migrate_ to be true at the same time?
> We could use a tri-state enum for this. Or in the worst case DCHECK(!(close_
&&
> migrate_));

Hmm -- yes, I think we'd want them to be mutually exclusive until we decide what
default behaviors we want. Given that there's mutual exclusion guaranteed below
by only registering for one of the two observers below, I'm happy with adding a
DCHECK here (done below). SG?

https://codereview.chromium.org/1327923002/diff/270001/net/quic/quic_stream_f...
net/quic/quic_stream_factory.cc:671:
NetworkChangeNotifier::AreNetworkHandlesSupported()) {
On 2015/12/18 21:51:23, Ryan Hamilton wrote:
> This is basically Android only currently?

Yes, Android >= L

https://codereview.chromium.org/1327923002/diff/270001/net/quic/quic_stream_f...
net/quic/quic_stream_factory.cc:1118: void
QuicStreamFactory::CloseAllSessions(int error) {
On 2015/12/18 21:51:23, Ryan Hamilton wrote:
> I'd be inclined to remove this method and just use your new one instead.

Done.

https://codereview.chromium.org/1327923002/diff/270001/net/quic/quic_stream_f...
net/quic/quic_stream_factory.cc:1188: // Find a new network that old sockets can
be migrated to.
On 2015/12/18 21:51:23, Ryan Hamilton wrote:
> nit: s/sockets/connections/?

Done.

https://codereview.chromium.org/1327923002/diff/270001/net/quic/quic_stream_f...
net/quic/quic_stream_factory.cc:1197: new_network = n;
On 2015/12/18 21:51:23, Ryan Hamilton wrote:
> Can you break; here?
> 
> for (...) {
>   if (n != network) {
>     new_newtwork = n;
>     break;
>   }
> }

Done.

https://codereview.chromium.org/1327923002/diff/270001/net/quic/quic_stream_f...
net/quic/quic_stream_factory.cc:1202: QUIC_CONNECTION_MIGRATION_NO_NEW_NETWORK);
On 2015/12/18 21:51:23, Ryan Hamilton wrote:
> It seems sad to do this in the "SoonToDisconnect" case, doesn't it?

Yes, it is a bit aggressive. I had two reasons for doing the mgiration here:
(i) This gives us some "drain time" to drain the old network., since the client
continues reading from the old socket. The client does not have another signal
later before "Disconnected", when migration may be not as seamless, since any
packets still inflight to the client from the server over the old network will
have to be retransmitted by the server over the new network, possibly after a
TLP period.
(ii) This signal is delivered when we're moving from Cellular to WiFi, and I
hypothesized that the sooner we migrate to WiFi, the better.

Do you have any other suggestions?

https://codereview.chromium.org/1327923002/diff/270001/net/quic/quic_stream_f...
net/quic/quic_stream_factory.cc:1212: if (session->GetNumActiveStreams() == 0) {
On 2015/12/18 21:51:23, Ryan Hamilton wrote:
> Ah, that's why you were needing this method.

Acknowledged.

https://codereview.chromium.org/1327923002/diff/270001/net/quic/quic_stream_f...
net/quic/quic_stream_factory.cc:1218: MIGRATION_STATUS_MAX);
On 2015/12/18 21:51:23, Ryan Hamilton wrote:
> Please factor this out into a helper method which simply takes a single enum
as
> an argument.

I thought of doing this too simply to reduce clutter, but it seemed an overkill
(3 args vs. 1 arg). If you have the same reaction, I'll do it :-)

https://codereview.chromium.org/1327923002/diff/270001/net/quic/quic_stream_f...
net/quic/quic_stream_factory.cc:1218: MIGRATION_STATUS_MAX);
On 2015/12/18 21:51:23, Ryan Hamilton wrote:
> Please factor this out into a helper method which simply takes a single enum
as
> an argument.

Done.

https://codereview.chromium.org/1327923002/diff/270001/net/quic/quic_stream_f...
net/quic/quic_stream_factory.cc:1235: // being migrated and not being newly
created.
On 2015/12/18 21:51:23, Ryan Hamilton wrote:
> I don't understand this comment.

The point of the PortSuggester, AIUI, is to use a port for a new 0RTT connect so
that it maps to the same GFE as previously, to possibly increase 0RTT chances.
Given that the IP is changing, retaining the same port as earlier shouldn't get
us anything... right?

Modifying the comment per our conversation.

https://codereview.chromium.org/1327923002/diff/270001/net/quic/quic_stream_f...
net/quic/quic_stream_factory.cc:1317: rv = socket->BindToDefaultNetwork();
On 2015/12/18 21:51:23, Ryan Hamilton wrote:
> Does this represent a behavior change? Is "BindToDefault" different from not
> doing BindToDefault()?

Yes. This is a behavior change in that there was no explicit or implicit
bind()ing before.

https://codereview.chromium.org/1327923002/diff/270001/net/quic/quic_stream_f...
net/quic/quic_stream_factory.cc:1323: }
On 2015/12/18 21:51:23, Ryan Hamilton wrote:
> nit: no {}s

I can never keep this together in my mind. Done.

https://codereview.chromium.org/1327923002/diff/270001/net/quic/quic_stream_f...
File net/quic/quic_stream_factory_test.cc (right):

https://codereview.chromium.org/1327923002/diff/270001/net/quic/quic_stream_f...
net/quic/quic_stream_factory_test.cc:160: mock_network_change_notifier_ = new
MockNetworkChangeNotifier();
On 2015/12/18 22:00:19, Ryan Hamilton wrote:
> I think you can initialize these in the initializer list instead of the body.

Done.

https://codereview.chromium.org/1327923002/diff/270001/net/quic/quic_stream_f...
net/quic/quic_stream_factory_test.cc:165: delete
disable_network_change_notifier_for_tests_;
On 2015/12/18 22:00:19, Ryan Hamilton wrote:
> Can you use a scoped_ptr<> instead of needing to explicitly delete them?

Done.

https://codereview.chromium.org/1327923002/diff/270001/net/quic/quic_stream_f...
net/quic/quic_stream_factory_test.cc:342: // For thread safety, the NCN queues
tasks to do the actual notifications,
On 2015/12/18 22:00:19, Ryan Hamilton wrote:
> I'd remove the first part of this (about thread safety) and just say:
> 
> // Spin the message loop so notifications are delivered.
> 
> Or actually, that method you're calling is ForTests(). Can it just spin the
> message loop for you?

I'm following the same pattern as is done with delivering NCN notifications for
tests elsewhere as well. (I've modified the comment there too)

https://codereview.chromium.org/1327923002/diff/270001/net/quic/quic_stream_f...
net/quic/quic_stream_factory_test.cc:1367: TEST_P(QuicStreamFactoryTest,
OnNetworkChange) {
On 2015/12/18 22:00:19, Ryan Hamilton wrote:
> Does this test all of the various combinations? Active streams vs no active
> streams. New network vs no new network, etc...

No it doesn't. I had meant to add them before sending out for review, but
clearly I didn't. I'll add them in the spit out CL.

https://codereview.chromium.org/1327923002/diff/270001/net/udp/datagram_clien...
File net/udp/datagram_client_socket.h (right):

https://codereview.chromium.org/1327923002/diff/270001/net/udp/datagram_clien...
net/udp/datagram_client_socket.h:35: virtual
NetworkChangeNotifier::NetworkHandle GetBoundNetwork() const = 0;
On 2015/12/18 21:51:24, Ryan Hamilton wrote:
> I'd definitely break this out, if possible.

Acknowledged.

[Ryan Hamilton, 2015-12-21 23:18:19]

https://codereview.chromium.org/1327923002/diff/270001/net/quic/quic_stream_f...
File net/quic/quic_stream_factory.cc (right):

https://codereview.chromium.org/1327923002/diff/270001/net/quic/quic_stream_f...
net/quic/quic_stream_factory.cc:1317: rv = socket->BindToDefaultNetwork();
On 2015/12/21 23:10:19, Jana wrote:
> On 2015/12/18 21:51:23, Ryan Hamilton wrote:
> > Does this represent a behavior change? Is "BindToDefault" different from not
> > doing BindToDefault()?
> 
> Yes. This is a behavior change in that there was no explicit or implicit
> bind()ing before.

I guess it probably doesn't matter, but I wonder if we want to "bind to default"
when we're not doing migration? Thinking about non-Android... Maybe it doesn't
matter?

[Jana, 2015-12-22 00:02:34]

On 2015/12/21 23:18:19, Ryan Hamilton wrote:
>
https://codereview.chromium.org/1327923002/diff/270001/net/quic/quic_stream_f...
> File net/quic/quic_stream_factory.cc (right):
> 
>
https://codereview.chromium.org/1327923002/diff/270001/net/quic/quic_stream_f...
> net/quic/quic_stream_factory.cc:1317: rv = socket->BindToDefaultNetwork();
> On 2015/12/21 23:10:19, Jana wrote:
> > On 2015/12/18 21:51:23, Ryan Hamilton wrote:
> > > Does this represent a behavior change? Is "BindToDefault" different from
not
> > > doing BindToDefault()?
> > 
> > Yes. This is a behavior change in that there was no explicit or implicit
> > bind()ing before.
> 
> I guess it probably doesn't matter, but I wonder if we want to "bind to
default"
> when we're not doing migration? Thinking about non-Android... Maybe it doesn't
> matter?

It's a NOP on non-Android platforms and on Android < L.
(https://code.google.com/p/chromium/codesearch#chromium/src/net/udp/udp_socket...).

[Jana, 2015-12-28 20:50:44]

On 2015/12/22 00:02:34, Jana wrote:
> On 2015/12/21 23:18:19, Ryan Hamilton wrote:
> >
>
https://codereview.chromium.org/1327923002/diff/270001/net/quic/quic_stream_f...
> > File net/quic/quic_stream_factory.cc (right):
> > 
> >
>
https://codereview.chromium.org/1327923002/diff/270001/net/quic/quic_stream_f...
> > net/quic/quic_stream_factory.cc:1317: rv = socket->BindToDefaultNetwork();
> > On 2015/12/21 23:10:19, Jana wrote:
> > > On 2015/12/18 21:51:23, Ryan Hamilton wrote:
> > > > Does this represent a behavior change? Is "BindToDefault" different from
> not
> > > > doing BindToDefault()?
> > > 
> > > Yes. This is a behavior change in that there was no explicit or implicit
> > > bind()ing before.
> > 
> > I guess it probably doesn't matter, but I wonder if we want to "bind to
> default"
> > when we're not doing migration? Thinking about non-Android... Maybe it
doesn't
> > matter?
> 
> It's a NOP on non-Android platforms and on Android < L.
>
(https://code.google.com/p/chromium/codesearch#chromium/src/net/udp/udp_socket...).

I've modified the code to now call it only if migrate_sessions_ is true, since
I'd prefer to not continue when we expect BindToDefaultNetwork to work but it
fails. Currently, it will return ERR_NOT_IMPLEMENTED when there's no platform
support, so it's now called only when the platform is expected to support it.

[Jana, 2015-12-29 01:50:17]

Ryan: I've added all the tests I wanted to, and I've changed behavior to allow
active sessions to continue using the old network when there is no new network
to migrate to. The CL is ready for review -- PTAL. Thanks for waiting patiently
while I worked through these CLs.

[Jana, 2015-12-29 01:53:42]

Description was changed from

==========
Initial cut at migrating connection to new socket when IP address of client
changes. This is an initial CL for feedback. Tests are yet to be added.

BUG=
==========

to

==========
Adds connection migration capability to QUIC. This CL causes QUIC connections to
migrate to a new network when an old network is disconnected or about to be
disconnected. These notifications are available for Android >= L, so QUIC
connection migration will only work for Android >= L.

BUG=
==========

[Ryan Hamilton, 2015-12-29 05:29:36]

Sweet! Just a few comments. Still need to look at the tests.

https://codereview.chromium.org/1327923002/diff/370001/net/quic/quic_stream_f...
File net/quic/quic_stream_factory.cc (right):

https://codereview.chromium.org/1327923002/diff/370001/net/quic/quic_stream_f...
net/quic/quic_stream_factory.cc:1183: void
QuicStreamFactory::OnNetworkSoonToDisconnect(
Can you add a comment here about this method not being called when going from
WiFi to cellular. (Or perhaps a more general comment)

https://codereview.chromium.org/1327923002/diff/370001/net/quic/quic_stream_f...
net/quic/quic_stream_factory.cc:1195:
NetworkChangeNotifier::GetConnectedNetworks(&network_list);
(Why does this method not return a NetworkList?)

https://codereview.chromium.org/1327923002/diff/370001/net/quic/quic_stream_f...
net/quic/quic_stream_factory.cc:1211: if
(session->GetDefaultSocket()->GetBoundNetwork() != network) {
It looks like this is the only non-private caller of GetDefaultSocket(). If so,
the Law of Demeter would suggest that instead of GetDefaultSocket, the session
should expose IsBoundToNetwork() (or perhaps something about Current or
Default). Admittedly this is not the most important thing in the world, but
still :>

https://en.wikipedia.org/wiki/Law_of_Demeter
"That is, the code a.b.Method() breaks the law where a.Method() does not."

https://codereview.chromium.org/1327923002/diff/370001/net/quic/quic_stream_f...
File net/quic/quic_stream_factory.h (right):

https://codereview.chromium.org/1327923002/diff/370001/net/quic/quic_stream_f...
net/quic/quic_stream_factory.h:218: int ConfigureDatagramClientSocket(
nit: I think you can omit "DatagramClient" from the name of this method. The
first argument (and your comment) make it clear that it only works on
DatagramClientSockets.

https://codereview.chromium.org/1327923002/diff/370001/net/quic/quic_stream_f...
File net/quic/quic_stream_factory_test.cc (right):

https://codereview.chromium.org/1327923002/diff/370001/net/quic/quic_stream_f...
net/quic/quic_stream_factory_test.cc:50: extern
NetworkChangeNotifier::NetworkHandle kDefaultNetworkForTests;
Is this coming from socket_test_utils.cc? If so, I think the declaration should
live in socket_test_utils.h

https://codereview.chromium.org/1327923002/diff/370001/net/socket/socket_test...
File net/socket/socket_test_util.cc (right):

https://codereview.chromium.org/1327923002/diff/370001/net/socket/socket_test...
net/socket/socket_test_util.cc:39: NET_EXPORT
NetworkChangeNotifier::NetworkHandle kNewNetworkForTests = 2;
Is this used?

[Ryan Hamilton, 2015-12-29 21:18:55]

https://codereview.chromium.org/1327923002/diff/370001/net/quic/quic_stream_f...
File net/quic/quic_stream_factory_test.cc (right):

https://codereview.chromium.org/1327923002/diff/370001/net/quic/quic_stream_f...
net/quic/quic_stream_factory_test.cc:139: void
SetConnectedNetworksList(NetworkList network_list) {
nit: const &

[Jana, 2016-01-06 23:01:17]

Thanks much Ryan. I've added tests to stream_factory and to
chromium_client_session (it took a while to get it all together), PTAL. I've
also addressed your comments.

(I seem to be having some linker trouble with the kDefaultNetworkForTests and
kNewNetworkForTests. Sending this along anyways, since the bulk of the new
code/tests needs review anyways, and I need to leave.)

https://codereview.chromium.org/1327923002/diff/370001/net/quic/quic_stream_f...
File net/quic/quic_stream_factory.cc (right):

https://codereview.chromium.org/1327923002/diff/370001/net/quic/quic_stream_f...
net/quic/quic_stream_factory.cc:1183: void
QuicStreamFactory::OnNetworkSoonToDisconnect(
On 2015/12/29 05:29:36, Ryan Hamilton wrote:
> Can you add a comment here about this method not being called when going from
> WiFi to cellular. (Or perhaps a more general comment)

I've copied comments from the network change notifier.

https://codereview.chromium.org/1327923002/diff/370001/net/quic/quic_stream_f...
net/quic/quic_stream_factory.cc:1195:
NetworkChangeNotifier::GetConnectedNetworks(&network_list);
On 2015/12/29 05:29:36, Ryan Hamilton wrote:
> (Why does this method not return a NetworkList?)

Not sure... I don't remember why this was designed this way. Paul?

https://codereview.chromium.org/1327923002/diff/370001/net/quic/quic_stream_f...
net/quic/quic_stream_factory.cc:1211: if
(session->GetDefaultSocket()->GetBoundNetwork() != network) {
On 2015/12/29 05:29:36, Ryan Hamilton wrote:
> It looks like this is the only non-private caller of GetDefaultSocket(). If
so,
> the Law of Demeter would suggest that instead of GetDefaultSocket, the session
> should expose IsBoundToNetwork() (or perhaps something about Current or
> Default). Admittedly this is not the most important thing in the world, but
> still :>
> 
> https://en.wikipedia.org/wiki/Law_of_Demeter
> "That is, the code a.b.Method() breaks the law where a.Method() does not."

Noted. GetDefaultSocket() is used directly elsewhere now in the session, so
leaving it as is.

https://codereview.chromium.org/1327923002/diff/370001/net/quic/quic_stream_f...
File net/quic/quic_stream_factory.h (right):

https://codereview.chromium.org/1327923002/diff/370001/net/quic/quic_stream_f...
net/quic/quic_stream_factory.h:218: int ConfigureDatagramClientSocket(
On 2015/12/29 05:29:36, Ryan Hamilton wrote:
> nit: I think you can omit "DatagramClient" from the name of this method. The
> first argument (and your comment) make it clear that it only works on
> DatagramClientSockets.

Done.

https://codereview.chromium.org/1327923002/diff/370001/net/quic/quic_stream_f...
File net/quic/quic_stream_factory_test.cc (right):

https://codereview.chromium.org/1327923002/diff/370001/net/quic/quic_stream_f...
net/quic/quic_stream_factory_test.cc:50: extern
NetworkChangeNotifier::NetworkHandle kDefaultNetworkForTests;
On 2015/12/29 05:29:36, Ryan Hamilton wrote:
> Is this coming from socket_test_utils.cc? If so, I think the declaration
should
> live in socket_test_utils.h

Done.

https://codereview.chromium.org/1327923002/diff/370001/net/quic/quic_stream_f...
net/quic/quic_stream_factory_test.cc:139: void
SetConnectedNetworksList(NetworkList network_list) {
On 2015/12/29 21:18:55, Ryan Hamilton wrote:
> nit: const &

Done.

https://codereview.chromium.org/1327923002/diff/370001/net/socket/socket_test...
File net/socket/socket_test_util.cc (right):

https://codereview.chromium.org/1327923002/diff/370001/net/socket/socket_test...
net/socket/socket_test_util.cc:39: NET_EXPORT
NetworkChangeNotifier::NetworkHandle kNewNetworkForTests = 2;
On 2015/12/29 05:29:36, Ryan Hamilton wrote:
> Is this used?

It's used in the tests, but I declared it here with the other constant for
context.

[Ryan Hamilton, 2016-01-08 05:01:06]

https://codereview.chromium.org/1327923002/diff/370001/net/quic/quic_stream_f...
File net/quic/quic_stream_factory.cc (right):

https://codereview.chromium.org/1327923002/diff/370001/net/quic/quic_stream_f...
net/quic/quic_stream_factory.cc:1183: void
QuicStreamFactory::OnNetworkSoonToDisconnect(
On 2016/01/06 23:01:17, Jana wrote:
> On 2015/12/29 05:29:36, Ryan Hamilton wrote:
> > Can you add a comment here about this method not being called when going
from
> > WiFi to cellular. (Or perhaps a more general comment)
> 
> I've copied comments from the network change notifier.

Perhaps I'm not reading carefully, but I don't seen anything (even in the .h
file) which says that it won't be called when going from WiFi to cellular. I'd
be inclined to add a comment here (in the .cc file, near the implementation of
this method) as a reminder to future readers.

https://codereview.chromium.org/1327923002/diff/430001/net/quic/quic_chromium...
File net/quic/quic_chromium_client_session_test.cc (right):

https://codereview.chromium.org/1327923002/diff/430001/net/quic/quic_chromium...
net/quic/quic_chromium_client_session_test.cc:90: std::move(socket_),
I'm a bit confused by this. If you move socket_ into session_, then socket_ will
be null. In that case, is socket_ still useful?

https://codereview.chromium.org/1327923002/diff/430001/net/quic/quic_chromium...
net/quic/quic_chromium_client_session_test.cc:176: MockRead reads_[1];
Does this need to be an array?

https://codereview.chromium.org/1327923002/diff/430001/net/quic/quic_chromium...
net/quic/quic_chromium_client_session_test.cc:363: maker_.MakePingPacket(1,
/*include_version=*/false));
Since this is the same packet as ping_in, you can just make a single packet and
use it in both the mock read and write.

https://codereview.chromium.org/1327923002/diff/430001/net/quic/quic_stream_f...
File net/quic/quic_stream_factory.h (right):

https://codereview.chromium.org/1327923002/diff/430001/net/quic/quic_stream_f...
net/quic/quic_stream_factory.h:254: // Called when |network| is made the default
network for communication.
nit: the style guide suggests that these comments go where the method is
defined, not where it is overridden.

https://codereview.chromium.org/1327923002/diff/430001/net/socket/socket_test...
File net/socket/socket_test_util.h (right):

https://codereview.chromium.org/1327923002/diff/430001/net/socket/socket_test...
net/socket/socket_test_util.h:50: NET_EXPORT
NetworkChangeNotifier::NetworkHandle kNewNetworkForTests = 2;
I suspect you want/need const for these.

https://codereview.chromium.org/1327923002/diff/490001/net/quic/quic_chromium...
File net/quic/quic_chromium_client_session_test.cc (right):

https://codereview.chromium.org/1327923002/diff/490001/net/quic/quic_chromium...
net/quic/quic_chromium_client_session_test.cc:180: const size_t kMaxOpenStreams
= session_.get_max_open_streams();
What's the story with this change?

https://codereview.chromium.org/1327923002/diff/490001/net/quic/quic_chromium...
net/quic/quic_chromium_client_session_test.cc:427: }
These tests look good! I think we also need to test read and write errors.
Errors on the old sockets are innocuous, but errors on the new sockets are
fatal, iirc.

[Jana, 2016-01-09 02:06:54]

Thanks, Ryan. I'll add socket error tests on Monday.

https://codereview.chromium.org/1327923002/diff/370001/net/quic/quic_stream_f...
File net/quic/quic_stream_factory.cc (right):

https://codereview.chromium.org/1327923002/diff/370001/net/quic/quic_stream_f...
net/quic/quic_stream_factory.cc:1183: void
QuicStreamFactory::OnNetworkSoonToDisconnect(
On 2016/01/08 05:01:06, Ryan Hamilton wrote:
> On 2016/01/06 23:01:17, Jana wrote:
> > On 2015/12/29 05:29:36, Ryan Hamilton wrote:
> > > Can you add a comment here about this method not being called when going
> from
> > > WiFi to cellular. (Or perhaps a more general comment)
> > 
> > I've copied comments from the network change notifier.
> 
> Perhaps I'm not reading carefully, but I don't seen anything (even in the .h
> file) which says that it won't be called when going from WiFi to cellular. I'd
> be inclined to add a comment here (in the .cc file, near the implementation of
> this method) as a reminder to future readers.

Done.

https://codereview.chromium.org/1327923002/diff/430001/net/quic/quic_chromium...
File net/quic/quic_chromium_client_session_test.cc (right):

https://codereview.chromium.org/1327923002/diff/430001/net/quic/quic_chromium...
net/quic/quic_chromium_client_session_test.cc:90: std::move(socket_),
On 2016/01/08 05:01:06, Ryan Hamilton wrote:
> I'm a bit confused by this. If you move socket_ into session_, then socket_
will
> be null. In that case, is socket_ still useful?

Not really -- it's initialized above and used here. I've moved this all into an
Initialize() method, which makes this all cleaner.

https://codereview.chromium.org/1327923002/diff/430001/net/quic/quic_chromium...
net/quic/quic_chromium_client_session_test.cc:176: MockRead reads_[1];
On 2016/01/08 05:01:06, Ryan Hamilton wrote:
> Does this need to be an array?

Good point -- changed to scoped_ptr

https://codereview.chromium.org/1327923002/diff/430001/net/quic/quic_chromium...
net/quic/quic_chromium_client_session_test.cc:363: maker_.MakePingPacket(1,
/*include_version=*/false));
On 2016/01/08 05:01:06, Ryan Hamilton wrote:
> Since this is the same packet as ping_in, you can just make a single packet
and
> use it in both the mock read and write.

Ah -- yes, of course. Left over from changing this a few times. Done.

https://codereview.chromium.org/1327923002/diff/430001/net/quic/quic_stream_f...
File net/quic/quic_stream_factory.h (right):

https://codereview.chromium.org/1327923002/diff/430001/net/quic/quic_stream_f...
net/quic/quic_stream_factory.h:254: // Called when |network| is made the default
network for communication.
On 2016/01/08 05:01:06, Ryan Hamilton wrote:
> nit: the style guide suggests that these comments go where the method is
> defined, not where it is overridden.

I removed the comments, since they were redundant. I have added the cellular ->
wifi comment at the definition, PTAL. I'm happy to add more comments where
appropriate, so let me know if you'd like me to add anything else here.

https://codereview.chromium.org/1327923002/diff/490001/net/quic/quic_chromium...
File net/quic/quic_chromium_client_session_test.cc (right):

https://codereview.chromium.org/1327923002/diff/490001/net/quic/quic_chromium...
net/quic/quic_chromium_client_session_test.cc:180: const size_t kMaxOpenStreams
= session_.get_max_open_streams();
On 2016/01/08 05:01:06, Ryan Hamilton wrote:
> What's the story with this change?

It's being set to kDefaultMaxNumStreams/2 in
MockCryptoClientStream::SetConfigNegotiated(), so when I added a mock cryptp
stream factory to the session, the session's max number of streams changed. I
figured since this test wasn't actually testing what the max was set to but that
the max was being respected, using the session's currently set max value made
more sense than a constant.

[Jana, 2016-01-11 22:59:34]

Ryan: I've added tests to verify correct behavior on socket read/write errors,
PTAL.

[Ryan Hamilton, 2016-01-12 20:13:01]

Looking GREAT! Only one more test and then I think you're good to go.

https://codereview.chromium.org/1327923002/diff/550001/net/quic/quic_chromium...
File net/quic/quic_chromium_client_session_test.cc (right):

https://codereview.chromium.org/1327923002/diff/550001/net/quic/quic_chromium...
net/quic/quic_chromium_client_session_test.cc:475: // Store old socket and
migrate session.
nit: I'd probably remove "Store old" since I think that describes what's
happening *in* MigrateToSocket?

https://codereview.chromium.org/1327923002/diff/550001/net/quic/quic_chromium...
net/quic/quic_chromium_client_session_test.cc:496: }
Great test!

I think we also need on which verifies that a read error on the old socket does
not close the connection.  It would be great if the test verified that it was
able to read data on the old socket after the migration, but then a read error
happened, and all was well.

https://codereview.chromium.org/1327923002/diff/550001/net/socket/socket_test...
File net/socket/socket_test_util.h (right):

https://codereview.chromium.org/1327923002/diff/550001/net/socket/socket_test...
net/socket/socket_test_util.h:49: NET_EXPORT_PRIVATE extern const
NetworkChangeNotifier::NetworkHandle
extern const is the magic? I hate C++

[Jana, 2016-01-12 22:04:33]

Thanks, Ryan. And thanks for pushing on the tests!

https://codereview.chromium.org/1327923002/diff/550001/net/quic/quic_chromium...
File net/quic/quic_chromium_client_session_test.cc (right):

https://codereview.chromium.org/1327923002/diff/550001/net/quic/quic_chromium...
net/quic/quic_chromium_client_session_test.cc:475: // Store old socket and
migrate session.
On 2016/01/12 20:13:00, Ryan Hamilton wrote:
> nit: I'd probably remove "Store old" since I think that describes what's
> happening *in* MigrateToSocket?

Ah -- sorry left over comment. Removed.

https://codereview.chromium.org/1327923002/diff/550001/net/quic/quic_chromium...
net/quic/quic_chromium_client_session_test.cc:496: }
On 2016/01/12 20:13:01, Ryan Hamilton wrote:
> Great test!
> 
> I think we also need on which verifies that a read error on the old socket
does
> not close the connection.  It would be great if the test verified that it was
> able to read data on the old socket after the migration, but then a read error
> happened, and all was well.

As discussed offline, this is being verified in this test.

https://codereview.chromium.org/1327923002/diff/550001/net/socket/socket_test...
File net/socket/socket_test_util.h (right):

https://codereview.chromium.org/1327923002/diff/550001/net/socket/socket_test...
net/socket/socket_test_util.h:49: NET_EXPORT_PRIVATE extern const
NetworkChangeNotifier::NetworkHandle
On 2016/01/12 20:13:01, Ryan Hamilton wrote:
> extern const is the magic? I hate C++

I think the extern was missing... I added the const.

[Ryan Hamilton, 2016-01-12 22:43:40]

lgtm

[Jana, 2016-01-13 02:05:18]

The CQ bit was checked by jri@chromium.org

[Jana, 2016-01-13 02:05:18]

The patchset sent to the CQ was uploaded after l-g-t-m from rch@chromium.org
Link to the patchset: https://codereview.chromium.org/1327923002/#ps750001
(title: " ")

[commit-bot: I haz the power, 2016-01-13 02:05:59]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1327923002/750001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1327923002/750001

[commit-bot: I haz the power, 2016-01-13 02:18:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-01-13 02:18:07]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Jana, 2016-01-13 02:22:31]

jri@chromium.org changed reviewers:
+ asvitkine@chromium.org, davidben@chromium.org

[Jana, 2016-01-13 02:22:32]

+asvitkine for histograms. +davidben for domain reliability.

[Jana, 2016-01-13 02:37:51]

Description was changed from

==========
Adds connection migration capability to QUIC. This CL causes QUIC connections to
migrate to a new network when an old network is disconnected or about to be
disconnected. These notifications are available for Android >= L, so QUIC
connection migration will only work for Android >= L.

BUG=
==========

to

==========
Adds connection migration capability to QUIC. This CL causes QUIC connections to
migrate to a new network when an old network is disconnected or about to be
disconnected. These notifications are available for Android >= L, so QUIC
connection migration will only work for Android >= L.

BUG=576998
==========

[Alexei Svitkine (slow), 2016-01-13 15:33:26]

histograms lgtm, didn't look at the rest

[davidben, 2016-01-13 17:51:45]

davidben@chromium.org changed reviewers:
+ ttuttle@chromium.org

[davidben, 2016-01-13 17:51:46]

+ttuttle who is probably better to review dom_rel stuff.

[Deprecated (see juliatuttle), 2016-01-13 22:11:03]

On 2016/01/13 17:51:46, davidben wrote:
> +ttuttle who is probably better to review dom_rel stuff.

components/domain_reliability lgtm.

[Jana, 2016-01-13 22:30:11]

Thanks much, Alexei and Julia!

[Ryan Hamilton, 2016-01-13 22:44:41]

recent changes lgtm

[Jana, 2016-01-13 23:05:20]

The CQ bit was checked by jri@chromium.org

[Jana, 2016-01-13 23:05:20]

The patchset sent to the CQ was uploaded after l-g-t-m from
asvitkine@chromium.org, ttuttle@chromium.org, rch@chromium.org
Link to the patchset: https://codereview.chromium.org/1327923002/#ps890001
(title: "Rebase.")

[commit-bot: I haz the power, 2016-01-13 23:10:10]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1327923002/890001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1327923002/890001

[commit-bot: I haz the power, 2016-01-14 01:20:51]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-01-14 01:20:52]

Try jobs failed on following builders:
  android_arm64_dbg_recipe on tryserver.chromium.android (JOB_TIMED_OUT, no
build URL)
  android_chromium_gn_compile_dbg on tryserver.chromium.android (JOB_TIMED_OUT,
no build URL)
  android_chromium_gn_compile_rel on tryserver.chromium.android (JOB_TIMED_OUT,
no build URL)
  android_clang_dbg_recipe on tryserver.chromium.android (JOB_TIMED_OUT, no
build URL)
  android_compile_dbg on tryserver.chromium.android (JOB_TIMED_OUT, no build
URL)
  cast_shell_android on tryserver.chromium.android (JOB_TIMED_OUT, no build URL)
  linux_android_rel_ng on tryserver.chromium.android (JOB_TIMED_OUT, no build
URL)
  cast_shell_linux on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  chromeos_amd64-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  chromeos_daisy_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  chromium_presubmit on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  linux_chromium_chromeos_compile_dbg_ng on tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  linux_chromium_clobber_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  linux_chromium_compile_dbg_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_TIMED_OUT, no build
URL)

[Jana, 2016-01-14 01:28:57]

The CQ bit was checked by jri@chromium.org

[commit-bot: I haz the power, 2016-01-14 01:31:08]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1327923002/890001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1327923002/890001

[Jana, 2016-01-14 02:17:01]

The CQ bit was checked by jri@chromium.org

[Jana, 2016-01-14 02:17:01]

The patchset sent to the CQ was uploaded after l-g-t-m from
asvitkine@chromium.org, ttuttle@chromium.org, rch@chromium.org
Link to the patchset: https://codereview.chromium.org/1327923002/#ps910001
(title: "Removing NET_EXPORT from vars in socket_test_util.h")

[commit-bot: I haz the power, 2016-01-14 02:18:20]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1327923002/910001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1327923002/910001

[Jana, 2016-01-14 02:38:55]

The CQ bit was checked by jri@chromium.org

[Jana, 2016-01-14 02:38:56]

The patchset sent to the CQ was uploaded after l-g-t-m from
asvitkine@chromium.org, ttuttle@chromium.org, rch@chromium.org
Link to the patchset: https://codereview.chromium.org/1327923002/#ps930001
(title: "Rebase.")

[commit-bot: I haz the power, 2016-01-14 02:40:05]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1327923002/930001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1327923002/930001

[commit-bot: I haz the power, 2016-01-14 03:18:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-01-14 03:18:49]

Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Jana, 2016-01-14 03:40:12]

The CQ bit was checked by jri@chromium.org

[Jana, 2016-01-14 03:40:13]

The patchset sent to the CQ was uploaded after l-g-t-m from
asvitkine@chromium.org, ttuttle@chromium.org, rch@chromium.org
Link to the patchset: https://codereview.chromium.org/1327923002/#ps950001
(title: "Rebase.")

[commit-bot: I haz the power, 2016-01-14 03:42:34]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1327923002/950001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1327923002/950001

[commit-bot: I haz the power, 2016-01-14 05:46:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-01-14 05:46:39]

Try jobs failed on following builders:
  android_arm64_dbg_recipe on tryserver.chromium.android (JOB_TIMED_OUT, no
build URL)
  android_chromium_gn_compile_dbg on tryserver.chromium.android (JOB_TIMED_OUT,
no build URL)
  android_chromium_gn_compile_rel on tryserver.chromium.android (JOB_TIMED_OUT,
no build URL)
  android_clang_dbg_recipe on tryserver.chromium.android (JOB_TIMED_OUT, no
build URL)
  android_compile_dbg on tryserver.chromium.android (JOB_TIMED_OUT, no build
URL)
  cast_shell_android on tryserver.chromium.android (JOB_TIMED_OUT, no build URL)

[Jana, 2016-01-14 06:10:35]

The CQ bit was checked by jri@chromium.org

[commit-bot: I haz the power, 2016-01-14 06:11:05]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1327923002/950001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1327923002/950001

[commit-bot: I haz the power, 2016-01-14 06:57:16]

Description was changed from

==========
Adds connection migration capability to QUIC. This CL causes QUIC connections to
migrate to a new network when an old network is disconnected or about to be
disconnected. These notifications are available for Android >= L, so QUIC
connection migration will only work for Android >= L.

BUG=576998
==========

to

==========
Adds connection migration capability to QUIC. This CL causes QUIC connections to
migrate to a new network when an old network is disconnected or about to be
disconnected. These notifications are available for Android >= L, so QUIC
connection migration will only work for Android >= L.

BUG=576998
==========

[commit-bot: I haz the power, 2016-01-14 06:57:17]

Committed patchset #49 (id:950001)

[commit-bot: I haz the power, 2016-01-14 06:58:43]

Description was changed from

==========
Adds connection migration capability to QUIC. This CL causes QUIC connections to
migrate to a new network when an old network is disconnected or about to be
disconnected. These notifications are available for Android >= L, so QUIC
connection migration will only work for Android >= L.

BUG=576998
==========

to

==========
Adds connection migration capability to QUIC. This CL causes QUIC connections to
migrate to a new network when an old network is disconnected or about to be
disconnected. These notifications are available for Android >= L, so QUIC
connection migration will only work for Android >= L.

BUG=576998

Committed: https://crrev.com/7e636640ca1dc9f71ebe8a746d1c9bf53c68a2f3
Cr-Commit-Position: refs/heads/master@{#369356}
==========

[commit-bot: I haz the power, 2016-01-14 06:58:45]

Patchset 49 (id:??) landed as
https://crrev.com/7e636640ca1dc9f71ebe8a746d1c9bf53c68a2f3
Cr-Commit-Position: refs/heads/master@{#369356}

[Jana, 2016-01-21 00:09:57]

Patchset #50 (id:970001) has been deleted