Fix the hostname used to setup a TURN/TSL connection.

Fix the hostname used to setup a TURN/TSL connection.

BUG=https://code.google.com/p/webrtc/issues/detail?id=3601

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=289972

[jiayl, 2014-08-12 19:22:46]

PTAL

[Mallinath (Gone from Chromium), 2014-08-12 21:22:07]

https://codereview.chromium.org/466843002/diff/20001/content/renderer/p2p/ipc...
File content/renderer/p2p/ipc_socket_factory.cc (right):

https://codereview.chromium.org/466843002/diff/20001/content/renderer/p2p/ipc...
content/renderer/p2p/ipc_socket_factory.cc:247: P2PHostAndIPEndPoint
remote_info(remote_address.hostname(), remote_endpoint);
are you sure remote_endpoint will have port, if remote_address has unresolved
address?

[jiayl, 2014-08-12 21:39:51]

https://codereview.chromium.org/466843002/diff/20001/content/renderer/p2p/ipc...
File content/renderer/p2p/ipc_socket_factory.cc (right):

https://codereview.chromium.org/466843002/diff/20001/content/renderer/p2p/ipc...
content/renderer/p2p/ipc_socket_factory.cc:247: P2PHostAndIPEndPoint
remote_info(remote_address.hostname(), remote_endpoint);
On 2014/08/12 21:22:07, mallinath2 wrote:
> are you sure remote_endpoint will have port, if remote_address has unresolved
> address?


Haha, I thought so but I was wrong. Added the code to set the port in that case.

[jiayl, 2014-08-12 21:40:18]

PTAL

[Mallinath (Gone from Chromium), 2014-08-12 21:47:51]

On 2014/08/12 21:40:18, jiayl wrote:
> PTAL

lgtm

[jiayl, 2014-08-13 21:11:23]

Ping Sergey.

[Sergey Ulanov, 2014-08-14 17:55:09]

https://codereview.chromium.org/466843002/diff/40001/content/renderer/p2p/ipc...
File content/renderer/p2p/ipc_socket_factory.cc (right):

