Fallback to IPv6 if IPv4 adb port not available.

Fallback to IPv6 if IPv4 adb port not available.

This make chromedriver works with android emulator in pure IPv6 env.

BUG=chromedriver:1515
Committed: https://crrev.com/1dceac70eca7453b57574a5ed9d91606e761bc58
Cr-Commit-Position: refs/heads/master@{#421212}

[lepton, 2016-09-20 00:33:19]

lepton@google.com changed reviewers:
+ samuong@chromium.org

[pmarks, 2016-09-20 00:42:13]

https://codereview.chromium.org/2349423003/diff/1/chrome/test/chromedriver/ne...
File chrome/test/chromedriver/net/adb_client_socket.cc (right):

https://codereview.chromium.org/2349423003/diff/1/chrome/test/chromedriver/ne...
chrome/test/chromedriver/net/adb_client_socket.cc:370: socket_.reset(new
net::TCPClientSocket(address_list, NULL, NULL,
It looks like TCPClientSocket already supports connecting to a list of IP
addresses.  Can you pass it a list instead of implementing your own fallback?

[lepton, 2016-09-20 08:04:12]

https://codereview.chromium.org/2349423003/diff/1/chrome/test/chromedriver/ne...
File chrome/test/chromedriver/net/adb_client_socket.cc (right):

https://codereview.chromium.org/2349423003/diff/1/chrome/test/chromedriver/ne...
chrome/test/chromedriver/net/adb_client_socket.cc:370: socket_.reset(new
net::TCPClientSocket(address_list, NULL, NULL,
On 2016/09/20 00:42:13, pmarks wrote:
> It looks like TCPClientSocket already supports connecting to a list of IP
> addresses.  Can you pass it a list instead of implementing your own fallback?

Done.

[samuong, 2016-09-21 15:07:34]

https://codereview.chromium.org/2349423003/diff/20001/chrome/test/chromedrive...
File chrome/test/chromedriver/net/adb_client_socket.cc (right):

https://codereview.chromium.org/2349423003/diff/20001/chrome/test/chromedrive...
chrome/test/chromedriver/net/adb_client_socket.cc:369:
net::IPAddress::IPv6Localhost()};
Would it be better to resolve "localhost"? On IPv4-only or IPv6-only hosts, this
will just resolve to 127.0.0.1 or ::1, instead of both. You could use
ResolveHost() from chrome/test/chromedriver/net/websocket.cc (but you'd need to
move it out of its anonymous namespace, maybe put it in
chrome/test/chromedriver/net/net_util.cc).

[chromium-reviews, 2016-09-21 19:20:48]

Hi Sam,

According to this page:
https://sites.google.com/a/google.com/ip-agnostic/topics/localhost

In a dualstack environment, it's implementation-defined whether localhost
will result in an IPv4 or IPv6 connection (or both).

So if we happen get an ipv6 address on a dual stack env,  then we could
 fail to connect to adb if that adb is just listening on Ipv4(that's the
current behavior). If we are fine with this and just require users to
change their configuration in this case. Then I can follow your suggestion,

What do you think?

On Wed, Sep 21, 2016 at 8:07 AM, <samuong@chromium.org> wrote:

>
> https://codereview.chromium.org/2349423003/diff/20001/
> chrome/test/chromedriver/net/adb_client_socket.cc
> File chrome/test/chromedriver/net/adb_client_socket.cc (right):
>
> https://codereview.chromium.org/2349423003/diff/20001/
> chrome/test/chromedriver/net/adb_client_socket.cc#newcode369
> chrome/test/chromedriver/net/adb_client_socket.cc:369:
> net::IPAddress::IPv6Localhost()};
> Would it be better to resolve "localhost"? On IPv4-only or IPv6-only
> hosts, this will just resolve to 127.0.0.1 or ::1, instead of both. You
> could use ResolveHost() from chrome/test/chromedriver/net/websocket.cc
> (but you'd need to move it out of its anonymous namespace, maybe put it
> in chrome/test/chromedriver/net/net_util.cc).
>
> https://codereview.chromium.org/2349423003/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[samuong, 2016-09-22 13:53:28]

I see, that is a case that I'd like to avoid. Given that this is the current
behavior of adb, we should reflect that in ChromeDriver. LGTM with one nit:
please add a comment explaining this.

[lepton, 2016-09-22 23:00:19]

On 2016/09/22 13:53:28, samuong wrote:
> I see, that is a case that I'd like to avoid. Given that this is the current
> behavior of adb, we should reflect that in ChromeDriver. LGTM with one nit:
> please add a comment explaining this.

done

[samuong, 2016-09-23 10:01:15]

https://codereview.chromium.org/2349423003/diff/40001/chrome/test/chromedrive...
File chrome/test/chromedriver/net/adb_client_socket.cc (right):

https://codereview.chromium.org/2349423003/diff/40001/chrome/test/chromedrive...
chrome/test/chromedriver/net/adb_client_socket.cc:369: // dual stack environment
while current adb (1.0.36) always listen on IPv4.
I'd suggest the following tweak to the wording to make it a bit clearer that adb
listens on IPv6 when running in an IPv6-only environment:

In a IPv4/IPv6 dual stack environment, getaddrinfo for localhost could only
return IPv6 address while the current adb (1.0.36) will always listen on IPv4.

[lepton, 2016-09-23 15:56:03]

On 2016/09/23 10:01:15, samuong wrote:
>
https://codereview.chromium.org/2349423003/diff/40001/chrome/test/chromedrive...
> File chrome/test/chromedriver/net/adb_client_socket.cc (right):
> 
>
https://codereview.chromium.org/2349423003/diff/40001/chrome/test/chromedrive...
> chrome/test/chromedriver/net/adb_client_socket.cc:369: // dual stack
environment
> while current adb (1.0.36) always listen on IPv4.
> I'd suggest the following tweak to the wording to make it a bit clearer that
adb
> listens on IPv6 when running in an IPv6-only environment:
> 
> In a IPv4/IPv6 dual stack environment, getaddrinfo for localhost could only
> return IPv6 address while the current adb (1.0.36) will always listen on IPv4.

Done

[lepton, 2016-09-26 15:54:13]

Ping

[lepton, 2016-09-26 15:54:14]

Ping

[samuong, 2016-09-27 10:44:49]

still lgtm

[lepton, 2016-09-27 14:59:21]

The CQ bit was checked by lepton@google.com

[lepton, 2016-09-27 14:59:24]

The CQ bit was unchecked by lepton@google.com

[lepton, 2016-09-27 14:59:25]

The CQ bit was checked by lepton@google.com

[commit-bot: I haz the power, 2016-09-27 14:59:40]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-27 15:09:48]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-09-27 15:12:49]

Description was changed from

==========
Fallback to IPv6 if IPv4 adb port not available.

This make chromedriver works with android emulator in pure IPv6 env.

BUG=chromedriver:1515
==========

to

==========
Fallback to IPv6 if IPv4 adb port not available.

This make chromedriver works with android emulator in pure IPv6 env.

BUG=chromedriver:1515

Committed: https://crrev.com/1dceac70eca7453b57574a5ed9d91606e761bc58
Cr-Commit-Position: refs/heads/master@{#421212}
==========

[commit-bot: I haz the power, 2016-09-27 15:12:50]

Patchset 4 (id:??) landed as
https://crrev.com/1dceac70eca7453b57574a5ed9d91606e761bc58
Cr-Commit-Position: refs/heads/master@{#421212}