https://codereview.chromium.org/466843002/diff/40001/content/renderer/p2p/ipc...
content/renderer/p2p/ipc_socket_factory.cc:244: if
(remote_address.IsUnresolvedIP()) {
In case when the anderss is unresolved do we really need to call
SocketAddressToIPEndPoint() above?

[jiayl, 2014-08-14 18:09:53]

PTAL

https://codereview.chromium.org/466843002/diff/40001/content/renderer/p2p/ipc...
File content/renderer/p2p/ipc_socket_factory.cc (right):

https://codereview.chromium.org/466843002/diff/40001/content/renderer/p2p/ipc...
content/renderer/p2p/ipc_socket_factory.cc:244: if
(remote_address.IsUnresolvedIP()) {
On 2014/08/14 17:55:09, Sergey Ulanov wrote:
> In case when the anderss is unresolved do we really need to call
> SocketAddressToIPEndPoint() above?

Done.

[Sergey Ulanov, 2014-08-14 20:47:24]

https://codereview.chromium.org/466843002/diff/60001/content/renderer/p2p/ipc...
File content/renderer/p2p/ipc_socket_factory.cc (right):

https://codereview.chromium.org/466843002/diff/60001/content/renderer/p2p/ipc...
content/renderer/p2p/ipc_socket_factory.cc:238: if (!remote_address.IsNil() &&
I suggest formatting this code as follows to make it more readable:

 if (!remote_address.IsNil()) {
   DCHECK(IsTcpClientSocket(type_));

   if (remote_address.IsUnresolvedIP()) {
      remote_endpoint =
          net::IPEndPoint(net::IPAddressNumber(), remote_address.port());
   } else {
      if (!jingle_glue::SocketAddressToIPEndPoint(
              remote_address, &remote_endpoint)) {
        return false;
      }
   }
 }

[jiayl, 2014-08-14 22:31:48]

https://codereview.chromium.org/466843002/diff/60001/content/renderer/p2p/ipc...
File content/renderer/p2p/ipc_socket_factory.cc (right):

https://codereview.chromium.org/466843002/diff/60001/content/renderer/p2p/ipc...
content/renderer/p2p/ipc_socket_factory.cc:238: if (!remote_address.IsNil() &&
That isn't the same logic. 
1. The DCHECK is not true if the IP is resolved.
2. If the IP is unresolved, is it safe to assert TCP? Could it cause problem in
release build?

On 2014/08/14 20:47:24, Sergey Ulanov wrote:
> I suggest formatting this code as follows to make it more readable:
> 
>  if (!remote_address.IsNil()) {
>    DCHECK(IsTcpClientSocket(type_));
> 
>    if (remote_address.IsUnresolvedIP()) {
>       remote_endpoint =
>           net::IPEndPoint(net::IPAddressNumber(), remote_address.port());
>    } else {
>       if (!jingle_glue::SocketAddressToIPEndPoint(
>               remote_address, &remote_endpoint)) {
>         return false;
>       }
>    }
>  }

[jiayl, 2014-08-15 00:05:33]

https://codereview.chromium.org/466843002/diff/60001/content/renderer/p2p/ipc...
File content/renderer/p2p/ipc_socket_factory.cc (right):

https://codereview.chromium.org/466843002/diff/60001/content/renderer/p2p/ipc...
content/renderer/p2p/ipc_socket_factory.cc:238: if (!remote_address.IsNil() &&
On 2014/08/14 22:31:47, jiayl wrote:
> That isn't the same logic. 
> 1. The DCHECK is not true if the IP is resolved.
> 2. If the IP is unresolved, is it safe to assert TCP? Could it cause problem
in
> release build?
> 
> On 2014/08/14 20:47:24, Sergey Ulanov wrote:
> > I suggest formatting this code as follows to make it more readable:
> > 
> >  if (!remote_address.IsNil()) {
> >    DCHECK(IsTcpClientSocket(type_));
> > 
> >    if (remote_address.IsUnresolvedIP()) {
> >       remote_endpoint =
> >           net::IPEndPoint(net::IPAddressNumber(), remote_address.port());
> >    } else {
> >       if (!jingle_glue::SocketAddressToIPEndPoint(
> >               remote_address, &remote_endpoint)) {
> >         return false;
> >       }
> >    }
> >  }
> 


I would rather just fix the hostname issue instead of changing other logic at
the last minute before a branch cut.

PTAL.

[Sergey Ulanov, 2014-08-15 00:37:06]

https://codereview.chromium.org/466843002/diff/60001/content/renderer/p2p/ipc...
File content/renderer/p2p/ipc_socket_factory.cc (right):

https://codereview.chromium.org/466843002/diff/60001/content/renderer/p2p/ipc...
content/renderer/p2p/ipc_socket_factory.cc:238: if (!remote_address.IsNil() &&
On 2014/08/14 22:31:47, jiayl wrote:
> That isn't the same logic. 
> 1. The DCHECK is not true if the IP is resolved.

What are the cases when it's not true? remote_address is used only for client
TCP sockets and AFAICT it's Nil in all other cases. 


> 2. If the IP is unresolved, is it safe to assert TCP? Could it cause problem
in
> release build?

browser process just ignores remote_address for UPD sockets and server TCP
sockets.

https://codereview.chromium.org/466843002/diff/60001/content/renderer/p2p/ipc...
content/renderer/p2p/ipc_socket_factory.cc:238: if (!remote_address.IsNil() &&
On 2014/08/15 00:05:33, jiayl wrote:
> On 2014/08/14 22:31:47, jiayl wrote:
> > That isn't the same logic. 
> > 1. The DCHECK is not true if the IP is resolved.
> > 2. If the IP is unresolved, is it safe to assert TCP? Could it cause problem
> in
> > release build?
> > 
> > On 2014/08/14 20:47:24, Sergey Ulanov wrote:
> > > I suggest formatting this code as follows to make it more readable:
> > > 
> > >  if (!remote_address.IsNil()) {
> > >    DCHECK(IsTcpClientSocket(type_));
> > > 
> > >    if (remote_address.IsUnresolvedIP()) {
> > >       remote_endpoint =
> > >           net::IPEndPoint(net::IPAddressNumber(), remote_address.port());
> > >    } else {
> > >       if (!jingle_glue::SocketAddressToIPEndPoint(
> > >               remote_address, &remote_endpoint)) {
> > >         return false;
> > >       }
> > >    }
> > >  }
> > 
> 
> 
> I would rather just fix the hostname issue instead of changing other logic at
> the last minute before a branch cut.
> 
> PTAL.

I don't think it changes logic if you take into account that remote_address is
specified only for tcp client sockets. But feel free to file a bug to cleanup it
later if you disagree.

[jiayl, 2014-08-15 16:23:11]

PTAL

https://codereview.chromium.org/466843002/diff/60001/content/renderer/p2p/ipc...
File content/renderer/p2p/ipc_socket_factory.cc (right):

https://codereview.chromium.org/466843002/diff/60001/content/renderer/p2p/ipc...
content/renderer/p2p/ipc_socket_factory.cc:238: if (!remote_address.IsNil() &&
OK. That makes sense. done.

On 2014/08/15 00:37:06, Sergey Ulanov wrote:
> On 2014/08/15 00:05:33, jiayl wrote:
> > On 2014/08/14 22:31:47, jiayl wrote:
> > > That isn't the same logic. 
> > > 1. The DCHECK is not true if the IP is resolved.
> > > 2. If the IP is unresolved, is it safe to assert TCP? Could it cause
problem
> > in
> > > release build?
> > > 
> > > On 2014/08/14 20:47:24, Sergey Ulanov wrote:
> > > > I suggest formatting this code as follows to make it more readable:
> > > > 
> > > >  if (!remote_address.IsNil()) {
> > > >    DCHECK(IsTcpClientSocket(type_));
> > > > 
> > > >    if (remote_address.IsUnresolvedIP()) {
> > > >       remote_endpoint =
> > > >           net::IPEndPoint(net::IPAddressNumber(),
remote_address.port());
> > > >    } else {
> > > >       if (!jingle_glue::SocketAddressToIPEndPoint(
> > > >               remote_address, &remote_endpoint)) {
> > > >         return false;
> > > >       }
> > > >    }
> > > >  }
> > > 
> > 
> > 
> > I would rather just fix the hostname issue instead of changing other logic
at
> > the last minute before a branch cut.
> > 
> > PTAL.
> 
> I don't think it changes logic if you take into account that remote_address is
> specified only for tcp client sockets. But feel free to file a bug to cleanup
it
> later if you disagree.

[Sergey Ulanov, 2014-08-15 17:09:55]

lgtm

[jiayl, 2014-08-15 17:11:18]

The CQ bit was checked by jiayl@chromium.org

[commit-bot: I haz the power, 2014-08-15 17:17:19]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jiayl@chromium.org/466843002/80001

[commit-bot: I haz the power, 2014-08-15 19:21:55]

Committed patchset #4 (80001) as 289972